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

Jump to bottom

nixos/unbound: add settings option, deprecate extraConfig #89572

Merged
merged 1 commit into from May 3, 2021
Merged

nixos/unbound: add settings option, deprecate extraConfig #89572

merged 1 commit into from May 3, 2021

Conversation

rissson
Copy link
Member

@rissson rissson commented Jun 5, 2020 •
edited

Motivation for this change

Follow RFC 42 by adding a settings option. Other options have been deprecated or removed when renaming them was not possible.

It builds on top of #79559 to include the latest modifications to the unbound service.

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.

@rissson rissson requested a review from kraem June 5, 2020 23:41
@rissson
Copy link
Member Author

rissson commented Jun 5, 2020

I am currently using this here without any problems so far.

@rissson
Copy link
Member Author

rissson commented Jun 5, 2020

Also, I'm willing to add myself as a maintainer for this module :D

@rissson rissson changed the title Nixos/unbound nixos/unbound: add settings option Jun 5, 2020
@rissson rissson changed the title nixos/unbound: add settings option nixos/unbound: add settings option, deprecate extraConfig Jun 5, 2020
lovesegfault
lovesegfault approved these changes Jun 6, 2020
View reviewed changes
Copy link
Member

@lovesegfault lovesegfault left a comment

Choose a reason for hiding this comment

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

Welcome improvement :)

@Ma27 Ma27 requested review from infinisil and aanderse June 7, 2020 20:09
aanderse
aanderse requested changes Jun 7, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I love PRs that implement settings options! Unfortunately I don't know anything at all about unbound, so if some of my comments are entirely invalid I apologize in advance. Hopefully at least some of my comments will be useful.

nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/unbound.nix Show resolved Hide resolved
@Ma27
Copy link
Member

Ma27 commented Jun 7, 2020

oops, @aanderse was faster explaning our concerns %)

@rissson rissson force-pushed the nixos/unbound branch 4 times, most recently from acc9e82 to 9f4e416 Compare June 8, 2020 14:05
@rissson
Copy link
Member Author

rissson commented Jun 13, 2020

Actually, before merging that, I'd like to add configuration checking at build time.

@rissson
Copy link
Member Author

rissson commented Jun 13, 2020

Actually, before merging that, I'd like to add configuration checking at build time.

Argh it seems unbound wants its chroot directory to exist when checking the configuration. Is there a way to create, for instance, /var/lib/unbound/ when using pkgs.runCommandNoCC?

@Ma27
Copy link
Member

Ma27 commented Jun 13, 2020

It should be sufficient to create it in the build sandbox (I'd expect to get the dir thrown away after that) or am I missing something?

@rissson
Copy link
Member Author

rissson commented Apr 21, 2021

Anything else blocking from merging this?

@lovesegfault
Copy link
Member

I suspect verifying that even without Restart = "on-failure"; all the tests pass is the only remaining thing here.

@rissson
Copy link
Member Author

rissson commented Apr 21, 2021

They don't:

authoritative # [    7.590374] unbound[775]: [1619032662] unbound[775:0] error: can't bind socket: Cannot assign requested address for fd21::1 port 53
authoritative # [    7.607125] unbound[775]: [1619032662] unbound[775:0] fatal error: could not open ports
authoritative # [    7.612835] systemd[1]: unbound.service: Main process exited, code=exited, status=1/FAILURE

and I have no idea why this is happening.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 22, 2021

It's trying to bind to a ULA address that has been configured manually. I'd say the service is running before network-setup.service and the interface is not set up, yet.

@rissson
Copy link
Member Author

rissson commented May 3, 2021

With the tests problem fixed, would it be possible to get this in for 21.05?

@andir
Copy link
Member

andir commented May 3, 2021

With the tests problem fixed, would it be possible to get this in for 21.05?

If you write the changelog entry this is good to go in :)

@rissson
Copy link
Member Author

rissson commented May 3, 2021

Done!

@andir
Copy link
Member

andir commented May 3, 2021

LGTM just remove that cherry pick line from the commit message. This should be the authoritative commit.

@rissson
Copy link
Member Author

rissson commented May 3, 2021

Done.

@andir
Copy link
Member

andir commented May 3, 2021

Done.

Did yo accidentally squash to changes? The commit message still doesn't look "clean".

@rissson
Copy link
Member Author

rissson commented May 3, 2021

I don't think so. Does this look better?

@lovesegfault
Copy link
Member

I don't think so. Does this look better?

I suspect nixos/unbound: deprecate extraConfig in favor of settings might be more descriptive.

As-is, it sounds like settings was added but nothing was deprecated when reading the commit title.

@rissson
nixos/unbound: deprecate extraConfig in favor of settings
52f6733 
Follow RFC 42 by having a settings option that is
then converted into an unbound configuration file
instead of having an extraConfig option.

Existing options have been renamed or kept if
possible.

An enableRemoteAccess has been added. It sets remote-control setting to
true in unbound.conf which in turn enables the new wrapping of
unbound-control to access the server locally.  Also includes options
'remoteAccessInterfaces' and 'remoteAccessPort' for remote access.

Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
@lovesegfault
Copy link
Member

Alright, with the new commit message and the tests passing as before this looks A-OK to me. Deferring to @andir to press the big green button :)

@andir andir merged commit 3ec6977 into NixOS:master May 3, 2021
@rissson rissson deleted the nixos/unbound branch May 3, 2021 20:13
@lovesegfault
Copy link
Member

Thank you for pushing this through @rissson! Using unbound in NixOS just got way better!

@kraem kraem mentioned this pull request May 4, 2021
Closed
10 tasks
@infinisil infinisil mentioned this pull request May 6, 2021
Closed
@rissson rissson mentioned this pull request Jun 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli left review comments

@andir andir andir left review comments

@wahjava wahjava wahjava approved these changes

@aanderse aanderse aanderse approved these changes

@tcbravo tcbravo tcbravo left review comments

@rnhmjoj rnhmjoj rnhmjoj approved these changes

@lovesegfault lovesegfault lovesegfault approved these changes

@kraem kraem Awaiting requested review from kraem

@Ma27 Ma27 Awaiting requested review from Ma27

@infinisil infinisil Awaiting requested review from infinisil

@ajs124 ajs124 Awaiting requested review from ajs124

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet