Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/bind: allow zone declaration in nix expression #87973

Closed
wants to merge 2 commits into from

Conversation

paumr
Copy link
Contributor

@paumr paumr commented May 16, 2020

Motivation for this change

This PR will allow the declaration of zone-files with a nix expression.

Minimal example:

        {
          name = "minimal";
          file = {
            records = [
              { name = "ns"; value = "192.168.1.1"; }
            ];
          };
        }
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

''
zone "${name}" {
type ${if master then "master" else "slave"};
file "${if (builtins.isAttrs file) && !(isDerivation file) then zoneFile { inherit name; opts = file; } else file}";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to solve this?

file was previously untyped, thus str, path, or derivation (which result in a path) was accepted (maybe even more types?)
Now file should allow the previous types OR the new submodule, what's the best way to check which one it is?

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reformatting noise, please.

nixos/modules/services/networking/bind.nix Outdated Show resolved Hide resolved
@paumr paumr changed the title WIP nixos/bind: allow zone declaration in nix expression nixos/bind: allow zone declaration in nix expression May 17, 2020
@paumr paumr mentioned this pull request Aug 10, 2020
10 tasks
@vcunat
Copy link
Member

vcunat commented Aug 15, 2020

No hard opinion, but I'd probably put tools for zone-file generation separately, as they could be used more universally (typically in other DNS servers).

🤔 though perhaps I'm personally still unclear why

{ name = "ns"; value = "192.168.1.1"; }

seems easier than

"ns A 192.168.1.1"

(using interpolations if need be)

@paumr
Copy link
Contributor Author

paumr commented Aug 15, 2020

No hard opinion, but I'd probably put tools for zone-file generation separately, as they could be used more universally (typically in other DNS servers).

Where would you put it?

thinking though perhaps I'm personally still unclear why

{ name = "ns"; value = "192.168.1.1"; }

seems easier than

"ns A 192.168.1.1"

(using interpolations if need be)

The idea was to abstract away the implementation details (aka. zone file).
This way:

  • the user can rely on nix, rather than reading through the zone-file specs before setting up a DNS server
  • the user input can be type-checked

Reasoning on why not nix A 192.168.1.1

  • As a user i wouldn't be sure if this implies a leading whitespace or not.
  • As a user i'd have to check the file specs to see if it's possible to mix Tabs/Spaces, and check the module source to see which one is used

Also: This format works quite nicely with more complex setups, e.g.:

  records = map # add default type (not required since it's already A)
    (x: x // { type = "A"; }) [
    { name = "toaster"; value = "10.0.3.1"; }
    { name = "localnews"; value = ip; }
    { name = "router"; value = ip; }
    { name = "ns"; value = ip; }
  ] ++ # reserved devices (aka poor man's zeroconf)
  map (x: { name = x.hostName; value = x.ipAddress; }) machines;

@vcunat
Copy link
Member

vcunat commented Aug 15, 2020

the user input can be type-checked

I'd rather have the zone checked by a DNS server (during build time of the configs).

Overall it's perhaps a personal preference of not introducing yet another format unless there's some significant benefit, but I might be a little biased due to knowing the zonefile format (though I know lots of nix as well).

Also: This format works quite nicely with more complex setups

I'm not sure whether it's such a big difference; I think nix is quite good with string manipulation, though perhaps those functions are a little more cumbersome:

lib.concatMapStringsSep "\n" (x: "${x.hostName} A ${x.ipAddress}")

@paumr
Copy link
Contributor Author

paumr commented Aug 15, 2020

the user input can be type-checked

I'd rather have the zone checked by a DNS server (during build time of the configs).

The DNS server still checks the format, this merely adds an additional check during compile-time (to prevent failure upon deployment).

Overall it's perhaps a personal preference of not introducing yet another format unless there's some significant benefit, but I might be a little biased due to knowing the zonefile format (though I know lots of nix as well).

Also: This format works quite nicely with more complex setups

I'm not sure whether it's such a big difference; I think nix is quite good with string manipulation, though perhaps those functions are a little more cumbersome:

lib.concatMapStringsSep "\n" (x: "${x.hostName} A ${x.ipAddress}")

I guess you are right, it's a matter of preference, everyone can easily write his/her own zone-file generator on a per-project basis.

To add a comparison of the two configuration methods (both would be possible after the change):

{
  name = ".";
  file =
    let
      enries = [
        { name = "ns"; value = "192.168.1.1"; }
        { name = "webserver"; value = "192.168.1.1"; }
      ];
      primary_name = "ns.example.org";
      mail = "admin.example.org";
    in
      pkgs.writeText "root.zone" ''
        $TTL 3600
        . IN SOA ${primary_name}. ${mail}. ( 1 8h 2h 4w 1d )
        . IN NS ${primary_name}.
        ${lib.concatMapStringsSep "\n" (x: "${x.name} A ${x.value}") entries}
      '';
}
{
  name = "example.org";
  file = {
    serial = 1;
    name_primary = "ns"; # default value - not required
    owner = "admin.example.org"; # default generated value - not required
    records = [
      { name = "ns"; value = "192.168.1.1"; }
      { name = "webserver"; value = "192.168.1.1"; }
    ];
  };
}

Note: this is pseudo code (aka i didn't run it)

@vcunat
Copy link
Member

vcunat commented Aug 15, 2020

No hard opinion, but I'd probably put tools for zone-file generation separately, as they could be used more universally (typically in other DNS servers).

Where would you put it?

Probably somewhere inside the lib attribute (i.e. lib/*.nix), but I don't know if there would be consensus about it in the community. It's true it seems way more specific than what I see in there now. It could e.g. get moved there later after something like this PR gets merged.

@stale
Copy link

stale bot commented Feb 12, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 12, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 17, 2021
@stale
Copy link

stale bot commented Nov 16, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021
@paumr paumr closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants