Enable network config generation via `netdog` #2066

zmrow · 2022-04-12T17:23:09Z

Description of changes:
Until now, metal variants were using the same hard-coded eth0.xml Wicked configuration file as other variants. This set of commits adds the initial support for user-supplied network interface configuration. Additional options and documentation will come after some additional use and feedback - this PR enables the feature.

    netdog: Add ability to generate network configuration
    
    This commit adds a subcommand `generate-net-config` to `netdog`.  It
    uses a user-provided TOML file `net.toml` to generate the proper wicked
    configuration files for the network interfaces in the system.  This file
    lives on the BOTTLEROCKET-PRIVATE partition.
    
    If no file is found, `netdog` looks at the kernel command line
    (/proc/cmdline) for the `netdog.default-interface` prefix and configures
    the interface listed there.  A single default interface may be defined
    on the kernel command line with the format:
    `netdog.default-interface=<interface name>:option1,option2`.
    "<interface name>" is the name of the interface, and valid options are
    "dhcp4" and "dhcp6".  A "?" may be added to the option to signify that
    the lease for the protocol is optional and the system chouldn't wait for
    it.  If the prefix does not exist on the kernel command line, no network
    configuration is generated.
    
    At configuration generation time, `netdog` writes the primary interface
    to `/var/lib/netdog/primary_interface`.  Primary interface may be set in
    `net.toml`; if it is absent in the file the first interface in the list
    is assumed to be the primary.  If the interface is passed via kernel
    command line, it is assumed to be the default.
    
    Previously, the `netdog install` command, which is called by wicked for
    configured interfaces, matched on the name of the interface (eth0)
    before it took any action.  The `install` command writes the
    resolv.conf, and the current_ip file.  For AWS/VMware we are explicitly
    only configuring `eth0`, and for Metal variants we will not be able to
    determine the configured interface names.  This commit changes the
    behavior.  Rather than filtering on interface name in order to write
    `resolv.conf` and the current IP file, we only write the files if the
    interface currently being configured is the primary interface (read from
    the previously written `primary_interface` file).
    
    Note: Previously, our network configuration for `eth0` included a few
    options, `arp-verify` and `arp-notify`.  These options were
    (theoretically) meant to speed up boot time in EC2 since we know our IP
    won't be a duplcate and therefore we don't need to ARP for it.  (That's
    not how networking in EC2 works)  In our testing, `arp-very/notify` made
    no significantly measureable difference in the time it took to get from
    "lease request" to "committed lease".  From this commit on, these
    options aren't part of our generated config for any variant.

    Use generated network configuration by default
    
    This commit removes the hard-coded `eth0.xml` network configuration file
    in favor of using `netdog` to generate the network configuration for all
    variants.  It adds a new systemd unit file to run `netdog
    generate-net-config` early in boot, before the network is up.  To
    generate the network configuration for AWS/VMware variants, we pass
    `netdog.default-interface=eth0:dhcp4,dhcp6?` which `netdog` interprets
    and generates network config similar to the hardcoded file previously
    used for these variants.
    
    For metal variants, we decided to use systemd-udevd's predictable device
    naming so we can count on network devices being named identically every
    boot.  We currently pass `net.ifnames=0` on the kernel command line
    which disables predictable naming, which is fine for AWS and VMware
    variants as hardware is controlled and instances typically initially
    come up with a single interface in the same PCIe location.  In order to
    continue using `net.ifnames=0` for AWS/VMware, we move this parameter
    out of the default kernel command line to the KERNEL_PARAMETERS section
    of each variants `Cargo.toml`.
    
    Aside: We previously passed `biosdevname=0` on the kernel command line
    as well.  `biosdevname` is a udev helper utility written by Dell for
    consistent device naming based on SMBIOS info.  We don't currently
    package the helper or include the udev rule that uses it, so we have
    removed the `biosdevname` parameter entirely.

Testing done:
Boot the following variants and ensure they come up correctly, network config is written properly, and they join the cluster, run pods, etc. Also ensure that the generated network configuration matches what we currently use today, minus the arp-verify/notify bits (see netdog commit message above). Ensure primary_interface is written.

bash-5.1# wicked show all
...
eth0            up
      link:     #2, state up, mtu 1500
      type:     ethernet, hwaddr 00:50:56:85:03:31
      config:   wicked:xml:/etc/wicked/ifconfig/eth0.xml
      leases:   ipv4 dhcp granted
      leases:   ipv6 dhcp requesting
      addr:     ipv4 198.19.129.141/15 [dhcp]
      route:    ipv4 default via 198.18.0.1 proto dhc
