netdog: Add systemd-networkd config generation #3266

zmrow · 2023-07-14T16:03:44Z

Issue number:
Related to #2449

Description of changes:
This PR is probably best read in commit order :)

This is the grand finale, yes, the PR that puts all the pieces together for systemd-networkd config generation!

This PR drives the previously added network/netdev config builders via the systemd-networkd devices structs to create systemd-networkd config files.

It also adds a new top-level type: NetworkDConfig. This is the main type that gets created from net config; it contains the devices and a map of VLANs. The devices are created via conversions of the various versions of net config; these conversions are contained in the newly added conversions.rs. NetworkDConfig has a method create_files() that outputs all of the necessary configuration files for all configured devices. Under the hood, each of the contained devices is driven to create the appropriate files for the device. The device structs implement traits depending on their need for .network or .netdev files. Inside the trait implementations, the devices drive the builders.

Conditional compilation bits are also tidied up, ensuring wicked stuff isn't included anywhere in builds that don't include it.

To update the diagram from #3220 , this PR implements the left side of the diagram: the conversion of netconfig to systemd-networkd devices, and driving the builders to create config.

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

Testing done:

Build an aws-k8s-1.24 (with wicked) image to ensure nothing is broken over there: all good
Boot an aws-dev (with systemd-networkd) image configured for early console in preconfigured.target, and observe the proper files being generated and network online!

bash-5.1# cat /run/systemd/network/10-eth0.network 
[Match]
Name=eth0
[Link]
RequiredForOnline=true
RequiredFamilyForOnline=ipv4
[Network]
DHCP=yes
IPv6AcceptRA=true
[DHCPv4]
UseDNS=true
UseDomains=true
[DHCPv6]
UseDNS=true
UseDomains=true

bash-5.1# networkctl status eth0
● 2: eth0                                                                                                      
                     Link File: /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/network/80-release.link
                  Network File: /run/systemd/network/10-eth0.network
                          Type: ether
                         State: routable (configured)
                  Online state: online 
...

Built and booted a metal-dev image with the following net.toml and observed the box boot, not hang waiting for optional DHCP6, and both interface come up and get DHCP4 addresses. Found the proper config files in /run/systemd/network/10-{eno1,eno2}.network. I also pulled the network cable from eno1, and observed the box wait for it to become online and systemd-networkd-wait-online.service fail, proving the RequiredForOnline bit is working.

bash-5.1# cat /var/lib/bottlerocket/net.toml
version=3
[eno1]
dhcp4=true
primary=true
[eno2]
dhcp4=true
[eno2.dhcp6]
enabled=true
optional=true

bash-5.1# cat /run/systemd/network/10-eno1.network
[Match]
Name=eno1
[Link]
RequiredForOnline=true
[Network]
DHCP=ipv4
[DHCPv4]
UseDNS=true
UseDomains=true

bash-5.1# cat /run/systemd/network/10-eno2.network
[Match]
Name=eno2
[Link]
RequiredForOnline=true
RequiredFamilyForOnline=ipv4
[Network]
DHCP=yes
[DHCPv4]
UseDNS=true
UseDomains=true
[DHCPv6]
UseDNS=true
UseDomains=true

Using the same metal image, tested that configuration via MAC address (static and DHCP) functions properly.
Using the same metal image, ensure simple bond configuration works properly with 2 devices.
Ensure the /etc/systemd/network directory gets the proper context:

bash-5.1# ls -lahZ /etc/systemd/
...
drwxr-xr-x.  2 root root system_u:object_r:lease_t:s0  60 Aug  3 20:15 network
...

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.

yeazelm

With all the extra conditional compilation, I just want to confirm we have run clippy and others for both sides of netdog with and without systemd-networkd enabled to ensure the wicked stuff still builds how we expect. It was hard to track all those changes and hopefully our tools will help us catch any missing bits.

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

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

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

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

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

zmrow · 2023-07-20T18:20:17Z

^ Addresses @yeazelm 's comments. Thanks!

zmrow · 2023-07-20T18:21:04Z

Also, I confirmed clippy is happy and all the tests continue to pass.

etungsten

🚀

bcressey · 2023-07-21T19:10:25Z

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

+        // Destructure self to ensure we don't skip any fields (unless we want to)
+        let Self {
+            name,
+            dhcp4: _,
+            dhcp6: _,
+            static4: _,
+            static6: _,
+            routes: _,
+            device: _,
+            id,
+        } = self;


The comment is "what" rather than "why" - it's obvious that some fields are skipped but it's not clear why.

For example, we never use the device field for a NetworkDVlan but I don't know why. If we never use it, why do we even have that field?

What failure case are you actually concerned about defending against here and is there a different mechanism we can use instead?

