daemon/config: change DNSConfig.DNS to a []net.IP by thaJeztah · Pull Request #46803 · moby/moby

thaJeztah · 2023-11-13T11:23:44Z

follow-up to / related to: opts: ValidateIPAddress: improve error, godoc, and tests #46801

Use a strong type for the DNS IP-addresses so that we can use flags.IPSliceVar, instead of implementing our own option-type and validation.

Behavior should be the same, although error-messages have slightly changed:

Before this patch:

dockerd --dns 1.1.1.1oooo --validate
Status: invalid argument "1.1.1.1oooo" for "--dns" flag: 1.1.1.1oooo is not an ip address
See 'dockerd --help'., Code: 125

cat /etc/docker/daemon.json
{"dns": ["1.1.1.1"]}

dockerd --dns 2.2.2.2 --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: dns: (from flag: [2.2.2.2], from file: [1.1.1.1])

cat /etc/docker/daemon.json
{"dns": ["1.1.1.1oooo"]}

dockerd --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: 1.1.1.1ooooo is not an ip address

With this patch:

dockerd --dns 1.1.1.1oooo --validate
Status: invalid argument "1.1.1.1oooo" for "--dns" flag: invalid string being converted to IP address: 1.1.1.1oooo
See 'dockerd --help'., Code: 125

cat /etc/docker/daemon.json
{"dns": ["1.1.1.1"]}

dockerd --dns 2.2.2.2 --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: dns: (from flag: [2.2.2.2], from file: [1.1.1.1])

cat /etc/docker/daemon.json
{"dns": ["1.1.1.1oooo"]}

dockerd --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: invalid IP address: 1.1.1.1oooo

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Use a strong type for the DNS IP-addresses so that we can use flags.IPSliceVar, instead of implementing our own option-type and validation. Behavior should be the same, although error-messages have slightly changed: Before this patch: dockerd --dns 1.1.1.1oooo --validate Status: invalid argument "1.1.1.1oooo" for "--dns" flag: 1.1.1.1oooo is not an ip address See 'dockerd --help'., Code: 125 cat /etc/docker/daemon.json {"dns": ["1.1.1.1"]} dockerd --dns 2.2.2.2 --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: dns: (from flag: [2.2.2.2], from file: [1.1.1.1]) cat /etc/docker/daemon.json {"dns": ["1.1.1.1oooo"]} dockerd --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: 1.1.1.1ooooo is not an ip address With this patch: dockerd --dns 1.1.1.1oooo --validate Status: invalid argument "1.1.1.1oooo" for "--dns" flag: invalid string being converted to IP address: 1.1.1.1oooo See 'dockerd --help'., Code: 125 cat /etc/docker/daemon.json {"dns": ["1.1.1.1"]} dockerd --dns 2.2.2.2 --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: dns: (from flag: [2.2.2.2], from file: [1.1.1.1]) cat /etc/docker/daemon.json {"dns": ["1.1.1.1oooo"]} dockerd --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: invalid IP address: 1.1.1.1oooo Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-11-13T11:24:11Z

Interesting; looks like we're not very consistent with "defaults" configured on the daemon. For log-driver (and options), we bake the defaults into the container when it's created, but looks like we're not doing that for DNS; i.e. if the daemon was started with --dns 1.1.1.1, then the container does use 1.1.1.1 as DNS;

docker run --rm busybox cat /etc/resolv.conf
nameserver 1.1.1.1

But inspecting a container won't show that in the config;

docker inspect --format '{{.HostConfig.Dns}}' defaultdns
[]

Whereas manually specifying the --dns does get baked into the container;

docker create --dns 2.2.2.2 --name customdns busybox
docker inspect --format '{{.HostConfig.Dns}}' customdns
[2.2.2.2]

I guess this was by design, but it's not very consistent.

thaJeztah · 2023-11-13T11:27:02Z

daemon/config/config_test.go

