nixos/traefik: update and revamp module - #553100
Conversation
4fa674a to
cdd1f41
Compare
cdd1f41 to
9ae459f
Compare
9ae459f to
4ff7df0
Compare
|
Could you please elaborate why you're using those default options which introduce so much evaluation logic (you call it "glue") on the option level instead of having "dumb" options and merge conditionally on the config level? Don't get me wrong, this is a serious question as I tried to wrap my head around it since the #490985 PR and just don't understand the reason behind it. To me it feels like it makes things much more complicated than they need to be. And it feels unintuitive to my understanding of options as the declaration- and config as the evaluation-layer. I think all that glue could be moved to the config layer and it would simplify reading the module code a lot. |
It's for a few reasons, but part of why I took this approach is because Traefik is somewhat unique in the way it handles configuration - it has several different providers, and each can provide configuration in one way or another. All of these providers, however, eventually boil down to the Since Stepping back from the development perspective, think about an end user. Imagine the user doesn't know how to read Nix source code yet, and is migrating from another platform like docker. If you were that user, would you prefer to:
While this is a cherry-picked scenario, I've spent enough time learning enough different things in life from scratch to know that having the information right there can make all the difference. Is it more work on the part of the module maintainers? Yes. But I think it's better for the maintainer to put in a little extra work so that every single person who ends up using the module has a better experience.
No worries. I realize this approach is slightly unorthodox, but I genuinely think it is a better way of doing it.
Not sure I can add much here that I haven't said elsewhere.
It would, but as I mentioned above that's not necessarily a good thing.
At risk of repeating myself, using the module might be more intuitive if you can read the source code.
(Warning: pseudocode below) I'm genuinely curious; which examples are you referring to? I checked while I was developing this, and couldn't find anything indicating this was the case. In my time using Traefik with docker, I can only ever recall one instance of using Said option has since been removed (it is now enabled by default), and I have never encountered a situation in which an empty value has been required.
This is one of the other reasons I took a different approach - the This provides a number of benefits when actually using Traefik as a proxy - each service has its own file, which can be inspected, copied and modified imperatively, all while (hopefully) seamlessly meshing with the rest of NixOS. Declarative is great, but the reason the routing provider exists is so that changes in it don't require a restart of the primary daemon. My implementation takes it a step further by not requiring a I do admit that this is a potential downside, but unless there is significant use of empty values I don't think it's an issue. If so, there's some trickery I can pull to differentiate empty option defaults from empty values set by the user Misc benefits:
|
|
I didn't forget you, just didn't have time to answer yet! Thank you very much for the detailed answer. |
4ff7df0 to
6d43489
Compare
|
I have removed the following assertions: {
# TODO ensure this works with install.settings being a submodule
assertion =
opt.install.file.highestPrio != defaultOptPrio
-> opt.install.settings.highestPrio == defaultOptPrio;
message = ''
The 'services.traefik.install.file' and 'services.traefik.install.settings' options are mutually exclusive.
It is recommended to use 'settings'.
'';
}
(
let
isEmpty = a: (a == { } || a == [ ] || a == null);
in
{
assertion =
(opt.install.file.highestPrio != defaultOptPrio)
-> (builtins.all isEmpty [
cfg.routing.extraFiles
cfg.routing.dir
cfg.routing.file
cfg.routing.settings
]);
message = ''
None of the routing configuration options may be used if Traefik is being managed imperatively.
The following options have non-default values:
- ${
concatMapStringsSep "\n - " (str: "'services.traefik.routing.${str}'") (
filter (attr: !(isEmpty cfg.routing."${attr}")) [
"extraFiles"
"dir"
"file"
"settings"
]
)
}
'';
}
)And the now-unused inherit (lib)
concatMapStringsSep
filter
opt = options.services.traefik;
defaultOptPrio = (lib.mkOptionDefault { }).priority;...Since I realized they unintentionally prevented the case of a user overriding the merge logic by setting the relevant option. These could be reintroduced as warnings or have an |
All good! Are you able to comment on the empty attribute set question? |
cb7889a to
ee334a0
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/7429 |
ee334a0 to
69bfcab
Compare
|
I have updated the module I use on my server from an older version of this PR, which I have been running for the past two years. The migration worked flawlessly, though it didn't hit the To use the module from this PR, import it with the following changes. meta = {
maintainers = with lib.maintainers; [
jackr
therealgramdalf
];
- doc = ./traefik.md;
+ # Fix eval error on local copy
+ #doc = ./traefik.md;
};let
...
in
{
+ # Replace the upstream module
+ disabledModules = [ "services/web-servers/traefik.nix" ];
}With this, I have manually tested a few things:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
|
Sorry for the delay!
These are the usages of empty curly braces I found:
From my understanding the thing you most care about is end user usability. And you're right with that. Also I agree, that I'd like to work through the Filtering Not a blocker, I just think it hurts usability and you could easily work around it by moving the This would simplify your implementation a lot as you can easily differentiate the user defined options from the default options.
Unfortunately my time is limited today, so I'll just drop a discoverability comparison of your option interface and mine.
Mine still contains the plugin logic which was part of #490985, so the comparison is not fully clean.
I don't know if this was possible with an earlier version of your PR, but now All in all this comes down to two design styles: I model each file-provider scenario as its own typed option. |
I did a search on github and it seems there are people using empty attribute sets in their configurations (these results do not include
Yes. I would like the module to be powerful, readable, and maximally flexible - Traefik is used in some pretty wacky setups, and I would like the module to support as many of those as possible on the configuration layer - let the user tweak things, don't let what the module does get in the way.
I do agree it does hurt usability. I personally think this is just bad design on the part of Traefik (or perhaps a consequence of yaml), but I don't think it's likely to change.
The other approaches I've considered:
The thing I dislike about a variable is that it has to be either documented manually or the user needs to be able to read the source code. More info below.
I must have accidentally changed that. I'll add that back in on my next round of fixes.
While it doesn't make as big of a difference for the |
69bfcab to
02e1d69
Compare
|
Added an assertion preventing empty attribute sets from being used in the config. This should notify users that there is an issue while allowing the filter logic to remain functional, since empty attribute sets are only exposed once the This could still be improved to show offending locations, but I have implemented it as a simple check for now. |
Updated the Traefik module to modernize it. Primary changes:
useEnvSubstin favor ofenvironmentFile*ConfigOptionsand*ConfigFileto*.settingsand*.filedynamictoroutingandstatictoinstallto remain consistent with upstreamrouting.extraFilesNote: I have been running an older variant of this PR on my server for the past two years without issue. It (primarily the extraFiles functionality) has worked seamlessly ever since, surviving many updates and continuing to proxy a slew of services. I have just updated the module to the version in this PR, and have not run into any issues. See the comment below.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Full changelog
The following changes have been implemented:
mkDefaultto match thekuboandcaddymodulesenvsubst)useEnvSubstas a breaking changedockerprovidersystemd-nspawncontainers, which are much faster. Multiple tests are run in parallel, using different configuration methodsmkRenamed/mkRemovedoption modulesenableoption description, which incorrectly called Traefik a web serverunitConfig.documentationlinknotify, allowing for more accurate unit statusProtectSystem = "strict"LimitNPROCtoTasksMax1, not64- I haven't tested if that works or not. Upstream systemd service hasn't been updated in 6 yearscfg.useroptionmetaattributes to the modulestaticanddynamicconfiguration to the newinstallandroutingterminologysettingsandfileinstead of<method>ConfigOptionsand<method>ConfigFilepkgs.formats.jsoninstead oftomlcfg.userandcfg.group(matching theredisserver options)supplementaryGroupsto give access to thedockerdaemon instead of setting the primary groupenvironmentFilesfor usage in place ofenvSubstroutingorinstallcould be considered "vendored" options - they add extra glue to make the module feel better to usedefaultTextorreadOnlyoptionsinstall.settingsnow houses most of themkIflogic that adds thefileproviderinstall.settingsis a submodule withfreeformtype.install.filenow contains the logic to generate the install configuration frominstall.settings[]), attribute sets ({}), or null values (null) are now filtered out in the generation logic to allow definition of typed options with default valueslet ... inblock at the top of the module - it was not really documented anywheretraefik.enableoptions asrouting.extraFiles, and it will work with eitherrouting.extraFilesorrouting.settingsreadOnlyoption,routing.settingsDrv, is used to merge everything together.What remains to be done:
Final Checks - just before merge