daemon: Improve NetworkingConfig & EndpointSettings validation

[akerouanton]

• fixes 1.10-rc1 docker run should not create container if network options are invalid #19418

Depends on:

• daemon/create.go: Supersede github.com/pkg/errors #46182
• Add a temporary drop-in replacement for errors.Join #46188
• libnet: Make sure network names are unique #46251

- What I did

So far, only a subset of NetworkingConfig was validated when calling ContainerCreate. Other parameters would be validated when the container was started. And the same goes for EndpointSettings on NetworkConnect.

Also, the validation code was early exiting as soon as it caught an error. It now tries to validate as much as it can, and returns all the errors.

- How to verify it

Testing with the CLI won't work because it has its own client-side layer of validation (although incomplete). Some Go code has to be written to test all validation cases.

- Description for the changelog

• NetworkConnect and ContainerCreate have improved data validation, and now return all validation errors at once.

[akerouanton]

I'd prefer not to delete this test, and I'd even prefer to expand it to test interesting cases. But given that verifyNetworkingConfig is now a method of daemon, it's quite hard. I'd have to instantiate a daemon with a working libnetwork controller.

To make it easier to test, I thought about keeping verifyNetworkingConfig as a function and add a new networkFinder func(string) *libnetwork.Network parameter to it. It'd take daemon.FindNetwork in the daemon code, and a mock in this test. However, I'm not sure if that's a common / acceptable pattern.

[thaJeztah]

Just some quick comments from having a first glance.

[thaJeztah]

Looks like a bunch of these are also done in buildCreateEndpointOptions. Wondering if we can extract some of these checks, and make both use the same validation 🤔

moby/daemon/network.go

Lines 808 to 838 in 220fba0

	if epConfig != nil {
	if ipam := epConfig.IPAMConfig; ipam != nil {
	var ipList []net.IP
	for _, ips := range ipam.LinkLocalIPs {
	linkIP := net.ParseIP(ips)
	if linkIP == nil && ips != "" {
	return nil, fmt.Errorf("invalid link-local IP address: %s", ipam.LinkLocalIPs)
	}
	ipList = append(ipList, linkIP)
	}
	ip := net.ParseIP(ipam.IPv4Address)
	if ip == nil && ipam.IPv4Address != "" {
	return nil, fmt.Errorf("invalid IPv4 address: %s", ipam.IPv4Address)
	}
	ip6 := net.ParseIP(ipam.IPv6Address)
	if ip6 == nil && ipam.IPv6Address != "" {
	return nil, fmt.Errorf("invalid IPv6 address: %s", ipam.IPv6Address)
	}
	createOptions = append(createOptions, libnetwork.CreateOptionIpam(ip, ip6, ipList, nil))
	}
	for _, alias := range epConfig.Aliases {
	createOptions = append(createOptions, libnetwork.CreateOptionMyAlias(alias))
	}
	for k, v := range epConfig.DriverOpts {
	createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(options.Generic{k: v}))
	}
	}

[akerouanton]

Yeah, I'm not sure what to do with these errors. For me, there're more like defensive checks than data validation. And there're needed because we actually need "strong-type".

