nixos/knot: allow full configuration by nix values #81460

vcunat · 2020-03-01T20:14:59Z

This seems desired by multiple people. Let's work out how to do this, starting with this prototype. See the commit messages.

settings RFC for reference: https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md

vcunat · 2020-03-01T20:19:24Z

One idea not yet implemented:

{
       # Example usage:
       zones.foo = "inline string";
       zones.bar = ./some-file;
       zones.baz = some derivation; # e.g. writeText, fetch* or whatever

       # where implementation might be something like:
       nixConfig.zone = flip mapAttrs zones
            (n: v: {
              storage = "/var/lib/knot";
              file = if isString v then pkgs.writeText n v else v;
            });
}

vcunat · 2020-03-01T20:45:08Z

So far I absolutely haven't looked at aspects like what happens on errors. Some common problems might lead to horrible nix traces, for example.

nixos-discourse · 2020-12-25T09:14:00Z

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

https://discourse.nixos.org/t/generating-yaml-in-a-nix-module/2519/5

mweinelt · 2021-05-23T19:47:35Z

One idea not yet implemented:

{
       # Example usage:
       zones.foo = "inline string";
       zones.bar = ./some-file;
       zones.baz = some derivation; # e.g. writeText, fetch* or whatever

       # where implementation might be something like:
       nixConfig.zone = flip mapAttrs zones
            (n: v: {
              storage = "/var/lib/knot";
              file = if isString v then pkgs.writeText n v else v;
            });
}

Ultimately something like this would be awesome https://github.com/kirelagin/dns.nix.

vcunat · 2021-05-23T20:28:10Z

Generating the zone files themselves seems like an orthogonal task. In particular, it should be the same for all DNS servers. I think I've seen multiple proposals for some such generator already.

stale · 2022-01-09T08:06:59Z

I marked this as stale due to inactivity. → More info

sinavir · 2023-07-09T19:45:00Z

Hello, any updates on this ? How can I help ?

vcunat · 2023-07-09T20:06:32Z

Test and review, I think. I got basically no reactions here, so I didn't feel like going forward with it. IIRC that's the only reason why this got left behind by me. EDIT: well and that other "competing PR", as we can't really have both at once.

I could rebase a look about possible silent conflicts, but I expect an issue is only in the tests even though it's been years.

mweinelt · 2023-07-09T21:50:14Z

Rebased.

Made the new declarative configuration into the settings option and removed extraConfig, because merging isn't really feasible.

Updated the remaining knot configs in our tests accordingly.

dummy987654321 · 2023-07-10T07:31:41Z

I think we should keep extraConfig with a deprecation warning for two reason:

This will help users making the transition
There may be some setups that are not doable with this pr that we didn't expected

dummy987654321 · 2023-07-10T09:35:18Z

