libnet: Make sure network names are unique by akerouanton · Pull Request #46251 · moby/moby

akerouanton · 2023-08-17T12:40:26Z

- What I did

Fixes #18864, #20648, #33561, #40901 (and probably more).

This GH comment makes clear network name uniqueness has never been enforced due to the eventually consistent nature of Classic Swarm datastores:

there is no guaranteed way to check for duplicates across a cluster of docker hosts.

And this is further confirmed by other comments made by @mrjana in that same issue, eg. this one:

we want to adopt a schema which can pave the way in the future for a completely decentralized cluster of docker hosts (if scalability is needed).

This decentralized model is what Classic Swarm was trying to be. It's been superseded since then by Docker Swarm, which has a centralized control plane.

To circumvent this drawback, the NetworkCreate endpoint accepts a CheckDuplicate flag. However it's not perfectly reliable as it won't catch concurrent requests.

Due to this design decision, API clients like Compose have to implement workarounds to make sure names are really unique (eg. docker/compose#9585). And the daemon itself has seen a string of issues due to that decision, including some that aren't fixed to this day (for instance #40901):

The problem is, that if you specify a network for a container using the ID, it will add that network to the container but it will then change it to reference the network by using the name.

To summarize, this "feature" is broken, has no practical use and is a source of pain for Docker users and API consumers. So let's just remove it for all API versions.

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

moby/moby#46251 marks CheckDuplicate as deprecated. Any NetworkCreate request with a conflicting network name will now return an error. Signed-off-by: Albin Kerouanton <[email protected]>

integration: check_duplicate is now the default behavior moby/moby#46251 marks CheckDuplicate as deprecated. Any NetworkCreate request with a conflicting network name will now return an error. Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton · 2023-09-08T20:38:09Z

CI failed on a unrelated flake:

=== Failed
=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references/local_volume_with_mount_options (2.54s)
    daemon_test.go:513: assertion failed: error is not nil: Error response from daemon: remove test-live-restore-volume-references-local: volume has active mounts
        --- FAIL: TestLiveRestore/volume_references/local_volume_with_mount_options (2.54s)

=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references (18.62s)
    --- FAIL: TestLiveRestore/volume_references (18.62s)

=== FAIL: amd64.integration.daemon TestLiveRestore (0.00s)

thaJeztah · 2023-09-11T12:35:34Z

 		NetworkCreate: options,
 		Name:          name,
 	}
+	if versions.LessThan(cli.version, "1.44") {


Note to self; I'm actually wondering now if these checks we have in the client work, as we moved negotiating the API version to be as late as possible, which (I think) in this case could mean that we postpone it to cli.post (which calls cli.sendRequest, which calls cli.getAPIPath (which handles the negotiation)) 🙈

(It's probably fine to keep this as-is for now, as it looks like it's something to fix for other parts as well)

opened a PR to fix this issue;

client: negotiate api version before handling version-specific code #46463

thaJeztah · 2023-09-11T12:40:08Z

 }