The destructuring itself is OK, at least for functions where we use more of the fields - it's kind of excessive here - but the comment implies something is being ensured and I don't follow what that is.

I'll update the comment.

For example, we never use the device field for a NetworkDVlan but I don't know why. If we never use it, why do we even have that field?

The device field gets used to create the map of devices -> VLANs. It doesn't directly get used to create files for the VLAN itself. I'll add a comment explaining that.

What failure case are you actually concerned about defending against here and is there a different mechanism we can use instead?

Basically I was trying to defend against the future addition of fields to the NetworkDVlan struct that accidentally don't get used to create files. If we destructure, the compiler would yell about a specific field not being used, at which point the dev could decide if it's needed or not.

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

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

bcressey · 2023-07-21T19:31:21Z

sources/api/netdog/test_data/networkd/network/eno1-ra.network

+[Match]
+Name=eno1
+[Link]
+RequiredForOnline=true
+[Network]
+DHCP=ipv4
+IPv6AcceptRA=true
+[DHCPv4]
+UseDNS=true
+UseDomains=true


nit: it'd be great if we could separate sections with empty lines

I believe that should be doable via our macro - will look into it

I could blindly emit a newline before new section headings in the macro. That would leave a newline at the beginning of the file.

I haven't figured out a "smarter" way, given the macro uses repetition to generate all the sections in order. AFAIK, there's not really a way to enumerate/index during that repetition.

sources/api/netdog/src/cli/mod.rs

sources/api/netdog/src/cli/generate_net_config.rs

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

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

sources/api/netdog/src/net_config/v1.rs

sources/api/netdog/src/networkd/conversions.rs

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

zmrow · 2023-07-25T23:45:05Z

^ The above addresses all of @cbgbt and @bcressey 's comments except for one:

Why is this net_config.toml different from the one for wicked tests? It seems like we'd want to validate the same input for both.

@bcressey's comment caused me to re-examine why i created a different net_config.toml for networkd. This in turn, made me realize that I had missed implementing a subtle piece of the puzzle for VLANs. With wicked, if an interface isn't otherwise configured, but is used by a VLAN, wicked will generate a "dummy" config for the interface. This config basically brings the interface up without any DHCP.

With systemd-networkd, we need to generate this same dummy config for the interface, but only if it is not otherwise configured. I'm working on implementing that and will include include it in this PR, but it's tricky as it subtly breaks my current model since it requires knowledge of the overall config.

zmrow · 2023-07-27T18:07:45Z

^ The above push addresses the couple issues we had identified in my last comment:

the ability to have an interface that is managed, but not otherwise configured - this interface is used only as the link for a VLAN.
Use the same net_config.toml for both wicked/networkd

This push adds an additional commit, which includes a new type parameter for the builder for .network files. This type parameter is called VlanLink and it's constructor turns off addressing autoconfiguration. The builder with this type parameter only allows a single method that adds VLANs to the link.

zmrow · 2023-07-27T18:09:36Z

sources/api/netdog/test_data/net_config.toml