Ok looked a bit deeper in knot docs and I believe that the current PR cannot handle modules (https://www.knot-dns.cz/docs/latest/html/modules.html).

It seems to me that all modules are parsed by sec_list_fa "id". Thus it can be handled by iterating into settings top-level ang extracting the keys that begin by mod- .

dummy987654321 · 2023-07-10T09:47:00Z

Here is a patch :

diff --git a/nixos/modules/services/networking/knot.nix b/nixos/modules/services/networking/knot.nix
index bea980066cd..bf4186f6c31 100644
--- a/nixos/modules/services/networking/knot.nix
+++ b/nixos/modules/services/networking/knot.nix
@@ -11,6 +11,7 @@ let
         # We output the config section in the upstream-mandated order.
         # Ordering is important due to forward-references not being allowed.
         [ (sec_list_fa "id" nix_def "module") ]
+        ++ map (sec_list_fa "id" nix_def) (filter (hasPrefix "mod-") (attrNames nix_def))
         ++ map (sec_plain nix_def)
           [ "server" "control" ]
         ++ [ (sec_list_fa "target" nix_def "log") ]

mweinelt · 2023-07-10T12:59:54Z

We're also going to run into ordering problems with mods.

https://www.knot-dns.cz/docs/latest/html/configuration.html#query-modules

dummy987654321 · 2023-07-10T14:01:48Z

Hmm, do you think it is worth continuing that way ? Or should the module let the user provide the ordering ?

mweinelt · 2023-07-10T15:16:29Z

We could either make this a fixed order for now, or work with priorities (e.g. nginx location blocks would be the prior art), or maybe even toposort.

But even then there is the issue, that the current approach silently ignores "extra" configuration.

vcunat · 2023-08-13T09:50:45Z

I think the PR now has everything that's been suggested. Also /cc @lheckemann as potentially interested in reacting to this PR.

vcunat · 2023-08-13T09:55:27Z

Nit: maybe assertions instead of assert would give more consistent errors. At the moment I didn't feel like moving the check away from its place, but it certainly wouldn't be a big deal.

mweinelt · 2023-08-29T16:46:26Z

Feel free to improve on that nit, I'm very much inclined to include this in NixOS 23.11 either way. Even in its current state, it is a big improvement.

vcunat · 2023-09-23T08:07:52Z

cleaned up history where I didn't consider it worth keeping the in-between states
added release notes and partial compatibility with NixOS 23.05 config

I think this is the last call. There has been too much opportunity for feedback already.

lheckemann

❤️

mweinelt · 2023-09-23T18:55:51Z

I think all interested parties have approved. Thanks for your work on this, it makes using knot much more pleasant than working with the upstream config format.

SuperSandro2000 · 2023-09-28T14:00:42Z

Naive question: wouldn't this be a bit simpler if knot's yaml parser would be a full yaml 1.1 parser and not only supporting a tiny subset?

SuperSandro2000 · 2023-09-28T14:02:00Z

nixos/modules/services/networking/knot.nix

+  in result;
+
+  configFile = if cfg.settingsFile != null then
+    assert cfg.settings == {} && cfg.keyFiles == [];


We should move this to a full fledged module system assertion to make it UX wise a lot better.
Right now I receive the following error which is a bit confusing.

… while evaluating a branch condition at /nix/store/4h9mf2ydqjgjawihf3zkshkznm4mkavg-source/lib/customisation.nix:256:8: 255| drv' = (lib.head outputsList).value; 256| in if drv == null then null else | ^ 257| lib.deepSeq drv' drv'; … while calling the 'head' builtin at /nix/store/4h9mf2ydqjgjawihf3zkshkznm4mkavg-source/lib/attrsets.nix:820:11: 819| || pred here (elemAt values 1) (head values) then 820| head values | ^ 821| else (stack trace truncated; use '--show-trace' to show the full trace) error: assertion '(((cfg).settings == { }) && ((cfg).keyFiles == [ ]))' failed at /nix/store/4h9mf2ydqjgjawihf3zkshkznm4mkavg-source/nixos/modules/services/networking/knot.nix:106:5: 105| configFile = if cfg.settingsFile != null then 106| assert cfg.settings == {} && cfg.keyFiles == []; | ^ 107| cfg.settingsFile

Also this isn't mentioned in the release-notes, so I am currently a bit confused how to migrate of the following:

keyFiles = [ config.sops.secrets."knot/keyFile".path ];

If you switch to settings instead of extraConfig, you can keep keyFiles.

As for the module, one way is to make the assertion better (as you suggest), another way is to implement full compatibility.

I ended up with this diff to have an identical knot.conf file without rewriting my config.

diff --git machines/neon/configuration.nix machines/neon/configuration.nix index 947b152..594c898 100644 --- machines/neon/configuration.nix +++ machines/neon/configuration.nix @@ -134,10 +134,8 @@ knot = { enable = true; - keyFiles = [ - config.sops.secrets."knot/keyFile".path - ]; - extraConfig = /* yaml */ '' + settingsFile = pkgs.writeText "knot.conf" /* yaml */ '' + include: ${config.sops.secrets."knot/keyFile".path} acl: - id: transfer key: transfer

@SuperSandro2000 Thank you, this was helpful. My configuration looked very similar to yours.

Actually, people may want to revert similar config changes when updating past PR #258361 - as such includes will surely often fail build-time validation that was disabled by accident. (I'm sure people do want validation when not using secrets.)

SuperSandro2000 · 2023-09-28T14:07:40Z

nixos/doc/manual/release-notes/rl-2311.section.md

@@ -97,6 +97,8 @@

 - `pass` now does not contain `password-store.el`.  Users should get `password-store.el` from Emacs lisp package set `emacs.pkgs.password-store`.

+- `services.knot` now supports `.settings` from RFC42.  The change is not 100% compatible with the previous `.extraConfig`.


This should relay contain what is not compatible or some link to maybe here or somewhere else because right now lonely from this line I am totally unsure what to other than just give it a shot.

I would also appreciate seeing more detail here. The error generated without making any changes to my previous config was (at least to my eyes) fairly cryptic:

Full error trace:

error: … while calling the 'head' builtin at /nix/store/8lg5579v1k5mc63ypzfagrcl8l32x3bh-source/lib/attrsets.nix:820:11: 819| || pred here (elemAt values 1) (head values) then 820| head values | ^ 821| else … while evaluating the attribute 'value' at /nix/store/8lg5579v1k5mc63ypzfagrcl8l32x3bh-source/lib/modules.nix:807:9: 806| in warnDeprecation opt // 807| { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value; | ^ 808| inherit (res.defsFinal') highestPrio; (stack trace truncated; use '--show-trace' to show the full trace) error: assertion '(((cfg).settings == { }) && ((cfg).keyFiles == [ ]))' failed at /nix/store/8lg5579v1k5mc63ypzfagrcl8l32x3bh-source/nixos/modules/services/networking/knot.nix:106:5: 105| configFile = if cfg.settingsFile != null then 106| assert cfg.settings == {} && cfg.keyFiles == []; | ^ 107| cfg.settingsFile

The error should be gone after merging PR #258361

SuperSandro2000 · 2023-09-28T14:14:27Z

Feel free to improve on that nit, I'm very much inclined to include this in NixOS 23.11 either way. Even in its current state, it is a big improvement.

I don't think it is that much currently. I mostly configured my knot by copying the yaml from the documentation page section by section. Now I would need to translate it to nix code and make sure that the limited parser still understands what I am doing. It would mean I could built my own functions to generate repetitive sections but there are currently no value adding options like nginx' enableACME or complete location blocks like nextcloud or mastodon has.

vcunat · 2023-09-28T14:22:14Z

I see a part the other way than you: translating to nix code will work around the limited YAML parser, e.g. it will ensure that ordering of the sections is correct. Either way, I don't think settingsFile will be going away.

SuperSandro2000 · 2023-09-28T14:32:49Z

That is an interesting fact I didn't know about. How about we write this into the release notes to motivate people to migrate their config?

vcunat · 2023-09-28T14:35:11Z

I think it's OK if they don't migrate. And keyFiles just insert include: ${file} lines, so you could put it into your YAML by hand. I could figure out how to layer this with the compat helper mkChangedOptionModule.

vcunat · 2023-09-28T14:41:27Z

Solving ordering was a necessity in this PR. Nix attrsets are unordered, so the user no longer can reliably affect ordering.

vcunat · 2023-10-01T08:57:39Z

Uh, so I guess continuing on PR #258361

ju1m · 2023-12-22T00:33:06Z

nixos/modules/services/networking/knot.nix

-        type = types.lines;
-        default = "";
+      settings = mkOption {
+        type = types.attrs;


Thanks for this huge improvement! Please be aware that this type.attrs does not merge the settings as expected with other options, especially when the configuration is split between modules. Using a freeform submodule as suggested in the RFC followed by this PR would solve that.

vcunat added 6.topic: nixos 2.status: work-in-progress 9.needs: community feedback labels Mar 1, 2020

ofborg bot added the 8.has: module (update) label Mar 1, 2020

vcunat mentioned this pull request Mar 1, 2020

nixos/knot: add keyFiles option #79266

Merged

10 tasks

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

This comment has been minimized.

Sign in to view

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 28, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 25, 2020

vcunat mentioned this pull request May 23, 2021

Feature request: validate kresd extraConfig syntax at build time #124155

Open

lheckemann mentioned this pull request Jun 3, 2021

RFC42 style knot config #125543

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 9, 2022

mweinelt removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 21, 2022

mweinelt force-pushed the p/knot-nixConfig branch from 3ff94a1 to a762cca Compare July 9, 2023 21:36

vcunat mentioned this pull request Aug 28, 2023

knot-dns: 3.2.9 -> 3.3.0 #251969

Merged

12 tasks

vcunat removed the 2.status: work-in-progress label Sep 23, 2023

vcunat and others added 5 commits September 23, 2023 10:04

nixos/knot: allow full configuration by nix values (RFC 42)

7fb737d

nixos/knot: also allow config by YAML file

ce85980

nixosTests.knot: use settings format

8e93f35

nixosTests.kea: use knot.settings for configuration

45e71a7

nixos/knot: add release notes and partial compatibility

1869818

vcunat force-pushed the p/knot-nixConfig branch from 71c894f to 1869818 Compare September 23, 2023 08:06

github-actions bot added 8.has: documentation 8.has: changelog labels Sep 23, 2023

lheckemann approved these changes Sep 23, 2023

View reviewed changes

mweinelt approved these changes Sep 23, 2023

View reviewed changes

mweinelt merged commit 1ff350f into NixOS:master Sep 23, 2023
19 checks passed

vcunat deleted the p/knot-nixConfig branch September 23, 2023 19:14

SuperSandro2000 reviewed Sep 28, 2023

View reviewed changes

vcunat mentioned this pull request Oct 1, 2023

nixos/knot: compatibility with 23.05 config #258361

Merged

12 tasks

ju1m reviewed Dec 22, 2023

View reviewed changes

		@@ -97,6 +97,8 @@

		- `pass` now does not contain `password-store.el`. Users should get `password-store.el` from Emacs lisp package set `emacs.pkgs.password-store`.

		- `services.knot` now supports `.settings` from RFC42. The change is not 100% compatible with the previous `.extraConfig`.

nixos/knot: allow full configuration by nix values #81460

nixos/knot: allow full configuration by nix values #81460

Conversation

vcunat commented Mar 1, 2020 • edited

vcunat commented Mar 1, 2020

vcunat commented Mar 1, 2020

This comment has been minimized.

nixos-discourse commented Dec 25, 2020

mweinelt commented May 23, 2021

vcunat commented May 23, 2021

stale bot commented Jan 9, 2022

sinavir commented Jul 9, 2023

vcunat commented Jul 9, 2023 • edited

mweinelt commented Jul 9, 2023

dummy987654321 commented Jul 10, 2023

dummy987654321 commented Jul 10, 2023

dummy987654321 commented Jul 10, 2023

mweinelt commented Jul 10, 2023

dummy987654321 commented Jul 10, 2023

mweinelt commented Jul 10, 2023 • edited

vcunat commented Aug 13, 2023

vcunat commented Aug 13, 2023

mweinelt commented Aug 29, 2023 • edited

vcunat commented Sep 23, 2023

lheckemann left a comment

Choose a reason for hiding this comment

mweinelt commented Sep 23, 2023

SuperSandro2000 commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcunat Sep 28, 2023 • 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

SuperSandro2000 commented Sep 28, 2023

vcunat commented Sep 28, 2023

SuperSandro2000 commented Sep 28, 2023

vcunat commented Sep 28, 2023

vcunat commented Sep 28, 2023

vcunat commented Oct 1, 2023

ju1m Dec 22, 2023 • edited

Choose a reason for hiding this comment

vcunat commented Mar 1, 2020 •

edited

vcunat commented Jul 9, 2023 •

edited

mweinelt commented Jul 10, 2023 •

edited

mweinelt commented Aug 29, 2023 •

edited

vcunat Sep 28, 2023 •

edited

ju1m Dec 22, 2023 •

edited