Proposal style guide for orchestration frameworks states

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Several things are inciting me to write this:

1. There is no style guide on the current Salt formulas, it doesn't use
Salt best practices and it appears to be the style from the first
batch of contributors, that was later reuse by everyone else copying
the file and replacing values
2. When doing Qusal (Qubes Salt formulas), when I needed to include
states from upstream (Qubes OS), it was a bit of a pain because some
things are coupled together and not very modular
3. There is an Ansible alternative, porting all Salt states to playbooks
will be some work, could be easier
4. Automating deployment of presentation laptop is recommended [0]

To solve these issues, I am planning on including several resources to
the documentation:

- - Salt best practices [1];
- - Ansible best practices [2];
- - Ansible tips and tricks [3];
- - Salt YAML idiosyncrasies (also relevant to Ansible) [4];
- - Style guide for states/playbooks. I have one for Salt [5], comments
are appreciated, discard things specific to Qusal. Ideally, this style
guide should be written to be used by any orchestration framework.

Please share links that you find useful.

There are some things that I'd say is a must in the style guide:

- - Separate files to apply state for each host:

- Already done by Qubes, but a lot of forum users seems to be doing
this wrong, so writing this here
- Separate files makes it easier to read and to include only
necessary things when linking files (`includes` on Salt)

- - Prefer dropins directories rather than modifying existent files:

- Package managers will prompt to see diff and select version,
upstream or modified, on Fedora, it just replaces depending if
`(noreplace)` is written or not;
- Dropins garantees enforcement when the user selects to apply
changes from the package system rather than from the state.
- Appending doesn't guarantee that the setting is not already there
on the file
- Replacing lines is not the best option when the program allows
dropins
- You may append or replace lines as a last resort

- - Place source files outside of the state:

- Benefic for orchestration tools/framework migration, when
replacing Salt for Ansible or for any other tool in the future,
they can reuse the same file or package list rather than
duplicating it
- Allows linting the file (if there is no Jinja in it...)
- Allows using the editor to see syntax errors
- Consistent use of this avoids some files being out of state and
some files being in the state, also avoids a huge file being
dumped in the state, making it difficult to read what is
important, which is the state

- - OS specific states should be conditionally included:

- Lowers the work when adding more supported template and avoids
mistakes if you change the base template of the formula/playbook
and try to run it

- - Declare packages per OS, in YAML:

- Current formulas target Fedora, would be nice if the transition to
any other template was easier by targetting OS families
- Should also be outside of the state, in Ansible, I believe it
should be in `vars` or `group_vars`, but maybe another place
(custom directory) so it can be reuse by Salt and other future
tools

- - Installing packages:

- Don't assume a package is already installed, always declare if it
is a dependency
- Add an variable to allow the administrator to choose to pull or
not "recommends/suggests"
- Include packages from other declarations to avoid duplication.
Example: set the packages for networking in a list, set the
packages for CTAP in another list etc, when a formula needs both,
include both lists instead of duplication the package names.
- Allow installing user chosen packages easily. Sometimes the user
is a non-english speaker and doesn't have his locales package
installed for example, easy installation of such things would be
nice
- Always update before attempting installation, Debian doesn't
refresh automatically and Fedora has a cache that might be
outdated, enforce refreshing. Also useful when adding new
repositories declarations
- Use a single install command, as in, don't invoke the package
manager to install multiple times. This slows down the process as
it doesn't allow the package manager to optimize the process as
well as being much slower on anonymity networks such as Tor, used
by Whonix

- - Naming states:

- Ansible and Salt allows free form text, although easier to read, I
think this is debatable when I want the machine to reference it,
such as Salt `require` key. Just voicing my opinion, we should,
nonetheless, follow upstream best practices. It should be
something that fits the best practice of both existing
orchestration tools

- - Use default user instead of hardcoding "user" user:

- On dom0 and domUs, it is the first user in the "qubes" group.
- For domUs, there is a `default_user` property available from dom0,
but if you are inside the qube, see group membership

- - Don't leave things for fate, set things explicitly:

- Set user and group ownership
- Set file mode
- Create parent directories
- Install dependencies
- Make dependent states depend on each other, with Salt, it is
"require" key

- - Don't use non-idempotent modules when a better module exists;

- Avoid Salt `cmd.*` or Ansible `ansible.builtin.command`. The only
exception is when the idempotent module does not meet all
requirements

- - Use the service manager:

- Don't add things to `rc.local`, it doesn't allow properly ordering
or dependencies, might be too early and also isn't the best thing
to log/debug. The journal of a service is easier to read and find
for failures.
- Conditionally enable services with `ConditionPathExists` and
enable a qube service

- - Setting properties:

- Global property:

