-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
nixos/networkmanager: add declarative profiles #254647
nixos/networkmanager: add declarative profiles #254647
Conversation
d014eaf
to
856bf66
Compare
cc @NixOS/freedesktop |
db6e45b
to
4680b85
Compare
I know that there is still the release notes entry missing before this can be merged other then that I build a small script that converts .nmconnection files to the needed nix code https://github.com/Janik-Haag/nm2nix if no one disagrees I would add that to the description of the module option. There is one small problem with the current implementation and that is that once a profile was created it will only get deleted after a reboot because there is now way to tell nix to delete the old files, one way to circumvent that would be to name every profile created by this module after a pattern like |
05696c9
to
d8b4531
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change, thanks! It doesn’t look too intrusive and I would likely use it. (Sharing wifi setups with multiple devices and household members sounds awesome.)
I gave a few suggestions but nothing deal-breaking.
However, I haven’t tried it out and I definitely think a few people should before we merge it.
Also, a NetworkManager maintainer (if we have something like that) should look over this.
I don’t think that it is a problem that profiles don’t get removed until reboot. That’s why we call it "ensureProfiles" and there is precedent for that e.g. for cups printers.
@@ -379,6 +380,52 @@ in { | |||
for more details. | |||
''; | |||
}; | |||
ensure-profiles = { | |||
profiles = mkOption { | |||
type = types.attrsOf ini.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there are some fields which are definitely mandatory for a profile to work and if we can add assertions that those are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the thing that would make the most sense to check is:
connection = {
id = "realraum";
type = "wifi";
interface-name = "wlp3s0";
};
I think checking if connection exits and has the id attribute makes sense, everything else seems like a lot of work for something that's not really specified.
Adding checks for wifi, ipv4, ipv6, gsm, etc just seems overkill. There is also networkamanger plugins that add custom types and and you can just create as many interfaces adhoc as you want so checking interface-name and type also doesn't make too much sense in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion on how I would implement this assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of something like this:
{
assertion = (forEach(mapAttrsToList(n: v: { inherit v; }) cfg.ensure-profiles.profiles) (profile: builtins.hasAttr "connection" profile));
message = ''
Every NetworkManager profile needs at least a connection setting with a id attribute
'';
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the assert will be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now added the bare-minimum of checks. I would have liked to add more for example something like:
networking.networkmanager.ensure-profiles.profiles = with lib.types; mkOption {
type = attrsOf (submodule {
freeformType = ini.type;
options = {
connection = {
id = lib.mkOption {
default = null;
type = str;
};
type = lib.mkOption {
default = null;
type = str;
};
};
wifi = {
mode = lib.mkOption {
default = null;
type = str;
};
ssid = lib.mkOption {
default = null;
type = str;
};
};
};
});
which works for every wifi connection but not for any other connection type and doing it with a sub modules that defaults to a empty attrset causes the ini contain empty sections which in turn cause the NM parser to fail when parsing the profile files and setting it to null also doesn't work because then the ini atom type check fails. The way to solve this would be to make wifi optional at this level:
wifi = here {
but I don't know of a way to do that and already spent a lot of time on looking at this so I would call it good enough if no one else has a idea on how to solve that in a simple-ish way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just massage it pre serialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look over all children and if you find that all subvalues are nulls, skip the section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should do the trick https://github.com/NixOS/nixpkgs/pull/254647/files#diff-0a708e7b053cf5df7620b5262936553af2242d2ce9dabde5bbeba221ece0a021R402 of filtering out empty attrsets now I have to figure out how to do the wifi submodule thing so it doesn't do the checks if it defaults to empty attrSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I spent my entire evening on this and I wasn't able to come up with a good solution, I honestly would just leave it as is. The NM profiles aren't really officially defined anyways (the official docs are incomplete), NM plugins can add arbitrary fields and in some circumstances you can leave out some fields if you add others. And everything that would in theory work would probably add more complexity then what is actually worth it. So I give up I will not spend anymore time on the checks.
Yeah same that's why I build it, I also had the idea of creating a public repo which exposes some profiles for hackspaces and events like the chaos congress or the mrmcd and people can just add more with PRs to that repo but that's going a bit offtopic for this PR
I checked it and for me it works great, I have ~30 wlan connections imported two wire guard vpns, 1 Ethernet connection and even one gsm/lte profile. A few of them have special characters in there name that's why I added |
7ca142c
to
5a43189
Compare
5a43189
to
aa9153e
Compare
aa9153e
to
b6d71ac
Compare
@@ -379,6 +380,52 @@ in { | |||
for more details. | |||
''; | |||
}; | |||
ensure-profiles = { | |||
profiles = mkOption { | |||
type = types.attrsOf ini.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say something like this:
let
isValidWiFiProfile = ...;
isValidConnection = connection: profile: hasAttr "id" connection && hasAttr "type" connection && (connection.type == "wifi" -> isValidWiFiProfile profile);
isValidProfile = profile: (hasAttr "connection" profile && isValidConnection profile.connection profile)
And hop, we realize that we are reinventing submodules type checking. So let's go for a submodule here with freeform type options.
Then, the asserts should only be on "if this is a WiFi connection, this should have those fields minimally", etc.
b6d71ac
to
e772175
Compare
e772175
to
8937dfb
Compare
8937dfb
to
6b42d52
Compare
aa4f58e
to
09c19f2
Compare
40b54ed
to
f81a116
Compare
f81a116
to
a316aa5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works :)
Description of changes
This PR adds a declarative way of defining network-manager profiles with secrets management. I would appreciate feedback on the nix code :)
Things done
Built on platform(s)
For non-Linux: Is
sandbox = true
set innix.conf
? (See Nix manual)Tested, as applicable:
23.11 Release Notes (or backporting 23.05 Release notes)
Fits CONTRIBUTING.md.