@@ -143,7 +143,7 @@ via = "2001:beef:beef::1"
 {{#if (eq version 3)}}
 [myvlan]
 kind = "vlan"
-device = "eno1"
+device = "eno100"


This device was changed to eno100 because for systemd-networkd we expect to generate a file for this link. Since we already have (and validate) an eno1 in this config, we need to change the name in order to generate and validate a different file.

Doesn't this point to a bug in the wicked backend rendering? It seems like an error to generate eno1.xml and myvlan.xml that both reference the same device.

wicked is the opposite of systemd-networkd in that the interface's config file doesn't include the VLAN. The VLAN's config file includes the interface name. Since we're now using the same net_config.toml, we have to use a different interface name since there's already eno1 in the file that is not a member of the VLAN.

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

zmrow · 2023-07-27T18:29:29Z

The above commit adds an additional integration test to handle the case where a device is a member of a VLAN, but also has it's own addressing config. Our current set of integration tests only covered the case where the underlying interface was only used for the VLAN.

zmrow · 2023-07-27T18:33:17Z

^ Oops - forgot to add the wicked reference files for the additional integration test. Added those

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

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

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

bcressey · 2023-07-28T16:38:31Z

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

@@ -7,7 +7,7 @@ use super::Result;
 pub(crate) use netdev::{NetDevBuilder, NetDevConfig};
 pub(crate) use network::{NetworkBuilder, NetworkConfig};

-const NETWORKD_CONFIG_DIR: &str = "/etc/systemd/network";
+const NETWORKD_CONFIG_DIR: &str = "/run/systemd/network";


I don't love this change - there are a lot more protections on /etc. Why is this needed?

Rather than writing to /run, I've changed it back to /etc/systemd/networkd, and am creating that directory via mount unit with the correct context attached.

I haven't done much yet with Bottlerocket's mounts, but isn't /etc already mounted as tmpfs? Why do we need an additional mount?

Indeed /etc is mounted as a tmpfs. We need the directory /etc/systemd/network to be created, but because systemd-networkd runs with DefaultDependencies=no (which includes local-fs), we can't use tmpfiles.d to help us there. A mount unit also allows us to create the directory with the proper SELinux label.

I'm open to suggestions if there's a better way that handles the timing issue we have!

sources/api/netdog/test_data/net_config.toml

zmrow · 2023-08-01T20:14:49Z

^ The above push addresses @bcressey 's comments!

zmrow · 2023-08-03T20:17:25Z

^ Changes the etc-systemd-network.mount ordering to Before=generate-network-config.service, rather than Before=network-pre.target since generate-network-config.service depends on the directory being present. (Both will be run before network-pre.target based on the ordering in generate-network-config.service)

bcressey

LGTM overall, some minor quibbles with the new unit's dependencies.

bcressey · 2023-08-04T20:37:53Z

packages/os/etc-systemd-network.mount

+Description=systemd-networkd configuration directory
+DefaultDependencies=no
+Conflicts=umount.target
+Before=generate-network-config.service


Need to add the standard mount unit dependencies:

Suggested change

Before=generate-network-config.service

Before=local-fs.target umount.target generate-network-config.service

But I'd really prefer that we used RequiresMountsFor in generate-network-config.service instead. Consider doing that in a drop-in for that unit that's added for %{with systemd_networkd}.

bcressey · 2023-08-04T20:39:36Z

packages/os/etc-systemd-network.mount

+Options=nosuid,nodev,noexec,noatime,context=system_u:object_r:lease_t:s0,mode=0755
+
+[Install]
+RequiredBy=systemd-networkd.service


A judgment call but I prefer "WantedBy" unless the other service can't possibly work without this.

Suggested change

RequiredBy=systemd-networkd.service

WantedBy=systemd-networkd.service

This commit includes a new mount unit that creates the systemd-networkd configuration directory (/etc/systemd/network) at runtime with the correct SELinux context. It also adds a drop-in unit that makes the mount required by generate-network-config.service.

This commit adds a new type parameter for the NetworkBuilder type specifically for interfaces meant only to be used as the link for a VLAN. These types of interfaces are typically used for "tagged only"-type setups. The interface requires config so it can be managed and a member of a VLAN but otherwise has all DHCP and addressing turned off.

This change adds two new traits, NetDevFileCreator and NetworkFileCreator. Device structs will implement these traits if they require .netdev and/or .network files. This change also implements said traits for the three devices we currently support (bonds, vlans, interfaces). Under the hood, these implementations drive the previously added netdev/network builders.

This change adds a new type `NetworkDConfig`, which is the "top-level" interface for systemd-networkd config creation. The type can be created from any version of network config, provided the struct implements the `TryInto<NetworkDDevice>` trait. The type presents a single method `create_files()` which will drive the underlying devices to create and return a list of appropriate configuration files.

Similar to the earlier change made to wicked's config here: bottlerocket-os@91e2cc9, this commit adds a method `accept_ra()` to `NetworkConfig` that adds the "IPv6AcceptRA" entry. Once we are able to add this setting via net config, this method can be removed.

This commit puts all the pieces together to enable systemd-networkd config generation via netdog. The change adds an additional trait method to the Interfaces trait that all versions of net config must implement: `as_networkd_config()`. This method returns a `NetworkDConfig`, from which all config files may be generated. Under the hood, this method takes advantage of the TryFrom conversions also included in this commit. This commit adds the trait method to all versions of net config, and also adds integration tests to ensure we generate the expected files for each device in the proper format. These tests use the same `net_config.toml` file as the wicked integration tests.

This commit tidies up the conditional compilation by ensuring imports pertaining to wicked and systemd-networkd are behind their respective cfg blocks and in consecutive lines. This should also ease removal in the future. A few missed remaining warnings regarding unused variables were cleaned up here as well.

This commit adds an additional integration test meant to cover the case where an interface is a member of a VLAN, but also has its own addressing configuration.

zmrow · 2023-08-04T22:23:31Z

^ Addresses @bcressey 's comments re: the mount unit / drop-in unit.

Proof of the drop-in working:

bash-5.1# systemctl show generate-network-config.service
...
DropInPaths=/x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/system/generate-network-config.service.d/requires-mounts-network-config.conf
...
RequiresMountsFor=/etc/systemd/network
...

zmrow requested review from bcressey, cbgbt and yeazelm July 14, 2023 16:03

zmrow changed the title ~~Add networkd generation~~ netdog: Add systemd-networkd config generation Jul 14, 2023

yeazelm reviewed Jul 18, 2023

View reviewed changes

yeazelm mentioned this pull request Jul 19, 2023

Add Whippet to bottlerocket #3270

Closed

zmrow force-pushed the add-networkd-generation branch from b35faae to a055b89 Compare July 20, 2023 18:20

yeazelm approved these changes Jul 20, 2023

View reviewed changes

etungsten approved these changes Jul 21, 2023

View reviewed changes

bcressey reviewed Jul 21, 2023

View reviewed changes

cbgbt reviewed Jul 24, 2023

View reviewed changes

zmrow force-pushed the add-networkd-generation branch from a055b89 to 0c455be Compare July 25, 2023 23:38

zmrow force-pushed the add-networkd-generation branch from 0c455be to bfdb8a7 Compare July 27, 2023 18:01

zmrow commented Jul 27, 2023

View reviewed changes

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

zmrow force-pushed the add-networkd-generation branch from 2881098 to dc21411 Compare July 27, 2023 18:32

yeazelm reviewed Jul 28, 2023

View reviewed changes

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

bcressey reviewed Jul 28, 2023

View reviewed changes

zmrow force-pushed the add-networkd-generation branch from dc21411 to b157308 Compare August 1, 2023 20:12

cbgbt approved these changes Aug 3, 2023

View reviewed changes

zmrow force-pushed the add-networkd-generation branch from b157308 to 0089638 Compare August 3, 2023 20:14

bcressey approved these changes Aug 4, 2023

View reviewed changes

zmrow added 8 commits August 4, 2023 22:01

netdog: Add integration test for VLAN members with their own config

fd7b2e1

This commit adds an additional integration test meant to cover the case where an interface is a member of a VLAN, but also has its own addressing configuration.

zmrow force-pushed the add-networkd-generation branch from 0089638 to fd7b2e1 Compare August 4, 2023 22:13

bcressey approved these changes Aug 4, 2023

View reviewed changes

yeazelm approved these changes Aug 7, 2023

View reviewed changes

zmrow merged commit 9f53a9a into bottlerocket-os:develop Aug 7, 2023
42 checks passed

zmrow deleted the add-networkd-generation branch August 7, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netdog: Add systemd-networkd config generation #3266

netdog: Add systemd-networkd config generation #3266

zmrow commented Jul 14, 2023 •

edited

Loading

yeazelm left a comment

zmrow commented Jul 20, 2023

zmrow commented Jul 20, 2023

etungsten left a comment

bcressey Jul 21, 2023

zmrow Jul 24, 2023 •

edited

Loading

bcressey Jul 21, 2023

zmrow Jul 24, 2023

zmrow Jul 25, 2023

zmrow commented Jul 25, 2023 •

edited

Loading

zmrow commented Jul 27, 2023 •

edited

Loading

zmrow Jul 27, 2023 •

edited

Loading

bcressey Jul 28, 2023

zmrow Aug 3, 2023 •

edited

Loading

zmrow commented Jul 27, 2023

zmrow commented Jul 27, 2023

bcressey Jul 28, 2023

zmrow Aug 1, 2023 •

edited

Loading

cbgbt Aug 1, 2023

zmrow Aug 2, 2023

zmrow commented Aug 1, 2023

zmrow commented Aug 3, 2023 •

edited

Loading

bcressey left a comment

bcressey Aug 4, 2023

bcressey Aug 4, 2023

zmrow commented Aug 4, 2023

	Before=generate-network-config.service
	Before=local-fs.target umount.target generate-network-config.service

	RequiredBy=systemd-networkd.service
	WantedBy=systemd-networkd.service

netdog: Add systemd-networkd config generation #3266

netdog: Add systemd-networkd config generation #3266

Conversation

zmrow commented Jul 14, 2023 • edited Loading

yeazelm left a comment

Choose a reason for hiding this comment

zmrow commented Jul 20, 2023

zmrow commented Jul 20, 2023

etungsten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow Jul 24, 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

zmrow commented Jul 25, 2023 • edited Loading

zmrow commented Jul 27, 2023 • edited Loading

zmrow Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

zmrow commented Jul 27, 2023

zmrow commented Jul 27, 2023

Choose a reason for hiding this comment

zmrow Aug 1, 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 commented Aug 1, 2023

zmrow commented Aug 3, 2023 • edited Loading

bcressey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow commented Aug 4, 2023

zmrow commented Jul 14, 2023 •

edited

Loading

zmrow Jul 24, 2023 •

edited

Loading

zmrow commented Jul 25, 2023 •

edited

Loading

zmrow commented Jul 27, 2023 •

edited

Loading

zmrow Jul 27, 2023 •

edited

Loading

zmrow Aug 3, 2023 •

edited

Loading

zmrow Aug 1, 2023 •

edited

Loading

zmrow commented Aug 3, 2023 •

edited

Loading