Update rspamd #40434

sshisk · 2018-05-13T12:49:27Z

Motivation for this change

Deprecate rmilter and all this more configurable.

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
- 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/)
Fits CONTRIBUTING.md.

Be able to test aliased options with isDefined.

Deprecated by rspamd_proxy.

Add locals and overrides to configure individual modules. Improve default configuration. Enable controller and milter proxy by default with unix sockets. Fix aliases, before they overrode module defaults. Fix types in submodules. Add postfix configuration for quick integration. Fix service dependencies so that systemd sockets aren't removed.

Use unix sockets, don't test IPv6, general cleanup.

oxij · 2018-05-13T16:30:57Z

ceb90ac LGTM, closes #40414. I think my #40418 is a much better fix for what 07cfa1a and new `import` lines in 8795776 try to do. I think we hit the same bug in the core, but I like my change more because 07cfa1a looks like a work-around that duplicates a piece of `doRename`, and I just fixed `doRename`. As to the rest, I'm not sure we should just straight up remove `rmilter`. nixos-mailserver project, for instance, depends on it.

xeji · 2018-05-13T16:37:23Z

/cc @fpletz

eqyiel · 2018-06-17T09:38:49Z

@sshisk @oxij @nbp is there anything I can do to help move this along?

oxij · 2018-06-17T10:38:53Z

Please review #40418. Let's also discuss the changes to the rspamd service. In my branch, when I made a support for milter-less setup, I _removed_ some options from the service to allow for more flexibility in `extraConfig`. This branch _adds_ options. Thoughts?

griff · 2018-06-29T12:17:54Z

