Add configurable DNS settings #2353

zmrow · 2022-08-16T20:18:25Z

Issue number:
Related to #2204

Description of changes:
It's probably easiest to review this commit by commit, rather than as a whole. All the pieces are pretty small.

    netdog: Add dns module

    This commit adds a new `dns` module to centralize the logic of gathering
    DNS information from an API-generated configuration file, and merging
    any missing settings from a given DHCP lease.  This module also handles
    writing the `/etc/resolv.conf` using the DNS information

    netdog: Update `install` subcommand to use `dns` module

    This change updates the `install` subcommand to make use of the
    newly-added `dns` module.  The most important change here is that the
    code will use DNS settings from config if they exist, supplementing any
    missing settings with settings from DHCP lease.  The code also takes
    into account DHCP6 lease renewals when rewriting the `resolv.conf`.
    When multiple lease files exist, the code uses the DHCP4 lease to gather
    DHCP lease data

    netdog: Add new `write-resolv-conf` subcommand

    This commit adds a new `write-resolv-conf` subcommand, which is meant to
    be used as a restart command for DNS API settings.  It will write the
    `resolv.conf` using DNS settings if they exist, supplementing any
    missing settings with settings from the DHCP lease of the primary
    interface

    settings: Add DNS configurability via `settings.dns`

    This commit adds the ability to configure DNS name servers and search
    suffixes via a new settings prefix `settings.dns`.  These DNS settings
    populate a configuration file which gets used by `netdog` to write the
    `resolv.conf`.  `netdog write-resolv-conf` is triggered via restart
    command.  If DNS settings exist, they are used to write the
    `resolv.conf`, otherwise the DHCP lease for the primary interface is
    used

    migrations: Add migrations for `settings.dns`

    This adds the migrations needed for `settings.dns`, one for the setting,
    its service and configuration file, and one for the setting metadata

    README: Add DNS settings documentation

Testing done:

Boot an aws-k8s-1.22 instance and confirm resolv.conf is still written correctly with no DNS settings
Boot a metal-dev image with and without user data and ensure resolv.conf is written properly with the correct settings
Manually change DNS settings via apiclient and observe resolv.conf written with desired settings, using DHCP if only one of the settings is set.

$ cat /etc/resolv.conf 
search us-west-2.compute.internal
nameserver 192.168.0.2

$ apiclient set -j '{"dns": {"name-servers": ["1.2.3.4", "2.3.4.5"]}}'

$ cat /etc/resolv.conf                                                                                       
search us-west-2.compute.internal
nameserver 1.2.3.4
nameserver 2.3.4.5

$ apiclient set -j '{"dns": {"search-list": ["foo.bar", "baz.foo"]}}'

$ cat /etc/resolv.conf 
search foo.bar baz.foo
nameserver 1.2.3.4
nameserver 2.3.4.5

Test migrations work properly

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-08-18T17:40:55Z

^ Rebases on develop and fix the merge conflict in Release.toml

zmrow · 2022-08-18T20:19:33Z

^ Fixed a clippy lint I originally missed

sources/api/netdog/src/dns.rs

sources/api/netdog/test_data/dns/leaseinfo.eth0.dhcp.ipv4.multiple-dns

packages/release/netdog.template

Release.toml

sources/api/migration/migrations/v1.10.0/dns-settings/src/main.rs

README.md

zmrow · 2022-08-19T20:05:10Z

^ Addresses @arnaldo2792 's comments

zmrow · 2022-08-19T20:09:23Z

^ Rebases on develop and fixes the sources/Cargo.toml merge conflict

zmrow · 2022-08-19T22:28:44Z

^ Changes the type of DnsSettings::name-servers to Option since a user may not supply it... (It was an oversight, search-list was already Option)

sources/models/shared-defaults/defaults.toml

sources/api/netdog/src/dns.rs

zmrow · 2022-08-23T16:09:56Z

^ Addresses @etungsten 's coments

sources/api/migration/migrations/v1.10.0/dns-settings/src/main.rs

zmrow · 2022-08-23T16:52:31Z

^ Fixes the migration filename (thanks @etungsten!)

bcressey · 2022-08-23T23:54:22Z

sources/api/netdog/src/dns.rs

