netdog: Add networkd devices and builders for config #3220

zmrow · 2023-06-21T21:40:51Z

Issue number:
Related to #2449

Description of changes:
This PR is best reviewed in commit order! :)

This is the second in a series of changes to netdog that add the ability to generate systemd-networkd config files. This PR adds the structs that represent the systemd-networkd devices we currently use; bonds, vlans, and interfaces. In code, these structs represent the latest version of the underlying config structures.

The bulk of this PR is the addition of builders for the network and netdev structs that represent config files. Using builders has a few advantages. First, a builder allows for centralization of the translation logic that is required to get from our network config related structs to the systemd-networkd structs. Second, a builder allows us to limit the creation methods that can be called, making creation of these structs safer for developers. (We use type parameters under the hood. Putting them on the builder keeps them from proliferating all over the code, another nice bonus)

Lastly, unit tests were added for the builders. They become somewhat redundant once the full integration tests are in place, but I wanted to ensure that everything stayed sane through this development process. (Another nice side effect is we can use the reference files for the integration tests as well.)

So a small diagram of the config generation process and where we are:

          Next to be implemented
 -------------------------------------           This PR                            MERGED #3134
           NET CONFIG                   -----------------------------------    --------------------
+---------------+                               NETWORKD DEVICES                 NETWORKD CONFIG
|               |
| InterfaceV1   +-----------------+   +-----------------+      Builder
|               |                 |   |                 +----------------+
+---------------+                 +-> |NetworkdInterface|                |
+---------------+                 +-> |                 |                |     +-----------------+
|               |                 |   |                 |                +---> |                 |
| InterfaceV2   +-----------------+   +-----------------+                      |                 |
|               |                                                 +----------> | .network        |
+---------------+                     +-----------------+ Builder |            |                 |
                                      |                 +---------+            |                 |
+---------------+               +---> | NetworkdBond    |              +-----> +-----------------+
|               |               |     |                 +---------+    |
|               +---------------+     |                 | Builder |    |       +-----------------+
| BondV1        |                     +-----------------+         |    |       |                 |
|               |                                                 +----+-----> |                 |
+---------------+                     +-----------------+              |       | .netdev         |
                                      |                 +--------------+       |                 |
+---------------+                     | NetworkdVlan    |                 +->  |                 |
|               |                +--> |                 |   Builder       |    +-----------------+
|               |                |    |                 +-----------------+
| VlanV1        +----------------+    +-----------------+
|               |
+---------------+

Testing done:

All old and new unit tests pass!
Build an aws-k8s-1.24 image to ensure nothing bled into existing images

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

This commit adds adds constants for the networkd config directory, file prefix. Network and netdev config each get an associated constant for the file extension, and each gain a method `write_config_file()` that writes an appropriately named file to the proper path.

zmrow · 2023-06-22T18:49:41Z

sources/api/netdog/src/networkd/config/netdev.rs

+    fn netdev_from_bond(bond: NetworkDBond) -> NetDevConfig {
+        let mut netdev = NetDevBuilder::new_bond(bond.name.clone());
+        netdev.with_mode(bond.mode);
+        bond.min_links.map(|m| netdev.with_min_links(m));
+        match bond.monitoring_config {
+            BondMonitoringConfigV1::MiiMon(miimon) => netdev.with_miimon_config(miimon),
+            BondMonitoringConfigV1::ArpMon(arpmon) => netdev.with_arpmon_config(arpmon),
+        }
+        netdev.build()
+    }
+
+    fn netdev_from_vlan(vlan: NetworkDVlan) -> NetDevConfig {
+        let mut netdev = NetDevBuilder::new_vlan(vlan.name.clone());
+        netdev.with_vlan_id(vlan.id);
+        netdev.build()
+    }


This logic will most likely go away once it's integrated into the NetworkD* device code. It exists to enable unit testing now before that code is available.

zmrow · 2023-06-22T18:50:09Z

sources/api/netdog/src/networkd/config/network.rs