nixos/modules/services/mail/rspamd.nix


    systemd.services.rspamd = {
      description = "Rspamd Service";

      wantedBy = mkIf (!cfg.socketActivation) [ "multi-user.target" ];
-      after = [ "network.target" ] ++
-       (if cfg.socketActivation then allSocketNames else []);
-      requires = mkIf cfg.socketActivation allSocketNames;


Why remove these? In your commit message you mention that service dependencies where changed to counteract the sockets getting deleted but that problem is fixed further down by not setting RuntimeDirectory

griff · 2018-06-30T22:10:08Z

nixos/modules/services/mail/rspamd.nix

@@ -323,6 +372,7 @@ in
        value = {
          description = "Rspamd socket ${toString each.index} for worker ${each.name}";
          wantedBy = [ "sockets.target" ];
+          after = optional (each.name == "rspamd_proxy") "postfix.service";


Shouldn't this be before? You want the socket to be started before postfix uses it.

This should be not required when socket activation is used.

This should be not required when socket activation is used.

You are right this line is not required. But thought this line is at best redundant and in my opinion also wrong since you want the socket to be created before not after the service, there is some logic behind adding an explicit dependency between postfix and the socket. In situations where the socket and postfix has been manually stopped an explicit dependency makes systemd also start the socket when you start postfix.

But I think this would have been a better way to add the dependency:

systemd.services.postfix = mkIf cfg.postfix.enable { after = [ "rspamd-normal-1.socket" ]; requires = [ "rspamd-normal-1.socket" ]; }

oxij · 2018-07-02T03:01:12Z

Status update.

#40418 got merged recently (thanks @7c6f434c and @nbp!) so the first patch in this patchset and import lines in 8795776 are no longer needed.

I picked rspamd update to #42858 for convenience in case anyone still wants that (two closed PRs for one, btw. hurry up to claim your prize! etc etc).

I guess the rest is stalled until someone with commit access decides to take interest...

Mic92 · 2018-09-27T13:12:32Z

Sorry this did not got into 18.09. I am now also using rspamd and would review this if rebased.

Mic92 · 2018-09-27T13:13:30Z

nixos/modules/services/mail/rspamd.nix

          controller = {};
+          rspamd_proxy = {};


This is a good default but should be also mentioned in the release notes.

Mic92 · 2018-09-27T13:14:16Z

nixos/modules/services/mail/rspamd.nix

+    services.postfix.extraConfig = mkIf cfg.postfix.enable ''
+      smtpd_milters = unix:private/rspamd
+      non_smtpd_milters = $smtpd_milters
+      milter_protocol = 6


This is apparently the default in our postfix version.

Mic92 · 2018-09-27T13:14:47Z

nixos/modules/services/mail/rspamd.nix

+
+    services.postfix.extraConfig = mkIf cfg.postfix.enable ''
+      smtpd_milters = unix:private/rspamd
+      non_smtpd_milters = $smtpd_milters


The official documentation does not mention this: https://rspamd.com/doc/integration.html

No but if you want rspamd to also scan mails from command line you need this. And if you use the DKIM module in rspamd to also add DKIM signatures you want rspamd to also be called for command line mails.

makes sense.

Mic92 · 2018-09-27T13:15:14Z

cc @avnik

griff · 2018-09-27T22:01:21Z

This might not be the place but since this is the latest change to the rspamd module I thought I might bring it up here. The socket activation code for rspamd doesn't work and can't be made to work until rspamd/rspamd#2035 is fixed. See also #47421

Also the worker configuration needs to be changed to better reflect how rspamd works. I am planning on working on these changes to the worker configuration in the near future. But in essence the problem is that a configuration of:

workers.controller = {};
workers.controller2 = {
  type = "controller";
  bindSockets = ["localhost:11336" ];
};

results in the bad rspamd config of:

workers {
  type = "controller";
  .includes "$CONFDIR/worker-controller.inc"
  bind_socket = "localhost:11334";
}
workers "controller2" {
  type = "controller";
  bind_socket = "localhost:11336";
}

But the essence of what I am trying to do with my controller2 config (i.e. a second controller with a different password) is actually possible with rspamd. The config just has to look like this:

workers {
  type = "controller";
  .includes "$CONFDIR/worker-controller.inc"
  bind_socket = "localhost:11334";
}
workers "controller" {
  bind_socket = "localhost:11336";
}

griff · 2018-09-27T22:05:51Z

On a more review note I like the locals and overrides in this PR though I would have liked them to be more like environment.etc.<name> options where there is both a .text and a .source so that you can set the content from either inline text or a path.

griff · 2018-11-06T00:22:20Z

My PR #49620 that adds my version of locals and overrides options has been merged.

I have also created #49809 that adds support for multiple workers as well as the postfix integration that this PR also included.

What this means is that once #49809 is merged all the changes made by this PR have in one form or another been implemented in other PRs and so this PR can probably be closed.

sshisk added 5 commits May 13, 2018 12:46

mkChangedOptionModule: don't set default value for 'from'

07cfa1a

Be able to test aliased options with isDefined.

rmilter: drop

8a3d8c9

Deprecated by rspamd_proxy.

rspamd: 1.6.6 -> 1.7.4

ceb90ac

rspamd test: update

187ac3e

Use unix sockets, don't test IPv6, general cleanup.

sshisk requested review from edolstra and nbp as code owners May 13, 2018 12:49

GrahamcOfBorg added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels May 13, 2018

xeji mentioned this pull request May 13, 2018

rspamd: 1.6.6 -> 1.7.3 #40414

Closed

eqyiel mentioned this pull request May 16, 2018

Move from rmilter to rspamd r-raymond/nixos-mailserver#25

Open

oxij mentioned this pull request May 29, 2018

lib, nixos: fix module aliases in presence of defaults #40418

Merged

2 tasks

griff reviewed Jun 29, 2018

View reviewed changes

griff reviewed Jun 30, 2018

View reviewed changes

oxij mentioned this pull request Jul 2, 2018

rspamd: 1.6.6 -> 1.7.4 #42858

Merged

Mic92 reviewed Sep 27, 2018

View reviewed changes

fpletz closed this Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rspamd #40434

Update rspamd #40434

sshisk commented May 13, 2018

oxij commented May 13, 2018 via email

xeji commented May 13, 2018

eqyiel commented Jun 17, 2018

oxij commented Jun 17, 2018 via email

griff Jun 29, 2018

griff Jun 30, 2018

Mic92 Sep 27, 2018

griff Sep 27, 2018 •

edited

oxij commented Jul 2, 2018

Mic92 commented Sep 27, 2018 •

edited

Mic92 Sep 27, 2018

Mic92 Sep 27, 2018

Mic92 Sep 27, 2018

griff Sep 27, 2018

Mic92 Sep 27, 2018

Mic92 commented Sep 27, 2018

griff commented Sep 27, 2018

griff commented Sep 27, 2018 •

edited

griff commented Nov 6, 2018

Update rspamd #40434

Update rspamd #40434

Conversation

sshisk commented May 13, 2018

Motivation for this change

Things done

oxij commented May 13, 2018 via email

xeji commented May 13, 2018

eqyiel commented Jun 17, 2018

oxij commented Jun 17, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

griff Sep 27, 2018 • edited

Choose a reason for hiding this comment

oxij commented Jul 2, 2018

Mic92 commented Sep 27, 2018 • edited

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

Mic92 commented Sep 27, 2018

griff commented Sep 27, 2018

griff commented Sep 27, 2018 • edited

griff commented Nov 6, 2018

griff Sep 27, 2018 •

edited

Mic92 commented Sep 27, 2018 •

edited

griff commented Sep 27, 2018 •

edited