From 53390f85ddf238dea30c899355ef5b2299156f8f Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Thu, 11 Sep 2025 10:27:34 +0100 Subject: [PATCH] Put clearNetworkResources() inline in its only caller Signed-off-by: Rob Murray --- daemon/libnetwork/default_gateway.go | 13 +++--- daemon/libnetwork/endpoint.go | 54 ++++++++++++++++------- daemon/libnetwork/sandbox.go | 64 +--------------------------- 3 files changed, 46 insertions(+), 85 deletions(-) diff --git a/daemon/libnetwork/default_gateway.go b/daemon/libnetwork/default_gateway.go index 078fd7c48e..6f49c34d3f 100644 --- a/daemon/libnetwork/default_gateway.go +++ b/daemon/libnetwork/default_gateway.go @@ -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 } diff --git a/daemon/libnetwork/endpoint.go b/daemon/libnetwork/endpoint.go index f32ecccbcd..7fc0ce21e4 100644 --- a/daemon/libnetwork/endpoint.go +++ b/daemon/libnetwork/endpoint.go @@ -820,16 +820,37 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force // 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() + // 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) - if err := sb.clearNetworkResources(ep); err != nil { - log.G(ctx).WithError(err).Warn("Failed to clean up network resources on container disconnect") + // 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 } - // 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) + } + } // Update the store about the sandbox detach only after we // have completed sb.clearNetworkResources above to avoid @@ -869,16 +890,17 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force 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") + } } } diff --git a/daemon/libnetwork/sandbox.go b/daemon/libnetwork/sandbox.go index 7d77b12f66..3c756a50bd 100644 --- a/daemon/libnetwork/sandbox.go +++ b/daemon/libnetwork/sandbox.go @@ -319,14 +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 + sb.endpoints = slices.Insert(sb.endpoints, i, ep) } func (sb *Sandbox) removeEndpoint(ep *Endpoint) { @@ -574,62 +570,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. //