nixos/rspamd: Multiple workers, extraConfig priority & postfix integration #49809

griff · 2018-11-05T23:59:16Z

Motivation for this change

Multiple workers

When the workers option for rspamd was originally implemented it was based on a flawed understanding of how workers are configured in rspamd. This meant that while rspamd supports configuring multiple workers of the same type, so that different controller workers could have different passwords, the NixOS module did not support this because it would write an invalid configuration file if you tried.

Specifically a configuration like the one below:

workers.controller = {};
workers.controller2 = {
  type = "controller";
};

Would result in a rspamd configuration of:

worker {
  type = "controller";
  count = 1;
  .include "$CONFDIR/worker-controller.inc"
}
worker "controller2" {
  type = "controller";
  count = 1;
}

While to get multiple controller workers it should instead be:

worker "controller" {
  type = "controller";
  count = 1;
  .include "$CONFDIR/worker-controller.inc"
}
worker "controller" {
  type = "controller";
  count = 1;
}

extraConfig priority

The lines stored in extraConfig and worker.<name?>.extraConfig should take precedent over values from included files but in order to do this in rspamd UCL they need to be stored in a file that then gets included with a high priority. This commit uses the overrides option to store the value of the two extraConfig options in extra-config.inc and worker-<name?>.inc respectively.

postfix integration

The rmilter module has options for configuring postfix to use it but since that module is deprecated because rspamd now has a builtin worker that supports the milter protocol this PR adds similar postfix integration options directly to the rspamd module.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
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/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

When the workers option for rspamd was originally implemented it was based on a flawed understanding of how workers are configured in rspamd. This meant that while rspamd supports configuring multiple workers of the same type, so that different controller workers could have different passwords, the NixOS module did not support this because it would write an invalid configuration file if you tried. Specifically a configuration like the one below: ``` workers.controller = {}; workers.controller2 = { type = "controller"; }; ``` Would result in a rspamd configuration of: ``` worker { type = "controller"; count = 1; .include "$CONFDIR/worker-controller.inc" } worker "controller2" { type = "controller"; count = 1; } ``` While to get multiple controller workers it should instead be: ``` worker "controller" { type = "controller"; count = 1; .include "$CONFDIR/worker-controller.inc" } worker "controller" { type = "controller"; count = 1; } ```

The lines stored in `extraConfig` and `worker.<name?>.extraConfig` should take precedent over values from included files but in order to do this in rspamd UCL they need to be stored in a file that then gets included with a high priority. This commit uses the overrides option to store the value of the two `extraConfig` options in `extra-config.inc` and `worker-<name?>.inc` respectively.

7c6f434c · 2018-11-06T06:57:28Z

@avnik @oxij @Mic92 I don't understand rspamd well enough to merge it myself (I don't use it), seems a priori reasonable, I can merge it in case of positive review + no objections for a day or two.

avnik · 2018-11-06T12:28:18Z

Looks good for me. It builds, and at least postix integration part works.

fpletz

Thanks a lot! Was about to open a similar PR because I already added a proxy worker with the current module layout.

fpletz · 2018-11-08T23:17:58Z

