Skip to content

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

Merged
andir merged 1 commit intoNixOS:masterfrom
rissson:nixos/unbound
May 3, 2021
Merged

nixos/unbound: add settings option, deprecate extraConfig#89572
andir merged 1 commit intoNixOS:masterfrom
rissson:nixos/unbound

Conversation

@rissson
Copy link
Member

@rissson rissson commented Jun 5, 2020 •
edited
Loading

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
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 5, 2020
@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

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jun 5, 2020
@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
@rissson rissson requested review from lovesegfault and volth June 5, 2020 23:58
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 aanderse and infinisil 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.

Ma27
Ma27 reviewed Jun 7, 2020
View reviewed changes
@Ma27
Copy link
Member

Ma27 commented Jun 7, 2020

oops, @aanderse was faster explaning our concerns %)

Ma27
Ma27 reviewed Jun 7, 2020
View reviewed changes
@rissson rissson force-pushed the nixos/unbound branch 4 times, most recently from acc9e82 to 9f4e416 Compare June 8, 2020 14:05
@rissson rissson requested review from Ma27, aanderse and lovesegfault June 8, 2020 14:06
aanderse
aanderse reviewed Jun 8, 2020
View reviewed changes
@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?

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 16, 2021
@rissson
Copy link
Member Author

rissson commented Apr 21, 2021

Anything else blocking from merging this?

@lovesegfault
Copy link
Member

lovesegfault commented Apr 21, 2021

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.

andir
andir reviewed Apr 22, 2021
View reviewed changes
@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!

@github-actions github-actions bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels May 3, 2021
@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

lovesegfault commented May 3, 2021

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

lovesegfault commented May 3, 2021

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

lovesegfault commented May 3, 2021

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

@aanderse aanderse aanderse approved these changes

@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

+2 more reviewers

@wahjava wahjava wahjava approved these changes

@tcbravo tcbravo tcbravo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.