DNS related changes #21999
DNS related changes #21999
Conversation
|
@rnhmjoj usually distincts packages updates are splitted in multiple PR. You PR mix package creation with package update and refactoring. IMHO, your changeset could be merged faster splitted in 4 PR (dnschain, power dns, namecoind, dnscrypt) |
| ''; | ||
| }; | ||
|
|
||
| extraConfig = mkOption { |
There was a problem hiding this comment.
An option to enable DNSSEC validation might be usefull https://doc.powerdns.com/md/recursor/dnssec/.
There was a problem hiding this comment.
From https://tools.ietf.org/html/rfc6761 section #6.1 Domain Name Reservation Considerations for Private Addresses
Some usual local address reverse zones should be provided :
... Instead, caching DNS servers SHOULD, by
default, generate immediate (positive or negative) responses for
all such queries. This is to avoid unnecessary load on the root
name servers and other name servers.
There was a problem hiding this comment.
I might be biased (as a DNS resolver dev for day-job), but "validate" seems a better default value.
|
Ok, thanks. I'll split the PR. |
| port = ${toString cfg.api.port} | ||
| tlsPort = ${toString cfg.api.tlsPort} | ||
|
|
||
| [namecoin] |
There was a problem hiding this comment.
The [namecoin] section should be added in dnschain config by assigning services.dnschain.extraConfig in nixos/modules/services/networking/namecoind.nix. There is no reason for nixos/modules/services/networking/dnschain.nix to use variables named cfg.namecoin.configFile and (above all) cfgs.namecoind.enable
There was a problem hiding this comment.
I haven't thought about this, it's definitely better.
| }; | ||
|
|
||
|
|
||
| namecoin.configFile = mkOption { |
|
|
||
| }; | ||
|
|
||
| services.dnsmasq.resolveDNSChainQueries = mkOption { |
There was a problem hiding this comment.
While technically possible, it is bad practice to have services.dnsmasq options defined in dnschain.nix. As first approximation, I would simply remove this option, and configure dnsmask unconditonnally. (see below).
|
|
||
| services.namecoind.enable = true; | ||
| services.dnsmasq.servers = optionals cfgs.dnsmasq.resolveDNSChainQueries |
There was a problem hiding this comment.
as explained above. remove the cfgs.dnsmasq.resolveDNSChainQueries option and add this config snippet unconditionally.
There was a problem hiding this comment.
I don't know, it doesn't look like a good idea to add these without the user knowing: it should be something optional. If it's really bad to have options defined in a different file then I may drop this entirely.
| createHome = true; | ||
| }; | ||
| services.dnschain.namecoin.configFile = | ||
| mkIf config.services.dnschain.enable configFile; |
There was a problem hiding this comment.
There is no need to make this conditionnal.
The following snippet would be sufficient.
services.dnschain.extraConfig = ''
[namecoin]
config = ${configFile}
'';If config.services.dnschain.enable is not true, then the extra config will not be used anyway.
|
|
||
| let | ||
| np = nodePackages.override { generated = ./package.nix; self = np; }; | ||
| in | ||
| nodePackages = callPackage (import ../../top-level/node-packages.nix) { |
There was a problem hiding this comment.
Why did you replace the previous expression by this import statement ? They are basically the same, but yours takes more space and uses top-level/node-packages.nix directly. Why not use nodePackages.override ?
Motivation for this change
I decided to migrate my homeserver to NixOS and found a few things still missing from nixpkgs.
This PR mainly focuses on the DNS stack, changes are:
Things done
nix-shell -p nox --run "nox-review wip"./result/bin/)I think it should be possible to pull these commits separately if you think it's too much for a single PR.