...

bash-5.1# more /etc/wicked/ifconfig/eth0.xml 
<interface><name>eth0</name><control><mode>boot</mode><link-detection><require-link></require-link></link-detection></control><ipv4:dhcp><enabled>
true</enabled></ipv4:dhcp><ipv6:dhcp><enabled>true</enabled><defer-timeout>1</defer-timeout><flags><optional></optional></flags></ipv6:dhcp></inte
rface>

For metal (tested on on my SuperMicro SYS-E200-8D):

ensure no config gets written in the absence of net.toml/kernel arg
write a net.toml and test all 4 interfaces work
ensure that the "primary" interface is chosen correctly via config or in the absence of the primary key the first interface is chosen
ensure that current_ip is written with the IP of the primary interface
ensure that all interfaces configured in net.toml get a configuration file written, and if they do not have an active link that wicked waits for them
ensure active links get an IP.

metal-dev
metal-k8s-1.21
metal-k8s-1.22

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.

zmrow · 2022-04-15T20:35:52Z

^ Push above:

fixes a few typos
Removes the last remaining references to eth0.xml from the release package
Ensures that the /etc/wicked/ifconfig directory is created
Changes the behavior of netdog install a little bit - rather than matching on network interface name (which, for metal, we can't know at the time of invocation), we only create the resolv.conf and current_ip files if they don't exist. This feels like the proper behavior since the netdog command is only called for configured interfaces (eth0 for AWS/VMware) and (for now) it shouldn't matter which interface is chosen to decide the resolv.conf or current IP.

sources/api/netdog/src/main.rs

arnaldo2792 · 2022-04-15T21:47:03Z

packages/os/network-config.service

+StandardError=journal+console
+
+[Install]
+WantedBy=multi-user.target


I don't think this directive is required, if you already made this unit RequiredBy=preconfigured.target

packages/os/network-config.service

arnaldo2792 · 2022-04-15T21:52:27Z

sources/api/netdog/README.md

@@ -14,6 +14,10 @@ It contains two subcommands meant for use as settings generators:

 The subcommand `set-hostname` sets the hostname for the system.

+The subcommand `generate-net-config` generates the network interface configuration for the host.
+If a `net.toml` exists, it is used to generate the configuration, otherwise a default configuration


Nit:

Suggested change

If a `net.toml` exists, it is used to generate the configuration, otherwise a default configuration

If a `net.toml` file exists in `/var/lib/bottlerocket`, it is used to generate the configuration, otherwise a default configuration

arnaldo2792 · 2022-04-15T21:53:20Z

packages/os/network-config.service

@@ -0,0 +1,20 @@
+[Unit]
+Description=Bottlerocket network configuration system
+Requires=var-lib-bottlerocket.mount


Nit: Maybe a note here saying that the mount should be there because the service will attempt to read from it? Also in other units we use this when we need a mount to be present:

Suggested change

Requires=var-lib-bottlerocket.mount

RequiresMountsFor=/var/lib/bottlerocket

arnaldo2792 · 2022-04-15T22:00:36Z

sources/api/netdog/build.rs

+        if variant.starts_with("aws") {
+            println!("cargo:rustc-cfg=bottlerocket_platform=\"aws\"");
+        } else if variant.starts_with("vmware") {
+            println!("cargo:rustc-cfg=bottlerocket_platform=\"vmware\"");
+        } else if variant.starts_with("metal") {
+            println!("cargo:rustc-cfg=bottlerocket_platform=\"metal\"");
+        } else {
+            eprintln!(
+            "For local builds, you must set the 'VARIANT' environment variable so we know which data \
+            provider to build. Valid values are the directories in models/src/variants/, for \
+            example 'aws-ecs-1'."
+            );
+            process::exit(1);
+        }


I had to do something similar somewhere else (not yet in the code), what do you think about?

Suggested change

if variant.starts_with("aws") {

println!("cargo:rustc-cfg=bottlerocket_platform=\"aws\"");

} else if variant.starts_with("vmware") {

println!("cargo:rustc-cfg=bottlerocket_platform=\"vmware\"");

} else if variant.starts_with("metal") {

println!("cargo:rustc-cfg=bottlerocket_platform=\"metal\"");

} else {

eprintln!(

"For local builds, you must set the 'VARIANT' environment variable so we know which data \

provider to build. Valid values are the directories in models/src/variants/, for \

example 'aws-ecs-1'."

);

process::exit(1);

}

let platform = variant.split('-').collect::<Vec<&str>>()[0].to_string();

println!("cargo:rustc-cfg=bottlerocket_platform=\"{}\"", platform);

It's definitely less code, and I like the fact that we don't have to specify all the variants, but I could see a few issues.

We don't get a nice error message if someone misspells the variant - we get a more cryptic compiler message

I think it's reasonable to assume that a variant name wouldn't be zero-length (covering the index issue), but we get the same issue as above where the build will happen with a misspelled cargo:rustc-cfg=bottlerocket_platform="awsk8s121" and we'd get a less than friendly error.

For 1), in the current implementation I can do aws-not-existing and get a compiler error

