nixos/adguardhome: Add settings option

[CRTified]

Motivation for this change

It was not possible to configure AdGuard declaratively before.
This PR adds services.adguard.settings to generate the AdGuardHome.yaml. It is merged with yaml-merge on start.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[aanderse]

I left a few very minor suggestions. I hope you agree with them.

I'm not an adguardhome user but I definitely like what you have done here. Great job adapting software that really wants to be managed imperatively and bending it to the will of nix in a reasonable way 👍

[CRTified]

These changes are really reasonable.

Thanks for the input, I tried checking a few options for links and the checked one didn't contain any links (Edit: I meant that they didn't contain any markup for the links).

[aanderse]

Have you tested this? If so, I'm happy to merge once @lunik1 reviews. Please ping back in a few days if we don't hear anything.

[CRTified]

I already migrated my config to it, using:

  disabledModules = [ "services/networking/adguardhome.nix" ];
  imports = [ ../../nixpkgs/nixos/modules/services/networking/adguardhome.nix ]; # This is a relative path to my nixpkgs checkout

Redeploying didn't result in any new closures, so your changes give identical results.

[aanderse]

Perfect! Thanks.

[lunik1]

Sorry for the delay, I really like this PR too. Good work! My one suggestion is that I think it now makes sense to make the config location configurable à la #131356, could you add this option?

[rhoriguchi]

@CRTified nice addition, a lot more refined than my initial PR.

[aanderse]

My one suggestion is that I think it now makes sense to make the config location configurable

I think this PR strikes a nice balance because adguardhome still manages its own configuration. This PR grafts configuration on top of what adguardhome manages, it doesn't attempt to provide any user management of the configuration in a sense that conflicts with the app. Personally, I think this PR is perfect as it is...

[CRTified]

My one suggestion is that I think it now makes sense to make the config location configurable à la #131356, could you add this option?

@lunik1 @rhoriguchi Do you have a specific use case in mind for making the configuration path configurable? Maybe this can be integrated, but the settings-merging step makes symlinks and hardlinks impossible right now (due to this line).
It would be possible to accommodate for a dynamic path, but if there is no need for this, I'd prefer to keep it simple.

@aanderse Would it make sense to add a test for the service? I don't see a way to test the merging-step, but at least the initial start would be covered.

[rhoriguchi]

My one suggestion is that I think it now makes sense to make the config location configurable à la #131356, could you add this option?

@lunik1 @rhoriguchi Do you have a specific use case in mind for making the configuration path configurable? Maybe this can be integrated, but the settings-merging step makes symlinks and hardlinks impossible right now (due to this line). It would be possible to accommodate for a dynamic path, but if there is no need for this, I'd prefer to keep it simple.

@aanderse Would it make sense to add a test for the service? I don't see a way to test the merging-step, but at least the initial start would be covered.

The proposal your are making works fine for me, I'm not a big fan of the merging of the config, would be nice if that could be controlled by a option. Since i personally want to have an idempotent configuration.

I currently solve it like this which is more or less how you do it.

[CRTified]

I'm not a big fan of the merging of the config, would be nice if that could be controlled by a option.

That's easy enough, by conditionally always cp-ing the config over. That would still keep the promise of checking the configuration while building due to the check phase for the config file. I'll edit the PR in a few minutes, just a bit busy right now.

[lunik1]

My one suggestion is that I think it now makes sense to make the config location configurable à la #131356, could you add this option?

@lunik1 @rhoriguchi Do you have a specific use case in mind for making the configuration path configurable? Maybe this can be integrated, but the settings-merging step makes symlinks and hardlinks impossible right now (due to this line). It would be possible to accommodate for a dynamic path, but if there is no need for this, I'd prefer to keep it simple.

Maybe I am missing something, but could the target of that mv not just be $SOME_USER_SPECIFIED_CONFIG_LOCATION?. The originial PR points to some use cases, but if this will overcomplicate the module I am happy to merge as-is.

[CRTified]

If the target was a symlink/hardlink, that mv would replace the symlink with an actual (and different) file.

[CRTified]

I've now added services.adguardhome.mutableSettings as an option. If it is true (default), we allow changes from the web interface to persist between restarts by choosing the yaml-merge path in preStart. Otherwise, we always override the configuration on service start.

@rhoriguchi Let me know if this is sufficient for you.

@lunik1 I think adding a configurable path would complicate things a lot, as we need to consider whether merging/overriding should happen at all if that configuration is set. I'm also unsure whether the minimal isolation provided by systemd would require some attention for arbitrary config paths.

But it appears that when having multiple --config arguments, only the last one is considered (at least with 1-2 quick'n'dirty tests in a nix-shell). I don't know whether this should be documented somewhere, but it might be sufficient for the "mutable config, not controlled by nix, and backups somewhere else"-usecase from the other PR, as it allows overriding the config path without problems (not even a warning). The module already provides extraArgs, so the user should be able to do this easily.

Please let me know what you think.

[rhoriguchi]

I've now added services.adguardhome.mutableSettings as an option. If it is true (default), we allow changes from the web interface to persist between restarts by choosing the yaml-merge path in preStart. Otherwise, we always override the configuration on service start.

@rhoriguchi Let me know if this is sufficient for you.

@lunik1 I think adding a configurable path would complicate things a lot, as we need to consider whether merging/overriding should happen at all if that configuration is set. I'm also unsure whether the minimal isolation provided by systemd would require some attention for arbitrary config paths.

But it appears that when having multiple --config arguments, only the last one is considered (at least with 1-2 quick'n'dirty tests in a nix-shell). I don't know whether this should be documented somewhere, but it might be sufficient for the "mutable config, not controlled by nix, and backups somewhere else"-usecase from the other PR, as it allows overriding the config path without problems (not even a warning). The module already provides extraArgs, so the user should be able to do this easily.

Please let me know what you think.

Thank you for adding the option. This is perfect, that way you can ensure that the config is always the same after a restart.

I don't really see a reason to add a path since the settings options just creats a file and if someone wants to use a config file, they can just read the yaml and input it to the settings option. Also I agree, being able to add a path might cause some strange behavior, for example adguardhome is not able to read or write to the file. Also through the checkPhase the config file is always valid, which is an added benefit of doing it this way.

[aanderse]

Looks good 👍

[CRTified]

@aanderse I think this can be merged now :)

[aanderse]

Thanks for the ping back!