From that perspective we could potentially do a quick check on well-formed-ness of options (we should really look if we can do such parsing/conversions earlier, and get to a "strong-type" ASAP in the process, as we're sometimes parsing the same string multiple times), and only if we get that far, proceed with the heavier lifting.

IMHO the best way to solve that would be to create an intermediate representation that's stored in-memory (and on-disk?) and not exposed on the API. Then, the validation step could be made responsible of instantiating these "stronger types". But that's a non-trivial chunk of work, and this PR is required by docker/cli#4493 (which in turn is required by docker/cli#4419, itself required by the other PRs adding proper support for multi --network parameters).

[akerouanton]

As shared privately, for now we can't really drop the validation steps in buildCreateEndpointOptions. We first need to make some refactorings to make sure user-supplied data are correctly sanitized and transformed into useful structs instead of keeping literals everywhere.

This will be addressed in a follow-up PR.

[thaJeztah]

This validation is called from 2 places;

In Daemon.updateNetworkConfig();

if err := validateEndpointSettings(n, n.Name(), endpointConfig); err != nil {
	return err
}

In Daemon.validateNetworkingConfig();

// The referenced network k might not exist when the container is created, so don't fail if that's the case.
// If any other error happen, it's not a validation error, so better early exit.
nw, err := daemon.FindNetwork(k)
if err != nil && !errdefs.IsNotFound(err) {
	return err
}

if err := validateEndpointSettings(nw, k, v); err != nil {
	errs = append(errs, fmt.Errorf("invalid endpoint config for network %s: %w", k, err))
}

• in both cases, I think we'd already barf out if the network is not to be found
• and, I think in both cases nwName is effectively equivalent to nw.Name() (although I'm not 100% sure for the second case; can it use network ID instead of name? does that matter for this case?)

So I'm wondering if we should continue to bail out early if there's no network (instead of continuing with other checks), as we probably would never reach this validation

Also slightly looking if we should split "heavy" and "lightweight" validation; e.g. validating for invalid / invalidly formatted IP-addresses is something that's relatively lightweight, whereas fetching the network (knowing libnetwork ...) may "not be". From that perspective we could potentially do a quick check on well-formed-ness of options (we should really look if we can do such parsing/conversions earlier, and get to a "strong-type" ASAP in the process, as we're sometimes parsing the same string multiple times), and only if we get that far, proceed with the heavier lifting.

[akerouanton]

in both cases, I think we'd already barf out if the network is not to be found

Nope, we actually accept a ContainerCreate request with a reference to a non-existant network:

moby/daemon/container_operations.go

Lines 577 to 580 in b227bc4

	// validateEndpointSettings checks whether the given epConfig is valid. The nw parameter might be nil as a container
	// can be created with a reference to a network that don't exist yet. In that case, only partial validation will be
	// done.
	func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *networktypes.EndpointSettings) error {

It's not a hypothetical case, it's how things are working right now 😉

$ docker network inspect foobar
[]
Error response from daemon: network foobar not found

$ docker container create --network foobar busybox
6697a506ee56c76d770218b49794255b8059a0f6deec836a0c26778379612b6a

we should really look if we can do such parsing/conversions earlier, and get to a "strong-type" ASAP in the process

Yeah, I looked into this a bit already. I'd like to have netip.* instead of strings, as it'd remove the need of constantly parsing the same thing. But that imply quite a bunch of work. I didn't had time to create the CLI PR that depends on this one, but it all have to do with the ability to provide multiple --network flags, hence I'd prefer to keep this one simple.

EDIT: For the record, here's the CLI PR that requires this one: docker/cli#4493

[thaJeztah]

Nope, we actually accept a ContainerCreate request with a reference to a non-existant network:

For context here (I stumbled upon that recently); I think this is the commit that changed that behavior; 99a98cc (part of #25962). In that commit, it's no longer checked if the network exists, with the assumption that it may be a dynamically rolled-out (but "attachable") network, and will be created "lazily"".

(also related: #26449, #27466)

Honestly, looking at some of that, I'm not sure I'm a "fan". Perhaps we should indeed;

• Always have the network "available" on every node (but it could be a "definition-only" for dynamically rolled-out networks)
• Or, for attachable networks, and a container attaching to it, we could still consider to initialize it when attaching

But this definitely requires some digging into the internals / underlying mechanisms to see if that's possible.

[akerouanton]

Thanks for the digging!

• Always have the network "available" on every node (but it could be a "definition-only" for dynamically rolled-out networks)
• Or, for attachable networks, and a container attaching to it, we could still consider to initialize it when attaching

Once this PR is merged, I'll submit a follow-up refactor PR to move attachment requests out of findAndAttachNetwork to a dedicated method called before doing anything else in initializeNetworking. With that change:

• On ContainerCreate, only soundness checks will be done ;
• On ContainerStart (and NetworkConnect), we'll determine whether referenced network exists locally, attach it if it's swarm scoped, or bail out because if it doesn't. And then, we can normalize and validate the settings thoroughly ;

I started doing some work in that direction here: fba50d6.

I think it'd be "conceptually" better to go back to the original semantic: you can only attach to a network that exists locally; but it'd be more work to implement and I guess we'd probably need to gate that behind an API version check, so anyway... I think we can get the same result by shuffling a bit the order of operations.

[thaJeztah]

I think it'd be "conceptually" better to go back to the original semantic: you can only attach to a network that exists locally; but it'd be more work to implement and I guess we'd probably need to gate that behind an API version check, so anyway... I think we can get the same result by shuffling a bit the order of operations.

Yes, we need to look at that! I guess overall the tricky bit is that we must assume that there's potentially many nodes and many networks, and not all of those networks to be used on each node.

• For "attachable" networks, it would be good to have a ("stub?") network on each node, so that docker network attache can be used for containers; we could probably still make it a stub (so that it doesn't take up resources on all notes in the swarm), and make it an actual network when attaching.
• For "non-attachable" networks, I guess it's TBD; do we want to show stubs for those? (if it's a worker node, the worker itself wouldn't have much purpose for it until a swarm task lands on the node).
• For managers it would be more useful (as you could run docker network rm <some swarm-scope network>)

Either way; all of that is for a follow-up discussion. We could create a ticket for it though (perhaps copy/paste some of the above)

[thaJeztah]

Just an observation here;

• hasUserDefinedIPAddress() will always be false if there's no ipamConfig, which may already invalidate other checks.
• ^^ slightly leaning toward inlining that function here, and just make it a hasUserDefinedIPAddress variable that we set (and use), as that function is only used inside this check, and we're now potentially calling it twice with the same results
• BOTH enableIPOnPredefinedNetwork and serviceDiscoveryOnDefaultNetwork are effectively if runtime.GOOS == "windows")
• ^^ wondering if we need to split the validation there, instead of having two utility functions that effectively means "different validation on windows vs linux" (not sure)

[akerouanton]

Hm, most of these would be better handled in their own PR. Also, the next step once we get proper support for multi --network flags, is to make the default bridge just a "normal" bridge (ie. no more links, no more artificial restriction, etc...). So better keep these refactorings for that time (unless you want to go ahead and do this beforehand).

[akerouanton]

• BOTH enableIPOnPredefinedNetwork and serviceDiscoveryOnDefaultNetwork are effectively if runtime.GOOS == "windows")
• ^^ wondering if we need to split the validation there, instead of having two utility functions that effectively means "different validation on windows vs linux" (not sure)

I added a TODO to remind us we should remove these functions once we remove legacy networking mode from the default bridge. As such, I think we shouldn't make the validation OS-dependent here, as it's going to be addressed (hopefully) soon.

[akerouanton]

The test TestCreateWithDuplicateNetworkNames from the service suite is failing because this condition will return a validation error if two networks with conflicting names are created.

Instead of working around this, I drop TestCreateWithDuplicateNetworkNames in #46251.

[akerouanton]

I don't like this, we need a proper multierror.Append instead. But I think it's ok to address that in a follow-up PR. @thaJeztah Any thoughts?

[thaJeztah]

Yeah, not against having a utility to add it.

I guess in that case we also may be able to skip the multierrors.Join() ? (i.e., an nil vs empty slice), and perhaps just return errs in that case?

I'm fine with improving in a follow-up though.

[thaJeztah]

overall looks good; there were some changes that landed in the wrong commit, so left comments about those, and I left a comment about a commit (renaming cfg => ipamConfig) that I think would be ok to squash with the commit after it)

[thaJeztah]

We're passing both k, and v as arguments, and nw may not be needed if we fail early inside the function.

Would it make sense to

• remove the nw, _ := daemon.FindNetwork(k) here
• make validateEndpointSettings a method on daemon (func (daemon *daemon) validateEndpointSettings( nwName string, epConfig *networktypes.EndpointSettings)
• perform looking up the network inside the function (if we reach that point)

However, it may still be an indicator that this function is trying to do too much, and maybe we should split parts of it (i.e. being able to validate the endpoint settings themselves, and settings that depend on the network's setting)

FWIW, not necessarily a blocker, but mostly considering "what's the overhead of getting the network"? (could this be trying plugins etc?)

[akerouanton]

However, it may still be an indicator that this function is trying to do too much

Mh, I have fixed feelings about whether it does too much things or not. But once the refactor PR I mentioned previously is merged, attachSwarmNetworks will happen first and we'll be able to take for granted that all networks exist when we start validation. This should make the code easier to follow, and help us draw a line between the "soundness checks" I mentioned and in-depth validation.

As some of this is already addressed in daemon: move most of validateEndpointSettings into api/t/net, I'll keep the code as-is in this commit.

[thaJeztah]

Nope, we actually accept a ContainerCreate request with a reference to a non-existant network:

For context here (I stumbled upon that recently); I think this is the commit that changed that behavior; 99a98cc (part of #25962). In that commit, it's no longer checked if the network exists, with the assumption that it may be a dynamically rolled-out (but "attachable") network, and will be created "lazily"".

(also related: #26449, #27466)

Honestly, looking at some of that, I'm not sure I'm a "fan". Perhaps we should indeed;

• Always have the network "available" on every node (but it could be a "definition-only" for dynamically rolled-out networks)
• Or, for attachable networks, and a container attaching to it, we could still consider to initialize it when attaching

But this definitely requires some digging into the internals / underlying mechanisms to see if that's possible.

[thaJeztah]

A bit on the fence on this one; generally, "the closer" a variable is define to where it's used, the shorter the name should be. For receivers, it's very common to use a single letter (as all code generally uses it very nearby)

If this was to prevent a conflict, perhaps just c works (although I don't think the original cfg was very ambiguous).

never mind, I see the follow-up commit, and there's some functions that are pretty long, so I guess it somewhat makes sense.

(I'd be OK with combining those in a single commit though, so that it's clear why you're renaming!)

[akerouanton]

I went the other way around and dropped this commit as I think it's okay to stick to cfg.

[thaJeztah]

Yeah, not against having a utility to add it.

I guess in that case we also may be able to skip the multierrors.Join() ? (i.e., an nil vs empty slice), and perhaps just return errs in that case?

I'm fine with improving in a follow-up though.

[thaJeztah]

I wanted to suggest: could we use the underlying netip.Parse instead, so that we can return an actual error with details "what is invalid"??

... only to realise that's new in Go 1.21 https://github.com/golang/go/blob/2c1e5b05fe39fc5e6c730dd60e82946b8e67c6ba/src/net/ip.go#L489-L502

[akerouanton]

Good catch!

... only to realise that's new in Go 1.21

netip.ParseAddr is actually available since go 1.18, so we can perfectly use it. But looking at the git blame, it looks like net parsers will use netip ones starting with go 1.21.

As a consequence, we might start accepting IPv6 addresses with scope zone (eg. fe80::1cc0:3e8c:119f:c2e1%ens18), whereas it's invalid. I need to double-check but we probably need a proper error for that.

EDIT: re-reading the diff, net.ParseIP will continue to return a nil if the IPv6 address includes a zone, so no need to worry about that.

[akerouanton]

Huh, that's what errors netip.ParseAddr(...) produces: ParseAddr("foo"): unable to parse IP. So, it'd look like this if we use it here:

* invalid IPv4 address foo: ParseAddr("foo"): unable to parse IP
* invalid IPv6 address bar: ParseAddr("bar"): unable to parse IP
* invalid link-local IP address baz: ParseAddr("baz"): unable to parse IP
* invalid link-local IP address foobar: ParseAddr("foobar"): unable to parse IP

I think we should rather stick with net.ParseIP.

[thaJeztah]

Ah, yes, that doesn't provide much more context indeed. I was hoping it would give the user more context what's wrong. Thanks for looking!

[thaJeztah]

Maybe we should rename this to ipamDriver at some point 😂

[thaJeztah]

If you're updating, can you add an empty line in between? ISTR GoDoc can hide "TODO" comments in the generated documentation as long as there's an empty comment-line in between, so;

// IpamConf contains all the ipam related configurations for a network.
//
// TODO(aker): use proper net/* structs instead of string literals.

(LOL, also this struct should really be named IPAMConf (to be properly camelcase, but that's a different topic)

[thaJeztah]

LGTM