For 2), in the current implementation, awsk8s121 will cause the same error as 1)

So the question is how much validation do we want to do while parsing the variant's name to provide a friendly error?, or else we have to trade off some errors for others when the user passes an invalid variant.

I implemented your suggestion, and opted to add a compile_error!() after a cfg directive - see wicked.rs

sources/api/netdog/src/wicked.rs

arnaldo2792 · 2022-04-15T23:49:41Z

For the lack of a better place to add this comment:

Nit: I believe the commit message in netdog: Add ability to generate network configuration has a grammatical error, or else I need to learn this 😄 :

For AWS/VMware we are explicitly only configure `eth0`

webern · 2022-04-18T17:01:06Z

sources/api/netdog/src/main.rs

+mod net_config;
+use net_config::NetConfig;
+
+mod wicked;
+use wicked::WickedInterface;
+


nit: is there a reason to keep these use statements separate from all the others? rust fmt will just sort the lists of mods and uses if they are aggregated.

sources/api/netdog/src/wicked.rs

sources/api/netdog/src/net_config.rs

sources/api/netdog/src/main.rs

zmrow · 2022-04-18T18:22:40Z

^ The above fixes a few typos and adds a rename to route-metric in net_config.

zmrow · 2022-04-18T18:23:32Z

Flipping this out of draft since I feel pretty comfortable with the functionality.

zmrow · 2022-04-18T18:44:13Z

^ fixed the test data 🤦

zmrow · 2022-04-20T22:24:04Z

^ The above push:

Corrects the systemd unit to RequireMountsFor
Simplify the build.rs logic in favor of a compile_error!() if the platform is not known
Reduce the conditionally compiled code into constants and implement Default for a few more types to simplify the Default implementation for WickedInterface
Change into_config_file() to write_config_file()
As a result of the above changes, simplify the test code in wicked.rs using conditionally compiled defaults

zmrow · 2022-04-20T22:40:04Z

^ Adds a quick net config version check. This check will be removed when we handle versioning (already in progress).

I believe at this point I've addressed all of the comments minus @webern 's comment about testing the generate_net_config() function. I do agree there's some logic that doesn't get tested there - some of it will be removed when we start handling versions.

packages/os/network-config.service

packages/os/os.spec

packages/wicked/wicked.spec

sources/api/netdog/src/wicked.rs

zmrow · 2022-05-04T23:19:16Z

^ This force push was a pretty good sized change, hence requesting all reviewers again.