nixos/modules/services/mail/rspamd.nix

    };
  };


  ###### implementation

  config = mkIf cfg.enable {
+    services.rspamd.overrides = configOverrides;
+    services.rspamd.workers = mkIf cfg.postfix.enable {
+      normal = {};


Note that when using the proxy worker with postfix, the normal worker isn't strictly needed. We might want do drop it here.

You are right it is not needed so I have removed it.

The `rmilter` module has options for configuring `postfix` to use it but since that module is deprecated because rspamd now has a builtin worker that supports the milter protocol this commit adds similar `postfix` integration options directly to the `rspamd` module.

infinisil · 2018-11-25T01:22:24Z

nixos/modules/services/mail/rspamd.nix

@@ -58,7 +59,7 @@ let
      };
      type = mkOption {
        type = types.nullOr (types.enum [
-          "normal" "controller" "fuzzy_storage" "proxy" "lua"


Why was "proxy" removed? This broke https://gitlab.com/simple-nixos-mailserver/nixos-mailserver

https://gitlab.com/simple-nixos-mailserver/nixos-mailserver/blob/c2ca4d1bb05a5c3886b433dc10b2c4d55bfa1f29/mail-server/rspamd.nix#L45

I guess yeah it got replaced by "rspamd_proxy", but then it should be made backwards compatible in some way.

proxy was renamed to rspamd_proxy because that is the actual type of the worker. I am sorry it broke those that used it. I should probably make another PR with a warning for proxy instead.

At least a changelog entry :)

What's an easy fix for this on the nixpkgs side? I can't expect nixos-mailserver to fix this because they need to support multiple versions.

Thanks!

I figured out that I can temporarily just use

{ services.rspamd.workers.${worker}.type = mkForce "rspamd_proxy"; }

in my NixOS config to fix it.

@grahamc I haven't changed release-notes before so can you guide me a little?

If I add back support for type having the value proxy but also then show a warning should I then add my release notes description in the Backward Incompatibilities section or the Other Notable Changes section?

@griff Since after the fix a configuration with the old value still works with the new nixpkgs, it's still compatible. Pretty sure it doesn't need a release note.

I have made #51012 to address this issue.

I don't think reallowing proxy is necessary or a good thing to do. These kind of workarounds just increase the complexity of NixOS modules. Nixos-mailserver can fix this with https://gitlab.com/simple-nixos-mailserver/nixos-mailserver/merge_requests/142.

NixOS/nixpkgs#49809 (comment)

oxij · 2018-11-25T10:07:39Z

I don't think reallowing `proxy` is necessary or a good thing to do. These kind of workarounds just increase the complexity of NixOS modules. Nixos-mailserver can fix this with https://gitlab.com/simple-nixos-mailserver/nixos-mailserver/merge_requests/142.

But adding a release note and producing a warning when it is used is still reasonable.

dotlambda · 2018-11-25T10:11:45Z

Adding a release note certainly is. But if we don't allow "proxy", then there's no need for a warning.

avnik · 2018-11-25T10:47:19Z

On Sun, Nov 25, 2018 at 02:11:49AM -0800, Robert Schütz wrote: Adding a release note certainly is. But if we don't allow `"proxy"`, then there's no need for a warning.

Just custom error ("proxy" is not allowed now, use "rspamd_proxy") can be useful for upgrading. Our upgrade process allow this, because system never become in middle-aka-broken state.

infinisil · 2018-11-25T13:14:39Z

I don't think reallowing proxy is necessary or a good thing to do. These kind of workarounds just increase the complexity of NixOS modules. Nixos-mailserver can fix this with gitlab.com/simple-nixos-mailserver/nixos-mailserver/merge_requests/142.

First this doesn't work in general, people don't always use stable versions only. SNM would have to maintain an ugly workaround for a while that can't be removed. Also, since SNM has a stable release cycle, a new minor release is needed.

All of that because we don't want to provide at least a little bit of backwards compatibility in nixpkgs? That's a no from me.

avnik · 2018-11-25T13:35:38Z

On Sun, Nov 25, 2018 at 05:14:42AM -0800, Silvan Mosberger wrote: > I don't think reallowing `proxy` is necessary or a good thing to do. These kind of workarounds just increase the complexity of NixOS modules. Nixos-mailserver can fix this with [gitlab.com/simple-nixos-mailserver/nixos-mailserver/merge_requests/142](https://gitlab.com/simple-nixos-mailserver/nixos-mailserver/merge_requests/142). First this doesn't work in general, people don't always use stable versions only. SNM would have to maintain an ugly workaround for a while that can't be removed. Also, since SNM has a stable release cycle, a new minor release is needed.

I am sit on master myself. But I understand. that message "please replace option xxx with yyy" is a compromise.

All of that because we don't want to provide at least a little bit of backwards compatibility in nixpkgs? That's a no from me.

Reasonable. Would be nice to have RFC explaining/guiding compatibility procedures. Any volunteers to write?

infinisil · 2018-11-25T13:38:05Z

Well I have an RFC (albeit going forward very slowly) that lays foundation for a deprecation guideline for nixpkgs attributes, but not NixOS modules (yet at least): NixOS/rfcs#33

griff · 2018-11-25T14:11:26Z

I personally always try to provide backwards compatibility and warnings when making module changes and had I not missed it originally the changes in #51012 would have been part of this PR. It adds a little module complexity for a lot of user convenience. The discussion to have is then for how many stable releases to keep these compatibility fixes and when to remove them.

griff added 3 commits November 6, 2018 00:26

nixos/rspamd: Add defaults for rspamd_proxy worker

46ef075

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 6, 2018

griff mentioned this pull request Nov 6, 2018

Update rspamd #40434

Closed

8 tasks

griff force-pushed the rspamd-workers branch from 6a03030 to 65b7e3e Compare November 6, 2018 03:58

globin requested a review from fpletz November 8, 2018 14:58

fpletz approved these changes Nov 8, 2018

View reviewed changes

griff force-pushed the rspamd-workers branch from 65b7e3e to e01605b Compare November 9, 2018 00:32

fpletz merged commit 8ba51ef into NixOS:master Nov 9, 2018

griff deleted the rspamd-workers branch November 10, 2018 20:00

infinisil reviewed Nov 25, 2018

View reviewed changes

infinisil added a commit to infinisil/system that referenced this pull request Nov 25, 2018

Fix for #49809

12efe92

NixOS/nixpkgs#49809 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/rspamd: Multiple workers, extraConfig priority & postfix integration #49809

nixos/rspamd: Multiple workers, extraConfig priority & postfix integration #49809

griff commented Nov 5, 2018

7c6f434c commented Nov 6, 2018

avnik commented Nov 6, 2018

fpletz left a comment

fpletz Nov 8, 2018

griff Nov 9, 2018

infinisil Nov 25, 2018

infinisil Nov 25, 2018

griff Nov 25, 2018

grahamc Nov 25, 2018

infinisil Nov 25, 2018

infinisil Nov 25, 2018

griff Nov 25, 2018

infinisil Nov 25, 2018

griff Nov 25, 2018

dotlambda Nov 25, 2018

oxij commented Nov 25, 2018 via email

dotlambda commented Nov 25, 2018

avnik commented Nov 25, 2018 via email

infinisil commented Nov 25, 2018

avnik commented Nov 25, 2018 via email

infinisil commented Nov 25, 2018

griff commented Nov 25, 2018

nixos/rspamd: Multiple workers, extraConfig priority & postfix integration #49809

nixos/rspamd: Multiple workers, extraConfig priority & postfix integration #49809

Conversation

griff commented Nov 5, 2018

Motivation for this change

Multiple workers

extraConfig priority

postfix integration

Things done

7c6f434c commented Nov 6, 2018

avnik commented Nov 6, 2018

fpletz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxij commented Nov 25, 2018 via email

dotlambda commented Nov 25, 2018

avnik commented Nov 25, 2018 via email

infinisil commented Nov 25, 2018

avnik commented Nov 25, 2018 via email

infinisil commented Nov 25, 2018

griff commented Nov 25, 2018