DRAFT: NetworkManager connection profile (keyfile) generation#1267
DRAFT: NetworkManager connection profile (keyfile) generation#1267Rolv-Apneseth wants to merge 2 commits intocoreos:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: the ability to generate NetworkManager connection profiles. The changes are well-structured, adding a new command-line option, implementing the configuration generation logic in src/network.rs, and integrating it into the provider infrastructure. The Packet provider is also updated to support this new feature.
My review focuses on a few key areas:
- A potential panic due to integer underflow which should be addressed.
- Opportunities to improve code maintainability by reducing duplication.
- A suggestion to improve code readability in one of the loops.
Overall, this is a great addition and the code is mostly in good shape for a draft. The feedback provided should help finalize it.
| writeln!( | ||
| config, | ||
| "autoconnect-priority={}", | ||
| // Lower number means higher priority for systemd, but it's the opposite for NM | ||
| 100 - (self.priority as u16) | ||
| )?; |
There was a problem hiding this comment.
The calculation for autoconnect-priority can cause a panic. self.priority is a u8 and can have a value up to 255. If it's greater than 100, subtracting it from 100 will cause an integer underflow, which panics in debug builds and wraps in release builds, leading to unexpected priority values.
You should use saturating_sub to prevent this, as you've correctly done for VirtualNetDev.
| writeln!( | |
| config, | |
| "autoconnect-priority={}", | |
| // Lower number means higher priority for systemd, but it's the opposite for NM | |
| 100 - (self.priority as u16) | |
| )?; | |
| writeln!( | |
| config, | |
| "autoconnect-priority={}", | |
| // Lower number means higher priority for systemd, but it's the opposite for NM | |
| 100i32.saturating_sub(i32::from(self.priority)) | |
| )?; |
There was a problem hiding this comment.
Maybe we should instead restrict the priority to a type that only allows 0-99? Seems like a misconfiguration of the priority value otherwise
| fn write_nm_config_common(&self, config: &mut String) -> Result<()> { | ||
| // [ipv4] section | ||
| writeln!(config, "\n[ipv4]")?; | ||
|
|
||
| let ipv4_addresses: Vec<_> = self.ip_addresses.iter().filter(|a| a.is_ipv4()).collect(); | ||
| if matches!(self.dhcp, Some(DhcpSetting::V4 | DhcpSetting::Both)) { | ||
| writeln!(config, "method=auto")?; | ||
| } else if ipv4_addresses.is_empty() { | ||
| writeln!(config, "method=disabled")?; | ||
| } else { | ||
| writeln!(config, "method=manual")?; | ||
| for (i, addr) in ipv4_addresses.iter().enumerate() { | ||
| writeln!(config, "address{}={}", i + 1, addr)?; | ||
| } | ||
|
|
||
| // IPv4 gateway | ||
| let (default_route, ipv4_routes): (Vec<NetworkRoute>, Vec<NetworkRoute>) = self | ||
| .routes | ||
| .iter() | ||
| .filter(|r| r.destination.is_ipv4()) | ||
| .partition(|r| r.destination.prefix() == 0); | ||
| if let Some(default_route) = default_route.first() { | ||
| writeln!(config, "gateway={}", default_route.gateway)?; | ||
| } | ||
|
|
||
| // IPv4 routes (non-default) | ||
| for (i, route) in ipv4_routes.iter().enumerate() { | ||
| writeln!( | ||
| config, | ||
| "route{}={},{},0", | ||
| i + 1, | ||
| route.destination, | ||
| route.gateway | ||
| )?; | ||
| } | ||
| } | ||
|
|
||
| // IPv4 DNS servers | ||
| let ipv4_dns: Vec<_> = self.nameservers.iter().filter(|n| n.is_ipv4()).collect(); | ||
| if !ipv4_dns.is_empty() { | ||
| let dns_list: Vec<_> = ipv4_dns | ||
| .iter() | ||
| .map(std::string::ToString::to_string) | ||
| .collect(); | ||
| writeln!(config, "dns={}", dns_list.join(";"))?; | ||
| } | ||
|
|
||
| // [ipv6] section | ||
| writeln!(config, "\n[ipv6]")?; | ||
|
|
||
| let ipv6_addresses: Vec<_> = self.ip_addresses.iter().filter(|a| a.is_ipv6()).collect(); | ||
| if matches!(self.dhcp, Some(DhcpSetting::V6 | DhcpSetting::Both)) { | ||
| writeln!(config, "method=auto")?; | ||
| } else if ipv6_addresses.is_empty() { | ||
| writeln!(config, "method=disabled")?; | ||
| } else { | ||
| writeln!(config, "method=manual")?; | ||
| for (i, addr) in ipv6_addresses.iter().enumerate() { | ||
| writeln!(config, "address{}={}", i + 1, addr)?; | ||
| } | ||
|
|
||
| // IPv6 gateway | ||
| let (default_route, ipv6_routes): (Vec<NetworkRoute>, Vec<NetworkRoute>) = self | ||
| .routes | ||
| .iter() | ||
| .filter(|r| r.destination.is_ipv6()) | ||
| .partition(|r| r.destination.prefix() == 0); | ||
| if let Some(default_route) = default_route.first() { | ||
| writeln!(config, "gateway={}", default_route.gateway)?; | ||
| } | ||
|
|
||
| // IPv6 routes (non-default) | ||
| for (i, route) in ipv6_routes.iter().enumerate() { | ||
| writeln!( | ||
| config, | ||
| "route{}={},{},0", | ||
| i + 1, | ||
| route.destination, | ||
| route.gateway | ||
| )?; | ||
| } | ||
| } | ||
|
|
||
| // IPv6 DNS servers | ||
| let ipv6_dns: Vec<_> = self.nameservers.iter().filter(|n| n.is_ipv6()).collect(); | ||
| if !ipv6_dns.is_empty() { | ||
| let dns_list: Vec<_> = ipv6_dns | ||
| .iter() | ||
| .map(std::string::ToString::to_string) | ||
| .collect(); | ||
| writeln!(config, "dns={}", dns_list.join(";"))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The function write_nm_config_common has significant code duplication between the IPv4 and IPv6 configuration blocks. The logic for determining the method, writing addresses, handling routes, and setting DNS servers is nearly identical for both IP versions.
To improve maintainability and readability, I recommend refactoring this logic. You could extract the common functionality into a helper function that is parameterized by the IP version. This would make the code more concise and easier to manage in the future.
Rolv-Apneseth
force-pushed
the
network_manager
branch
from
13dd15c to
e5cfdae
Compare
1 participant
I will be creating an issue for discussion, but as mentioned in #1266, I think this would be nice to have in
afterburn. It is particularly important for the Hetzner implementation though as we need to fallback to IPv4 DHCP initially in order to request the network configuration metadata from Hetzner's locally available HTTP endpoint, meaning that as far as I understand it passing network kargs to initrd can't work.With these changes and the Hetner PR, I've managed to configure an FCOS Hetzner system using the following butane snippet:
If we do go ahead with adding this as a feature, I would appreciate any feedback on the code.
Also, I've initially pushed 2 commits, with the difference being how VLANs/Bonds are handled. The first commit tries to directly translate from the
VirtualNetDev::sd_netdev_sectionsto the NetworkManager equivalents, though this had some limitations due to the fundamentally different 1 file vs several per connection, and required a lot of conversions of both keys and values. The second commit instead shifts the burden to the provider implementation, requiring a newVirtualNetDev::nm_sections, which comes out a lot simpler overall in my opinion but I'm open to suggestions.