consul: rewrite nixos module #107798

cpcloud · 2020-12-28T13:16:11Z

Motivation for this change

This is a major simplification of the consul service exposed in nixos.

The primary motivation was to match the systemd unit settings provided by Hashicorp here.

Additionally, this service was previously discovering host IP addresses in an unnecessarily complex way, when Consul supports using go-sockaddr templates in configuration.

Things done

cpcloud · 2020-12-28T13:24:14Z

This PR has some backwards incompatible changes to the module's configuration. I am happy to adjust that if necessary.

aanderse · 2020-12-28T13:37:54Z

The primary motivation was to match the systemd unit settings provided by Hashicorp here.

Great. Maybe you can submit a PR to actually include that systemd unit in the upstream code repository. If upstream maintains and ships a systemd unit we can utilize this unit via systemd.packages and automatically stay (mostly) in sync with the upstream unit.

nh2 · 2020-12-28T14:38:55Z

we can utilize this unit via systemd.packages and automatically stay (mostly) in sync with the upstream unit.

Is it explains somewhere how that works, e.g. will it still be overridable in NixOS?

nixos/modules/services/networking/consul.nix

aanderse · 2020-12-28T14:59:28Z

I'm not sure. I don't see it in the manual... It is fairly straightforward, though. If pkgs.consul has $out/lib/systemd/system/*.service then these will be included in the resulting system if you specify systemd.packages = [ pkgs.consul ]; You can then specify customization/overrides like so:

systemd.services.consul = {
  restartTriggers = settingsFiles;
  serviceConfig = {
    ExecStart = [ "" "${cfg.package}/bin/consul agent"
      + concatMapStrings (file: " -config-file=${file}") settingsFiles ];
    StateDirectory = "consul";
    ExecStop = mkIf cfg.leaveOnStop "${cfg.package}/bin/consul leave";
  };
};

The idea is to get as much upstream as possible:

free review from upstream
minimize customization in NixOS module in favor of PRs upstream
easy to keep in sync with upstream unit
other distros benefit
etc...

As a simple example we want StateDirectory = "consul"; so we should propose the addition of this in the upstream unit, instead of the NixOS module.

cpcloud · 2020-12-28T15:13:11Z

The primary motivation was to match the systemd unit settings provided by Hashicorp here.

Great. Maybe you can submit a PR to actually include that systemd unit in the upstream code repository. If upstream maintains and ships a systemd unit we can utilize this unit via systemd.packages and automatically stay (mostly) in sync with the upstream unit.

I agree with you here. Is an upstream unit a blocker to get this PR merged? This service hasn't seen any maintenance in NixOS for a while, so I do think there's benefit to getting this in before an upstream unit is merged.

aanderse · 2020-12-28T15:16:40Z

I agree with you here. Is an upstream unit a blocker to get this PR merged? This service hasn't seen any maintenance in NixOS for a while, so I do think there's benefit to getting this in before an upstream unit is merged.

Most certainly not, but if you circle back around to that later you get full bonus points ✨
You also might want to consider listing yourself as a maintainer in the NixOS module, if you have interest.

nh2 · 2020-12-28T15:32:42Z

Thanks for the PR! I will make a bit longer comment on the after work, if I can fit it in today.

nh2

consul: rewrite nixos module

This commit message will need some significant expansion:

I know multiple production setups where Consul is a critical infrastructure part that will go down if something in the Consul NixOS module breaks (including my own), so it would be great if every significant change to the module could be clearly outlined in the commit message.

I will try to point at some of them in the in-code comments with Significant enough for changelog entry.

The user-visible changes also need to go into a NixOS release notes entry (the equivalent of nixos/doc/manual/release-notes/rl-2103.xml).

I am also wondering if it makes sense to keep the old module for 1 release, in addition to the new module proposed here (e.g. keep the old one as services.consul_deprecated or so), so that people who use this in production servers have an easier migration path. I don't know if there is something similar in nixpkgs for service modules yet, I'm just quite sure that this will be a risky upgrade for my org at least. (So I'm not requesting this as a change here yet but would like to know what the PR author and other nixpkgs/consul users think.)

nh2 · 2020-12-28T19:53:08Z

nixos/modules/services/networking/consul.nix

+              + concatMapStrings (file: " -config-file=${file}") settingsFiles;
+            ExecReload = "${pkgs.coreutils}/bin/kill --signal HUP $MAINPID";
+            ExecStop = optionalString cfg.leaveOnStop "${cfg.package}/bin/consul leave";
+            DynamicUser = cfg.dropPrivileges;


I believe that the description of dropPrivileges should now be slightly adjusted to reflect this.

nh2 · 2020-12-28T19:53:18Z

nixos/modules/services/networking/consul.nix

  };

  config = mkIf cfg.enable (
-    mkMerge [{
-
-      users.users.consul = {


Significant enough for changelog entry

nh2 · 2020-12-28T20:06:42Z

nixos/modules/services/networking/consul.nix

-
-      systemd.services.consul = {
-        wantedBy = [ "multi-user.target" ];
-        after = [ "network.target" ] ++ systemdDevices;


this service was previously discovering host IP addresses in an unnecessarily complex way, when Consul supports using go-sockaddr templates in configuration

It's unclear to me whether go-sockaddr does the equivalent of the removed ++ systemdDevices, does anyone know more?

nh2 · 2020-12-28T20:16:16Z

nixos/modules/services/networking/consul.nix

-        bindsTo = systemdDevices;
-        restartTriggers = [ config.environment.etc."consul.json".source ]
-          ++ mapAttrsToList (_: d: d.source)
-            (filterAttrs (n: _: hasPrefix "consul.d/" n) config.environment.etc);


Significant enough for changelog entry (the no-longer-using of consul.d)

In particular, this changes how users of the module need to write modular configs.

For example, a common way how we would define additional checks so far was to create more consul.d JSON files, e.g. like so:

{ systemd.services.consul.restartTriggers = [ config.environment.etc."consul.d/nginx-service.json".source ]; environment.etc."consul.d/nginx-service.json" = { text = builtins.toJSON { service = { name = "nginx"; checks = [ { id = "nginx-status-page"; name = "nginx status page"; http = "http://localhost:80/nginx_status"; interval = "1s"; timeout = "1s"; } ]; }; }; }; }

I think we need to explain how this is to be translated to the new setup.

I guess there are 2 ways to do it:

cfg.settingsFiles

translating them to checks = [ ] as described in https://www.consul.io/docs/discovery/checks#multiple-check-definitions and putting them into cfg.settings.

Does one of these have more benefits / should we recommend one over the other?

For the second approach, does type = format.type; take care to concatenate lists if set from multiple different NixOS modules?

nh2 · 2020-12-28T20:17:43Z

nixos/modules/services/networking/consul.nix

@@ -40,15 +29,6 @@ in
        '';
      };

-
-      webUi = mkOption {


Instead of simply removing the config. options, I think it would be better to make use of them emit an explicit error message, that explains what to use instead (can be removed in the subsequent NixOS release).

nh2 · 2020-12-28T20:18:38Z

nixos/modules/services/networking/consul.nix

@@ -40,15 +29,6 @@ in
        '';
      };

-
-      webUi = mkOption {


I believe nixos/tests/consul.nix needs to be adjusted to still evaluate given this change.

nh2 · 2020-12-28T20:36:12Z

The NixOS tests fail, which is misleading:

The Github action https://github.com/NixOS/nixpkgs/pull/107798/checks?check_run_id=1617881367 says

Success

Attempted: consul

The following builds were skipped because they don't evaluate on x86_64-linux: consul.passthru.tests

So it says success when in fact it wasn't successful.

What can we do against that in general?

From https://logs.nix.ci/?key=nixos/nixpkgs.107798&attempt_id=2ae55234-6480-4039-a2ae-1214c7ec5326 the actual failure output is:

Cannot nix-instantiate `consul.passthru.tests' because:
...
The option `services.consul.extraConfig' does not exist.

nixos-discourse · 2020-12-28T20:42:21Z

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

https://discourse.nixos.org/t/can-we-make-ofborg-show-broken-tests-as-red/10717/1

cpcloud · 2020-12-28T22:44:46Z

@nh2 Thanks for your review! I think this PR is probably easier to review and get merged if I split it into a number of smaller ones:

Use the systemd unit provided by upstream (well, the consul website) and move to dynamic user creation.
Replace the ip usage with go-sockaddr templates.
Deprecate extraConfig(Files)? in favor of settings(Files)?
Remove the interface config altogether.
Optionally remove the directory usage (I personally don't mind keeping that)

cpcloud · 2020-12-28T22:45:25Z

In light of that I'll close this one out and start on the split.

aanderse · 2020-12-28T23:07:47Z

I am also wondering if it makes sense to keep the old module for 1 release, in addition to the new module proposed here (e.g. keep the old one as services.consul_deprecated or so), so that people who use this in production servers have an easier migration path.

Since NixOS is so flexible I don't think there is any value in that. You can just import the module from 20.09 and continue with that.

nh2 · 2020-12-28T23:22:29Z

Since NixOS is so flexible I don't think there is any value in that. You can just import the module from 20.09 and continue with that.

@aanderse Hmm, I guess that is true. You mean like in here, right?

nh2 · 2020-12-28T23:27:38Z

I think this PR is probably easier to review and get merged if I split it into a number of smaller ones

@cpcloud Not sure if it's easier/faster, as it'll require more roundtrips, and it won't allow people to see the entire plan at once as a single commit or multiple commits in a single PR would, but I'll try to review any approach you pick.

Use the systemd unit provided by upstream (well, the consul website) and move to dynamic user creation.

Replace the ip usage with go-sockaddr templates.

Deprecate extraConfig(Files)? in favor of settings(Files)?

Remove the interface config altogether.

Optionally remove the directory usage (I personally don't mind keeping that)

Do you want to do the release notes at the end or also add them step by step (just so I know whether I should check for them in the individual PRs)?

aanderse · 2020-12-28T23:29:08Z

@nh2 exactly 👍

consul: rewrite nixos module

ca87a32

ofborg bot added 6.topic: nixos 8.has: module (update) labels Dec 28, 2020

consul: fix settings generate call

f587132

SuperSandro2000 reviewed Dec 28, 2020

View reviewed changes

nixos/modules/services/networking/consul.nix Show resolved Hide resolved

consul: fix format reference

f84c234

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 28, 2020

cpcloud mentioned this pull request Dec 28, 2020

Add systemd unit from deployment guide hashicorp/consul#9472

Closed

nh2 requested changes Dec 28, 2020

View reviewed changes

cpcloud closed this Dec 28, 2020

cpcloud deleted the consul-cleanup branch December 28, 2020 22:45

cpcloud mentioned this pull request Dec 28, 2020

consul: use settings from consul.io unit #107853

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consul: rewrite nixos module #107798

consul: rewrite nixos module #107798

cpcloud commented Dec 28, 2020 •

edited

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

aanderse commented Dec 28, 2020

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

nh2 left a comment

nh2 Dec 28, 2020

nh2 Dec 28, 2020

nh2 Dec 28, 2020

nh2 Dec 28, 2020

nh2 Dec 28, 2020

nh2 Dec 28, 2020

nh2 commented Dec 28, 2020

Success

nixos-discourse commented Dec 28, 2020

cpcloud commented Dec 28, 2020

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

nh2 commented Dec 28, 2020

aanderse commented Dec 28, 2020

consul: rewrite nixos module #107798

consul: rewrite nixos module #107798

Conversation

cpcloud commented Dec 28, 2020 • edited

Motivation for this change

Things done

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

aanderse commented Dec 28, 2020

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

nh2 left a comment

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 Dec 28, 2020

Choose a reason for hiding this comment

nh2 commented Dec 28, 2020

Success

nixos-discourse commented Dec 28, 2020

cpcloud commented Dec 28, 2020

cpcloud commented Dec 28, 2020

aanderse commented Dec 28, 2020

nh2 commented Dec 28, 2020

nh2 commented Dec 28, 2020

aanderse commented Dec 28, 2020

cpcloud commented Dec 28, 2020 •

edited