+    fn network_from_interface(iface: NetworkDInterface) -> NetworkConfig {
+        let mut network = NetworkBuilder::new_interface(iface.name);
+        network.with_dhcp(iface.dhcp4, iface.dhcp6);
+        iface.static4.map(|s| network.with_static_config(s));
+        iface.static6.map(|s| network.with_static_config(s));
+        iface.routes.map(|r| network.with_routes(r));
+        network.build()
+    }
+
+    fn network_from_vlan(vlan: NetworkDVlan) -> NetworkConfig {
+        let mut network = NetworkBuilder::new_vlan(vlan.name);
+        network.with_dhcp(vlan.dhcp4, vlan.dhcp6);
+        vlan.static4.map(|s| network.with_static_config(s));
+        vlan.static6.map(|s| network.with_static_config(s));
+        vlan.routes.map(|r| network.with_routes(r));
+        network.build()
+    }
+
+    fn network_from_bond(bond: NetworkDBond) -> NetworkConfig {
+        let mut network = NetworkBuilder::new_bond(bond.name.clone());
+        network.with_dhcp(bond.dhcp4, bond.dhcp6);
+        bond.static4.map(|s| network.with_static_config(s));
+        bond.static6.map(|s| network.with_static_config(s));
+        bond.routes.map(|r| network.with_routes(r));
+        network.build()
+    }


This logic will most likely go away once it's integrated into the NetworkD* device code. It exists to enable unit testing now before that code is available.

sources/api/netdog/test_data/networkd/network/eno3.network

sources/api/netdog/test_data/networkd/network/eno4.network

sources/api/netdog/test_data/networkd/network/eno7.network

sources/api/netdog/test_data/networkd/builder.toml

sources/api/netdog/src/networkd/devices/mod.rs

cbgbt · 2023-06-22T20:57:10Z

sources/api/netdog/src/networkd/config/mod.rs

+const CONFIG_FILE_PREFIX: &str = "10-";
+


How is this prefix selected? How do we know it doesn't conflict with other files?

Netplan uses a similar 10- prefix, and I didn't see a compelling reason to change prefix.

The reason for the prefix is that systemd will process all of these files in lexicographical order. We should be the only process putting files here.

sources/api/netdog/src/networkd/config/netdev.rs

cbgbt · 2023-07-01T01:38:35Z

sources/api/netdog/src/networkd/config/netdev.rs

