nixos/modules/virtualisation: additional configuration options #349537
Conversation
github-actions
bot
added
6.topic: nixos
|
@ofborg test oci-containers |
NixOSInfra
added
the
12. first-time contribution
ofborg
bot
added
10.rebuild-darwin: 0
| capAdd = mkOption { | ||
| type = with types; listOf (enum capabilities); | ||
| default = []; | ||
| description = '' | ||
| Capabilities to add to container | ||
| ''; | ||
| example = literalExpression '' | ||
| [ | ||
| SYS_ADMIN | ||
| ] | ||
| ''; | ||
| }; | ||
|
|
||
| capDrop = mkOption { | ||
| type = with types; listOf (enum capabilities); | ||
| default = []; | ||
| description = '' | ||
| Capabilities to drop from container | ||
| ''; | ||
| example = literalExpression '' | ||
| [ | ||
| SYS_ADMIN | ||
| ] | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
A type that's nicer to use instead of these two options would be lazyAttrsOf (nullOr bool). Now that doesn't have built-in checking of the attribute names, but it does allow for better control of overriding, allowing mkDefault true and mkForce false for example.
That's what arion does for example, but it doesn't check the names; I think docker compose checks it later.
You could check the attribute names by adding to assertions.
A parameter for checking the names would be a nice addition to attrsWith when that lands.
There was a problem hiding this comment.
I've extended the existing test to cover new config options. I see arion doesn't assert capabilities names either perhaps it's not necessary given a nonexistent capability will cause the service to fail to start
|
@ofborg test oci-containers |
|
@ofborg test oci-containers |
1 similar comment
|
@ofborg test oci-containers |
|
@roberth I tried adding other reviewers but it didn't work for some reason, can you add people more suited to review this? |
|
Ah sorry I haven't looked at this - I'm about to be offline on a sailboat for a week, but if this is still pending when I get back to land (approximately Nov 21) I will review it then. |
|
@benley Hey, did you manage to get back to land yet? |
Yethal
force-pushed
the
more-configuration-options-for-podman
branch
from
4326376 to
5a33c10
Compare
| capAdd = mkOption { | ||
| type = with types; lazyAttrsOf (nullOr bool); | ||
| default = { }; | ||
| description = '' | ||
| Capabilities to add to container | ||
| ''; | ||
| example = literalExpression '' | ||
| { | ||
| SYS_ADMIN = true; | ||
| { | ||
| ''; | ||
| }; | ||
|
|
||
| capDrop = mkOption { | ||
| type = with types; lazyAttrsOf (nullOr bool); | ||
| default = { }; | ||
| description = '' | ||
| Capabilities to drop from container | ||
| ''; | ||
| example = literalExpression '' | ||
| { | ||
| SYS_ADMIN = true; | ||
| { | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
Couldn't these be merged into a single capabilities option?
true-> addfalse-> removenullor omitted -> default
There was a problem hiding this comment.
That's technically possible, but in my humble opinion it would be a more confusing interface. Part of this module's appeal is that it is a fairly thin abstraction over the underlying container runner implementations; podman and docker (and their various documentation and examples) have separate --cap-add and --cap-drop options so it seems reasonable for us to do the same.
There was a problem hiding this comment.
Various declarative wrappers over Docker/podman (Ansible, docker-compose etc) also use separate cap_add and cap_drop options
There was a problem hiding this comment.
A practical benefit of modeling it that way is that you can override the capabilities using the module system.
It is also conceptually simpler, because you don't have to make users bend to the quirks of an listOf str-typed CLI. That's just not a good data model. I shouldn't be able to express this, because it has no clear meaning:
capAdd.SYS_ADMIN = false;
capDrop.SYS_ADMIN = false;
Various declarative wrappers over Docker/podman (Ansible, docker-compose etc) also use separate cap_add and cap_drop options
Arion - a declarative docker-compose wrapper - does not, and I've given that as an example.
I haven't heard of another wrapper; would like to know more.
Apologies if I come across a bit sour. GitHub marked my earlier comment as outdated, so I'll blame them for cutting the conversation short :)
I'd be happy to propose a refactor.
There was a problem hiding this comment.
I shouldn't be able to express this, because it has no clear meaning:
I agree in principle that this makes no sense but I feel like this is more of a philosophical question, do we want to have the most correct interface or the one that's going to feel the most familiar for users coming in from other ecosystems. Is there a process for asking the community to vote on something (other than starting a thread on Discourse)? If the users decide they prefer the more correct approach vs the more compatible one I'd be happy to rewrite this
There was a problem hiding this comment.
We don't have a defined process for this, and I believe that's a good thing.
Despite occasionally being the right thing to ask, "we need more data" is also a really good way to kill an effort.
Also data is not enough to drive development. We also need "human" factors like reasoning and creative thinking to make that work.
I think the crux of the disagreement here is whether or not we should "improve" the things when we wrap or integrate them.
This isn't unique to these flags. It happens all the time. At a large scale we resolve this tension by having both Nixpkgs and NixOS, but even within NixOS, we have multiple layers of abstraction, defined not so much by software component boundaries, but by a causal relationship between options. For instance, we have services.something options, which affects systemd options, which affect environment.etc. This is good, because we can't make assumptions about the level of abstraction users need to work with, and each "layer" can generally be worked with simultaneously without creating conflict.
A similar pattern is possible with RFC-42 style settings, where a service option can effect a high level, perhaps NixOS-specific behavior by contributing to the settings option.
In this situation, the settings correspond directly to an existing configuration format, so they cover the need for the config to be easy to manipulate for existing users of that service.
In this case, the extraOptions cover that need.
RFC 42 provides a recommendation about option quantity
Quality is encouraged over quantity: Authors should spend more time on writing documentation, NixOS tests or useful high-level abstractions.
By adding support for overriding, we make these options/option more useful, and we abstract away the ambiguous and incorrect inputs.
(Also we probably have a bug if the backend doesn't throw an error when a capability is both added and removed. Haven't checked.)
There was a problem hiding this comment.
Something like this?
#363574
There was a problem hiding this comment.
You know what, after reading #363574 I think I actually withdraw my objection. That interface and option description make it quite understandable.
Added six new configuration options to containerOptions
pullPull policy for the containercapAddCapabilities to add to the containercapDropCapabilities to drop from the containerdevicesDevices to pass through to the containernetworksNetworks container should be connected toprivilegedWhether container should run in privileged modeThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.