Python config script policy
Follow PEP8, but do not forget the "A Foolish Consistency is the Hobgoblin of Little Minds" paragraph. Especially regarding the line length: VyOS naturally has quite a lot of string formatting for error messages etc. that gives rise to lines that may run over 72 characters, and only get less readable if you break them. Do not break strings solely for PEP8 compliance.
Functions should not exceed 100 SLOC.
Global variables should not be used.
Building configs by string concatenation should not be used. The only case when it's unquestinably ok is configs with uniform, flat, and trivial syntax, such as sysctl.conf that consists of an unstructured sequence of key-value pairs. Otherwise, template processor should be used. If config syntax allows nested statements, such as dhcpd.conf, template processor must be used.
Functions that modify the system state or rely on system state must be isolated from the rest of the logic.
Every function that doesn't modify the system state or rely on it must be testable, i.e. it must be possible to execute it in isolation and test its return value automatically.
Ideally the VyOS config should be read into an intermediate representation, and everything else should work with that representation instead of reading the config directly. This will also make it easier to change the config syntax, without having to find every occurence of the affected config path in the scripts.
Python 3 shall be used.
Rationale: In VyOS, we control the environment the code runs in, and maintaining compatibility with 2.7 gives us no benefits, so the effort is not worth it. Most popular libraries already support Python 3 (see py3readiness.org for instance).
The official template processor of VyOS will be Jinja2.
Rationale: It has just enough logic in templates to do what we want, but not enough to turn templates into a Turing tarpit.
2.0 compatibility considerations
This is the most important part. We have to avoid making the same mistakes and introduce anything that will hinder migrating the code written in 1.2.x to 2.0.
General script structure
The logic for config consistency/validity checking, generating application configs, and applying configs or restarting services shall not be mixed.
Rationale: That's the biggest obstacle in implementing commit dry run and non-disruptive rollback.
Scripts should follow something of this structure, or its OOP equivalent:
def verify(config): pass def generate(config): pass def apply(config): pass config = vyos.config() try: verify(config) generate(config) apply(config) except vyos.InvalidConfig: ... sys.exit(1) except vyos.ConfigError: ... sys.exit(1)
The verify() method must not change the system state in any way whatsoever. It must raise an exception if an error is found in the config.
The generate() method may create updated config files in a temporary location, but must not restart any services or make any changes to the OS or the applications.
The apply() method may do whatever it takes to apply the configs or data produced by the generate() method including restarting services (though it still should avoid being more disruptive than absolutely necessary).
The verify/generate/apply functions and auxillary functions must not output anything to stdout or stderr. Output is only allowed in the body of the script. Errors must be communicated through exceptions or return values. Debug information can be sent to syslog if necessary.
Rationale: VyOS 2.0 is going to be way more message oriented and script output will not automatically end up in console output, the result of script execution will be packed into a message and sent to the vyconf daemon instead.
Applying system/applications configs
If an application or OS component provides live config reload functionality, it must be used.
Rationale: Not using it wastes time and effort on reimplementing functionality that already exists, and interferes with live rollback.
In VyOS, historically, OpenVPN script would start openvpn process with a long string of options, and firewall scripts would manipular individual rules, even though OpenVPN is perfectly capable ot reloading its config, and iptables-restore has been around for ages too. This approach not only wastes time, but also makes live rollback a lot harder to implement, since we have to not only figure out how to apply the new config, but also how to go from it to the old config.
Whenever at all possible, the generate() method should only produce updated config files, and the apply() method should only invoke a utility that tells the application to reload the config. If an application does not have this capability, things get interesting. First thing to do is to check if there is a third party application for managing it in this manner (e.g. wicked for managing NICs). The last resort is writing such an application ourselves.
The only functions from the vyos.config library that can be used freely are exists(), list_nodes(), return_value(), and return_values(). Everything else (exists_orig etc.) should not be used, at least not without a very compelling reason to do so.