Skip to content

resolvconf: use correct output files when used with dnsmasq#349320

Merged
corngood merged 1 commit intoNixOS:masterfrom
corngood:resolvconf-dnsmasq
Oct 17, 2024
Merged

resolvconf: use correct output files when used with dnsmasq#349320
corngood merged 1 commit intoNixOS:masterfrom
corngood:resolvconf-dnsmasq

Conversation

@corngood
Copy link
Contributor

@corngood corngood commented Oct 17, 2024

Fixes the issue described at #336988 (comment)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@corngood corngood requested a review from rnhmjoj October 17, 2024 16:27
@github-actions github-actions 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 Oct 17, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this instead of a default, so it would be merged with the other values.

Copy link
Contributor Author

@corngood corngood Oct 17, 2024

Choose a reason for hiding this comment

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

Perhaps we should expose the full list of required read/write paths (including /run/resolvconf)? Either by renaming subscriberFiles or by adding a new option?

This may end up needing to be used in other places (e.g. VPN client).

Copy link
Contributor

@rnhmjoj rnhmjoj Oct 17, 2024

Choose a reason for hiding this comment

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

Uhm, let's try to keep it simple, for now: there's not much that interacts with resolvconf and it seems the only module using this subscriber mechanism is dnsmasq.

Before worrying about other unprivileged clients invoking resolvconf, I should deal with the fact it doesn't set the right group on the generated files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before worrying about other unprivileged clients invoking resolvconf, I should deal with the fact it doesn't set the right group on the generated files.

Are you talking about something other than what this PR is fixing? The files all have resolvconf as the group for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, unrelated to dnsmasq: the ownership of files in /run/resolvconf seem to use the wrong group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. Just in case it's useful, this is what I see:

-rw-r----- 1 dhcpcd dhcpcd     115 Oct 17 13:24  /run/resolvconf/interfaces/enp4s0.dhcp
-rw-rw-r-- 1 root   resolvconf  23 Oct 15 23:23  /run/resolvconf/interfaces/static
-rw-rw-r-- 1 root   resolvconf 151 Oct 17 08:32  /run/resolvconf/interfaces/tun0
-rw-rw-r-- 1 root   resolvconf   2 Oct 15 23:23 '/run/resolvconf/metrics/0000001 static'
-rw-r----- 1 dhcpcd dhcpcd       2 Oct 17 13:24 '/run/resolvconf/metrics/0001002 enp4s0.dhcp'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly: those written by dhcpcd have dhcpcd's own uid/gid.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Oct 17, 2024

@GrahamcOfBorg build dhcpcd.tests

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Oct 17, 2024
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@corngood corngood merged commit cd286b2 into NixOS:master Oct 17, 2024
@corngood corngood deleted the resolvconf-dnsmasq branch October 17, 2024 19:44
@Shados
Copy link
Member

Shados commented Oct 27, 2024

@corngood This change means the resolvconf service additionally needs a After and Requires dependencies on the dnsmasq service, as the specified files otherwise may not yet exist and the service will fail like so:

Oct 27 16:34:14 terminus-2 systemd[1]: Starting resolvconf update...
Oct 27 16:34:16 terminus-2 resolvconf-start[2130]: chgrp: cannot access '/etc/dnsmasq-conf.conf': No such file or directory
Oct 27 16:34:16 terminus-2 resolvconf-start[2130]: chgrp: cannot access '/etc/dnsmasq-resolv.conf': No such file or directory
Oct 27 16:34:16 terminus-2 systemd[1]: resolvconf.service: Main process exited, code=exited, status=1/FAILURE
Oct 27 16:34:16 terminus-2 systemd[1]: resolvconf.service: Failed with result 'exit-code'.
Oct 27 16:34:16 terminus-2 systemd[1]: Failed to start resolvconf update.

@corngood
Copy link
Contributor Author

@Shados can you share what's in your /etc/resolvconf.conf? resolvconf should be creating those dnsmasq config files.

@Shados
Copy link
Member

Shados commented Nov 1, 2024

@corngood

resolvconf should be creating those dnsmasq config files.

Ahhh, I see, my bad, I'd mis-understood from dnsmasq's pre-start script touching them. This is just an issue with my specific setup after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants