@@ -731,10 +731,14 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config,

 			}

 		}

 	}()

-	ctr.NetworkSettings.Networks[nwName] = endpointConfig

 	delete(ctr.NetworkSettings.Networks, n.ID())

-

+	ctr.NetworkSettings.Networks[nwName] = endpointConfig

+	defer func() {

+		if retErr != nil {

+			delete(ctr.NetworkSettings.Networks, nwName)

+		}

+	}()

 	if err := daemon.updateEndpointNetworkSettings(cfg, ctr, n, ep); err != nil {

 		return err

 	}

@@ -100,7 +100,7 @@ func (sb *Sandbox) clearDefaultGW() error {

 	if ep = sb.getEndpointInGWNetwork(); ep == nil {

 		return nil

 	}

-	if err := ep.sbLeave(context.TODO(), sb, false); err != nil {

+	if err := ep.sbLeave(context.TODO(), sb, ep.getNetwork(), false); err != nil {

 		return fmt.Errorf("container %s: endpoint leaving GW Network failed: %v", sb.containerID, err)

 	}

 	if err := ep.Delete(context.TODO(), false); err != nil {

@@ -176,17 +176,16 @@ func (c *Controller) defaultGwNetwork() (*Network, error) {

 	return n, err

 }

+func (sb *Sandbox) isGatewayEndpoint(epId string) bool {

+	gw4, gw6 := sb.getGatewayEndpoint()

+	return (gw4 != nil && epId == gw4.ID()) || (gw6 != nil && epId == gw6.ID())

+}

+

 // getGatewayEndpoint returns the endpoints providing external connectivity to

 // the sandbox. If the gateway is dual-stack, ep4 and ep6 will point at the same

 // endpoint. If there is no IPv4/IPv6 connectivity, nil pointers will be returned.

 func (sb *Sandbox) getGatewayEndpoint() (ep4, ep6 *Endpoint) {

-	return selectGatewayEndpoint(sb.Endpoints())

-}

-

-// selectGatewayEndpoint is like getGatewayEndpoint, but selects only from

-// endpoints.

-func selectGatewayEndpoint(endpoints []*Endpoint) (ep4, ep6 *Endpoint) {

-	for _, ep := range endpoints {

+	for _, ep := range sb.Endpoints() {

 		if ep.getNetwork().Type() == "null" || ep.getNetwork().Type() == "host" {

 			continue

 		}

@@ -514,9 +514,12 @@ func (ep *Endpoint) sbJoin(ctx context.Context, sb *Sandbox, options ...Endpoint

 	ep.mu.Unlock()

 	defer func() {

 		if retErr != nil {

-			ep.mu.Lock()

-			ep.sandboxID = ""

-			ep.mu.Unlock()

+			if err := ep.sbLeave(ctx, sb, n, true); err != nil {

+				log.G(ctx).WithFields(log.Fields{

+					"error":  err,

+					"retErr": retErr,

+				}).Warn("Failed to remove endpoint after join error")

+			}

 		}

 	}()

@@ -561,11 +564,6 @@ func (ep *Endpoint) sbJoin(ctx context.Context, sb *Sandbox, options ...Endpoint

 	gwepBefore4, gwepBefore6 := sb.getGatewayEndpoint()

 	sb.addEndpoint(ep)

-	defer func() {

-		if retErr != nil {

-			sb.removeEndpoint(ep)

-		}

-	}()

 	if err := sb.populateNetworkResources(ctx, ep); err != nil {

 		return err

@@ -753,20 +751,20 @@ func (ep *Endpoint) Leave(ctx context.Context, sb *Sandbox) error {

 	sb.joinLeaveMu.Lock()

 	defer sb.joinLeaveMu.Unlock()

-	return ep.sbLeave(ctx, sb, false)

-}

-

-func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, force bool) error {

 	n, err := ep.getNetworkFromStore()

 	if err != nil {

 		return fmt.Errorf("failed to get network from store during leave: %v", err)

 	}

-	ep, err = n.getEndpointFromStore(ep.ID())

+	storedEp, err := n.getEndpointFromStore(ep.ID())

 	if err != nil {

 		return fmt.Errorf("failed to get endpoint from store during leave: %v", err)

 	}

+	return storedEp.sbLeave(ctx, sb, n, false)

+}

+

+func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force bool) error {

 	ctx = log.WithLogger(ctx, log.G(ctx).WithFields(log.Fields{

 		"nid": n.ID(),

 		"net": n.Name(),

@@ -820,17 +818,38 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, force bool) error

 	// Capture the addresses that were added to the container's /etc/hosts here,

 	// before the endpoint is deleted, so that they can be removed from /etc/hosts.

 	etcHostsAddrs := ep.getEtcHostsAddrs()

-

-	if err := sb.clearNetworkResources(ep); err != nil {

-		log.G(ctx).WithError(err).Warn("Failed to clean up network resources on container disconnect")

+	// Before removing the Endpoint from the Sandbox's list of endpoints, check whether

+	// it's acting as a gateway so that new gateways can be selected if it is.

+	wasGwEp := sb.isGatewayEndpoint(ep.id)

+

+	// Remove the sb's references to ep.

+	sb.mu.Lock()

+	osSbox := sb.osSbox

+	delete(sb.populatedEndpoints, ep.id)

+	delete(sb.epPriority, ep.id)

+	sb.endpoints = slices.DeleteFunc(sb.endpoints, func(other *Endpoint) bool { return other.id == ep.id })

+	sb.mu.Unlock()

+

+	// Delete interfaces, routes etc. from the OS.

+	if osSbox != nil {

+		releaseOSSboxResources(osSbox, ep)

+

+		// Even if the interface was initially created in the container's namespace, it's

+		// now been moved out. When a legacy link is deleted, the Endpoint is removed and

+		// then re-added to the Sandbox. So, to make sure the re-add works, note that the

+		// interface is now outside the container's netns.

+		ep.iface.createdInContainer = false

+	}

+

+	// Update gateway / static routes if the ep was the gateway.

+	var gwepAfter4, gwepAfter6 *Endpoint

+	if wasGwEp {

+		gwepAfter4, gwepAfter6 = sb.getGatewayEndpoint()

+		if err := sb.updateGateway(gwepAfter4, gwepAfter6); err != nil {

+			return fmt.Errorf("updating gateway endpoint: %w", err)

+		}

 	}

-	// Even if the interface was initially created in the container's namespace, it's

-	// now been moved out. When a legacy link is deleted, the Endpoint is removed and

-	// then re-added to the Sandbox. So, to make sure the re-add works, note that the

-	// interface is now outside the container's netns.

-	ep.iface.createdInContainer = false

-

 	// Update the store about the sandbox detach only after we

 	// have completed sb.clearNetworkResources above to avoid

 	// spurious logs when cleaning up the sandbox when the daemon

@@ -869,16 +888,17 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, force bool) error

 		sb.resolver.SetForwardingPolicy(sb.hasExternalAccess())

 	}

-	// Find new endpoint(s) to provide external connectivity for the sandbox.

-	gwepAfter4, gwepAfter6 := sb.getGatewayEndpoint()

-	if gwepAfter4 != nil {

-		if err := gwepAfter4.programExternalConnectivity(ctx, gwepAfter4, gwepAfter6); err != nil {

-			log.G(ctx).WithError(err).Error("Failed to set IPv4 gateway")

+	// Configure the endpoints that now provide external connectivity for the sandbox.

+	if wasGwEp {

+		if gwepAfter4 != nil {

+			if err := gwepAfter4.programExternalConnectivity(ctx, gwepAfter4, gwepAfter6); err != nil {

+				log.G(ctx).WithError(err).Error("Failed to set IPv4 gateway")

+			}

 		}

-	}

-	if gwepAfter6 != nil && gwepAfter6 != gwepAfter4 {

-		if err := gwepAfter6.programExternalConnectivity(ctx, gwepAfter4, gwepAfter6); err != nil {

-			log.G(ctx).WithError(err).Error("Failed to set IPv6 gateway")

+		if gwepAfter6 != nil && gwepAfter6 != gwepAfter4 {

+			if err := gwepAfter6.programExternalConnectivity(ctx, gwepAfter4, gwepAfter6); err != nil {

+				log.G(ctx).WithError(err).Error("Failed to set IPv6 gateway")

+			}

 		}

 	}

@@ -914,15 +934,20 @@ func (ep *Endpoint) Delete(ctx context.Context, force bool) error {

 	sbid := ep.sandboxID

 	ep.mu.Unlock()

-	sb, _ := n.getController().SandboxByID(sbid)

-	if sb != nil && !force {

-		return &ActiveContainerError{name: name, id: epid}

-	}

-

-	if sb != nil {

-		if e := ep.sbLeave(context.WithoutCancel(ctx), sb, force); e != nil {

-			log.G(ctx).Warnf("failed to leave sandbox for endpoint %s : %v", name, e)

+	if sb, _ := n.getController().SandboxByID(sbid); sb != nil {

+		if !force {

+			return &ActiveContainerError{name: name, id: epid}

 		}

+		func() {

+			// Make sure this Delete isn't racing a Join/Leave/Delete that might also be

+			// updating the Sandbox's selection of gateway endpoints.

+			sb.joinLeaveMu.Lock()

+			defer sb.joinLeaveMu.Unlock()

+

+			if e := ep.sbLeave(context.WithoutCancel(ctx), sb, n, force); e != nil {

+				log.G(ctx).Warnf("failed to leave sandbox for endpoint %s : %v", name, e)

+			}

+		}()

 	}

 	if err = n.getController().deleteStoredEndpoint(ep); err != nil {

@@ -36,27 +36,34 @@ func (sb *Sandbox) processOptions(options ...SandboxOption) {

 // Sandbox provides the control over the network container entity.

 // It is a one to one mapping with the container.

 type Sandbox struct {

-	id                 string

-	containerID        string

-	config             containerConfig

-	extDNS             []extDNSEntry

-	osSbox             *osl.Namespace

-	controller         *Controller

-	resolver           *Resolver

-	resolverOnce       sync.Once

+	id              string

+	containerID     string

+	config          containerConfig

+	extDNS          []extDNSEntry

+	osSbox          *osl.Namespace

+	controller      *Controller

+	resolver        *Resolver

+	resolverOnce    sync.Once

+	dbIndex         uint64

+	dbExists        bool

+	isStub          bool

+	inDelete        bool

+	ingress         bool

+	ndotsSet        bool

+	oslTypes        []osl.SandboxType // slice of properties of this sandbox

+	loadBalancerNID string            // NID that this SB is a load balancer for

+	mu              sync.Mutex

+

+	// joinLeaveMu is required as well as mu to modify the following fields,

+	// acquire joinLeaveMu first, and keep it at-least until gateway changes

+	// have been applied following updates to endpoints.

+	//

+	// mu is required to access these fields.

+	joinLeaveMu        sync.Mutex

 	endpoints          []*Endpoint

 	epPriority         map[string]int

 	populatedEndpoints map[string]struct{}

-	joinLeaveMu        sync.Mutex

-	dbIndex            uint64

-	dbExists           bool

-	isStub             bool

-	inDelete           bool

-	ingress            bool

-	ndotsSet           bool

-	oslTypes           []osl.SandboxType // slice of properties of this sandbox

-	loadBalancerNID    string            // NID that this SB is a load balancer for

-	mu                 sync.Mutex

+

 	// This mutex is used to serialize service related operation for an endpoint

 	// The lock is here because the endpoint is saved into the store so is not unique

 	service sync.Mutex

@@ -312,30 +319,10 @@ func (sb *Sandbox) addEndpoint(ep *Endpoint) {

 	sb.mu.Lock()

 	defer sb.mu.Unlock()

-	l := len(sb.endpoints)

-	i := sort.Search(l, func(j int) bool {

+	i := sort.Search(len(sb.endpoints), func(j int) bool {

 		return ep.Less(sb.endpoints[j])

 	})

-

-	sb.endpoints = append(sb.endpoints, nil)

-	copy(sb.endpoints[i+1:], sb.endpoints[i:])

-	sb.endpoints[i] = ep

-}

-

-func (sb *Sandbox) removeEndpoint(ep *Endpoint) {

-	sb.mu.Lock()

-	defer sb.mu.Unlock()

-

-	sb.removeEndpointRaw(ep)

-}

-

-func (sb *Sandbox) removeEndpointRaw(ep *Endpoint) {

-	for i, e := range sb.endpoints {

-		if e == ep {

-			sb.endpoints = append(sb.endpoints[:i], sb.endpoints[i+1:]...)

-			return

-		}

-	}

+	sb.endpoints = slices.Insert(sb.endpoints, i, ep)

 }

 func (sb *Sandbox) GetEndpoint(id string) *Endpoint {

@@ -567,62 +554,6 @@ func (sb *Sandbox) DisableService() error {

 	return nil

 }

-func (sb *Sandbox) clearNetworkResources(origEp *Endpoint) error {

-	ep := sb.GetEndpoint(origEp.id)

-	if ep == nil {

-		return fmt.Errorf("could not find the sandbox endpoint data for endpoint %s",

-			origEp.id)

-	}

-

-	sb.mu.Lock()

-	osSbox := sb.osSbox

-	inDelete := sb.inDelete

-	sb.mu.Unlock()

-	if osSbox != nil {

-		releaseOSSboxResources(osSbox, ep)

-	}

-

-	sb.mu.Lock()

-	delete(sb.populatedEndpoints, ep.ID())

-

-	if len(sb.endpoints) == 0 {

-		// sb.endpoints should never be empty and this is unexpected error condition

-		// We log an error message to note this down for debugging purposes.

-		log.G(context.TODO()).Errorf("No endpoints in sandbox while trying to remove endpoint %s", ep.Name())

-		sb.mu.Unlock()

-		return nil

-	}

-

-	if !slices.Contains(sb.endpoints, ep) {

-		log.G(context.TODO()).Warnf("Endpoint %s has already been deleted", ep.Name())

-		sb.mu.Unlock()

-		return nil

-	}

-

-	gwepBefore4, gwepBefore6 := selectGatewayEndpoint(sb.endpoints)

-	sb.removeEndpointRaw(ep)

-	gwepAfter4, gwepAfter6 := selectGatewayEndpoint(sb.endpoints)

-	delete(sb.epPriority, ep.ID())

-

-	sb.mu.Unlock()

-

-	if (gwepAfter4 != nil && gwepBefore4 != gwepAfter4) || (gwepAfter6 != nil && gwepBefore6 != gwepAfter6) {

-		if err := sb.updateGateway(gwepAfter4, gwepAfter6); err != nil {

-			return fmt.Errorf("updating gateway endpoint: %w", err)

-		}

-	}

-

-	// Only update the store if we did not come here as part of

-	// sandbox delete. If we came here as part of delete then do

-	// not bother updating the store. The sandbox object will be

-	// deleted anyway

-	if !inDelete {

-		return sb.storeUpdate(context.TODO())

-	}

-

-	return nil

-}

-

 // Less defines an ordering over endpoints, with better candidates for the default

 // gateway sorted first.

 //

@@ -1075,7 +1075,8 @@ func TestBridgeIPAMStatus(t *testing.T) {

 	c := d.NewClientT(t, client.WithVersion("1.52"))

 	checkSubnets := func(

-		netName string, want networktypes.SubnetStatuses) bool {

+		netName string, want networktypes.SubnetStatuses,

+	) bool {

 		t.Helper()

 		nw, err := c.NetworkInspect(ctx, netName, client.NetworkInspectOptions{})

 		if assert.Check(t, err) && assert.Check(t, nw.Status != nil) {

@@ -1114,7 +1115,8 @@ func TestBridgeIPAMStatus(t *testing.T) {

 				AuxAddress: map[string]string{

 					"reserved":   auxIPv4FromRange,

 					"reserved_1": auxIPv4OutOfRange,

-				}}),

+				},

+			}),

 			network.WithIPv6(),

 			network.WithIPAMConfig(networktypes.IPAMConfig{

 				Subnet:  cidrv6.String(),

@@ -1216,3 +1218,76 @@ func TestBridgeIPAMStatus(t *testing.T) {

 		})

 	})

 }

+

+// TestJoinError checks that if network connection fails late in the process, it's

+// rolled back properly - the failed connection should not show up in container

+// or network inspect, and the container should not gain a network interface.

+func TestJoinError(t *testing.T) {

+	ctx := setupTest(t)

+	d := daemon.New(t)

+	d.StartWithBusybox(ctx, t)

+	defer d.Stop(t)

+	c := d.NewClientT(t)

+

+	const intNet = "intnet"

+	const gwAddr = "192.168.123.1"

+	network.CreateNoError(ctx, t, c, intNet,

+		network.WithInternal(),

+		network.WithIPAM("192.168.123.0/24", gwAddr),

+	)

+	defer network.RemoveNoError(ctx, t, c, intNet)

+

+	const extNet = "extnet"

+	network.CreateNoError(ctx, t, c, extNet)

+	defer network.RemoveNoError(ctx, t, c, extNet)

+

+	cid := ctr.Run(ctx, t, c,

+		ctr.WithNetworkMode(intNet),

+		ctr.WithPrivileged(true),

+	)

+	defer c.ContainerRemove(ctx, cid, client.ContainerRemoveOptions{Force: true})

+

+	// Add a default route to the container, so that connecting extNet will fail to

+	// set up its own default route.

+	res := ctr.ExecT(ctx, t, c, cid, []string{"ip", "route", "add", "default", "via", gwAddr})

+	assert.Equal(t, res.ExitCode, 0)

+

+	// Expect an error when connecting extNet.

+	err := c.NetworkConnect(ctx, extNet, cid, &networktypes.EndpointSettings{})

+	assert.Check(t, is.ErrorContains(err, "failed to set gateway: file exists"))

+

+	// Only intNet 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, intNet))

+

+	// extNet should not report any attached containers

+	extNetInsp, err := c.NetworkInspect(ctx, extNet, client.NetworkInspectOptions{})

+	assert.Check(t, err)

+	assert.Check(t, is.Len(extNetInsp.Containers, 0))

+

+	// The container should have an eth0, but no eth1.

+	res = ctr.ExecT(ctx, t, c, cid, []string{"ip", "link", "show", "eth0"})

+	assert.Check(t, is.Equal(res.ExitCode, 0), "container should have an eth0")

+	res = ctr.ExecT(ctx, t, c, cid, []string{"ip", "link", "show", "eth1"})

+	assert.Check(t, is.Contains(res.Stderr(), "can't find device"), "container should not have an eth1")

+

+	// Remove the dodgy route.

+	res = ctr.ExecT(ctx, t, c, cid, []string{"ip", "route", "del", "default", "via", gwAddr})

+	assert.Equal(t, res.ExitCode, 0)

+

+	// Check network connect now succeeds.

+	err = c.NetworkConnect(ctx, extNet, cid, &networktypes.EndpointSettings{})

+	assert.Check(t, err)

+	ctrInsp = ctr.Inspect(ctx, t, c, cid)

+	assert.Check(t, is.Len(ctrInsp.NetworkSettings.Networks, 2))

+	assert.Check(t, is.Contains(ctrInsp.NetworkSettings.Networks, intNet))

+	assert.Check(t, is.Contains(ctrInsp.NetworkSettings.Networks, extNet))

+	extNetInsp, err = c.NetworkInspect(ctx, extNet, client.NetworkInspectOptions{})

+	assert.Check(t, err)

+	assert.Check(t, is.Len(extNetInsp.Containers, 1))

+	res = ctr.ExecT(ctx, t, c, cid, []string{"ip", "link", "show", "eth0"})

+	assert.Check(t, is.Equal(res.ExitCode, 0), "container should have an eth0")

+	res = ctr.ExecT(ctx, t, c, cid, []string{"ip", "link", "show", "eth1"})

+	assert.Check(t, is.Equal(res.ExitCode, 0), "container should have an eth1")

+}