Remove all conditionally compiled code - This is the biggest change here. Per @bcressey 's request, I've moved all default interface definition to a kernel command line argument. All AWS/VMware variants now set their default interface in the KERNEL_PARAMETERS. netdog parses this value in the absence of a net.toml. See the commit message for all the nitty gritty.
Add interface name validation according to the rules defined in the kernel
Add the ability for a user to specify the primary interface via a key in the net.toml. In the absence of the key, the first value in the map is chosen. In order to preserve the order of the interfaces (it's a map), we are now using the toml crate feature preserve-order which uses an IndexMap under the hood; we also use IndexMap for the interfaces.
Generate resolve.conf and current_ip only for the primary interface
Add default interface definitions to all AWS/VMware variants

zmrow · 2022-05-04T23:20:19Z

I'll spin back through the variants again to make sure they're all tested, but my smoke testing of metal-dev, aws-dev, and aws-k8s-1.21 look great.

packages/os/generate-network-config.service

sources/api/netdog/README.md

sources/api/netdog/src/interface_name.rs

sources/api/netdog/src/wicked.rs

bcressey · 2022-05-09T01:35:01Z

sources/api/netdog/src/main.rs

+        if config.dhcp4.is_none() && config.dhcp6.is_none() {
+            return error::InvalidNetConfigSnafu {
+                interface: name.to_string(),
+                reason: "must configure dhcp4 or dhcp6, or both",
+            }


If we don't specify either DHCP4 or DHCP6 in the wicked interface XML file, what does wicked do? Bring it up with just local addresses? Leave it down? Error out?

zmrow · 2022-05-10T20:57:15Z

Addresses all of @bcressey 's comments!

Centralizes all parsing (from file and command line) into the net_config module, which in turn simplifies the calling code in main.
Enforces the same guards on valid network configuration whether it is read from file or command line, and does so in the parsing code rather than in main

bcressey

🥅 🐶

sources/api/netdog/src/main.rs

This commit adds a subcommand `generate-net-config` to `netdog`. It uses a user-provided TOML file `net.toml` to generate the proper wicked configuration files for the network interfaces in the system. This file lives on the BOTTLEROCKET-PRIVATE partition. If no file is found, `netdog` looks at the kernel command line (/proc/cmdline) for the `netdog.default-interface` prefix and configures the interface listed there. A single default interface may be defined on the kernel command line with the format: `netdog.default-interface=interface name:option1,option2`. "interface name" is the name of the interface, and valid options are "dhcp4" and "dhcp6". A "?" may be added to the option to signify that the lease for the protocol is optional and the system chouldn't wait for it. If the prefix does not exist on the kernel command line, no network configuration is generated. At configuration generation time, `netdog` writes the primary interface to `/var/lib/netdog/primary_interface`. Primary interface may be set in `net.toml`; if it is absent in the file the first interface in the list is assumed to be the primary. If the interface is passed via kernel command line, it is assumed to be the default. Previously, the `netdog install` command, which is called by wicked for configured interfaces, matched on the name of the interface (eth0) before it took any action. The `install` command writes the resolv.conf, and the current_ip file. For AWS/VMware we are explicitly only configuring `eth0`, and for Metal variants we will not be able to determine the configured interface names. This commit changes the behavior. Rather than filtering on interface name in order to write `resolv.conf` and the current IP file, we only write the files if the interface currently being configured is the primary interface (read from the previously written `primary_interface` file). Note: Previously, our network configuration for `eth0` included a few options, `arp-verify` and `arp-notify`. These options were (theoretically) meant to speed up boot time in EC2 since we know our IP won't be a duplcate and therefore we don't need to ARP for it. (That's not how networking in EC2 works) In our testing, `arp-very/notify` made no significantly measureable difference in the time it took to get from "lease request" to "committed lease". From this commit on, these options aren't part of our generated config for any variant.

This commit removes the hard-coded `eth0.xml` network configuration file in favor of using `netdog` to generate the network configuration for all variants. It adds a new systemd unit file to run `netdog generate-net-config` early in boot, before the network is up. To generate the network configuration for AWS/VMware variants, we pass `netdog.default-interface=eth0:dhcp4,dhcp6?` which `netdog` interprets and generates network config similar to the hardcoded file previously used for these variants. For metal variants, we decided to use systemd-udevd's predictable device naming so we can count on network devices being named identically every boot. We currently pass `net.ifnames=0` on the kernel command line which disables predictable naming, which is fine for AWS and VMware variants as hardware is controlled and instances typically initially come up with a single interface in the same PCIe location. In order to continue using `net.ifnames=0` for AWS/VMware, we move this parameter out of the default kernel command line to the KERNEL_PARAMETERS section of each variants `Cargo.toml`. Aside: We previously passed `biosdevname=0` on the kernel command line as well. `biosdevname` is a udev helper utility written by Dell for consistent device naming based on SMBIOS info. We don't currently package the helper or include the udev rule that uses it, so we have removed the `biosdevname` parameter entirely.

zmrow · 2022-05-16T16:27:38Z

^ Fixed the last couple nits!

zmrow requested review from bcressey, arnaldo2792 and etungsten April 12, 2022 17:23

jpculp self-requested a review April 12, 2022 23:11

zmrow force-pushed the netdog-generate branch from 65654c4 to a5a5cad Compare April 15, 2022 20:30

zmrow commented Apr 15, 2022

View reviewed changes

sources/api/netdog/src/main.rs Show resolved Hide resolved

arnaldo2792 reviewed Apr 15, 2022

View reviewed changes

webern reviewed Apr 18, 2022

View reviewed changes

zmrow force-pushed the netdog-generate branch from a5a5cad to adc0ceb Compare April 18, 2022 18:21

zmrow marked this pull request as ready for review April 18, 2022 18:23

zmrow force-pushed the netdog-generate branch from adc0ceb to e569da3 Compare April 18, 2022 18:43

etungsten approved these changes Apr 18, 2022

View reviewed changes

zmrow force-pushed the netdog-generate branch from e569da3 to 9312ea3 Compare April 20, 2022 22:20

zmrow force-pushed the netdog-generate branch from 9312ea3 to 69a0548 Compare April 20, 2022 22:35

zmrow requested review from webern, etungsten and arnaldo2792 April 20, 2022 22:41

webern approved these changes Apr 20, 2022

View reviewed changes

bcressey reviewed Apr 21, 2022

View reviewed changes

zmrow force-pushed the netdog-generate branch from 69a0548 to 090732f Compare May 4, 2022 23:10

zmrow requested review from bcressey and webern May 4, 2022 23:19

bcressey reviewed May 9, 2022

View reviewed changes

webern approved these changes May 9, 2022

View reviewed changes

zmrow force-pushed the netdog-generate branch from 090732f to 5ea8832 Compare May 10, 2022 20:53

bcressey approved these changes May 12, 2022

View reviewed changes

sources/api/netdog/src/main.rs Outdated Show resolved Hide resolved

sources/api/netdog/src/main.rs Outdated Show resolved Hide resolved

sources/api/netdog/src/main.rs Show resolved Hide resolved

zmrow added 2 commits May 16, 2022 16:26

zmrow force-pushed the netdog-generate branch from 5ea8832 to f68eec8 Compare May 16, 2022 16:27

etungsten approved these changes May 16, 2022

View reviewed changes

zmrow merged commit d340619 into bottlerocket-os:develop May 17, 2022

zmrow deleted the netdog-generate branch May 17, 2022 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable network config generation via `netdog` #2066

Enable network config generation via `netdog` #2066

zmrow commented Apr 12, 2022 •

edited

Loading

zmrow commented Apr 15, 2022

arnaldo2792 Apr 15, 2022

arnaldo2792 Apr 15, 2022

arnaldo2792 Apr 15, 2022

arnaldo2792 Apr 15, 2022

zmrow Apr 18, 2022 •

edited

Loading

arnaldo2792 Apr 18, 2022

zmrow Apr 20, 2022

arnaldo2792 commented Apr 15, 2022

webern Apr 18, 2022

zmrow commented Apr 18, 2022

zmrow commented Apr 18, 2022

zmrow commented Apr 18, 2022

zmrow commented Apr 20, 2022

zmrow commented Apr 20, 2022

zmrow commented May 4, 2022

zmrow commented May 4, 2022

bcressey May 9, 2022

zmrow commented May 10, 2022

bcressey left a comment

zmrow commented May 16, 2022

	If a `net.toml` exists, it is used to generate the configuration, otherwise a default configuration
	If a `net.toml` file exists in `/var/lib/bottlerocket`, it is used to generate the configuration, otherwise a default configuration

	Requires=var-lib-bottlerocket.mount
	RequiresMountsFor=/var/lib/bottlerocket

Enable network config generation via netdog #2066

Enable network config generation via netdog #2066

Conversation

zmrow commented Apr 12, 2022 • edited Loading

zmrow commented Apr 15, 2022

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 Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaldo2792 commented Apr 15, 2022

Choose a reason for hiding this comment

zmrow commented Apr 18, 2022

zmrow commented Apr 18, 2022

zmrow commented Apr 18, 2022

zmrow commented Apr 20, 2022

zmrow commented Apr 20, 2022

zmrow commented May 4, 2022

zmrow commented May 4, 2022

Choose a reason for hiding this comment

zmrow commented May 10, 2022

bcressey left a comment

Choose a reason for hiding this comment

zmrow commented May 16, 2022

Enable network config generation via `netdog` #2066

Enable network config generation via `netdog` #2066

zmrow commented Apr 12, 2022 •

edited

Loading

zmrow Apr 18, 2022 •

edited

Loading