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.

Sign up for GitHub

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

Jump to bottom

nixos/traefik create service #29865

Merged
merged 2 commits into from
Oct 4, 2017
Merged

nixos/traefik create service #29865

merged 2 commits into from
Oct 4, 2017

Conversation

moredhel
Copy link
Member

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@globin
Copy link
Member

globin commented Sep 27, 2017

You might want to look at https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/continuous-integration/gitlab-runner.nix#L7-L17 to generate TOML from NixOS options.

Mic92
Mic92 reviewed Sep 28, 2017
View reviewed changes
nixos/modules/services/web-servers/traefik.nix Outdated
after = [ "network-online.target" ];
wantedBy = [ "multi-user.target" ];
serviceConfig = {
ExecStart = ''${cfg.package.bin}/bin/traefik -l debug --configfile=${configFile}'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is debug a good default for logging?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I forgot to remove that. I had it there while debugging.

Mic92
Mic92 reviewed Sep 29, 2017
View reviewed changes
nixos/modules/services/web-servers/traefik.nix Outdated

config = mkOption {
default = "";
example = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @globin that the using remarshal to convert nix to toml would be an a nice enhancement here.

@moredhel moredhel requested a review from FRidh as a code owner October 1, 2017 12:59
@moredhel
Copy link
Member Author

moredhel commented Oct 1, 2017

Thank you @globin & @Mic92 for the constructive feedback. I've changed the configuration to use remarshal allowing for considerably easier configuration.

Mic92
Mic92 reviewed Oct 1, 2017
View reviewed changes
nixos/modules/services/web-servers/traefik.nix
options.services.traefik = {
enable = mkEnableOption "Traefik web server";

configFile = mkOption {
Copy link
Member

@Mic92 Mic92 Oct 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your use case for this option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had forgotten to update the example to accurately portray the expected type: https://github.com/NixOS/nixpkgs/pull/29865/files#diff-beb91534bbf628b6e0727a0bb7d14c4aR25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean in which case it is necessary to specify a toml path directly rather then using the configOptions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have it if I am doing an incremental change over to nixos from my current orchestration system for configuration files.

The other issue is secrets, which one may want to store in a separate store. This can be worked around for sure, but this gives people an alternative, less invasive route to initially integrating Traefik.

Are there any specific reasons why you think we shouldn't have this option?
Reasons that come to mind:

  • complexity
  • against the ethos of NixOs
  • allowing bad habits & configuration drift

For:

  • already complex setups with templates & config-file generation.
  • lower barrier for entry to users wanting to keep using traefik as-is with minimal overhead.

These issues aren't unsolvable, in fact nix is very good at solving them.
I'm happy to remove it if you think it won't find any use.

Copy link
Member

@Mic92 Mic92 Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want keep the API we expose simple as we have to deal every option stable in future to not break configuration. But secrets is a good point for storing the configuration outside of nix store: https://docs.traefik.io/configuration/backends/web/#basic-authentication

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think then? We can keep it in pending #8 and deprecate it when secrets are properly managed within Nix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, #24288. Wrong link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can stay then, I would say.

@moredhel
Copy link
Member Author

moredhel commented Oct 3, 2017

It seems that paths are validated. What path should I use as an example?

@Mic92
Copy link
Member

Mic92 commented Oct 3, 2017 •
edited
Loading

There is a function called literalExample which can be used to prevent evaluation.

@Mic92 Mic92 closed this Oct 3, 2017
@Mic92
Copy link
Member

Mic92 commented Oct 3, 2017

closed by accident.

@Mic92 Mic92 reopened this Oct 3, 2017
@moredhel
Copy link
Member Author

moredhel commented Oct 4, 2017

I have stripped out the file option for configuring Traefik. How does it look?

Mic92
Mic92 reviewed Oct 4, 2017
View reviewed changes
nixos/modules/services/web-servers/traefik.nix Outdated
ExecStartPre = [
''${pkgs.coreutils}/bin/mkdir -p "${cfg.dataDir}"''
''${pkgs.coreutils}/bin/install -d -m700 --owner traefik --group traefik "${cfg.dataDir}"''
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to make acme work by default.

Mic92
Mic92 reviewed Oct 4, 2017
View reviewed changes
nixos/modules/services/web-servers/traefik.nix Outdated
configOptions = mkOption {
description = ''
Config for Traefik.
'';
type = types.attrs;
default = {
defaultEntryPoints = ["http"];
entryPoints.http.address = ":80";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this configuration does nothing it is good to have a nice to have a running example by default (Even if it just for testing new versions of traefik).

@Mic92
Copy link
Member

Mic92 commented Oct 4, 2017

I restored the configFile option for secrets this might be required.

@Mic92 Mic92 merged commit b8288f1 into NixOS:master Oct 4, 2017
@Mic92
Copy link
Member

Mic92 commented Oct 4, 2017

Thanks for your work on this module!

@Mic92
Copy link
Member

Mic92 commented Oct 4, 2017

followup: a320034

@moredhel
Copy link
Member Author

moredhel commented Oct 4, 2017

Thanks @Mic92, helpful and constructive feedback as always 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moredhel @globin @Mic92 @joachifm