Validate IPAM config before handing it over to libnetwork

[akerouanton]

- What I did

Prior to this change, IPAM config was never validated by the API. Some checks are done by the CLI, but it's not exhaustive (and doesn't cover API users anyway). Some of these errors might be caught early by libnetwork (eg. when the network is created), and other errors would surface only when connecting a container to a network with faulty IPAM. In both cases, the API would return a 500 instead of returning a proper 400 at creation time.

Although the NetworkCreate endpoint might already return warnings, these are never displayed by the CLI. As such, it was decided during a maintainer's call to return validation errors for all API versions.

Prior to #44968, the daemon would happily accept an --ip-range with a larger mask than its parent subnet. In such case, the IPAM allocator would implicitly ignore the ip-range (not because it detected it was a wrong value, but due to the way computation was done). As such, the 2nd commit explicitly discard the ip-range and replace it with the subnet value when it detects it's a bad value. This error would be caught by validation added in the first commit, so it's only useful for network restored from state stored on-disk.

One such example can be seen in docker/for-mac#6870. The user creates a network with an ip-range bigger than its subnet, and with some host bits set. When they try to create a container connected to that network, the daemon returns this error: docker: Error response from daemon: invalid bit range [0, 16777215).. This defensive check is right, but the user should be warned of bad IPAM config earlier and in a more graceful way.

- How to verify it

$ docker network create --subnet 172.92.0.0/16 --ip-range 172.92.0.0/8 --gateway 172.92.0.1 main2
Error response from daemon: invalid network config:
* invalid ip-range 172.92.0.0/8: CIDR block is bigger than its parent subnet 172.92.0.0/16
* invalid ip-range 172.92.0.0/8: it should be 172.0.0.0/8

- Description for the changelog

• Validate IPAM config before creating a network.

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

[neersighted]

Discussed in the maintainers call / @akerouanton is making the below changes:

• Semantics for existing networks look good
• We want to hard fail in new major versions of the engine, and soft fail in older versions:
  • This is in place of the current behavioral difference based on API version.
  • This should be accomplished by a two-commit dance: one commit introduces the warning, another changes it to an error.
  • Cherry-picks to older branches should exclude the commit that makes this an error.

[thaJeztah]

Thanks; I left some suggestions; let me know what you think.

I committed some of my suggestions in a branch, and just opened a PR against your fork in case that helps you seeing them as a whole (except for moving the validation to libnetwork if you agree on that);

• review suggestions for IPAM validation akerouanton/docker#2

[thaJeztah]

I'm a bit on the fence on having all this validation code in the API router. Given that we're already importing libnetwork, perhaps it'd make sense to move the validation code there (not sure where the right place would be inside libnetwork, perhaps in the ipam or ipamapi package.