- `default_guivm`: When finalizing a GUI domain, allow to set it
as the global property, but not by default
- `default_audiovm`: When finalizing an Audio domain, allow to
set it as the global property, but not by default
- `default_template`: Don't fiddle with it, we will enforce the
correct template on the "qube property" below so the state
only runs on supported templates
- `default_dispvm`: Don't fiddle with it, unless the formula is
really intended for that, and in such case, do not set it by
default but still allow for it to be easily set

- Qube property:

- `template`: enforce template
- `audiovm`: most qubes don't need it, unset if unecessary and
also allow the user to configure it via vars. I believe it is
safer to remove it
- `default_dispvm`: some qubes don't need it, unset if
unecessary and also allow the user to configure it via vars.
Also safer to remove when unecessary, but might break
workflows, so this needs some though on a per formula basis

- Qube feature:

- Allow setting certain features to a value by default to all
qubes. This is specially important to disable Gnome tracker,
CUPS etc
- Use "set" rather than "enable" or "disable", as it allows
seeing the value on the same line when grepping as well as
toggling the value by setting a variable


- In general, allow some settings that is not enforce by the formula
to be set by the user via variables/pillars. A user in this issue
to qusal has a repo with a lot of data being set by pillar [6]

- - Scripts on domUs:

- Don't add, when possible. Might be better to package it. Allows
broader testing
- When using shellscript, don't use bashisms, follow the POSIX
standard. This is intended to allow having different Unix
templates in the future that aren't even GNU, porting them would
be easier if Qubes tools in general, didn't use GNU specific code
when unecessary

- - Use macros/functions:

- Salt has macros that can be done with Jinja. They don't look
pretty or readable, avoid them. But when necessary, use it to
avoid code duplication. I have some macros I like [7]

- - CI:

- Gitlab CI for lint tools: `ansible-lint`, `salt-lint`, `yamllint`.
- Gitlab CI for unit tests, if possible...
- OpenQA at last, iterating the formulas over all supported
templates would be good. Testing on minimal templates even better,
as it would allow checking if dependencies are correctly defined

- - Documentation:

- Don't replicate upstream documentation unless mentioning things
that deviate from upstream
- Keep it simple, keep it short, keep it consistent
- Don't tell users to run scripts after the state, if it is manual,
it should be automated when possible
- Must mention if there is a Qrexec policy that should be configured

I don't think that's all, it could be much more, but this is all that I
can can remember now, as it's been some time since I took a break from
Qusal. Eager to hear about issues and improvements. I did not write the
above exactly as a style guide, it was informal, intended for the
mailing list. Writing the style guide would take more time...

References:

[0] https://github.com/QubesOS/qubes-issues/issues/10701
[1] https://docs.saltproject.io/en/latest/topics/best_practices.html
[2] https://ansible.github.io/lightbulb/decks/ansible-best-practices.html
[3] https://docs.ansible.com/projects/ansible/latest/tips_tricks/index.html
[4] https://docs.saltproject.io/en/latest/topics/troubleshooting/yaml_idiosyncrasies.html
[5] https://github.com/ben-grande/qusal/blob/main/docs/DESIGN.md
[6] https://github.com/ben-grande/qusal/issues/74
[7] https://github.com/ben-grande/qusal/tree/main/salt/utils/macros


- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iHUEARYIAB0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCaZ8odQAKCRAbcxS/DMyW
hwFtAQCay1gFjsuybgrJaOqy1nRpYrZal+qB9/hEB250x5kEDQEAy92027k28QLN
NbLzoWW+bDDzeQUm5j4PeFEMt91XBgQ=
=y0cB
-----END PGP SIGNATURE-----

[deeplow]

Thanks for putting some thoughts down. I think most of these makes sense.

1. One thing that's particularly concerning to me is lingering state. When we remove something (e.g. a package installation), it doesn't remove the package, or when we remove a VM from the file, it will still be there and we need to forever state its removal. I don't think there's an easy way out of that with these tools' paradigms have a way around it). But I wonder if it's worth adding some note on this. It would be interesting to hear how others deal with this.

2. I think in some ways the move to Ansible will make the code structure stricter since it neatly breaks down types of files by pre-defined directories (tasks for "code", defaults for variables, templates for file templates, etc.). So perhaps it could make sense to have a common section in the style, but some of the advice to be a bit more framework-specfic.

3. It just occured to me that instead of this being just a styleguide, what if this actually became a lint tool? I've never done such a thing, but it feels like the ultimate way to ensure consistency. Of course, some things are more general and would be rather hard make a (non-AI-based lint tool). Anyways, I don't meant to distract from the writing this guide (because that is a necessary condition).

Cheers,
deeplow