diff --git a/daemon/container_operations.go b/daemon/container_operations.go index c6a4e2cdf0..d1b378e410 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -744,6 +744,22 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config, } } + // 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 = prevNetworks + } + }() + if err := daemon.updateNetworkConfig(ctr, n, endpointConfig.EndpointSettings); err != nil { return err } @@ -772,11 +788,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/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 42b5fa54d6..ac8101cacd 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -623,6 +623,31 @@ 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")) + + // 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 @@ -1460,6 +1485,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")