Merge pull request moby#50321 from robmry/simplify_gateway_programming

Simplify gateway programming

Author: robmry

libnetwork/driverapi/driverapi.go

@@ -81,13 +81,19 @@ type Driver interface {
 
 // ExtConner is an optional interface for a network driver.
 type ExtConner interface {
-	// ProgramExternalConnectivity invokes the driver method which does the necessary
-	// programming to allow the external connectivity dictated by the passed options
-	ProgramExternalConnectivity(ctx context.Context, nid, eid string, options map[string]interface{}, gw4Id, gw6Id string) error
-
-	// RevokeExternalConnectivity asks the driver to remove any external connectivity
-	// programming that was done so far
-	RevokeExternalConnectivity(nid, eid string) error
+	// ProgramExternalConnectivity tells the driver the ids of the endpoints
+	// currently acting as the container's default gateway for IPv4 and IPv6,
+	// passed as gw4Id/gw6Id. (Those endpoints may be managed by different network
+	// drivers. If there is no gateway, the id will be the empty string.)
+	//
+	// This method is called after Driver.Join, before Driver.Leave, and when eid
+	// is or was equal to gw4Id or gw6Id, and there's a change. It may also be
+	// called when the gateways have not changed.
+	//
+	// When an endpoint acting as a gateway is deleted, this function is called
+	// with that endpoint's id in eid, and empty gateway ids (even if another
+	// is present and will shortly be selected as the gateway).
+	ProgramExternalConnectivity(ctx context.Context, nid, eid string, gw4Id, gw6Id string) error
 }
 
 // GwAllocChecker is an optional interface for a network driver.

libnetwork/drivers/bridge/bridge_linux.go

@@ -1,3 +1,6 @@
+// FIXME(thaJeztah): remove once we are a module; the go:build directive prevents go from downgrading language version to go1.16:
+//go:build go1.23
+
 package bridge
 
 import (
@@ -6,6 +9,7 @@ import (
 	"net"
 	"net/netip"
 	"os"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -30,6 +34,7 @@ import (
 	"github.com/docker/docker/libnetwork/options"
 	"github.com/docker/docker/libnetwork/scope"
 	"github.com/docker/docker/libnetwork/types"
+	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
@@ -1442,6 +1447,10 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
 	if err != nil {
 		return err
 	}
+	endpoint.extConnConfig, err = parseConnectivityOptions(sbOpts)
+	if err != nil {
+		return err
+	}
 
 	iNames := jinfo.InterfaceName()
 	containerVethPrefix := defaultContainerVethPrefix
@@ -1461,6 +1470,10 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
 		}
 	}
 
+	if !network.config.EnableICC {
+		return d.link(network, endpoint, true)
+	}
+
 	return nil
 }
 
@@ -1494,7 +1507,7 @@ type portBindingMode struct {
 	ipv6 bool
 }
 
-func (d *driver) ProgramExternalConnectivity(ctx context.Context, nid, eid string, options map[string]interface{}, gw4Id, gw6Id string) error {
+func (d *driver) ProgramExternalConnectivity(ctx context.Context, nid, eid string, gw4Id, gw6Id string) (retErr error) {
 	ctx, span := otel.Tracer("").Start(ctx, spanPrefix+".ProgramExternalConnectivity", trace.WithAttributes(
 		attribute.String("nid", nid),
 		attribute.String("eid", eid),
@@ -1521,11 +1534,6 @@ func (d *driver) ProgramExternalConnectivity(ctx context.Context, nid, eid strin
 		return endpointNotFoundError(eid)
 	}
 
-	endpoint.extConnConfig, err = parseConnectivityOptions(options)
-	if err != nil {
-		return err
-	}
-
 	var pbmReq portBindingMode
 	// Act as the IPv4 gateway if explicitly selected.
 	if gw4Id == eid {
@@ -1538,30 +1546,31 @@ func (d *driver) ProgramExternalConnectivity(ctx context.Context, nid, eid strin
 		pbmReq.ipv6 = true
 	}
 
-	// Program any required port mapping and store them in the endpoint
-	if endpoint.extConnConfig != nil && endpoint.extConnConfig.PortBindings != nil {
-		endpoint.portMapping, err = network.addPortMappings(
-			ctx,
-			endpoint,
-			endpoint.extConnConfig.PortBindings,
-			network.config.DefaultBindingIP,
-			pbmReq,
-		)
-		if err != nil {
-			return err
-		}
+	// If no change is needed, return.
+	if endpoint.portBindingState == pbmReq {
+		return nil
 	}
 
+	// Remove port bindings that aren't needed due to a change in mode.
+	undoTrim, err := endpoint.trimPortBindings(ctx, network, pbmReq)
+	if err != nil {
+		return err
+	}
 	defer func() {
-		if err != nil {
-			if e := network.releasePorts(endpoint); e != nil {
-				log.G(ctx).Errorf("Failed to release ports allocated for the bridge endpoint %s on failure %v because of %v",
-					eid, err, e)
-			}
-			endpoint.portMapping = nil
+		if retErr != nil && undoTrim != nil {
+			endpoint.portMapping = append(endpoint.portMapping, undoTrim()...)
 		}
 	}()
 
+	// Set up new port bindings, and store them in the endpoint.
+	if (pbmReq.ipv4 || pbmReq.ipv6) && endpoint.extConnConfig != nil && endpoint.extConnConfig.PortBindings != nil {
+		newPMs, err := network.addPortMappings(ctx, endpoint, endpoint.extConnConfig.PortBindings, network.config.DefaultBindingIP, pbmReq)
+		if err != nil {
+			return err
+		}
+		endpoint.portMapping = append(endpoint.portMapping, newPMs...)
+	}
+
 	// Remember the new port binding state.
 	endpoint.portBindingState = pbmReq
 
@@ -1573,50 +1582,62 @@ func (d *driver) ProgramExternalConnectivity(ctx context.Context, nid, eid strin
 		return fmt.Errorf("failed to update bridge endpoint %.7s to store: %v", endpoint.id, err)
 	}
 
-	if !network.config.EnableICC {
-		return d.link(network, endpoint, true)
-	}
-
 	return nil
 }
 
-func (d *driver) RevokeExternalConnectivity(nid, eid string) error {
-	// Make sure this function isn't deleting iptables rules while handleFirewalldReloadNw
-	// is restoring those same rules.
-	d.configNetwork.Lock()
-	defer d.configNetwork.Unlock()
-
-	network, err := d.getNetwork(nid)
-	if err != nil {
-		return err
-	}
-
-	endpoint, err := network.getEndpoint(eid)
-	if err != nil {
-		return err
+// trimPortBindings compares pbmReq with the current port bindings, and removes
+// port bindings that are no longer required.
+//
+// ep.portMapping is updated when bindings are removed.
+func (ep *bridgeEndpoint) trimPortBindings(ctx context.Context, n *bridgeNetwork, pbmReq portBindingMode) (func() []portBinding, error) {
+	// If the endpoint is the gateway for IPv4 and IPv6, there's nothing to drop.
+	if pbmReq.ipv4 && pbmReq.ipv6 {
+		return nil, nil
 	}
 
-	if endpoint == nil {
-		return endpointNotFoundError(eid)
+	toDrop := make([]portBinding, 0, len(ep.portMapping))
+	toKeep := slices.DeleteFunc(ep.portMapping, func(pb portBinding) bool {
+		is4 := pb.HostIP.To4() != nil
+		if (is4 && !pbmReq.ipv4) || (!is4 && !pbmReq.ipv6) {
+			toDrop = append(toDrop, pb)
+			return true
+		}
+		return false
+	})
+	if len(toDrop) == 0 {
+		return nil, nil
 	}
 
-	err = network.releasePorts(endpoint)
-	if err != nil {
-		log.G(context.TODO()).Warn(err)
+	if err := releasePortBindings(toDrop, n.firewallerNetwork); err != nil {
+		log.G(ctx).WithFields(log.Fields{
+			"error": err,
+			"gw4":   pbmReq.ipv4,
+			"gw6":   pbmReq.ipv6,
+			"nid":   stringid.TruncateID(n.id),
+			"eid":   stringid.TruncateID(ep.id),
+		}).Error("Failed to release port bindings")
+		return nil, err
 	}
+	ep.portMapping = toKeep
 
-	endpoint.portMapping = nil
-
-	// Clean the connection tracker state of the host for the specific endpoint. This is a precautionary measure to
-	// avoid new endpoints getting the same IP address to receive unexpected packets due to bad conntrack state leading
-	// to bad NATing.
-	clearConntrackEntries(d.nlh, endpoint)
-
-	if err = d.storeUpdate(context.TODO(), endpoint); err != nil {
-		return fmt.Errorf("failed to update bridge endpoint %.7s to store: %v", endpoint.id, err)
+	undo := func() []portBinding {
+		pbReq := make([]types.PortBinding, 0, len(toDrop))
+		for _, pb := range toDrop {
+			pbReq = append(pbReq, pb.PortBinding)
+		}
+		pbs, err := n.addPortMappings(ctx, ep, pbReq, n.config.DefaultBindingIP, ep.portBindingState)
+		if err != nil {
+			log.G(ctx).WithFields(log.Fields{
+				"error": err,
+				"nid":   stringid.TruncateID(n.id),
+				"eid":   stringid.TruncateID(ep.id),
+			}).Error("Failed to restore port bindings following join failure")
+			return nil
+		}
+		return pbs
 	}
 
-	return nil
+	return undo, nil
 }
 
 // clearConntrackEntries flushes conntrack entries matching endpoint IP address
@@ -1677,8 +1698,8 @@ func (d *driver) handleFirewalldReloadNw(nid string) {
 		return
 	}
 
-	// Make sure the network isn't being deleted, and ProgramExternalConnectivity/RevokeExternalConnectivity
-	// aren't modifying iptables rules, while restoring the rules.
+	// Make sure the network isn't being deleted, and ProgramExternalConnectivity
+	// isn't modifying iptables rules, while restoring the rules.
 	d.configNetwork.Lock()
 	defer d.configNetwork.Unlock()
 

libnetwork/drivers/bridge/bridge_linux_test.go

@@ -843,7 +843,7 @@ func testQueryEndpointInfo(t *testing.T, ulPxyEnabled bool) {
 		t.Fatalf("Failed to join the endpoint: %v", err)
 	}
 
-	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep1", sbOptions, "ep1", "")
+	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep1", "ep1", "")
 	if err != nil {
 		t.Fatalf("Failed to program external connectivity: %v", err)
 	}
@@ -874,7 +874,7 @@ func testQueryEndpointInfo(t *testing.T, ulPxyEnabled bool) {
 		}
 	}
 
-	err = d.RevokeExternalConnectivity("net1", "ep1")
+	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep1", "", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -947,7 +947,7 @@ func TestLinkContainers(t *testing.T) {
 		t.Fatalf("Failed to join the endpoint: %v", err)
 	}
 
-	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep1", sbOptions, "ep1", "")
+	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep1", "ep1", "")
 	if err != nil {
 		t.Fatalf("Failed to program external connectivity: %v", err)
 	}
@@ -978,7 +978,7 @@ func TestLinkContainers(t *testing.T) {
 		t.Fatal("Failed to link ep1 and ep2")
 	}
 
-	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep2", sbOptions, "ep2", "ep2")
+	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep2", "ep2", "ep2")
 	if err != nil {
 		t.Fatalf("Failed to program external connectivity: %v", err)
 	}
@@ -997,7 +997,7 @@ func TestLinkContainers(t *testing.T) {
 	}
 	checkLink(true)
 
-	err = d.RevokeExternalConnectivity("net1", "ep2")
+	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep2", "", "")
 	if err != nil {
 		t.Fatalf("Failed to revoke external connectivity: %v", err)
 	}
@@ -1014,10 +1014,6 @@ func TestLinkContainers(t *testing.T) {
 	}
 
 	err = d.Join(context.Background(), "net1", "ep2", "", te2, nil, sbOptions)
-	if err != nil {
-		t.Fatal(err)
-	}
-	err = d.ProgramExternalConnectivity(context.Background(), "net1", "ep2", sbOptions, "ep2", "ep2")
 	assert.Check(t, err != nil, "Expected Join to fail given link conditions are not satisfied")
 	checkLink(false)
 }

libnetwork/drivers/bridge/bridge_store.go

@@ -477,4 +477,5 @@ func (n *bridgeNetwork) restorePortAllocations(ep *bridgeEndpoint) {
 	if err != nil {
 		log.G(context.TODO()).Warnf("Failed to reserve existing port mapping for endpoint %.7s:%v", ep.id, err)
 	}
+	ep.portBindingState = pbm
 }

libnetwork/drivers/bridge/port_mapping_linux_test.go

@@ -73,7 +73,7 @@ func TestPortMappingConfig(t *testing.T) {
 		t.Fatalf("Failed to join the endpoint: %v", err)
 	}
 
-	if err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", sbOptions, "ep1", ""); err != nil {
+	if err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", "ep1", ""); err != nil {
 		t.Fatalf("Failed to program external connectivity: %v", err)
 	}
 
@@ -102,7 +102,7 @@ func TestPortMappingConfig(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = d.RevokeExternalConnectivity("dummy", "ep1")
+	err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", "", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -159,7 +159,7 @@ func TestPortMappingV6Config(t *testing.T) {
 		t.Fatalf("Failed to join the endpoint: %v", err)
 	}
 
-	if err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", sbOptions, "ep1", ""); err != nil {
+	if err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", "ep1", "ep1"); err != nil {
 		t.Fatalf("Failed to program external connectivity: %v", err)
 	}
 
@@ -178,7 +178,7 @@ func TestPortMappingV6Config(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = d.RevokeExternalConnectivity("dummy", "ep1")
+	err = d.ProgramExternalConnectivity(context.Background(), "dummy", "ep1", "", "")
 	if err != nil {
 		t.Fatal(err)
 	}