+
+impl NetDevBuilder<Bond> {
+    /// Create a new .netdev config for a bond.
+    pub(crate) fn new_bond(name: InterfaceName) -> Self {


naming nit: I might just refer to this as new, calling it new_bond implied to me an ability to create multiple new bonds at once, but this is instantiating the builder.

Unfortunately, a NetDevBuilder<T> can't have multiple new() methods (I was a little surprised too...), hence making the constructors a bit more descriptive about what will be returned.

cbgbt · 2023-07-01T01:41:04Z

sources/api/netdog/src/networkd/config/netdev.rs

+    }
+
+    /// Add bond mode
+    pub(crate) fn with_mode(&mut self, mode: BondModeV1) {


nit: You might be able to avoid maintaining this match statement using Into.

Suggested change

pub(crate) fn with_mode(&mut self, mode: BondModeV1) {

pub(crate) fn with_mode<B: Into<BondMode>>(&mut self, mode: B) {

I thought about that and decided against it for no reason other than it felt like an odd place to toss in a slightly different interface and another trait to impl, which would contain the match statement anyway. Happy to make the change if you feel it's important.

sources/api/netdog/src/networkd/config/mod.rs

cbgbt · 2023-07-01T01:53:15Z

sources/api/netdog/src/networkd/config/network.rs

+    static ref DEFAULT_ROUTE_IPV4: IpNet = "0.0.0.0/0".parse().unwrap();
+    static ref DEFAULT_ROUTE_IPV6: IpNet = "::/0".parse().unwrap();


nit: It's probably safer/better practice to use the proper constructors for these, rather than parsing a string. While we can guarantee these are correct in code review, this would theoretically panic at runtime and can't be checked by the compiler.

Unless the constructor is a const function then it's unavoidable, right? What you could do is move the constants to the same module that defines IpNet and put these strings directly into the struct bypassing the parsing constructor.

I think it's unavoidable, even if we use the proper constructors.

move the constants to the same module that defines IpNet

IpNet is an external crate, so I don't think that will be possible.

cbgbt · 2023-07-01T01:56:14Z

sources/api/netdog/src/networkd/config/network.rs

+impl NetworkBuilder<Interface> {
+    /// Create a new .network config for an interface; not meant for an interface that will be a
+    /// bond worker.  See `.new_bond_worker()` for that case.
+    pub(crate) fn new_interface<I>(id: I) -> Self


Same nit here as with bonds.

This commit adds a module that contains the structs representing the network devices we currently configure: interfaces, bonds, and vlans. These structures are meant to contain the latest versions of all the underlying config.

This commit adds attributes to the networkd device structs to derive Deserialize to enable unit testing. Since we never directly deserialize into these device structs during normal use, gating the Deserialize implementation behind the test attribute keeps the binary size a bit smaller for "normal" builds.

sources/api/netdog/src/networkd/config/netdev.rs

webern · 2023-07-05T17:28:55Z

sources/api/netdog/src/networkd/config/network.rs

+    /// Build the proper prefixed path for the config file
+    fn config_path(&self) -> Result<PathBuf> {
+        let match_section = self
+            .r#match


woah what is this syntax?

The r#match? Since match is a rust keyword you need to "escape" it in order to use it as an identifier. "match" makes the most sense in this context, so I opted not to rename it.

sources/api/netdog/src/networkd/config/network.rs

sources/api/netdog/src/networkd/config/mod.rs

sources/api/netdog/src/networkd/config/netdev.rs

webern · 2023-07-05T17:46:14Z

sources/api/netdog/src/networkd/config/netdev.rs

+        let netdev = NetDevConfig {
+            netdev: Some(NetDevSection {
+                name: Some(name),
+                kind: Some(NetDevKind::Bond),
+            }),
+            ..Default::default()
+        };


I tried to think of a way you could have a single new function and somehow specialize on type, but that is not possible. Overall the idea of specializing on type in order to expose only the correct functions seems consistent with Rust idioms.

webern · 2023-07-05T17:50:06Z

sources/api/netdog/src/networkd/config/network.rs

+    static ref DEFAULT_ROUTE_IPV4: IpNet = "0.0.0.0/0".parse().unwrap();
+    static ref DEFAULT_ROUTE_IPV6: IpNet = "::/0".parse().unwrap();


Unless the constructor is a const function then it's unavoidable, right? What you could do is move the constants to the same module that defines IpNet and put these strings directly into the struct bypassing the parsing constructor.

zmrow · 2023-07-05T21:13:06Z

The above pushes address all comments other than those still being discussed:

Omit spurious DHCP4/6 sections of the protocol is disabled
Pass a path to config_path() to enable testing and add tests
Remove spurious *Device from bond/vlan devices in networkd device enum

This commit adds a buidler for the systemd-networkd NetDevConfig struct. The builder contains the logic to translate the structures deserialized from `net.toml` into NetDev structures representing systemd-networkd .netdev files. Since NetDevConfig will be created for bonds and vlans (and potentially more later), using a builder centralizes the logic to avoid duplication. Use a builder also means methods can be limited based on the type of device being built, providing safety at development time.

This commit adds a builder for the systemd-networkd NetworkConfig struct. The builder contains the logic to translate the structures deserialized from `net.toml` into NetworkConfig structures representing systemd-networkd .network files. Since NetworkConfig will be created for interfaces, bonds, and vlans (and potentially more later), using a builder centralizes the logic to avoid duplication. Using a builder also means methods can limited based on the type of device being built, providing safety at development time.

zmrow · 2023-07-05T22:46:30Z

^ Uses include_str!() for the unit test reference data as suggested by @cbgbt

yeazelm

LGTM!

zmrow added 3 commits June 15, 2023 21:39

netdog: Add IPv6AcceptRA field to networkd config struct

e388c86

netdog: Add "any" to ArpAllTargets enum in netdev config

27cdbb7

zmrow requested review from bcressey, cbgbt and yeazelm June 21, 2023 21:40

zmrow commented Jun 22, 2023

View reviewed changes

yeazelm reviewed Jun 29, 2023

View reviewed changes

sources/api/netdog/test_data/networkd/network/eno3.network Outdated Show resolved Hide resolved

sources/api/netdog/test_data/networkd/network/eno4.network Outdated Show resolved Hide resolved

sources/api/netdog/test_data/networkd/network/eno7.network Show resolved Hide resolved

yeazelm reviewed Jun 29, 2023

View reviewed changes

sources/api/netdog/test_data/networkd/builder.toml Show resolved Hide resolved

zmrow commented Jun 30, 2023

View reviewed changes

sources/api/netdog/src/networkd/devices/mod.rs Outdated Show resolved Hide resolved

cbgbt reviewed Jul 1, 2023

View reviewed changes

zmrow added 2 commits July 5, 2023 16:12

netdog: Add systemd-networkd devices

1294f6e

This commit adds a module that contains the structs representing the network devices we currently configure: interfaces, bonds, and vlans. These structures are meant to contain the latest versions of all the underlying config.

webern reviewed Jul 5, 2023

View reviewed changes

zmrow force-pushed the add-networkd-cfg-builder branch 2 times, most recently from 9b67a53 to bfad4a1 Compare July 5, 2023 21:09

zmrow added 2 commits July 5, 2023 22:39

zmrow force-pushed the add-networkd-cfg-builder branch from bfad4a1 to e1b4e28 Compare July 5, 2023 22:45

zmrow requested review from webern, yeazelm and cbgbt July 6, 2023 17:28

webern approved these changes Jul 6, 2023

View reviewed changes

yeazelm approved these changes Jul 6, 2023

View reviewed changes

cbgbt approved these changes Jul 7, 2023

View reviewed changes

zmrow merged commit abd2599 into bottlerocket-os:develop Jul 11, 2023

zmrow deleted the add-networkd-cfg-builder branch July 11, 2023 15:13

zmrow mentioned this pull request Jul 14, 2023

netdog: Add systemd-networkd config generation #3266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netdog: Add networkd devices and builders for config #3220

netdog: Add networkd devices and builders for config #3220

zmrow commented Jun 21, 2023

zmrow Jun 22, 2023 •

edited

Loading

zmrow Jun 22, 2023 •

edited

Loading

cbgbt Jun 22, 2023

zmrow Jul 5, 2023

cbgbt Jul 1, 2023

zmrow Jul 5, 2023

cbgbt Jul 1, 2023

zmrow Jul 5, 2023 •

edited

Loading

cbgbt Jul 1, 2023

webern Jul 5, 2023

zmrow Jul 5, 2023 •

edited

Loading

cbgbt Jul 1, 2023

webern Jul 5, 2023

zmrow Jul 5, 2023

webern Jul 5, 2023

webern Jul 5, 2023

zmrow commented Jul 5, 2023 •

edited

Loading

zmrow commented Jul 5, 2023

yeazelm left a comment

	pub(crate) fn with_mode(&mut self, mode: BondModeV1) {
	pub(crate) fn with_mode<B: Into<BondMode>>(&mut self, mode: B) {

		static ref DEFAULT_ROUTE_IPV4: IpNet = "0.0.0.0/0".parse().unwrap();
		static ref DEFAULT_ROUTE_IPV6: IpNet = "::/0".parse().unwrap();

netdog: Add networkd devices and builders for config #3220

netdog: Add networkd devices and builders for config #3220

Conversation

zmrow commented Jun 21, 2023

zmrow Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

zmrow Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow commented Jul 5, 2023 • edited Loading

zmrow commented Jul 5, 2023

yeazelm left a comment

Choose a reason for hiding this comment

zmrow Jun 22, 2023 •

edited

Loading

zmrow Jun 22, 2023 •

edited

Loading

zmrow Jul 5, 2023 •

edited

Loading

zmrow Jul 5, 2023 •

edited

Loading

zmrow commented Jul 5, 2023 •

edited

Loading