+        if config_exists {
+            let config_str =
+                fs::read_to_string(path).context(error::DnsConfReadFailedSnafu { path })?;
+            let dns_config =
+                toml::from_str(&config_str).context(error::DnsConfParseSnafu { path })?;
+
+            Ok(Some(dns_config))


In the from_lease_info() path, if we're given a lease info file with no relevant fields, we return a DnsSettings struct with both optional fields set to None.

Would it make sense to do that here as well, and always return a Result<Self> instead?

bcressey · 2022-08-24T00:02:01Z

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

+            // Use DNS API settings if they exist, otherwise use DHCP lease
+            let dns_settings = if let Some(settings) =
+                DnsSettings::from_config().context(error::DnsFromConfSnafu)?
+            {
+                settings
+            } else {
+                DnsSettings::from_lease_info(&info)
+            };
+            dns_settings
+                .write_resolv_conf()
+                .context(error::ResolvConfWriteFailedSnafu)?;


This relates to from_config() returning a valid DnsSettings struct - potentially we want to divide this up rather than making it all API settings or all lease settings:

if we have name-servers in the config, use those, otherwise use the lease

if we have a search-list in the config, use it, otherwise use the lease

bcressey · 2022-08-24T00:06:03Z

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

+pub(crate) fn run() -> Result<()> {
+    // Use DNS API settings if they exist, otherwise use the primary interface's lease
+    let dns_settings =
+        if let Some(settings) = DnsSettings::from_config().context(error::DnsFromConfSnafu)? {
+            settings
+        } else {
+            let lease_path = primary_interface_lease()?;
+            DnsSettings::from_lease_file(lease_path).context(error::DnsFromLeaseSnafu)?
+        };
+    dns_settings
+        .write_resolv_conf()
+        .context(error::ResolvConfWriteFailedSnafu)?;
+    Ok(())
+}


Can the merging of the DNS config settings and the lease settings be a helper function? Especially if becomes slightly more complicated than using one source or the other.

We're doing almost but not quite exactly the same thing as in the DHCP helper entry point, which seems like it could lead to bugs.

bcressey · 2022-08-24T00:09:43Z

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

+    // If both ipv4 and ipv6 leases exist, use the ipv4 lease for DNS settings
+    let ipv4_exists = Path::exists(&ipv4);
+    let ipv6_exists = Path::exists(&ipv6);
+    match (ipv4_exists, ipv6_exists) {
+        (true, true) => Ok(ipv4),
+        (true, false) => Ok(ipv4),
+        (false, true) => Ok(ipv6),
+        (false, false) => error::MissingLeaseSnafu { path: LEASE_FOLDER }.fail(),
+    }


We entirely ignore the DHCP6 lease coming from the "install" path - it seems like we'd want the behavior to be consistent, otherwise we'll lose the IPv6 version of the settings the next time the lease is renewed and resolv.conf is rewritten.

zmrow · 2022-08-26T22:17:29Z

^ Addresses @bcressey 's comments. The highlights:

DnsSettings public interface is much smaller, preferring creation from config and merging in settings from DHCP lease as necessary. This reduces the paths possible, addressing the possibility for bug-ridden multiple paths to the DNS settings.
DNS settings from DHCP lease are used if the DNS API settings aren't set
The install DHCP helper now correctly uses the proper DHCP path when gathering DNS settings. This means we won't inadvertently lose settings on DHCP lease renewal, and ensures the write-resolv-conf and install paths are doing the same thing.

zmrow · 2022-08-30T18:31:53Z

^ Above push avoids returning an error if the DHCP lease doesn't exist. The lease won't exist in the case a user is not using DHCP (static addressing).

It also makes the DHCP lease optional when constructing a DnsSettings.

zmrow · 2022-08-30T19:37:41Z

^ Rebases on develop and fixes the merge conflict with Release.toml

bcressey · 2022-08-31T04:54:57Z

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

+        (InterfaceType::Dhcp, InterfaceFamily::Ipv4 | InterfaceFamily::Ipv6) => {
+            // A lease should exist when using DHCP
+            let primary_lease_path =
+                lease_path(&primary_interface).context(error::MissingLeaseSnafu {
+                    interface: primary_interface,


I'd still like to use args.data_file in some way, if only to check that it matches primary_lease_path with an early return if it doesn't.

Otherwise we potentially end up rewriting resolv.conf whenever any DHCP lease is renewed, instead of just the one for the primary interface.

Also, depending on the order in which leases are obtained, we might not have a lease for the primary interface yet early in the boot, if other interfaces are also using DHCP.

Good point about the timing!

Also, there's a check just before the match statement that returns if the install interface isn't the primary interface

bcressey · 2022-08-31T05:04:35Z

sources/api/netdog/src/dns.rs

+        if self.nameservers.is_none() {
+            // Randomize name server order, for libc implementations like musl that send
+            // queries to the first N servers.
+            let mut dns_servers: Vec<IpAddr> = lease.dns_servers.clone().into_iter().collect();
+            dns_servers.shuffle(&mut thread_rng());
+
+            self.nameservers = Some(BTreeSet::from_iter(dns_servers.into_iter()));
+        }


I'd prefer to shuffle the nameservers after the merge, just before writing resolv.conf - to guarantee that they're always shuffled. Here we don't shuffle if the nameservers are set via API.

bcressey · 2022-08-31T05:09:36Z

sources/api/netdog/src/dns.rs

+    #[test]
+    fn empty_config() {


Maybe add a test for missing_config too, where the file doesn't exist.

bcressey · 2022-08-31T05:13:57Z

sources/api/netdog/src/main.rs

@@ -47,6 +52,7 @@ static PRIMARY_INTERFACE: &str = "/var/lib/netdog/primary_interface";
 static DEFAULT_NET_CONFIG_FILE: &str = "/var/lib/bottlerocket/net.toml";
 static PRIMARY_SYSCTL_CONF: &str = "/etc/sysctl.d/90-primary_interface.conf";
 static SYSTEMD_SYSCTL: &str = "/usr/lib/systemd/systemd-sysctl";
+static LEASE_FOLDER: &str = "/var/run/wicked";


nit: 🪟 -> 🐧

Suggested change

static LEASE_FOLDER: &str = "/var/run/wicked";

static LEASE_DIR: &str = "/var/run/wicked";

This commit adds a new `dns` module to centralize the logic of gathering DNS information from an API-generated configuration file, and merging any missing settings from a given DHCP lease. This module also handles writing the `/etc/resolv.conf` using the DNS information.

This change updates the `install` subcommand to make use of the newly-added `dns` module. The most important change here is that the code will use DNS settings from config if they exist, supplementing any missing settings with settings from DHCP lease. The code also takes into account DHCP6 lease renewals when rewriting the `resolv.conf`. When multiple lease files exist, the code uses the DHCP4 lease to gather DHCP lease data.

This commit adds a new `write-resolv-conf` subcommand, which is meant to be used as a restart command for DNS API settings. It will write the `resolv.conf` using DNS settings if they exist, supplementing any missing settings with settings from the DHCP lease of the primary interface if it exists.

This commit adds the ability to configure DNS name servers and search suffixes via a new settings prefix `settings.dns`. These DNS settings populate a configuration file which gets used by `netdog` to write the `resolv.conf`. `netdog write-resolv-conf` is triggered via restart command. If DNS settings exist, they are used to write the `resolv.conf`, otherwise the DHCP lease for the primary interface is used.

This adds the migrations needed for `settings.dns`, one for the setting, its service and configuration file, and one for the setting metadata.

zmrow · 2022-09-01T16:54:55Z

^ Addresses @bcressey 's comments.

I also simplified a few tests - I was calling DnsSettings::from_config_impl() multiple times when it was unnecessary...

sources/api/netdog/src/main.rs

rickymulder · 2022-09-15T18:18:58Z

Is there plans to merge this into 1.9.x? Attempting to spin up an EKS 1.21 cluster with bottlerocket and my host-network pods can't do cluster dns resolution without calling out the kube-dns service ip.

webern · 2022-10-12T21:46:27Z

sources/api/netdog/src/dns.rs

+        assert_ne!(resolv_conf == format1, resolv_conf == format2)
+    }
+
+    #[test]


Nice tests!

zmrow requested review from bcressey, etungsten and cbgbt August 16, 2022 20:18

zmrow marked this pull request as ready for review August 16, 2022 22:38

zmrow force-pushed the dns-settings branch from 268b647 to a07c938 Compare August 18, 2022 17:40

zmrow force-pushed the dns-settings branch from a07c938 to 90ff967 Compare August 18, 2022 20:18

arnaldo2792 requested changes Aug 18, 2022

View reviewed changes

zmrow force-pushed the dns-settings branch from 90ff967 to dc11d1f Compare August 19, 2022 20:04

zmrow force-pushed the dns-settings branch from dc11d1f to 871cb76 Compare August 19, 2022 20:07

zmrow requested a review from arnaldo2792 August 19, 2022 20:59

zmrow force-pushed the dns-settings branch from 871cb76 to 6722789 Compare August 19, 2022 22:26

etungsten approved these changes Aug 22, 2022

View reviewed changes

sources/models/shared-defaults/defaults.toml Outdated Show resolved Hide resolved

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

zmrow force-pushed the dns-settings branch from 6722789 to e81b8d8 Compare August 23, 2022 16:09

etungsten reviewed Aug 23, 2022

View reviewed changes

sources/api/migration/migrations/v1.10.0/dns-settings/src/main.rs Outdated Show resolved Hide resolved

zmrow force-pushed the dns-settings branch from e81b8d8 to 48b8dba Compare August 23, 2022 16:51

etungsten approved these changes Aug 23, 2022

View reviewed changes

arnaldo2792 approved these changes Aug 23, 2022

View reviewed changes

bcressey reviewed Aug 24, 2022

View reviewed changes

zmrow force-pushed the dns-settings branch from 48b8dba to 030b34d Compare August 26, 2022 22:07

zmrow force-pushed the dns-settings branch from 030b34d to 94f07e8 Compare August 30, 2022 18:30

zmrow force-pushed the dns-settings branch from 94f07e8 to 5cb4dda Compare August 30, 2022 19:37

bcressey reviewed Aug 31, 2022

View reviewed changes

zmrow added 6 commits August 31, 2022 22:31

migrations: Add migrations for settings.dns

228d981

This adds the migrations needed for `settings.dns`, one for the setting, its service and configuration file, and one for the setting metadata.

README: Add DNS settings documentation

9659374

zmrow force-pushed the dns-settings branch from 5cb4dda to 9659374 Compare September 1, 2022 16:53

zmrow commented Sep 1, 2022

View reviewed changes

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

bcressey approved these changes Sep 1, 2022

View reviewed changes

zmrow merged commit a3e92e9 into bottlerocket-os:develop Sep 1, 2022

zmrow deleted the dns-settings branch September 1, 2022 17:59

rickymulder mentioned this pull request Sep 15, 2022

Support static addressing via net.toml #2204

Closed

zmrow mentioned this pull request Sep 20, 2022

Support static addressing via net.toml #2445

Merged

webern reviewed Oct 12, 2022

View reviewed changes

sources/api/netdog/src/dns.rs

assert_ne!(resolv_conf == format1, resolv_conf == format2)

}

#[test]

Copy link

Contributor

webern Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable DNS settings #2353

Add configurable DNS settings #2353

zmrow commented Aug 16, 2022 •

edited

Loading

zmrow commented Aug 18, 2022

zmrow commented Aug 18, 2022

zmrow commented Aug 19, 2022

zmrow commented Aug 19, 2022 •

edited

Loading

zmrow commented Aug 19, 2022

zmrow commented Aug 23, 2022

zmrow commented Aug 23, 2022

bcressey Aug 23, 2022

bcressey Aug 24, 2022

bcressey Aug 24, 2022

bcressey Aug 24, 2022

zmrow commented Aug 26, 2022

zmrow commented Aug 30, 2022 •

edited

Loading

zmrow commented Aug 30, 2022

bcressey Aug 31, 2022

zmrow Aug 31, 2022

bcressey Aug 31, 2022

bcressey Aug 31, 2022

bcressey Aug 31, 2022

zmrow commented Sep 1, 2022

rickymulder commented Sep 15, 2022

webern Oct 12, 2022

	static LEASE_FOLDER: &str = "/var/run/wicked";
	static LEASE_DIR: &str = "/var/run/wicked";

Add configurable DNS settings #2353

Add configurable DNS settings #2353

Conversation

zmrow commented Aug 16, 2022 • edited Loading

zmrow commented Aug 18, 2022

zmrow commented Aug 18, 2022

zmrow commented Aug 19, 2022

zmrow commented Aug 19, 2022 • edited Loading

zmrow commented Aug 19, 2022

zmrow commented Aug 23, 2022

zmrow commented Aug 23, 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 commented Aug 26, 2022

zmrow commented Aug 30, 2022 • edited Loading

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

Choose a reason for hiding this comment

zmrow commented Sep 1, 2022

rickymulder commented Sep 15, 2022

Choose a reason for hiding this comment

zmrow commented Aug 16, 2022 •

edited

Loading

zmrow commented Aug 19, 2022 •

edited

Loading

zmrow commented Aug 30, 2022 •

edited

Loading