diff options
Diffstat (limited to 'docs/contributing/md-development.md')
| -rw-r--r-- | docs/contributing/md-development.md | 694 |
1 files changed, 694 insertions, 0 deletions
diff --git a/docs/contributing/md-development.md b/docs/contributing/md-development.md new file mode 100644 index 00000000..dcfe257a --- /dev/null +++ b/docs/contributing/md-development.md @@ -0,0 +1,694 @@ +(development)= + +# Development + +All VyOS source code is hosted on GitHub under the VyOS organization which can +be found here: <https://github.com/vyos> + +Our code is split into several modules. VyOS is composed of multiple individual +packages, some of them are forks of upstream packages and are periodically +synced with upstream, so keeping the whole source under a single repository +would be very inconvenient and slow. There is now an ongoing effort to +consolidate all VyOS-specific framework/config packages into vyos-1x package, +but the basic structure is going to stay the same, just with fewer and fewer +packages while the base code is rewritten from Perl/BASH into Python using and +XML based interface definition for the CLI. + +The repository that contains all the ISO build scripts is: +<https://github.com/vyos/vyos-build> + +The README.md file will guide you to use the this top level repository. + +## Submit a Patch + +Patches are always more than welcome. To have a clean and easy to maintain +repository we have some guidelines when working with Git. A clean repository +eases the automatic generation of a changelog file. + +A good approach for writing commit messages is actually to have a look at the +file(s) history by invoking `git log path/to/file.txt`. + +(prepare_commit)= + +### Prepare patch/commit + +In a big system, such as VyOS, that is comprised of multiple components, it's +impossible to keep track of all the changes and bugs/feature requests in one's +head. We use a bugtracker known as [Phabricator] for it ("issue tracker" would +be a better term, but this one stuck). + +The information is used in three ways: + +- Keep track of the progress (what we've already done in this branch and what + we still need to do). +- Prepare release notes for upcoming releases +- Help future maintainers of VyOS (it could be you!) to find out why certain + things have been changed in the codebase or why certain features have been + added + +To make this approach work, every change must be associated with a task number +(prefixed with **T**) and a component. If there is no bug report/feature request +for the changes you are going to make, you have to create a [Phabricator] task +first. Once there is an entry in [Phabricator], you should reference its id in +your commit message, as shown below: + +- `ddclient: T1030: auto create runtime directories` +- `Jenkins: add current Git commit ID to build description` + +If there is no [Phabricator] reference in the commits of your pull request, we +have to ask you to amend the commit message. Otherwise we will have to reject +it. + +#### Writing good commit messages + +The format should be and is inspired by: <https://git-scm.com/book/ch5-2.html> +It is also worth reading <https://chris.beams.io/posts/git-commit/> + +- A single, short, summary of the commit (recommended 50 characters or less, + not exceeding 80 characters) containing a prefix of the changed component + and the corresponding [Phabricator] reference e.g. `snmp: T1111:` or + `ethernet: T2222:` - multiple components could be concatenated as in + `snmp: ethernet: T3333` + +- In some contexts, the first line is treated as the subject of an email and + the rest of the text as the body. The blank line separating the summary from + the body is critical (unless you omit the body entirely); tools like rebase + can get confused if you run the two together. + +- Followed by a message which describes all the details like: + + - What/why/how something has been changed, makes everyone's life easier when + working with `git bisect` + - All text of the commit message should be wrapped at 72 characters if + possible which makes reading commit logs easier with `git log` on a + standard terminal (which happens to be 80x25) + - If applicable a reference to a previous commit should be made linking + those commits nicely when browsing the history: `After commit abcd12ef + ("snmp: this is a headline") a Python import statement is missing, + throwing the following exception: ABCDEF` + +- Always use the `-x` option to the `git cherry-pick` command when back or + forward porting an individual commit. This automatically appends the line: + `(cherry picked from commit <ID>)` to the original authors commit message + making it easier when bisecting problems. + +- Every change set must be consistent (self containing)! Do not fix multiple + bugs in a single commit. If you already worked on multiple fixes in the same + file use `git add --patch` to only add the parts related to the one issue + into your upcoming commit. + +Limits: + +- We only accept bugfixes in packages other than <https://github.com/vyos/vyos-1x> + as no new functionality should use the old style templates (`node.def` and + Perl/BASH code. Use the new style XML/Python interface instead. + +Please submit your patches using the well-known GitHub pull-request against our +repositories found in the VyOS GitHub organisation at <https://github.com/vyos> + +### Determinine source package + +Suppose you want to make a change in the webproxy script but yet you do not know +which of the many VyOS packages ship this file. You can determine the VyOS +package name in question by using Debian's `dpkg -S` command of your running +VyOS installation. + +```none +vyos@vyos:~ dpkg -S /opt/vyatta/sbin/vyatta-update-webproxy.pl +vyatta-webproxy: /opt/vyatta/sbin/vyatta-update-webproxy.pl +``` + +This means the file in question (`/opt/vyatta/sbin/vyatta-update-webproxy.pl`) +is located in the `vyatta-webproxy` package which can be found here: +<https://github.com/vyos/vyatta-webproxy> + +### Fork Repository and submit Patch + +Forking the repository and submitting a GitHub pull-request is the preferred +way of submitting your changes to VyOS. You can fork any VyOS repository to your +very own GitHub account by just appending `/fork` to any repository's URL on +GitHub. To e.g. fork the `vyos-1x` repository, open the following URL in your +favourite browser: <https://github.com/vyos/vyos-1x/fork> + +You then can proceed with cloning your fork or add a new remote to your local +repository: + +- Clone: `git clone https://github.com/<user>/vyos-1x.git` +- Fork: `git remote add myfork https://github.com/<user>/vyos-1x.git` + +In order to record you as the author of the fix please identify yourself to Git +by setting up your name and email. This can be done local for this one and only +repository `git config` or globally using `git config --global`. + +```none +git config --global user.name "J. Random Hacker" +git config --global user.email "jrhacker@example.net" +``` + +Make your changes and save them. Do the following for all changes files to +record them in your created Git commit: + +- Add file to Git index using `git add myfile`, or for a whole directory: + `git add somedir/*` +- Commit the changes by calling `git commit`. Please use a meaningful commit + headline (read above) and don't forget to reference the [Phabricator] ID. +- Submit the patch `git push` and create the GitHub pull-request. + +### Attach patch to Phabricator task + +Follow the above steps on how to "Fork repository to submit a Patch". Instead +of uploading "pushing" your changes to GitHub you can export the patches/ +commits and send it to <mailto:maintainers@vyos.net> or attach it directly to the bug +(preferred over email) + +- Export last commit to patch file: `git format-patch` or export the last two + commits into its appropriate patch files: `git format-patch -2` + +## Coding Guidelines + +Like any other project we have some small guidelines about our source code, too. +The rules we have are not there to punish you - the rules are in place to help +us all. By having a consistent coding style it becomes very easy for new +and also longtime contributors to navigate through the sources and all the +implied logic of any one source file.. + +Python 3 **shall** be used. How long can we keep Python 2 alive anyway? No +considerations for Python 2 compatibility **should** be taken at any time. + +### Formatting + +- Python: Tabs **shall not** be used. Every indentation level should be 4 spaces +- XML: Tabs **shall not** be used. Every indentation level should be 2 spaces + +:::{note} +There are extensions to e.g. VIM (xmllint) which will help you to get +your indention levels correct. Add to following to your .vimrc file: +`au FileType xml setlocal equalprg=xmllint\ --format\ --recover\ -\ +2>/dev/null` now you can call the linter using `gg=G` in command mode. +::: + +#### Text generation + +Template processor **should** be used for generating config files. Built-in +string formatting **may** be used for simple line-oriented formats where every +line is self-contained, such as iptables rules. Template processor **must** be +used for structured, multi-line formats such as those used by ISC DHCPd. + +The default template processor for VyOS code is [Jinja2]. + +### Summary + +When modifying the source code, remember these rules of the legacy elimination +campaign: + +- No new features in Perl +- No old style command definitions +- No code incompatible with Python3 + +## Python + +The switch to the Python programming language for new code is not merely a +change of the language, but a chance to rethink and improve the programming +approach. + +Let's face it: VyOS is full of spaghetti code where logic for reading the VyOS +config, generating daemon configs, and restarting processes is all mixed up. + +Python (or any other language, for that matter) does not provide automatic +protection from bad design, so we need to also devise design guidelines and +follow them to keep the system extensible and maintainable. + +But we are here to assist you and want to guide you through how you can become +a good VyOS contributor. The rules we have are not there to punish you - the +rules are in place to help us all. What does it mean? By having a consistent +coding style it becomes very easy for new contributors and also longtime +contributors to navigate through the sources and all the implied logic of +the spaghetti code. + +Please use the following template as good starting point when developing new +modules or even rewrite a whole bunch of code in the new style XML/Python +interface. + +### Configuration Script Structure and Behaviour + +Your configuration script or operation mode script which is also written in +Python3 should have a line break on 80 characters. This seems to be a bit odd +nowadays but as some people also work remotely or program using vi(m) this is +a fair good standard which I hope we can rely on. + +In addition this also helps when browsing the GitHub codebase on a mobile +device if you happen to be a crazy scientist. + +```python +#!/usr/bin/env python3 +# +# Copyright (C) 2020 VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +import sys + +from vyos.config import Config +from vyos import ConfigError + +def get_config(): + if config: + conf = config + else: + conf = Config() + + # Base path to CLI nodes + base = ['...', '...'] + # Convert the VyOS config to an abstract internal representation + config_data = conf.get_config_dict(base, key_mangling=('-', '_'), get_first_key=True) + return config_data + +def verify(config): + # Verify that configuration is valid + if invalid: + raise ConfigError("Descriptive message") + return True + +def generate(config): + # Generate daemon configs + pass + +def apply(config): + # Apply the generated configs to the live system + pass + +try: + c = get_config() + verify(c) + generate(c) + apply(c) +except ConfigError as e: + print(e) + sys.exit(1) +``` + +The `get_config()` function must convert the VyOS config to an abstract, +internal representation. No other function is allowed to call the `vyos.config. +Config` object method directly. The rationale for it is that when config reads +are mixed with other logic, it's very hard to change the config syntax since +you need to weed out every occurrence of the old syntax. If syntax-specific +code is confined to a single function, the rest of the code can be left +untouched as long as the internal representation remains compatible. + +Another advantage is testability of the code. Mocking the entire config +subsystem is hard, while constructing an internal representation by hand is +way simpler. + +The `verify()` function takes your internal representation of the config and +checks if it's valid, otherwise it must raise `ConfigError` with an error +message that describes the problem and possibly suggests how to fix it. It must +not make any changes to the system. The rationale for it is again testability +and, in the future when the config backend is ready and every script is +rewritten in this fashion, ability to execute commit dry run ("commit test" +like in JunOS) and abort commit before making any changes to the system if an +error is found in any component. + +The `generate()` function generates config files for system components. + +The `apply()` function applies the generated configuration to the live +system. It should use non-disruptive reload whenever possible. It may execute +disruptive operations such as daemon process restart if a particular component +does not support non-disruptive reload, or when the expected service degradation +is minimal (for example, in case of auxiliary services such as LLDPd). In case +of high impact services such as VPN daemon and routing protocols, when non- +disruptive reload is supported for some but not all types of configuration +changes, scripts authors should make effort to determine if a configuration +change can be done in a non-disruptive way and only resort to disruptive restart +if it cannot be avoided. + +Unless absolutely necessary, configuration scripts should not modify the active +configuration of system components directly. Whenever at all possible, scripts +should generate a configuration file or files that can be applied with a single +command such as reloading a service through systemd init. Inserting statements +one by one is particularly discouraged, for example, when configuring netfilter +rules, saving them to a file and loading it with iptables-restore should always +be preferred to executing iptables directly. + +The `apply()` and `generate()` functions may `raise ConfigError` if, for +example, the daemon failed to start with the updated config. It shouldn't be a +substitute for proper config checking in the `verify()` function. All +reasonable effort should be made to verify that generated configuration is +valid and will be accepted by the daemon, including, when necessary, cross- +checks with other VyOS configuration subtrees. + +Exceptions, including `VyOSError` (which is raised by `vyos.config.Config` +on improper config operations, such as trying to use `list_nodes()` on a +non-tag node) should not be silenced or caught and re-raised as config error. +Sure this will not look pretty on user's screen, but it will make way better +bug reports, and help users (and most VyOS users are IT professionals) do their +own debugging as well. + +For easy orientation we suggest you take a look on the `ntp.py` or +`interfaces-bonding.py` (for tag nodes) implementation. Both files can be +found in the [vyos-1x] repository. + +## XML (used for CLI definitions) + +The bash (or better vbash) completion in VyOS is defined in *templates*. +Templates are text files (called `node.def`) stored in a directory tree. The +directory names define the command names, and template files define the command +behaviour. Before VyOS 1.2 (crux) this files were created by hand. After a +complex redesign [process] the new style template are automatically generated +from a XML input file. + +XML interface definitions for VyOS come with a RelaxNG schema and are located +in the [vyos-1x] module. This schema is a slightly modified schema from [VyConf] +alias VyOS 2.0 So VyOS 1.2.x interface definitions will be reusable in Nextgen +VyOS Versions with very minimal changes. + +The great thing about schemas is not only that people can know the complete +grammar for certain, but also that it can be automatically verified. The +`scripts/build-command-templates` script that converts the XML definitions to +old style templates also verifies them against the schema, so a bad definition +will cause the package build to fail. I do agree that the format is verbose, but +there is no other format now that would allow this. Besides, a specialized XML +editor can alleviate the issue with verbosity. + +Example: + +```xml +<?xml version="1.0"?> +<!-- Cron configuration --> +<interfaceDefinition> + <node name="system"> + <children> + <node name="task-scheduler"> + <properties> + <help>Task scheduler settings</help> + </properties> + <children> + <tagNode name="task" owner="${vyos_conf_scripts_dir}/task_scheduler.py"> + <properties> + <help>Scheduled task</help> + <valueHelp> + <format><string></format> + <description>Task name</description> + </valueHelp> + <priority>999</priority> + </properties> + <children> + <leafNode name="crontab-spec"> + <properties> + <help>UNIX crontab time specification string</help> + </properties> + </leafNode> + <leafNode name="interval"> + <properties> + <help>Execution interval</help> + <valueHelp> + <format><minutes></format> + <description>Execution interval in minutes</description> + </valueHelp> + <valueHelp> + <format><minutes>m</format> + <description>Execution interval in minutes</description> + </valueHelp> + <valueHelp> + <format><hours>h</format> + <description>Execution interval in hours</description> + </valueHelp> + <valueHelp> + <format><days>d</format> + <description>Execution interval in days</description> + </valueHelp> + <constraint> + <regex>[1-9]([0-9]*)([mhd]{0,1})</regex> + </constraint> + </properties> + </leafNode> + <node name="executable"> + <properties> + <help>Executable path and arguments</help> + </properties> + <children> + <leafNode name="path"> + <properties> + <help>Path to executable</help> + </properties> + </leafNode> + <leafNode name="arguments"> + <properties> + <help>Arguments passed to the executable</help> + </properties> + </leafNode> + </children> + </node> + </children> + </tagNode> + </children> + </node> + </children> + </node> +</interfaceDefinition> +``` + +Command definitions are purely declarative, and cannot contain any logic. All +logic for generating config files for target applications, restarting services +and so on is implemented in configuration scripts instead. + +### GNU Preprocessor + +XML interface definition files use the `xml.in` file extension which was +implemented in {vytask}`T1843`. XML interface definitions tend to have a lot of +duplicated code in areas such as: + +- VIF (incl. VIF-S/VIF-C) +- Address +- Description +- Enabled/Disabled + +Instead of supplying all those XML nodes multiple times there are now include +files with predefined features. Brief overview: + +- [IPv4, IPv6 and DHCP(v6)] address assignment +- [IPv4, IPv6] address assignment +- [VLAN (VIF)] definition +- [MAC address] assignment + +All interface definition XML input files (.in suffix) will be sent to the GCC +preprocess and the output is stored in the `build/interface-definitions` +folder. The previously mentioned `scripts/build-command-templates` script +operates on the `build/interface-definitions` folder to generate all required +CLI nodes. + +```none +$ make interface_definitions +install -d -m 0755 build/interface-definitions +install -d -m 0755 build/op-mode-definitions +Generating build/interface-definitions/intel_qat.xml from interface-definitions/intel_qat.xml.in +Generating build/interface-definitions/interfaces-bonding.xml from interface-definitions/interfaces-bonding.xml.in +Generating build/interface-definitions/cron.xml from interface-definitions/cron.xml.in +Generating build/interface-definitions/pppoe-server.xml from interface-definitions/pppoe-server.xml.in +Generating build/interface-definitions/mdns-repeater.xml from interface-definitions/mdns-repeater.xml.in +Generating build/interface-definitions/tftp-server.xml from interface-definitions/tftp-server.xml.in +[...] +``` + +### Guidelines + +#### Use of numbers + +Use of numbers in command names **should** be avoided unless a number is a +part of a protocol name or similar. Thus, `protocols ospfv3` is perfectly +fine, but something like `server-1` is questionable at best. + +#### Help String + +To ensure uniform look and feel, and improve readability, we should follow a +set of guidelines consistently. + +##### Capitalization and punctuation + +The first word of every help string **must** be capitalized. There **must not** +be a period at the end of help strings. + +Rationale: this seems to be the unwritten standard in network device CLIs, and +a good aesthetic compromise. + +Examples: + +- Good: "Frobnication algorithm" +- Bad: "frobnication algorithm" +- Bad: "Frobnication algorithm." +- Horrible: "frobnication algorithm." + +##### Use of abbreviations and acronyms + +Abbreviations and acronyms **must** be capitalized. + +Examples: + +- Good: "TCP connection timeout" +- Bad: "tcp connection timeout" +- Horrible: "Tcp connection timeout" + +Acronyms also **must** be capitalized to visually distinguish them from normal +words: + +Examples: + +- Good: RADIUS (as in remote authentication for dial-in user services) +- Bad: radius (unless it's about the distance between a center of a circle and + any of its points) + +Some abbreviations are traditionally written in mixed case. Generally, if it +contains words "over" or "version", the letter **should** be lowercase. If +there's an accepted spelling (especially if defined by an RFC or another +standard), it **must** be followed. + +Examples: + +- Good: PPPoE, IPsec +- Bad: PPPOE, IPSEC +- Bad: pppoe, ipsec + +##### Use of verbs + +Verbs **should** be avoided. If a verb can be omitted, omit it. + +Examples: + +- Good: "TCP connection timeout" +- Bad: "Set TCP connection timeout" + +If a verb is essential, keep it. For example, in the help text of `set system +ipv6 disable-forwarding`, "Disable IPv6 forwarding on all interfaces" is a +perfectly justified wording. + +##### Prefer infinitives + +Verbs, when they are necessary, **should** be in their infinitive form. + +Examples: + +- Good: "Disable IPv6 forwarding" +- Bad: "Disables IPv6 forwarding" + +### Migrating old CLI + +```{eval-rst} +.. list-table:: + :widths: 25 25 50 + :header-rows: 1 + + * - Old concept/syntax + - New syntax + - Notes + * - mynode/node.def + - <node name="mynode"> </node> + - Leaf nodes (nodes with values) use <leafNode> tag instead + * - mynode/node.tag , tag: + - <tagNode name="mynode> </node> + - + * - help: My node + - <properties> <help>My node</help> + - + * - val_help: <format>; some string + - <properties> <valueHelp> <format> format </format> <description> some + string </description> + - Do not add angle brackets around the format, they will be inserted + automatically + * - syntax:expression: pattern + - <properties> <constraint> <regex> ... + - <constraintErrorMessage> will be displayed on failure + * - syntax:expression: $VAR(@) in "foo", "bar", "baz" + - None + - Use regex + * - syntax:expression: exec ... + - <properties> <constraint> <validator> <name ="foo" argument="bar"> + - "${vyos_libexecdir}/validators/foo bar $VAR(@)" will be executed, + <constraintErrorMessage> will be displayed on failure + * - syntax:expression: (arithmetic expression) + - None + - External arithmetic validator may be added if there's demand, complex + validation is better left to commit-time scripts + * - priority: 999 + - <properties> <priority>999</priority> + - Please leave a comment explaining why the priority was chosen + (e.g. "after interfaces are configured") + * - multi: + - <properties> <multi/> + - Only applicable to leaf nodes + * - allowed: echo foo bar + - <properties> <completionHelp> <list> foo bar </list> + - + * - allowed: cli-shell-api listNodes vpn ipsec esp-group + - <properties> <completionHelp> <path> vpn ipsec esp-group </path> ... + - + * - allowed: /path/to/script + - <properties> <completionHelp> <script> /path/to/script </script> ... + - + * - default: + - None + - Move default values to scripts + * - commit:expression: + - None + - All commit time checks should be in the verify() function of the script + * - begin:/create:/delete: + - None + - All logic should be in the scripts +``` + +## C++ Backend Code + +The CLI parser used in VyOS is a mix of bash, bash-completion helper and the +C++ backend library \[vyatta-cfg\](<https://github.com/vyos/vyatta-cfg>). This +section is a reference of common CLI commands and the respective entry point +in the C/C++ code. + +- `set` + + - <https://github.com/vyos/vyatta-cfg/blob/0f42786a0b3/src/cstore/cstore.cpp#L352> + - <https://github.com/vyos/vyatta-cfg/blob/0f42786a0b3/src/cstore/cstore.cpp#L2549> + +- `commit` + + - <https://github.com/vyos/vyatta-cfg/blob/0f42786a0b3/src/commit/commit-algorithm.cpp#L1252> + +## Continuous Integration + +VyOS makes use of [Jenkins] as our Continuous Integration (CI) service. Our +[VyOS CI] server is publicly accessible here: <https://ci.vyos.net>. You can get +a brief overview of all required components shipped in a VyOS ISO. + +To build our modules we utilize a CI/CD Pipeline script. Each and every VyOS +component comes with it's own `Jenkinsfile` which is (more or less) a copy. +The Pipeline utilizes the Docker container from the {ref}`build_iso` section - +but instead of building it from source on every run, we rather always fetch a +fresh copy (if needed) from [Dockerhub]. + +Each module is build on demand if a new commit on the branch in question is +found. After a successful run the resulting Debian Package(s) will be deployed +to our Debian repository which is used during build time. It is located here: +<http://dev.packages.vyos.net/repositories/>. + + +```{include} /_include/common-references.txt +``` + + +[dockerhub]: https://hub.docker.com/u/vyos/ +[ipv4, ipv6]: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/interface/address-ipv4-ipv6.xml.i +[ipv4, ipv6 and dhcp(v6)]: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/interface/address-ipv4-ipv6-dhcp.xml.i +[jenkins]: https://jenkins.io/ +[jinja2]: https://jinja.palletsprojects.com/ +[mac address]: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/interface/mac.xml.i +[process]: https://blog.vyos.io/vyos-development-digest-10 +[vlan (vif)]: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/interface/vif.xml.i +[vyconf]: https://github.com/vyos/vyconf/tree/master/data/schemata +[vyos-1x]: https://github.com/vyos/vyos-1x/tree/current/schema |