We can either have the function exported (and call it here; ipam.Validate(...)), or perhaps as part of the final commit (which turns it into an error), move it into backend.CreateNetwork(). That way calling that function would also validate before creating (regardless of where it's called from).

[akerouanton]

I'm a bit on the fence on having all this validation code in the API router.

I think the best would be to add IPAM validation call to (*Daemon).CreateNetwork(). However, the following method from swarmkit would prevent converting errors into warnings when the network is created from a non-leader manager node, as it returns either the network created or an error, but not both:

https://github.com/moby/swarmkit/blob/ad0f3ae162fa30d0710105211d37996d5ec38b27/api/control.pb.go#L6678-L6685

Also, there's already some sort of network validation in the daemon pkg (ie. d.containerStart() -> d.initializeNetworking() -> d.allocateNetwork() -> d.connectToNetwork() -> d.updateNetworkConfig() -> validateNetworkingConfig()).

Given that we're already importing libnetwork, perhaps it'd make sense to move the validation code there

Currently the API only uses constants and errors from libnetwork packages. If we call IPAM validation helper from the API due to my previous point, these helpers would need to either:

1. Rely on the API type IPAMConfig, and thus import it into libnetwork -- that'd be a bad separation of concern IMO ;
2. Convert API type IPAMConfig into libnetwork IpamConf. And then, errors would have to either reference fields from the latter struct and re-translated to match the former -- that'd make the validation layer more complex with no real gains. Or modeled after IPAMConfig, but then we're back at bad separation of concerns ;

Also, I think we should make sure as much as possible that libnetwork data structures never end up in an invalid state -- that's the crux of the original issue I'm solving here. Maintaining libnetwork is way harder if we can't trust this rule is enforced. This also discards option 2.

[thaJeztah]

Currently the API only uses constants and errors from libnetwork packages. If we call IPAM validation helper from the API due to my previous point, these helpers would need to either:

• Rely on the API type IPAMConfig, and thus import it into libnetwork -- that'd be a bad separation of concern IMO ;
• Convert API type IPAMConfig into libnetwork IpamConf. And then, errors would have to either reference fields from the latter struct and re-translated to match the former -- that'd make the validation layer more complex with no real gains. Or modeled after IPAMConfig, but then we're back at bad separation of concerns ;
• Also, I think we should make sure as much as possible that libnetwork data structures never end up in an invalid state -- that's the crux of the original issue I'm solving here. Maintaining libnetwork is way harder if we can't trust this rule is enforced. This also discards option 2.

• Looking at the validation we have in this PR, we only need create.IPAM, and create.EnableIPv6.
• backend.CreateNetwork() is implemented by daemon.CreateNetwork() / daemon.createNetwork()
• .. which already imports "github.com/docker/docker/libnetwork/ipamapi"
• .. and has a getIpamConfig() function (which can produce an error for invalid configuration)

And on the calling site (network_routes.go), we already have some checking for error-types returned by the backend (e.g. libnetwork.NetworkNameError) to turn errors into warnings.

So my thinking is to

• have the validation either in the backend (daemon.createNetwork() -> getIpamConfig()) itself (or in the ipamapi package, and called from there)
• make it return a sentinel error (for example, libnetwork.InvalidIPAMConfig)
• ^^ Ideally that error would implement errdefs.InvalidParam

For the current release, we catch that (libnetwork.InvalidIPAMConfig), and turn them into a warning, and for the next release, we remove that check, and return the error as-is (producing a 4XX status if it implements errdefs.InvalidParam)

Would that work?

[akerouanton]

I did a few things since the last round of reviews:

• The data validation logic is now moved to the daemon package ;
• Improved error messages to make them less chatty while retaining important info ;
• Rebased on top of api: Endpoints can now return a tree of errors #46099, to see how the formatted message field is shown on the CLI. There's no conflict between that branch and this PR, so it's safe to ignore first commit while reviewing.

$ docker network create --subnet 172.92.0.0/16 --ip-range 172.92.0.0/8 --gateway 172.92.0.1 main2
Error response from daemon: 2 errors occurred:
	* invalid ip-range 172.92.0.0/8: CIDR block is bigger than its parent subnet 172.92.0.0/16
	* invalid ip-range 172.92.0.0/8: some bits are set in the host fragment

• And finally, github.com/pkg/errors is replaced by stdlib's errors in daemon/network.go, to make errors.Join() available

[thaJeztah]

Pushed a small fix to temporarily skip the docker-py tests that we know are currently failing until we have a version with docker/docker-py#3169 included; https://github.com/moby/moby/compare/fbb597e1cf2f207fb84ae7649222f31fceaaa055..3e8af0817a968809d477cd8bdf1832dacc791d69

- : "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules --deselect=tests/integration/api_volume_test.py::TestVolumes::test_prune_volumes}"
+ # TODO re-enable test_connect_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
+ # TODO re-enable test_create_network_ipv6_enabled after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
+ # TODO re-enable test_create_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
+ : "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules --deselect=tests/integration/api_volume_test.py::TestVolumes::test_prune_volumes --deselect=tests/integration/api_network_test.py::TestNetworks::test_connect_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_network_ipv6_enabled --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_with_ipv6_address}"

[thaJeztah]

LGTM if CI goes green 👍