nixos/adguardhome: init #120568

lunik1 · 2021-04-24T22:35:07Z

Motivation for this change

Adds a system service for AdGuard Home.

The service definition is largely based on the one generated by AdGuard Home itself when passed adguardhome --service install (this won't work on NixOS, as it will try to write to a read-only filesystem), with an additional choice of config, work, and pid file locations that should follow the FHS.

May provide some relief for followers of #61617 while they wait for #108055 & the following PRs (but won't actually work on a pi without overriding the adguardhome package because of #113900)

Things done

dotlambda · 2021-04-25T07:40:48Z

nixos/modules/services/networking/adguardhome.nix

+    confFile = mkOption {
+      default = "/etc/AdGuardHome.yaml";
+      type = path;
+      description = ''
+        Path to the config file.
+      '';
+    };


Please follow NixOS/rfcs#42.

I think having a non-mutable config file will not work well for this service. Management via the webui does not work and the config file is migrated automatically between schema versions on some application updates.

Maybe it would be better, as it is mutable, to store the config file in the work dir by default? Anyone who really wants to try for a stateless config can then override confFile and use environment.etc.

The application also fails the initial set-up if the config file is not writable.

lunik1 · 2021-04-25T16:41:24Z

I think it might make sense to introduce an openFirewall option to this module, although I am not exactly sure what it should cover. The port the webUI is served on is specified by port, but not the port the DNS server is served on (which is specified in the config file / on initial setup). However, ports 53 will be used in the vast majority of cases.

nixos/modules/services/networking/adguardhome.nix

SuperSandro2000 · 2021-04-26T09:19:07Z

nixos/modules/services/networking/adguardhome.nix

+    };
+
+    pidFile = mkOption {
+      default = "/run/AdGuardHome.pid";


Suggested change

default = "/run/AdGuardHome.pid";

default = "/run/AdGuardHome/AdGuardHome.pid";

This fails with a "no such file or directory error", looks like the directory must exist. Is leaving it in /run ok or should there be some mechanism to ensure /run/AdGuardHome exists?

I am not sure what the correct way is to create the directory but otherwise it isn't easily mountable in containers.

The correct way is using https://www.freedesktop.org/software/systemd/man/systemd.exec.html#RuntimeDirectory=.

nixos/modules/services/networking/adguardhome.nix

aanderse · 2021-04-28T01:01:18Z

nixos/modules/services/networking/adguardhome.nix

+      '';
+    };
+
+    logFile = mkOption {


Please default to stdout.

aanderse · 2021-04-28T01:01:37Z

nixos/modules/services/networking/adguardhome.nix

+      '';
+    };
+
+    pidFile = mkOption {


Under what scenario does this need to be configurable?

pidFile can't be specified in the config file and if it wasn't configurable (e.g. fixed to /run/AdGuardHome/AdGuardHome.pid) it would be inconvenient to override. A few other modules (mongodb, bitcoind) do allow pidFile to be configured, but I admit I can't think of a use case.

Alternatively, the pid file could be disabled by default and enabled when needed through an extraArgs option. Could probably at least remove verbose in that case too.

nixos/modules/services/networking/adguardhome.nix

aanderse · 2021-04-28T01:03:23Z

nixos/modules/services/networking/adguardhome.nix

+  options.services.adguardhome = with types; {
+    enable = mkEnableOption "AdGuard Home network-wide ad blocker";
+
+    confFile = mkOption {


Is this intended to be mutable? Who manages this file?

The config file should be managed by AdGuard Home, see my response to dotlambda's review for my rationale. If this is the case should it be placed in stateDir?

I have briefly read some documentation from upstream and I understand now. This application is definitely an example of an application that should manage its own configuration... not NixOS, as you mention.

I would recommend not including this as an option so. You could probably do something really slick with DynamicUser, RuntimeDirectory (for the pid file and reloads) and StateDirectory as well...

DynamicUser works! What do you think should remain configurable when using it? In my view host and port are the essentials, pidFile could just be set to /run/AdGuardHome/AdGuardHome.pid (I can't see a strong case as to the need to disable it), and logs could always be to stdout?

There could be an extraArgs flag, allowing verbose logs if a user needs it and supplying an escape hatch for future command-line options.

That sounds good 👍 Whatever you think is needed.

aanderse · 2021-04-28T01:03:47Z

nixos/modules/services/networking/adguardhome.nix

+      '';
+    };
+
+    workDir = mkOption {


Usually we call this stateDir, depending on what it is for...

This is where AdGuard Home stores filter lists / it's internal database. I was just copying AdGuard Home's terminology for workDir (which I saw used in a few other modules). But stateDir is more appropriate, I'll change it (in the morning 😴).

lunik1 · 2021-04-28T17:38:36Z

Marking as draft while I look into DynamicUser

lunik1 · 2021-04-29T15:35:03Z

Pushed a commit that uses DynamicUser, remove options with no clear use case, and adds an openFirewall option for the webui.

Probably should all be squashed on merge.

nixos/modules/services/networking/adguardhome.nix

aanderse

After the pending changes you are about to push I approve this module 👍 Great work 🎉

dotlambda

LGTM, but I didn't test it.

nixos/modules/services/networking/adguardhome.nix

lunik1 · 2021-04-30T21:12:21Z

Thanks everybody for the help and feedback!

github-actions bot added 6.topic: nixos 8.has: module (update) labels Apr 24, 2021

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 24, 2021

dotlambda changed the title ~~adguardhome: add service~~ nixos/adguardhome: init Apr 25, 2021

dotlambda reviewed Apr 25, 2021

View reviewed changes

SuperSandro2000 reviewed Apr 26, 2021

View reviewed changes

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Apr 26, 2021

View reviewed changes

SuperSandro2000 requested a review from aanderse April 26, 2021 09:19

SuperSandro2000 reviewed Apr 26, 2021

View reviewed changes

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Apr 26, 2021

View reviewed changes

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

lunik1 force-pushed the adguardhome-service branch 3 times, most recently from 67e096f to 6547fb9 Compare April 27, 2021 13:48

aanderse reviewed Apr 28, 2021

View reviewed changes

lunik1 force-pushed the adguardhome-service branch from 6547fb9 to 7639c23 Compare April 28, 2021 17:37

lunik1 marked this pull request as draft April 28, 2021 17:38

lunik1 marked this pull request as ready for review April 29, 2021 15:35

dotlambda reviewed Apr 30, 2021

View reviewed changes

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

aanderse approved these changes Apr 30, 2021

View reviewed changes

lunik1 force-pushed the adguardhome-service branch from f23f716 to 838a156 Compare April 30, 2021 17:33

dotlambda approved these changes Apr 30, 2021

View reviewed changes

nixos/modules/services/networking/adguardhome.nix Outdated Show resolved Hide resolved

nixos/adguardhome: init

a2e0d1c

lunik1 force-pushed the adguardhome-service branch from 838a156 to a2e0d1c Compare April 30, 2021 18:17

dotlambda merged commit 248a57d into NixOS:master Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/adguardhome: init #120568

nixos/adguardhome: init #120568

lunik1 commented Apr 24, 2021 •

edited

dotlambda Apr 25, 2021

lunik1 Apr 25, 2021

lunik1 Apr 25, 2021

lunik1 commented Apr 25, 2021

SuperSandro2000 Apr 26, 2021

lunik1 Apr 26, 2021 •

edited

SuperSandro2000 Apr 27, 2021

dotlambda Apr 27, 2021

aanderse Apr 28, 2021

aanderse Apr 28, 2021

lunik1 Apr 28, 2021

aanderse Apr 28, 2021

lunik1 Apr 28, 2021

aanderse Apr 28, 2021

lunik1 Apr 28, 2021

aanderse Apr 28, 2021

aanderse Apr 28, 2021

lunik1 Apr 28, 2021

lunik1 commented Apr 28, 2021

lunik1 commented Apr 29, 2021

aanderse left a comment

dotlambda left a comment

lunik1 commented Apr 30, 2021

	default = "/run/AdGuardHome.pid";
	default = "/run/AdGuardHome/AdGuardHome.pid";

nixos/adguardhome: init #120568

nixos/adguardhome: init #120568

Conversation

lunik1 commented Apr 24, 2021 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunik1 commented Apr 25, 2021

Choose a reason for hiding this comment

lunik1 Apr 26, 2021 • 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

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

lunik1 commented Apr 28, 2021

lunik1 commented Apr 29, 2021

aanderse left a comment

Choose a reason for hiding this comment

dotlambda left a comment

Choose a reason for hiding this comment

lunik1 commented Apr 30, 2021

lunik1 commented Apr 24, 2021 •

edited

lunik1 Apr 26, 2021 •

edited