From 10f23552534977d82691ae0c9029712169768de5 Mon Sep 17 00:00:00 2001 From: Sahil Singh Date: Wed, 29 Apr 2026 17:28:35 +0530 Subject: [PATCH 1/2] daemon: connectToNetwork: keep Networks on failure When connectToNetwork fails (e.g. ep.Join hits a "port already allocated"), the deferred rollback unconditionally deleted the entry it had just written under nwName. For the start-time path that entry was the original Networks["bridge"] put there by docker create -- not something this function introduced -- so deleting it wiped the container's network config. With Networks empty, the next docker start ranges over an empty map in allocateNetwork, returns nil, and the container "starts" with no networking attached. HostConfig.PortBindings is preserved but never applied, and the conflicting port goes silently unbound. Snapshot Networks before any mutation in the function, and on retErr restore the snapshot. Pre-existing entries (created at docker-create time) are restored; entries this function itself added (e.g. on a failed docker network connect to a new network) are dropped. Extend TestPublishedPortAlreadyInUse to check that a second ContainerStart against the same port conflict also errors and that NetworkSettings.Networks still contains the bridge entry. Signed-off-by: Sahil Singh --- daemon/container_operations.go | 16 +++++++++++----- integration/network/bridge/bridge_linux_test.go | 12 ++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 7f9d30595d..7d3cba55e7 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -744,6 +744,17 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config, } } + // Snapshot Networks so a failure anywhere in the rest of this function + // rolls it back to its pre-call state. Without this, a failed start (e.g. + // published port already in use) would leave Networks empty and the next + // start would silently "succeed" with no networking configured. + origNetworks := maps.Clone(ctr.NetworkSettings.Networks) + defer func() { + if retErr != nil { + ctr.NetworkSettings.Networks = origNetworks + } + }() + if err := daemon.updateNetworkConfig(ctr, n, endpointConfig.EndpointSettings); err != nil { return err } @@ -772,11 +783,6 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config, delete(ctr.NetworkSettings.Networks, n.ID()) ctr.NetworkSettings.Networks[nwName] = endpointConfig - defer func() { - if retErr != nil { - delete(ctr.NetworkSettings.Networks, nwName) - } - }() if nwName == network.DefaultNetwork { // Legacy links must be prepared before the Endpoint.Join, because the network diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index 10cb963686..9c714cb863 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -623,6 +623,18 @@ func TestPublishedPortAlreadyInUse(t *testing.T) { inspect, err := apiClient.ContainerInspect(ctx, ctr2, client.ContainerInspectOptions{}) assert.NilError(t, err) assert.Check(t, is.Equal(inspect.Container.State.Status, containertypes.StateCreated)) + + _, err = apiClient.ContainerStart(ctx, ctr2, client.ContainerStartOptions{}) + assert.Assert(t, is.ErrorContains(err, "failed to set up container networking")) + + inspect, err = apiClient.ContainerInspect(ctx, ctr2, client.ContainerInspectOptions{}) + assert.NilError(t, err) + assert.Check(t, is.Equal(inspect.Container.State.Status, containertypes.StateCreated)) + assert.Assert(t, is.Len(inspect.Container.HostConfig.PortBindings[mappedPort], 1)) + assert.Check(t, is.Equal(inspect.Container.HostConfig.PortBindings[mappedPort][0].HostPort, "8000")) + // The bridge entry must survive the failed start, otherwise the next start + // would silently succeed with no networking attached. + assert.Assert(t, is.Contains(inspect.Container.NetworkSettings.Networks, "bridge")) } // TestAllPortMappingsAreReturned check that dual-stack ports mapped through From b31e42d42e0ff12015cfac4233bd7a283d55bf9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Mon, 12 Oct 2026 16:40:21 +0200 Subject: [PATCH 2/2] daemon: Don't report a stale endpoint after a failed container start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When connecting a container to a network failed during container start, the restored network settings still held the endpoint ID and addresses from the failed attempt, and the container list reported them for a container that wasn't connected. On container start, the endpoint config passed to connectToNetwork is the entry stored in the container's network settings. The rollback restored a shallow copy of the map, so it put back the same entry, which had already been updated with the failed endpoint's details. Copy each entry before connecting, so the rollback restores the config as it was before the call. Also test that a running container whose connection to a network fails, because the requested address is already in use, doesn't keep that network in its settings and can still be restarted. Signed-off-by: Paweł Gronowski --- daemon/container_operations.go | 17 +++-- daemon/network/settings.go | 12 ++++ .../network/bridge/bridge_linux_test.go | 62 +++++++++++++++++++ 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 7d3cba55e7..4c338fea8d 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -744,14 +744,19 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config, } } - // Snapshot Networks so a failure anywhere in the rest of this function - // rolls it back to its pre-call state. Without this, a failed start (e.g. - // published port already in use) would leave Networks empty and the next - // start would silently "succeed" with no networking configured. - origNetworks := maps.Clone(ctr.NetworkSettings.Networks) + // If the connection fails, restore the container's networks so that the + // failed endpoint config isn't used when the container next starts. Restore + // rather than delete, because on container start the entry already holds + // the user's config for this network. Copy the entries too, because on + // container start endpointConfig is the stored entry, and it's modified + // below. + prevNetworks := maps.Clone(ctr.NetworkSettings.Networks) + for name, es := range prevNetworks { + prevNetworks[name] = es.Copy() + } defer func() { if retErr != nil { - ctr.NetworkSettings.Networks = origNetworks + ctr.NetworkSettings.Networks = prevNetworks } }() diff --git a/daemon/network/settings.go b/daemon/network/settings.go index c8be7baab6..6007e2cf4a 100644 --- a/daemon/network/settings.go +++ b/daemon/network/settings.go @@ -2,6 +2,7 @@ package network import ( "net" + "slices" "sync" networktypes "github.com/moby/moby/api/types/network" @@ -31,6 +32,17 @@ type EndpointSettings struct { DesiredMacAddress networktypes.HardwareAddr } +// Copy returns a deep copy of es. +func (es *EndpointSettings) Copy() *EndpointSettings { + if es == nil { + return nil + } + esCopy := *es + esCopy.EndpointSettings = es.EndpointSettings.Copy() + esCopy.DesiredMacAddress = slices.Clone(es.DesiredMacAddress) + return &esCopy +} + // AttachmentStore stores the load balancer IP address for a network id. type AttachmentStore struct { sync.Mutex diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index 9c714cb863..0fbcacc9ea 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -635,6 +635,19 @@ func TestPublishedPortAlreadyInUse(t *testing.T) { // The bridge entry must survive the failed start, otherwise the next start // would silently succeed with no networking attached. assert.Assert(t, is.Contains(inspect.Container.NetworkSettings.Networks, "bridge")) + + // The container list must not report the endpoint that was rolled back. + res, err := apiClient.ContainerList(ctx, client.ContainerListOptions{ + All: true, + Filters: make(client.Filters).Add("id", ctr2), + }) + assert.NilError(t, err) + assert.Assert(t, is.Len(res.Items, 1)) + assert.Assert(t, res.Items[0].NetworkSettings != nil) + ep := res.Items[0].NetworkSettings.Networks["bridge"] + assert.Assert(t, ep != nil) + assert.Check(t, is.Equal(ep.EndpointID, "")) + assert.Check(t, !ep.IPAddress.IsValid()) } // TestAllPortMappingsAreReturned check that dual-stack ports mapped through @@ -1404,6 +1417,55 @@ func TestJoinError(t *testing.T) { assert.Check(t, is.Equal(res.ExitCode, 0), "container should have an eth1") } +// TestEndpointCreateErrorRollback checks that if connecting a running container +// to a network fails while creating the endpoint (here, because the requested +// static IPv4 address is already in use), the failed network is not left in the +// container's network settings. Otherwise, the container could not be restarted. +func TestEndpointCreateErrorRollback(t *testing.T) { + ctx := setupTest(t) + c := testEnv.APIClient() + + const netName = "testnetrollback" + const gwAddr = "192.168.126.1" + const staticIP = "192.168.126.10" + network.CreateNoError(ctx, t, c, netName, + network.WithIPAM("192.168.126.0/24", gwAddr), + ) + defer network.RemoveNoError(ctx, t, c, netName) + + // Hold the static address, so that connecting another container to it fails. + holderID := ctr.Run(ctx, t, c, + ctr.WithNetworkMode(netName), + ctr.WithIPv4(netName, staticIP), + ) + defer c.ContainerRemove(ctx, holderID, client.ContainerRemoveOptions{Force: true}) + + cid := ctr.Run(ctx, t, c) + defer c.ContainerRemove(ctx, cid, client.ContainerRemoveOptions{Force: true}) + + _, err := c.NetworkConnect(ctx, netName, client.NetworkConnectOptions{ + Container: cid, + EndpointConfig: &networktypes.EndpointSettings{ + IPAMConfig: &networktypes.EndpointIPAMConfig{ + IPv4Address: netip.MustParseAddr(staticIP), + }, + }, + }) + assert.Check(t, is.ErrorContains(err, "Address already in use")) + + // Only the default bridge network should show up in container inspect. + ctrInsp := ctr.Inspect(ctx, t, c, cid) + assert.Check(t, is.Len(ctrInsp.NetworkSettings.Networks, 1)) + assert.Check(t, is.Contains(ctrInsp.NetworkSettings.Networks, "bridge")) + + // Restarting would fail if the failed network config had been kept. + _, err = c.ContainerRestart(ctx, cid, client.ContainerRestartOptions{}) + assert.NilError(t, err) + ctrInsp = ctr.Inspect(ctx, t, c, cid) + assert.Check(t, is.Equal(ctrInsp.State.Running, true)) + assert.Check(t, is.Len(ctrInsp.NetworkSettings.Networks, 1)) +} + func TestPreferredSubnetRestore(t *testing.T) { skip.If(t, testEnv.IsRootless(), "fails before and after restart")