Merge pull request #51592 from robmry/sbleave_gw_config_error

Suppress errors from gateway re-config when disconnecting a network

daemon/libnetwork/endpoint.go

@@ -745,12 +745,17 @@ func (ep *Endpoint) Leave(ctx context.Context, sb *Sandbox) error {
 
 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(),
+		"nid": stringid.TruncateID(n.ID()),
 		"net": n.Name(),
-		"eid": ep.ID(),
+		"eid": epShortId(ep),
 		"ep":  ep.Name(),
+		"sid": stringid.TruncateID(sb.ID()),
+		"cid": stringid.TruncateID(sb.ContainerID()),
 	}))
 
+	sb.mu.Lock()
+	sbInDelete := sb.inDelete
+	sb.mu.Unlock()
 	ep.mu.Lock()
 	sid := ep.sandboxID
 	ep.mu.Unlock()
@@ -798,8 +803,9 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force
 	// 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)
+	// it's acting as a gateway so that new gateways can be selected if it is. If the
+	// sandbox is being deleted, skip the check as no new gateway will be needed.
+	needNewGwEp := !sbInDelete && sb.isGatewayEndpoint(ep.id)
 
 	// Remove the sb's references to ep.
 	sb.mu.Lock()
@@ -822,10 +828,16 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force
 
 	// Update gateway / static routes if the ep was the gateway.
 	var gwepAfter4, gwepAfter6 *Endpoint
-	if wasGwEp {
+	if needNewGwEp {
 		gwepAfter4, gwepAfter6 = sb.getGatewayEndpoint()
 		if err := sb.updateGateway(gwepAfter4, gwepAfter6); err != nil {
-			return fmt.Errorf("updating gateway endpoint: %w", err)
+			// Don't return an error here without adding proper rollback of the work done above.
+			// See https://github.com/moby/moby/issues/51578
+			log.G(ctx).WithFields(log.Fields{
+				"gw4":   epShortId(gwepAfter4),
+				"gw6":   epShortId(gwepAfter6),
+				"error": err,
+			}).Warn("Configuring gateway after network disconnect")
 		}
 	}
 
@@ -858,7 +870,7 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force
 	// the hosts entries with addresses on that network.
 	sb.deleteHostsEntries(etcHostsAddrs)
 
-	if !sb.inDelete && sb.needDefaultGW() && sb.getEndpointInGWNetwork() == nil {
+	if !sbInDelete && sb.needDefaultGW() && sb.getEndpointInGWNetwork() == nil {
 		return sb.setupDefaultGW()
 	}
 
@@ -867,17 +879,16 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, n *Network, force
 		sb.resolver.SetForwardingPolicy(sb.hasExternalAccess())
 	}
 
-	// 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")
-			}
+	// Configure the endpoints that now provide external connectivity for the sandbox
+	// if endpoints have been selected.
+	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")
 		}
 	}
 

integration/networking/bridge_linux_test.go

@@ -2090,3 +2090,52 @@ func TestSetIPWithNoConfiguredSubnet(t *testing.T) {
 		assert.Check(t, is.Contains(res.Stdout.String(), ip6))
 	}
 }
+
+// Regression test for https://github.com/moby/moby/issues/51578
+func TestGatewayErrorOnNetDisconnect(t *testing.T) {
+	ctx := setupTest(t)
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+	c := d.NewClientT(t)
+
+	network.CreateNoError(ctx, t, c, "n1")
+	defer network.RemoveNoError(ctx, t, c, "n1")
+	network.CreateNoError(ctx, t, c, "n2")
+	defer network.RemoveNoError(ctx, t, c, "n2")
+
+	// Run a container attached to both networks, with n1 providing the default gateway
+	// and n2's interface named "eth2".
+	ctrID := container.Run(ctx, t, c,
+		container.WithEndpointSettings("n1", &networktypes.EndpointSettings{GwPriority: 1}),
+		container.WithEndpointSettings("n2", &networktypes.EndpointSettings{DriverOpts: map[string]string{
+			netlabel.Ifname: "eth2",
+		}}),
+		container.WithCapability("NET_ADMIN"),
+	)
+	defer container.Remove(ctx, t, c, ctrID, client.ContainerRemoveOptions{Force: true})
+
+	// Break n2 so it can't be used as a gateway (there will be no route).
+	execRes := container.ExecT(ctx, t, c, ctrID, []string{"ip", "link", "set", "eth2", "down"})
+	assert.Assert(t, is.Equal(execRes.ExitCode, 0))
+
+	// Disconnect n1, n2 will be selected as the gateway and its config will fail.
+	// The error is only logged and the disconnect proceeds.
+	_, err := c.NetworkDisconnect(ctx, "n1", client.NetworkDisconnectOptions{Container: ctrID})
+	assert.Check(t, err)
+
+	// Check n1 can be reconnected.
+	_, err = c.NetworkConnect(ctx, "n1", client.NetworkConnectOptions{Container: ctrID})
+	assert.Check(t, err)
+
+	// Check the container can be restarted.
+	timeout := 0
+	_, err = c.ContainerRestart(ctx, ctrID, client.ContainerRestartOptions{Timeout: &timeout})
+	assert.Check(t, err)
+
+	// Both networks should be attached.
+	ctrInsp := container.Inspect(ctx, t, c, ctrID)
+	assert.Check(t, is.Len(ctrInsp.NetworkSettings.Networks, 2))
+	assert.Check(t, is.Contains(ctrInsp.NetworkSettings.Networks, "n1"))
+	assert.Check(t, is.Contains(ctrInsp.NetworkSettings.Networks, "n2"))
+}