Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0042] NixOS settings options #42

Merged
merged 44 commits into from Jan 14, 2021
Merged

[RFC 0042] NixOS settings options #42

merged 44 commits into from Jan 14, 2021

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Mar 23, 2019

Summary

Part 1: Structural settings instead of stringly extraConfig

NixOS modules often use stringly-typed options like extraConfig to allow specifying extra settings in addition to the default ones. This has multiple disadvantages: The defaults can't be changed, multiple values might not get merged properly, inspection of it is almost impossible because it's an opaque string and more. The first part of this RFC aims to discourage such options and proposes generic settings options instead, which can encode the modules configuration file as a structural Nix value. Here is an example showcasing some advantages:

{
  # Old way
  services.foo.extraConfig = ''
    # Can't be set in multiple files because string concatenation doesn't merge such lists
    listen-ports = 456, 457, 458 
    
    # Can't override this setting because the module hardcodes it
    # bootstrap-ips = 172.22.68.74

    enable-ipv6 = 0
    ${optionalString isServer "check-interval = 3600"}
  '';
  
  # New way
  services.foo.settings = {
    listen-ports = [ 456 457 458 ];
    bootstrap-ips = [ "172.22.68.74" ];
    enable-ipv6 = false;
    check-interval = mkIf isServer 3600;
  };
}

Jump to the detailed design of part 1

Part 2: Balancing module option count

Since with this approach there will be no more hardcoded defaults and composability is not a problem anymore, there is not a big need to have NixOS options for every setting anymore. Traditionally this has lead to huge modules with dozens of options, each of them only for a single field in the configuration. Such modules are problematic because they're hard to write, review and maintain, are generally of lower quality, fill the option listing with noise and more. Additional options aren't without advantages however: They are presented in the NixOS manual and can have better type checking than the equivalent with settings.

The second part of this RFC aims to encourage module authors to strike a balance for the number of additional options such as to not make the module too big, but still provide the most commonly used settings as separate options. Quality is encouraged over quantity: Authors should spend more time on writing documentation, NixOS tests or useful high-level abstractions. This is in contrast to the fiddly labor of copying dozens of options from upstream to NixOS. With a settings option, it's also very easy to add additional options over time if the need arises. In contrast, removing options has always been nigh impossible.

Jump to the detailed design of part 2

Rendered

Write (not-so-)detailed design and drawbacks

Adjust formatting

Expand Additional config options

Add section for configuration types

Add format writers and documentation sections

Adjust additional options example

Add some more links and change name

Move file
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
@timokau
Copy link
Member

@timokau timokau commented Mar 23, 2019

I think I like this overall. Documentation is important here. Also I'd like to keep the extraConfig escape hatch.

One (admittedly minor) drawback is that it can feel a little bit uncanny-valley like when the upstream configuration options don't adhere to nix conventions like starting with a lowercase letter (NixOS/nixpkgs#57554 (comment)).

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Mar 23, 2019

@timokau

Also I'd like to keep the extraConfig escape hatch.

