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.

Sign up for GitHub

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: fix define-tag option #124799

Merged
merged 1 commit into from Jul 21, 2021
Merged

nixos/unbound: fix define-tag option #124799

merged 1 commit into from Jul 21, 2021

Conversation

rissson
Copy link
Member

@rissson rissson commented May 28, 2021

Motivation for this change

Fix #124780

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/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@rissson rissson requested a review from andir May 28, 2021 20:13
andir
andir reviewed May 29, 2021
View reviewed changes
nixos/tests/unbound.nix Outdated
@@ -63,6 +63,7 @@ import ./make-test-python.nix ({ pkgs, lib, ... }:
enable = true;
settings = {
server = {
define-tag = [ "local" "test" ];
interface = [ "192.168.0.1" "fd21::1" "::1" "127.0.0.1" ];
access-control = [ "192.168.0.0/24 allow" "fd21::/64 allow" "::1 allow" "127.0.0.0/8 allow" ];
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also add an access-control-tag statement that uses the tags to ensure that the config is valid? If I read the bug report right then the issue only occurs when the tag is being "used" before it is defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's why I define these tags before anything else.

@rissson
nixos/unbound: fix define-tag option
6b12cff 
Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
@rissson rissson requested a review from andir May 29, 2021 18:27
@rissson rissson mentioned this pull request Jun 12, 2021
Closed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/547

@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Jun 18, 2021 •
edited

I tested this locally, and it fixed one major blocker for me upgrading to 21.05 (namely syntax errors related to the server block)

But I ran into another critical issue with forward-zones: #127386

@rissson
Copy link
Member Author

rissson commented Jun 19, 2021

@andir is there anything blocking this?

@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Jun 22, 2021 •
edited

I was able to load and successfully use my full config and that #127386 isn't an issue with @rissson 's detected fix.

This looks 100% good for me. Now to just have it merged so I don't have to build my system off of a branch :(

@rissson
Copy link
Member Author

rissson commented Jul 1, 2021

Friendly ping that this should get merged.

@pennae
Copy link
Contributor

pennae commented Jul 1, 2021

(still) lgtm for what that's worth

@rissson
Copy link
Member Author

rissson commented Jul 14, 2021

Friendly ping

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/390

@andir
Copy link
Member

andir commented Jul 18, 2021

It is unrealtisic that I'll get around to testing this some time soon. Feel free to merge if it fixes your problem. My overall fear with the new settings structure is that this won't be the last issue or this sort. Not all formats are simple YAML/JSON/... as we see here.

jasoncarr0
jasoncarr0 approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@jasoncarr0 jasoncarr0 left a comment

Choose a reason for hiding this comment

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

I don't recall if my approval counts for anything but it's worth a shot since the official maintainer is good to go with it

@andir andir merged commit ef9be92 into NixOS:master Jul 21, 2021
@github-actions github-actions bot mentioned this pull request Jul 21, 2021
Merged
1 task
@github-actions
Copy link
Contributor

Successfully created backport PR #130978 for release-21.05.

@rissson rissson deleted the nixos-unbound-fix-124780 branch July 21, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pennae pennae pennae left review comments

@jasoncarr0 jasoncarr0 jasoncarr0 approved these changes

@andir andir Awaiting requested review from andir

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

Successfully merging this pull request may close these issues.

unbound config emitted with wrong order of keys
5 participants
@rissson @nixos-discourse @jasoncarr0 @pennae @andir