Merge pull request #46493 from rhansen/bridge-cleanups

bridge driver: various code quality improvements

libnetwork/drivers/bridge/bridge_linux.go

@@ -73,7 +73,7 @@ type networkConfiguration struct {
 	Mtu                  int
 	DefaultBindingIP     net.IP
 	DefaultBridge        bool
-	HostIP               net.IP
+	HostIPv4             net.IP
 	ContainerIfacePrefix string
 	// Internal fields set after ipam data parsing
 	AddressIPv4        *net.IPNet
@@ -266,8 +266,8 @@ func (c *networkConfiguration) fromLabels(labels map[string]string) error {
 			}
 		case netlabel.ContainerIfacePrefix:
 			c.ContainerIfacePrefix = value
-		case netlabel.HostIP:
-			if c.HostIP = net.ParseIP(value); c.HostIP == nil {
+		case netlabel.HostIPv4:
+			if c.HostIPv4 = net.ParseIP(value); c.HostIPv4 == nil {
 				return parseErr(label, value, "nil ip")
 			}
 		}

libnetwork/drivers/bridge/bridge_linux_test.go

@@ -298,7 +298,7 @@ func TestCreateFullOptionsLabels(t *testing.T) {
 	}
 
 	bndIPs := "127.0.0.1"
-	testHostIP := "1.2.3.4"
+	testHostIPv4 := "1.2.3.4"
 	nwV6s := "2001:db8:2600:2700:2800::/80"
 	gwV6s := "2001:db8:2600:2700:2800::25/80"
 	nwV6, _ := types.ParseCIDR(nwV6s)
@@ -310,7 +310,7 @@ func TestCreateFullOptionsLabels(t *testing.T) {
 		EnableICC:          "true",
 		EnableIPMasquerade: "true",
 		DefaultBindingIP:   bndIPs,
-		netlabel.HostIP:    testHostIP,
+		netlabel.HostIPv4:  testHostIPv4,
 	}
 
 	netOption := make(map[string]interface{})
@@ -358,9 +358,9 @@ func TestCreateFullOptionsLabels(t *testing.T) {
 		t.Fatalf("Unexpected: %v", nw.config.DefaultBindingIP)
 	}
 
-	hostIP := net.ParseIP(testHostIP)
-	if !hostIP.Equal(nw.config.HostIP) {
-		t.Fatalf("Unexpected: %v", nw.config.HostIP)
+	hostIP := net.ParseIP(testHostIPv4)
+	if !hostIP.Equal(nw.config.HostIPv4) {
+		t.Fatalf("Unexpected: %v", nw.config.HostIPv4)
 	}
 
 	if !types.CompareIPNet(nw.config.AddressIPv6, nwV6) {

libnetwork/drivers/bridge/bridge_store.go

@@ -146,7 +146,8 @@ func (ncfg *networkConfiguration) MarshalJSON() ([]byte, error) {
 	nMap["Internal"] = ncfg.Internal
 	nMap["DefaultBridge"] = ncfg.DefaultBridge
 	nMap["DefaultBindingIP"] = ncfg.DefaultBindingIP.String()
-	nMap["HostIP"] = ncfg.HostIP.String()
+	// This key is "HostIP" instead of "HostIPv4" to preserve compatibility with the on-disk format.
+	nMap["HostIP"] = ncfg.HostIPv4.String()
 	nMap["DefaultGatewayIPv4"] = ncfg.DefaultGatewayIPv4.String()
 	nMap["DefaultGatewayIPv6"] = ncfg.DefaultGatewayIPv6.String()
 	nMap["ContainerIfacePrefix"] = ncfg.ContainerIfacePrefix
@@ -189,8 +190,9 @@ func (ncfg *networkConfiguration) UnmarshalJSON(b []byte) error {
 		ncfg.ContainerIfacePrefix = v.(string)
 	}
 
+	// This key is "HostIP" instead of "HostIPv4" to preserve compatibility with the on-disk format.
 	if v, ok := nMap["HostIP"]; ok {
-		ncfg.HostIP = net.ParseIP(v.(string))
+		ncfg.HostIPv4 = net.ParseIP(v.(string))
 	}
 
 	ncfg.DefaultBridge = nMap["DefaultBridge"].(bool)

libnetwork/drivers/bridge/setup_ip_tables_linux.go

@@ -157,15 +157,11 @@ func (n *bridgeNetwork) setupIPTables(ipVersion iptables.IPVersion, maskedAddr *
 			return setupInternalNetworkRules(config.BridgeName, maskedAddr, config.EnableICC, false)
 		})
 	} else {
-		hostIP := config.HostIP
-		if ipVersion != iptables.IPv4 {
-			hostIP = nil
-		}
-		if err = setupIPTablesInternal(hostIP, config.BridgeName, maskedAddr, config.EnableICC, config.EnableIPMasquerade, hairpinMode, true); err != nil {
+		if err = setupIPTablesInternal(ipVersion, config, maskedAddr, hairpinMode, true); err != nil {
 			return fmt.Errorf("Failed to Setup IP tables: %s", err.Error())
 		}
 		n.registerIptCleanFunc(func() error {
-			return setupIPTablesInternal(hostIP, config.BridgeName, maskedAddr, config.EnableICC, config.EnableIPMasquerade, hairpinMode, false)
+			return setupIPTablesInternal(ipVersion, config, maskedAddr, hairpinMode, false)
 		})
 		natChain, filterChain, _, _, err := n.getDriverChains(ipVersion)
 		if err != nil {
@@ -200,140 +196,138 @@ func (n *bridgeNetwork) setupIPTables(ipVersion iptables.IPVersion, maskedAddr *
 }
 
 type iptRule struct {
-	table   iptables.Table
-	chain   string
-	preArgs []string
-	args    []string
+	ipv   iptables.IPVersion
+	table iptables.Table
+	chain string
+	args  []string
+}
+
+// Exists returns true if the rule exists in the kernel.
+func (r iptRule) Exists() bool {
+	return iptables.GetIptable(r.ipv).Exists(r.table, r.chain, r.args...)
+}
+
+func (r iptRule) cmdArgs(op iptables.Action) []string {
+	return append([]string{"-t", string(r.table), string(op), r.chain}, r.args...)
+}
+
+func (r iptRule) exec(op iptables.Action) error {
+	return iptables.GetIptable(r.ipv).RawCombinedOutput(r.cmdArgs(op)...)
+}
+
+// Append appends the rule to the end of the chain. If the rule already exists anywhere in the
+// chain, this is a no-op.
+func (r iptRule) Append() error {
+	if r.Exists() {
+		return nil
+	}
+	return r.exec(iptables.Append)
 }
 
-func setupIPTablesInternal(hostIP net.IP, bridgeIface string, addr *net.IPNet, icc, ipmasq, hairpin, enable bool) error {
+// Insert inserts the rule at the head of the chain. If the rule already exists anywhere in the
+// chain, this is a no-op.
+func (r iptRule) Insert() error {
+	if r.Exists() {
+		return nil
+	}
+	return r.exec(iptables.Insert)
+}
+
+// Delete deletes the rule from the kernel. If the rule does not exist, this is a no-op.
+func (r iptRule) Delete() error {
+	if !r.Exists() {
+		return nil
+	}
+	return r.exec(iptables.Delete)
+}
+
+func setupIPTablesInternal(ipVer iptables.IPVersion, config *networkConfiguration, addr *net.IPNet, hairpin, enable bool) error {
 	var (
 		address   = addr.String()
-		skipDNAT  = iptRule{table: iptables.Nat, chain: DockerChain, preArgs: []string{"-t", "nat"}, args: []string{"-i", bridgeIface, "-j", "RETURN"}}
-		outRule   = iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-i", bridgeIface, "!", "-o", bridgeIface, "-j", "ACCEPT"}}
+		skipDNAT  = iptRule{ipv: ipVer, table: iptables.Nat, chain: DockerChain, args: []string{"-i", config.BridgeName, "-j", "RETURN"}}
+		outRule   = iptRule{ipv: ipVer, table: iptables.Filter, chain: "FORWARD", args: []string{"-i", config.BridgeName, "!", "-o", config.BridgeName, "-j", "ACCEPT"}}
 		natArgs   []string
 		hpNatArgs []string
 	)
-	// if hostIP is set use this address as the src-ip during SNAT
-	if hostIP != nil {
-		hostAddr := hostIP.String()
-		natArgs = []string{"-s", address, "!", "-o", bridgeIface, "-j", "SNAT", "--to-source", hostAddr}
-		hpNatArgs = []string{"-m", "addrtype", "--src-type", "LOCAL", "-o", bridgeIface, "-j", "SNAT", "--to-source", hostAddr}
+	// If config.HostIPv4 is set, the user wants IPv4 SNAT with the given address.
+	if config.HostIPv4 != nil && ipVer == iptables.IPv4 {
+		hostAddr := config.HostIPv4.String()
+		natArgs = []string{"-s", address, "!", "-o", config.BridgeName, "-j", "SNAT", "--to-source", hostAddr}
+		hpNatArgs = []string{"-m", "addrtype", "--src-type", "LOCAL", "-o", config.BridgeName, "-j", "SNAT", "--to-source", hostAddr}
 		// Else use MASQUERADE which picks the src-ip based on NH from the route table
 	} else {
-		natArgs = []string{"-s", address, "!", "-o", bridgeIface, "-j", "MASQUERADE"}
-		hpNatArgs = []string{"-m", "addrtype", "--src-type", "LOCAL", "-o", bridgeIface, "-j", "MASQUERADE"}
+		natArgs = []string{"-s", address, "!", "-o", config.BridgeName, "-j", "MASQUERADE"}
+		hpNatArgs = []string{"-m", "addrtype", "--src-type", "LOCAL", "-o", config.BridgeName, "-j", "MASQUERADE"}
 	}
 
-	natRule := iptRule{table: iptables.Nat, chain: "POSTROUTING", preArgs: []string{"-t", "nat"}, args: natArgs}
-	hpNatRule := iptRule{table: iptables.Nat, chain: "POSTROUTING", preArgs: []string{"-t", "nat"}, args: hpNatArgs}
-
-	ipVer := iptables.IPv4
-	if addr.IP.To4() == nil {
-		ipVer = iptables.IPv6
-	}
+	natRule := iptRule{ipv: ipVer, table: iptables.Nat, chain: "POSTROUTING", args: natArgs}
+	hpNatRule := iptRule{ipv: ipVer, table: iptables.Nat, chain: "POSTROUTING", args: hpNatArgs}
 
 	// Set NAT.
-	if ipmasq {
-		if err := programChainRule(ipVer, natRule, "NAT", enable); err != nil {
+	if config.EnableIPMasquerade {
+		if err := programChainRule(natRule, "NAT", enable); err != nil {
 			return err
 		}
 	}
 
-	if ipmasq && !hairpin {
-		if err := programChainRule(ipVer, skipDNAT, "SKIP DNAT", enable); err != nil {
+	if config.EnableIPMasquerade && !hairpin {
+		if err := programChainRule(skipDNAT, "SKIP DNAT", enable); err != nil {
 			return err
 		}
 	}
 
 	// In hairpin mode, masquerade traffic from localhost. If hairpin is disabled or if we're tearing down
 	// that bridge, make sure the iptables rule isn't lying around.
-	if err := programChainRule(ipVer, hpNatRule, "MASQ LOCAL HOST", enable && hairpin); err != nil {
+	if err := programChainRule(hpNatRule, "MASQ LOCAL HOST", enable && hairpin); err != nil {
 		return err
 	}
 
 	// Set Inter Container Communication.
-	if err := setIcc(ipVer, bridgeIface, icc, enable); err != nil {
+	if err := setIcc(ipVer, config.BridgeName, config.EnableICC, enable); err != nil {
 		return err
 	}
 
 	// Set Accept on all non-intercontainer outgoing packets.
-	return programChainRule(ipVer, outRule, "ACCEPT NON_ICC OUTGOING", enable)
+	return programChainRule(outRule, "ACCEPT NON_ICC OUTGOING", enable)
 }
 
-func programChainRule(version iptables.IPVersion, rule iptRule, ruleDescr string, insert bool) error {
-	iptable := iptables.GetIptable(version)
-
-	var (
-		prefix    []string
-		operation string
-		condition bool
-		doesExist = iptable.Exists(rule.table, rule.chain, rule.args...)
-	)
-
+func programChainRule(rule iptRule, ruleDescr string, insert bool) error {
+	operation := "disable"
+	fn := rule.Delete
 	if insert {
-		condition = !doesExist
-		prefix = []string{"-I", rule.chain}
 		operation = "enable"
-	} else {
-		condition = doesExist
-		prefix = []string{"-D", rule.chain}
-		operation = "disable"
-	}
-	if rule.preArgs != nil {
-		prefix = append(rule.preArgs, prefix...)
+		fn = rule.Insert
 	}
-
-	if condition {
-		if err := iptable.RawCombinedOutput(append(prefix, rule.args...)...); err != nil {
-			return fmt.Errorf("Unable to %s %s rule: %s", operation, ruleDescr, err.Error())
-		}
+	if err := fn(); err != nil {
+		return fmt.Errorf("Unable to %s %s rule: %s", operation, ruleDescr, err.Error())
 	}
-
 	return nil
 }
 
 func setIcc(version iptables.IPVersion, bridgeIface string, iccEnable, insert bool) error {
-	iptable := iptables.GetIptable(version)
-	var (
-		table      = iptables.Filter
-		chain      = "FORWARD"
-		args       = []string{"-i", bridgeIface, "-o", bridgeIface, "-j"}
-		acceptArgs = append(args, "ACCEPT")
-		dropArgs   = append(args, "DROP")
-	)
-
+	args := []string{"-i", bridgeIface, "-o", bridgeIface, "-j"}
+	acceptRule := iptRule{ipv: version, table: iptables.Filter, chain: "FORWARD", args: append(args, "ACCEPT")}
+	dropRule := iptRule{ipv: version, table: iptables.Filter, chain: "FORWARD", args: append(args, "DROP")}
 	if insert {
 		if !iccEnable {
-			iptable.Raw(append([]string{"-D", chain}, acceptArgs...)...)
-
-			if !iptable.Exists(table, chain, dropArgs...) {
-				if err := iptable.RawCombinedOutput(append([]string{"-A", chain}, dropArgs...)...); err != nil {
-					return fmt.Errorf("Unable to prevent intercontainer communication: %s", err.Error())
-				}
+			acceptRule.Delete()
+			if err := dropRule.Append(); err != nil {
+				return fmt.Errorf("Unable to prevent intercontainer communication: %s", err.Error())
 			}
 		} else {
-			iptable.Raw(append([]string{"-D", chain}, dropArgs...)...)
-
-			if !iptable.Exists(table, chain, acceptArgs...) {
-				if err := iptable.RawCombinedOutput(append([]string{"-I", chain}, acceptArgs...)...); err != nil {
-					return fmt.Errorf("Unable to allow intercontainer communication: %s", err.Error())
-				}
+			dropRule.Delete()
+			if err := acceptRule.Insert(); err != nil {
+				return fmt.Errorf("Unable to allow intercontainer communication: %s", err.Error())
 			}
 		}
 	} else {
 		// Remove any ICC rule.
 		if !iccEnable {
-			if iptable.Exists(table, chain, dropArgs...) {
-				iptable.Raw(append([]string{"-D", chain}, dropArgs...)...)
-			}
+			dropRule.Delete()
 		} else {
-			if iptable.Exists(table, chain, acceptArgs...) {
-				iptable.Raw(append([]string{"-D", chain}, acceptArgs...)...)
-			}
+			acceptRule.Delete()
 		}
 	}
-
 	return nil
 }
 
@@ -404,11 +398,13 @@ func setupInternalNetworkRules(bridgeIface string, addr *net.IPNet, icc, insert
 	if addr.IP.To4() != nil {
 		version = iptables.IPv4
 		inDropRule = iptRule{
+			ipv:   version,
 			table: iptables.Filter,
 			chain: IsolationChain1,
 			args:  []string{"-i", bridgeIface, "!", "-d", addr.String(), "-j", "DROP"},
 		}
 		outDropRule = iptRule{
+			ipv:   version,
 			table: iptables.Filter,
 			chain: IsolationChain1,
 			args:  []string{"-o", bridgeIface, "!", "-s", addr.String(), "-j", "DROP"},
@@ -416,21 +412,23 @@ func setupInternalNetworkRules(bridgeIface string, addr *net.IPNet, icc, insert
 	} else {
 		version = iptables.IPv6
 		inDropRule = iptRule{
+			ipv:   version,
 			table: iptables.Filter,
 			chain: IsolationChain1,
 			args:  []string{"-i", bridgeIface, "!", "-o", bridgeIface, "!", "-d", addr.String(), "-j", "DROP"},
 		}
 		outDropRule = iptRule{
+			ipv:   version,
 			table: iptables.Filter,
 			chain: IsolationChain1,
 			args:  []string{"!", "-i", bridgeIface, "-o", bridgeIface, "!", "-s", addr.String(), "-j", "DROP"},
 		}
 	}
 
-	if err := programChainRule(version, inDropRule, "DROP INCOMING", insert); err != nil {
+	if err := programChainRule(inDropRule, "DROP INCOMING", insert); err != nil {
 		return err
 	}
-	if err := programChainRule(version, outDropRule, "DROP OUTGOING", insert); err != nil {
+	if err := programChainRule(outDropRule, "DROP OUTGOING", insert); err != nil {
 		return err
 	}
 

libnetwork/drivers/bridge/setup_ip_tables_linux_test.go

@@ -31,12 +31,12 @@ func TestProgramIPTable(t *testing.T) {
 		rule  iptRule
 		descr string
 	}{
-		{iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-d", "127.1.2.3", "-i", "lo", "-o", "lo", "-j", "DROP"}}, "Test Loopback"},
-		{iptRule{table: iptables.Nat, chain: "POSTROUTING", preArgs: []string{"-t", "nat"}, args: []string{"-s", iptablesTestBridgeIP, "!", "-o", DefaultBridgeName, "-j", "MASQUERADE"}}, "NAT Test"},
-		{iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-o", DefaultBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}}, "Test ACCEPT INCOMING"},
-		{iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "!", "-o", DefaultBridgeName, "-j", "ACCEPT"}}, "Test ACCEPT NON_ICC OUTGOING"},
-		{iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "-o", DefaultBridgeName, "-j", "ACCEPT"}}, "Test enable ICC"},
-		{iptRule{table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "-o", DefaultBridgeName, "-j", "DROP"}}, "Test disable ICC"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Filter, chain: "FORWARD", args: []string{"-d", "127.1.2.3", "-i", "lo", "-o", "lo", "-j", "DROP"}}, "Test Loopback"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Nat, chain: "POSTROUTING", args: []string{"-s", iptablesTestBridgeIP, "!", "-o", DefaultBridgeName, "-j", "MASQUERADE"}}, "NAT Test"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Filter, chain: "FORWARD", args: []string{"-o", DefaultBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}}, "Test ACCEPT INCOMING"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "!", "-o", DefaultBridgeName, "-j", "ACCEPT"}}, "Test ACCEPT NON_ICC OUTGOING"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "-o", DefaultBridgeName, "-j", "ACCEPT"}}, "Test enable ICC"},
+		{iptRule{ipv: iptables.IPv4, table: iptables.Filter, chain: "FORWARD", args: []string{"-i", DefaultBridgeName, "-o", DefaultBridgeName, "-j", "DROP"}}, "Test disable ICC"},
 	}
 
 	// Assert the chain rules' insertion and removal.
@@ -103,20 +103,19 @@ func createTestBridge(config *networkConfiguration, br *bridgeInterface, t *test
 // Assert base function which pushes iptables chain rules on insertion and removal.
 func assertIPTableChainProgramming(rule iptRule, descr string, t *testing.T) {
 	// Add
-	if err := programChainRule(iptables.IPv4, rule, descr, true); err != nil {
+	if err := programChainRule(rule, descr, true); err != nil {
 		t.Fatalf("Failed to program iptable rule %s: %s", descr, err.Error())
 	}
 
-	iptable := iptables.GetIptable(iptables.IPv4)
-	if iptable.Exists(rule.table, rule.chain, rule.args...) == false {
+	if !rule.Exists() {
 		t.Fatalf("Failed to effectively program iptable rule: %s", descr)
 	}
 
 	// Remove
-	if err := programChainRule(iptables.IPv4, rule, descr, false); err != nil {
+	if err := programChainRule(rule, descr, false); err != nil {
 		t.Fatalf("Failed to remove iptable rule %s: %s", descr, err.Error())
 	}
-	if iptable.Exists(rule.table, rule.chain, rule.args...) == true {
+	if rule.Exists() {
 		t.Fatalf("Failed to effectively remove iptable rule: %s", descr)
 	}
 }
@@ -159,7 +158,7 @@ func assertBridgeConfig(config *networkConfiguration, br *bridgeInterface, d *dr
 }
 
 // Regression test for https://github.com/moby/moby/issues/46445
-func TestSetupIP6TablesWithHostIP(t *testing.T) {
+func TestSetupIP6TablesWithHostIPv4(t *testing.T) {
 	defer netnsutils.SetupTestOSContext(t)()
 	d := newDriver()
 	dc := &configuration{
@@ -175,7 +174,7 @@ func TestSetupIP6TablesWithHostIP(t *testing.T) {
 		EnableIPMasquerade: true,
 		EnableIPv6:         true,
 		AddressIPv6:        &net.IPNet{IP: net.ParseIP("2001:db8::1"), Mask: net.CIDRMask(64, 128)},
-		HostIP:             net.ParseIP("192.0.2.2"),
+		HostIPv4:           net.ParseIP("192.0.2.2"),
 	}
 	nh, err := netlink.NewHandle()
 	if err != nil {

libnetwork/netlabel/labels.go

@@ -44,8 +44,8 @@ const (
 	// ContainerIfacePrefix can be used to override the interface prefix used inside the container
 	ContainerIfacePrefix = Prefix + ".container_iface_prefix"
 
-	// HostIP is the Source-IP Address used to SNAT container traffic
-	HostIP = Prefix + ".host_ipv4"
+	// HostIPv4 is the Source-IPv4 Address used to SNAT IPv4 container traffic
+	HostIPv4 = Prefix + ".host_ipv4"
 
 	// LocalKVClient constants represents the local kv store client
 	LocalKVClient = DriverPrivatePrefix + "localkv_client"