I'd rather not if possible, because then we have problem of not being able to merge values set in extraConfig with all other values, there are essentially two incompatible ways of specifying configuration then. I didn't think of this earlier (I'll add it to the RFC), but with a config option, you can even do this which alleviates the need for extraConfig (for formats we have a parser for at least):

{
  services.foo.config = builtins.fromJSON ''
    {
      "logDir": "/var/log/foo"
      # ...
    }
  '';
}

If the user really doesn't want to use the modular config, what I suggest instead is to have a configFile option as an escape hatch, which when set, overrides the file generated by the config option. This also has the advantage that the user can just set that option to something stateful if they wish, like "/var/lib/foo/config.json" and manage it outside of NixOS.

@timokau
Copy link
Member

@timokau timokau commented Mar 23, 2019

Also I'd like to keep the extraConfig escape hatch.

I'd rather not if possible, because then we have problem of not being able to merge values set in extraConfig with all other values, there are essentially two incompatible ways of specifying configuration then.
[...]
If the user really doesn't want to use the modular config, what I suggest instead is to have a configFile option as an escape hatch

The advantage of extraConfig is that the user can keep a standard format config file around (which can also be used outside of nixos) and just import that with readFile. But nix can still set some options, e.g. when another module needs to do some configuration on this module.

That mostly makes sense for rc-type configs, less for json I guess.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Mar 23, 2019

On the other hand, JSON can actually be imported with fromJSON

Ideally, declarative config formats could be parsed and merged, but for imperative configs indeed extraConfig makes sense.

Copy link
Member

@Ma27 Ma27 left a comment

Thanks a lot for bringing this up! After reading through the RFC I'm confident that the overall idea is a good thing to do, both from a collaborator's and user's perspective 👍

Below are some thoughts and comments from my side.

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
# Set minimal config to get service working by default
services.foo.config = {
dataPath = "/var/lib/foo";
logLevel = mkDefault "WARN";

This comment has been minimized.

@Ma27

Ma27 Mar 23, 2019
Member

I just thought a little bit about this and I personally think that one would like to see some kind of mechanism that prints all default values into the summary of config. But IIRC only config options are evaluated when generating the manual, so I'm not even sure if that's possible without too much effort.

This comment has been minimized.

@Infinisil

Infinisil Mar 24, 2019
Author Member

That would certainly be nice in the future, I feel like it should be possible, by just determining the default by actually evaluating the options value with an empty configuration.nix, instead of just looking at the options default attribute.

This comment has been minimized.

@Ma27

Ma27 Mar 24, 2019
Member

That's a good idea and would be something I'd probably help to implement. How about adding this to "Future work" for now? :)

This comment has been minimized.

@rycee

rycee Apr 7, 2019
Member

I think it is pretty important that the defaults in use should show up in the documentation. Perhaps the mkOption function could be adjusted to allow setting a priority of the default option? Then you could have something like

mkOption {
  default = {
    logLevel = "WARN";
    port = 2546;
  };
  defaultModifier = def: mapAttrs (n: mkOptionDefault) def;
  # …
}

To have user config merged into the option default value unless the user applies mkForce.

I haven't thought this through very carefully, though, so I might have missed some point causing this not to work.

But this may be an easier way to solve this documentation issue instead of attempting to evaluate an "empty" configuration. A difficulty, for example, is that we have to figure out how to actually get the config option to be populated. In the foo module, for example, you would have to know that the enable option should be set to true.

This comment has been minimized.

@Infinisil

Infinisil Apr 8, 2019
Author Member

How about something like

mkOption {
  default = lib.noPriority (mapAttrs (n: mkOptionDefault) {
    logLevel = "WARN";
    port = 2546;
  });
}

The manual could generate

default = {
  logLevel = <default> "WARN";
  port = <default> 2546;
}

But then there's also the problem with defaults that depend on other options, these can't be in the manual. And then the default declarations would have to be split up in the module itself.

And there's also the problem with serializing nix values from nix itself. We can't properly escape stuff, and multi-line strings won't work, and interpolated values, derivations.. I feel like we almost need some Nix language change to make this work properly.

And then there's also the discussion on which defaults should be seen by the user at all. Does it make sense that logType = "syslog" should be in the manual for the user to see? Probably not. See the Defaults section of this RFC as well, the first type of default specifically shouldn't be seen by the user.

I guess the simplest solution is to just say "A reasonable default base configuration is set in order for the module to work, see for details". We already link to the module files already anyways.

Not entirely sure what to do about this. But then again, having this problem is nothing inherent to this RFC, lots of modules already set defaults in this way invisible to users.

This comment has been minimized.

@pacien

pacien Jun 18, 2019

Is there any advantage in using mkDefault to override some configuration keys over using apply = recursiveUpdate default in mkOption like here?

      config = mkOption rec {
        type = types.attrs;
        apply = recursiveUpdate default;
        default = {
          ircService = {
            databaseUri = "nedb://${dataDir}/data";
            passwordEncryptionKeyPath = "${dataDir}/passkey.pem";
          };
      };

This has the advantage of already showing the default values in the documentation.

This comment has been minimized.

@Infinisil

Infinisil Jul 14, 2019
Author Member

While this apply way doesn't seem too bad, I don't think it's a good idea because it completely sidesteps the module system, which leads to some unexpected behavior. E.g. in your example if a user sets config.ircService = mkForce {}, this doesn't do anything, because recursiveUpdate doesn't know not to recurse over that.

This comment has been minimized.

@jtojnar

jtojnar Jan 9, 2020

Could we add addDefaultAnnotation = annotation: content: { _type = "annotation:default"; inherit content; }; and let the documentation builder use that for describing the value in the manual?

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
Infinisil added 3 commits Mar 24, 2019
…ions
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
Infinisil added 2 commits Mar 24, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Mar 27, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/open-for-nomination-rfc-0042-nixos-config-options/2538/1

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
Infinisil added 2 commits Mar 27, 2019
@danbst

This comment was marked as off-topic.

@Infinisil

This comment was marked as off-topic.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Apr 1, 2019
With `sshd -t` config validation for SSH is possible. Until now, the
config generated by Nix was applied without any validation (which is
especially a problem for advanced config like `Match` blocks).

When deploying broken ssh config with nixops to a remote machine it gets
even harder to fix the problem due to the broken ssh that makes reverts
with nixops impossible.

This change performs the validation in a Nix build environment by
creating a store path with the config and generating a mocked host key
which seems to be needed for the validation. With a broken config, the
deployment already fails during the build of the derivation.

The original attempt was done in NixOS#56345 by adding a submodule for Match
groups to make it harder screwing that up, however that made the module
far more complex and config should be described in an easier way as
described in NixOS/rfcs#42.
@Mic92 Mic92 mentioned this pull request Dec 17, 2020
10 of 10 tasks complete
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Dec 17, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-5/10560/1

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 18, 2020

I have now cleaned up the RFC, removing old bits and pieces that aren't needed anymore with freeform modules. Would appreciate a final round of reviews, so that we can merge this soon!

@Mic92
Mic92 approved these changes Dec 18, 2020
Copy link
Contributor

@Mic92 Mic92 left a comment

I think this is good to go.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Jan 5, 2021

I would move this to FCP if @edolstra, @fpletz and @rycee agree.

@rycee
Copy link
Member

@rycee rycee commented Jan 5, 2021

@Mic92 Agreed!

@edolstra
Copy link
Member

@edolstra edolstra commented Jan 7, 2021

👍 on FCP.

fpletz has not been available for meetings, nor to approve FCP. Given that the other three shepherds are in agreement that this should move to FCP, the RFC steering committee has decided to remove fpletz from the shepherd team in order to make progress without implying that he has approved it.
@lheckemann
Copy link
Member

@lheckemann lheckemann commented Jan 7, 2021

The steering committee has agreed to proceed without fpletz's approval :)

Copy link

@roberth roberth left a comment

I'm glad that this ambitious rfc is nearing its finalization.

Thank you @Infinisil for persevering, for 22 months as of today.

I've had the honor of reviewing the crucial PRs to the module system that underpin this rfc, like this one, which - despite their apparently simple goals - have taken quite a journey to complete.
Your efforts to improve the module system are greatly appreciated.

@Mic92 Mic92 merged commit 45b76f2 into NixOS:master Jan 14, 2021
@Ma27
Copy link
Member

@Ma27 Ma27 commented Jan 14, 2021

@Infinisil congratulations on getting this through! And also thanks a lot for your incredible effort here!

@Mic92 Mic92 added status: accepted and removed status: FCP labels Jan 14, 2021
@ryantm
Copy link
Member

@ryantm ryantm commented Jan 14, 2021

I'm happy this was merged, but sad that the FCP was not advertised widely like it should be. There isn't a post on Discourse about it which I feel like should be the bare minimum. Am I missing some other channel it was announced aside from here on GitHub?

Also, it looks like maybe the FCP only lasted 7 days, which is less than the 10 specified in RFC 36.

@piegamesde
Copy link

@piegamesde piegamesde commented Jan 14, 2021

This would be stuff for a new MSC, but Matrix and Rust for example have a weekly recap blog that also announces FCPs and new RFCs (among other things). People can then easily subscribe to it or simply follow some account on Mastodon/Twitter.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 15, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-42-nixos-settings-options-has-been-accepted/10992/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.