feat(facts, operations): add support for s6-rc service management - #1854
feat(facts, operations): add support for s6-rc service management#1854epicrazzmatazz wants to merge 25 commits into
Conversation
|
|
There was a problem hiding this comment.
Issues
1. Shell injection in command parameter (inline)
2. raise RuntimeWarning instead of warnings.warn (inline)
Notes
- The
idempotent_noticeonservice()says "only when at least one ofcommit_setorinstall_setareTrue" butrestarted,reloaded, andcommandalso break idempotency. The notice should list all non-idempotent triggers or the operation should be split. S6SetStatus.check_preconditionsalways checks/etc/s6/frontend.confeven when the fact will uses6-rc-set-status(non-default repository/set). This causes false negatives when using a non-default repository.check_preconditionsdoesn't receive the fact's parameters, so this is a design limitation worth documenting in the fact's docstring.S6SetStatusdocstring warns about_success_exit_codes=[3]being needed, but the command usesecho EXIT CODE: $?to capture the exit code as text output, so the overall exit code is always 0. The note is misleading and should be removed.- The
setfunction name shadows the built-inset, requiringimport builtins+builtins.set(...). Consider renaming the operation or the parameter. dont_restart_if_stopped.yamlfixture is missingnoop_description(test warns about it).
| yield from live_install._inner() | ||
|
|
||
| if command: | ||
| yield make_formatted_string_command("s6 {0}", command) |
There was a problem hiding this comment.
command is user-controlled and passed unquoted to make_formatted_string_command. When command = "system reboot; rm -rf /", the rendered command is s6 system reboot; rm -rf / — the semicolon is interpreted by the shell. Since command is meant to be a multi-word s6 subcommand, use shlex.split and quote each token:
if command:
yield StringCommand("s6", *map(QuoteString, shlex.split(command)))| ) | ||
| if len(lines) != 1: | ||
| # no OperationWarning | ||
| raise RuntimeWarning( |
There was a problem hiding this comment.
RuntimeWarning is a Warning subclass, not an exception meant for raising. Use warnings.warn(...) or log via logger.warning(...). Raising it will crash the deploy with a traceback, which is not the intent of a warning.
|
Thanks for the early review. I'm still working on this feature, albeit slowly due to other priorities, so I may not address the concerns for some time. I will remove the draft status and squash the commits when I am satisfied with the API and implementation that I still may change a little and when I have tested a local deploy on my machine with s6. |
What
s6 is a set of utilities for process supervision, service management, linux PID 1, and more. It is in active development; a new frontend was released in January, tying all the components together in an easier to use CLI. A few linux distributions use s6: antiX, Artix linux, Bedrock linux, Obarun, and glaucus, as well as docker containers using the s6-overlay project.
Why
Because I use it on my machine, and I think it will be more popular in the future. Alpine linux devs plan to switch to s6 as the init system and service manager once it is ready.
Implementation
"Big" operations implement logic for idempotence while calling smaller stateless operations. The smaller operations often correspond directly to an s6 program, just being a light wrapper around them.
Unlike other service management operations,
s6.servicecan manage multiple services in one operation call, leveragings6 live's option to specify multiple services at once.At this time, most of the code relies on thes6program froms6-frontend. The facts sometimes fall back to the lower-level programs such ass6-rc-repo-listas needed when thes6program is insufficient. Because of the reliance ons6, the operations inherit the management policy froms6-frontend(below).^ No longer true, I have since migrated most operations to use the lower-level programs.
Currently, the "big" operations don't support managing repositories outside of the one configured in
/etc/s6.confor the compiled-in defaults, which are the implicit repositories for thes6commands. As such, user services are not yet supported.Checklist
Will check these off as I complete them, as this is a draft PR
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)conventional commits format