+func TestConfigInvalidDNS(t *testing.T) {
+	tests := []struct {
+		doc         string
+		input       string
+		expectedErr string
+	}{
+		{
+			doc:         "single DNS, invalid IP-address",
+			input:       `{"dns": ["1.1.1.1o"]}`,
+			expectedErr: `invalid IP address: 1.1.1.1o`,
+		},
+		{
+			doc:         "multiple DNS, invalid IP-address",
+			input:       `{"dns": ["2.2.2.2", "1.1.1.1o"]}`,
+			expectedErr: `invalid IP address: 1.1.1.1o`,
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			var cfg Config
+			err := json.Unmarshal([]byte(tc.input), &cfg)
+			assert.Check(t, is.Error(err, tc.expectedErr))
+		})
+	}
+}


I was a bit on the fence whether this test was actually useful, as effectively, it's just testing Go stdlib. OTOH, without this, we would be removing test-cases, so 🤷‍♂️

I'd be happy to remove this test though if we don't think it adds value.

It tests slighty more than just the stdlib as it'd fail if the dns field was renamed, or if it was changed from a slice to something else. But yeah, honestly it has pretty much no value.

akerouanton · 2023-11-13T20:11:16Z

daemon/container_operations.go

 		sboxOptions = append(sboxOptions, libnetwork.OptionDNS(container.HostConfig.DNS))
 	} else if len(cfg.DNS) > 0 {
-		sboxOptions = append(sboxOptions, libnetwork.OptionDNS(cfg.DNS))
+		sboxOptions = append(sboxOptions, libnetwork.OptionDNS(ipAddresses(cfg.DNS)))


Probably best in a follow-up; change OptionDNS signature to make it take a slice of net.IP.

Yes, I initially had that, but line 58 above takes a slice of strings as input 😞, so it was a case of "win some" / "lose some", so I reverted it back to accepting strings.

akerouanton · 2023-11-13T20:11:55Z

daemon/container_operations.go

 	// TODO(thaJeztah): should this fail early if no sandbox was found?
 	sb, _ := daemon.netController.GetSandbox(container.ID)
-	createOptions, err := buildCreateEndpointOptions(container, n, endpointConfig, sb, cfg.DNS)
+	createOptions, err := buildCreateEndpointOptions(container, n, endpointConfig, sb, ipAddresses(cfg.DNS))


And same here; it qualifies for a follow-up clean up.

akerouanton

LGTM

thaJeztah · 2023-11-15T13:23:38Z

Interesting; looks like we're not very consistent with "defaults" configured on the daemon. For log-driver (and options), we bake the defaults into the container when it's created, but looks like we're not doing that for DNS; i.e. if the daemon was started with --dns 1.1.1.1, then the container does use 1.1.1.1 as DNS;

opened a tracking ticket for the above; enhancement: surface DNS in-use for container, and consider "reload" DNS #46822

thaJeztah added status/2-code-review area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code labels Nov 13, 2023

thaJeztah requested a review from tonistiigi as a code owner November 13, 2023 11:23

thaJeztah commented Nov 13, 2023

View reviewed changes

thaJeztah requested review from akerouanton and neersighted November 13, 2023 15:12

akerouanton reviewed Nov 13, 2023

View reviewed changes

akerouanton approved these changes Nov 13, 2023

View reviewed changes

neersighted approved these changes Nov 14, 2023

View reviewed changes

thaJeztah added this to the 25.0.0 milestone Nov 15, 2023

thaJeztah mentioned this pull request Nov 15, 2023

enhancement: surface DNS in-use for container, and consider "reload" DNS #46822

Open

thaJeztah merged commit 847f306 into moby:master Nov 15, 2023

thaJeztah deleted the daemon_no_custom_opts branch November 15, 2023 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon/config: change DNSConfig.DNS to a []net.IP#46803

daemon/config: change DNSConfig.DNS to a []net.IP#46803
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:daemon_no_custom_opts

thaJeztah commented Nov 13, 2023

Uh oh!

thaJeztah commented Nov 13, 2023

Uh oh!

thaJeztah Nov 13, 2023

Uh oh!

akerouanton Nov 13, 2023

Uh oh!

akerouanton Nov 13, 2023

Uh oh!

thaJeztah Nov 13, 2023

Uh oh!

akerouanton Nov 13, 2023 •

edited

Loading

Uh oh!

akerouanton left a comment

Uh oh!

thaJeztah commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Nov 13, 2023

Uh oh!

thaJeztah commented Nov 13, 2023

Uh oh!

thaJeztah Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton Nov 13, 2023 •

edited

Loading