nixos/networkmanager: tiny cleanups #15560
Conversation
|
By analyzing the blame information on this pull request, we identified @lethalman, @rickynils and @nmikhailov to be potential reviewers |
|
cc @joachifm |
| @@ -132,6 +134,22 @@ in { | |||
| apply = list: (attrValues cfg.basePackages) ++ list; | |||
| }; | |||
|
|
|||
| dhcp = mkOption { | |||
| type = types.enum [ "dhclient" "dhcpcd" "internal" ]; | |||
| default = "dhclient"; | |||
There was a problem hiding this comment.
Why not use internal as the default?
There was a problem hiding this comment.
Upstream's default is dhclient and dhcp is being pulled in as a dependency anyway. Next phase should be to not have to pull those in ref my commit note.
|
I don't use NetworkManager, so perhaps this is obvious, but what happens if I set |
|
My guess is that networkmanager will assume that unbound is available and probably fail in a very non-graceful manner. Updating PR. |
|
Actually, it totally doesn't work with unbound as there are some expectations about script locations that break. So one way to deal with this is simply to not support |
|
Generally, I think its okay to not support the full range of values the underlying daemon accepts, as long as this is clearly communicated and/or invalid states are eliminated via asserts. I wonder whether the interface is too crude, though. Primarily, it's unclear to me how these options interact with the corresponding services NixOS provides. Like, if I use both the NixOS dnsmasq service and set As a prospective user, I'd care more about features than implementations; without knowing what each does, the choice between Anyway, it'd be better to get some feedback from actual users of NetworkManager ... maybe all this makes perfect sense, but it really doesn't to me. |
peterhoeg
force-pushed
the
nm_dnsdhcp
branch
4 times, most recently
from
7a3f3c1
to
f5dbf5f
Compare
|
Is there any consensus on what the user-facing solution should look like? If not, should we close this? |
|
Is there any consensus on what the user-facing solution should look like?
If not, should we close this?
I've been using this patch locally since I submitted the PR as it is the
only way (that I know of) to get name resolution working over VPN.
The recent NM also supports using systemd-resolved but I haven't looked
at that yet.
I'll gladly make changes to support whatever is the consensus as I think
it's an important feature.
|
| ${optionalString (cfg.unmanaged != []) | ||
| ''unmanaged-devices=${lib.concatStringsSep ";" cfg.unmanaged}''} | ||
|
|
||
| [logging] | ||
| level=WARN | ||
| level=${cfg.logLevel} | ||
| audit=${if config.security.audit.enable then "true" else "false"} |
There was a problem hiding this comment.
Looks like this patch has already been integrated into master. Except for this line.
There was a problem hiding this comment.
Nice catch. There the audit bit as well as dbus config for dnsmasq.
peterhoeg
force-pushed
the
nm_dnsdhcp
branch
from
2c826c4
to
5a49163
Compare
peterhoeg
force-pushed
the
nm_dnsdhcp
branch
from
5a49163
to
7d97c25
Compare
These are the leftovers of an older PR. a. Send messages to auditd if auditing is enabled. b. Add missing dbus configuration if dnsmasq is used for DNS
peterhoeg
force-pushed
the
nm_dnsdhcp
branch
from
7d97c25
to
1c7aaf2
Compare
peterhoeg
changed the title
This adds support for specifying the dhcp and dns options to networkmanager. It just follows upstream's defaults if not specified.
Especially the
dnspart is interesting as it allows us to do split lookups when connected to a VPN.The
dhcptechnically allows us not to pull indhclientordhcpcdas it can use the internal library, but this hasn't been done yet.Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)