+
+func TestForbidDuplicateNetworkNames(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")


Do you know why this didn't fail on Windows?

(didn't fail => why the test didn't work?)

I force pushed my commit to check what's the error, and here it is:

=== Failed === FAIL: github.com/docker/docker/integration/network TestForbidDuplicateNetworkNames (0.01s) network_test.go:266: [d46d27c88db30] failed to start daemon with arguments [--data-root D:\a\moby\moby\go\src\github.com\docker\docker\bundles\tmp\TestForbidDuplicateNetworkNames\d46d27c88db30\root --exec-root C:\Users\RUNNER~1\AppData\Local\Temp\dxr\d46d27c88db30 --pidfile D:\a\moby\moby\go\src\github.com\docker\docker\bundles\tmp\TestForbidDuplicateNetworkNames\d46d27c88db30\docker.pid --userland-proxy=true --containerd-namespace d46d27c88db30 --containerd-plugins-namespace d46d27c88db30p --containerd /var/run/docker/containerd/containerd.sock --host unix://C:\Users\RUNNER~1\AppData\Local\Temp\docker-integration\d46d27c88db30.sock --debug] : protocol not available

So, it's totally unrelated to this change. I'll create a follow-up PR addressing that.

Interesting; thanks!

(mostly commented because we have many tests with a skip, and for some nobody even remembers "why" 😂)

thaJeztah · 2023-09-12T08:37:14Z

🙈 I think one of your other PRs conflicted; looks like this needs a rebase now (changes LGTM though)

@mrjana

Fixes moby#18864, moby#20648, moby#33561, moby#40901. [This GH comment][1] makes clear network name uniqueness has never been enforced due to the eventually consistent nature of Classic Swarm datastores: > there is no guaranteed way to check for duplicates across a cluster of > docker hosts. And this is further confirmed by other comments made by @mrjana in that same issue, eg. [this one][2]: > we want to adopt a schema which can pave the way in the future for a > completely decentralized cluster of docker hosts (if scalability is > needed). This decentralized model is what Classic Swarm was trying to be. It's been superseded since then by Docker Swarm, which has a centralized control plane. To circumvent this drawback, the `NetworkCreate` endpoint accepts a `CheckDuplicate` flag. However it's not perfectly reliable as it won't catch concurrent requests. Due to this design decision, API clients like Compose have to implement workarounds to make sure names are really unique (eg. docker/compose#9585). And the daemon itself has seen a string of issues due to that decision, including some that aren't fixed to this day (for instance moby#40901): > The problem is, that if you specify a network for a container using > the ID, it will add that network to the container but it will then > change it to reference the network by using the name. To summarize, this "feature" is broken, has no practical use and is a source of pain for Docker users and API consumers. So let's just remove it for _all_ API versions. [1]: moby#18864 (comment) [2]: moby#18864 (comment) Signed-off-by: Albin Kerouanton <[email protected]>

thaJeztah

LGTM

Docker has disallowed multiple networks with the same name: moby/moby#46251 Change-Id: I4e5979dcf15b5270a0fc47a204b3a63405663809

* Update kuryr-libnetwork from branch 'master' to a4b276cfff1b2d88a58fd6c6a1c99e3761984ebd - Remove test_create_network_with_same_name Docker has disallowed multiple networks with the same name: moby/moby#46251 Change-Id: I4e5979dcf15b5270a0fc47a204b3a63405663809

akerouanton added area/api API status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking Networking impact/api impact/changelog labels Aug 17, 2023

akerouanton mentioned this pull request Aug 16, 2023

daemon: Improve NetworkingConfig & EndpointSettings validation #46183

Merged

akerouanton commented Aug 17, 2023

View reviewed changes

Comment thread integration/network/network_test.go Outdated

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch from f4bd1ba to 280795e Compare August 17, 2023 13:30

akerouanton commented Aug 18, 2023

View reviewed changes

Comment thread api/swagger.yaml Outdated

akerouanton commented Aug 18, 2023

View reviewed changes

Comment thread docs/api/version-history.md Outdated

akerouanton mentioned this pull request Aug 19, 2023

integration: Remove test_create_check_duplicate docker/docker-py#3170

Merged

stasadev mentioned this pull request Sep 7, 2023

Multiple ambiguous docker networks created ddev/ddev#5193

Closed

1 task

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch from 280795e to 07dcf62 Compare September 8, 2023 17:01

akerouanton marked this pull request as ready for review September 8, 2023 17:07

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch 2 times, most recently from bcbec62 to c8ba829 Compare September 8, 2023 17:30

akerouanton requested a review from tianon as a code owner September 8, 2023 17:30

akerouanton requested a review from thaJeztah September 10, 2023 17:04

thaJeztah requested changes Sep 11, 2023

View reviewed changes

Comment thread api/types/types.go Outdated

Comment thread client/network_create_test.go

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch from c8ba829 to fbb011e Compare September 11, 2023 11:40

thaJeztah reviewed Sep 11, 2023

View reviewed changes

Comment thread client/network_create.go Outdated

Comment thread api/types/types.go Outdated

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch from fbb011e to e77601b Compare September 11, 2023 12:24

akerouanton requested a review from thaJeztah September 11, 2023 12:29

thaJeztah reviewed Sep 11, 2023

View reviewed changes

Comment thread hack/make/test-docker-py

thaJeztah reviewed Sep 11, 2023

View reviewed changes

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch 2 times, most recently from 9b58911 to a5c0d9b Compare September 11, 2023 13:56

rfay mentioned this pull request Sep 11, 2023

fix: make project network external, fixes #5193 ddev/ddev#5305

Merged

akerouanton force-pushed the libnet-forbid-duplicated-network-names branch from a5c0d9b to 78479b1 Compare September 12, 2023 08:50

thaJeztah added this to the 25.0.0 milestone Sep 12, 2023

thaJeztah approved these changes Sep 12, 2023

View reviewed changes

thaJeztah mentioned this pull request Sep 12, 2023

client: negotiate api version before handling version-specific code #46463

Merged

neersighted approved these changes Sep 12, 2023

View reviewed changes

thaJeztah merged commit 3b04fd1 into moby:master Sep 12, 2023

akerouanton deleted the libnet-forbid-duplicated-network-names branch September 12, 2023 14:53

This was referenced Sep 12, 2023

race conditions lead to duplicate docker networks #20648

Closed

Networking - container always uses Name of network #40901

Closed

This was referenced Nov 9, 2023

fix: ensure no duplicates for docker networks, fixes #5193 ddev/ddev#5508

Merged

test: check for duplicate networks, for #5508 ddev/ddev#5541

Merged

akerouanton mentioned this pull request Dec 20, 2023

daemon: remove getNetworkID() helper #46970

Draft

dmartin mentioned this pull request Feb 22, 2024

Specifying docker run --network <network-id> attempts to match on name #41520

Closed

thaJeztah mentioned this pull request Jun 7, 2024

api/types/network: make CheckDuplicate optional #47924

Merged

1 task

openstack-mirroring pushed a commit to openstack/kuryr-libnetwork that referenced this pull request Sep 25, 2024

Remove test_create_network_with_same_name

a4b276c

Docker has disallowed multiple networks with the same name: moby/moby#46251 Change-Id: I4e5979dcf15b5270a0fc47a204b3a63405663809

thaJeztah mentioned this pull request Sep 17, 2025

api/types/network: CreateRequest: remove deprecated CheckDuplicate field #50998

Merged

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akerouanton commented Sep 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah Sep 11, 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 Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 12, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton commented Aug 17, 2023 •

edited

Loading

thaJeztah Sep 11, 2023 •

edited

Loading