Merge pull request #818 from aboch/ir

Change isolation rules for multiple bridge networks

libnetwork/drivers/bridge/bridge.go

@@ -106,12 +106,13 @@ type bridgeNetwork struct {
 }
 
 type driver struct {
-	config      *configuration
-	network     *bridgeNetwork
-	natChain    *iptables.ChainInfo
-	filterChain *iptables.ChainInfo
-	networks    map[string]*bridgeNetwork
-	store       datastore.DataStore
+	config         *configuration
+	network        *bridgeNetwork
+	natChain       *iptables.ChainInfo
+	filterChain    *iptables.ChainInfo
+	isolationChain *iptables.ChainInfo
+	networks       map[string]*bridgeNetwork
+	store          datastore.DataStore
 	sync.Mutex
 }
 
@@ -244,15 +245,15 @@ func (n *bridgeNetwork) registerIptCleanFunc(clean iptableCleanFunc) {
 	n.iptCleanFuncs = append(n.iptCleanFuncs, clean)
 }
 
-func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, error) {
+func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
 	n.Lock()
 	defer n.Unlock()
 
 	if n.driver == nil {
-		return nil, nil, types.BadRequestErrorf("no driver found")
+		return nil, nil, nil, types.BadRequestErrorf("no driver found")
 	}
 
-	return n.driver.natChain, n.driver.filterChain, nil
+	return n.driver.natChain, n.driver.filterChain, n.driver.isolationChain, nil
 }
 
 func (n *bridgeNetwork) getNetworkBridgeName() string {
@@ -282,26 +283,16 @@ func (n *bridgeNetwork) getEndpoint(eid string) (*bridgeEndpoint, error) {
 // from each of the other networks
 func (n *bridgeNetwork) isolateNetwork(others []*bridgeNetwork, enable bool) error {
 	n.Lock()
-	thisV4 := n.bridge.bridgeIPv4
-	thisV6 := getV6Network(n.config, n.bridge)
+	thisIface := n.config.BridgeName
 	n.Unlock()
 
 	// Install the rules to isolate this networks against each of the other networks
 	for _, o := range others {
 		o.Lock()
-		otherV4 := o.bridge.bridgeIPv4
-		otherV6 := getV6Network(o.config, o.bridge)
+		otherIface := o.config.BridgeName
 		o.Unlock()
-
-		if !types.CompareIPNet(thisV4, otherV4) {
-			// It's ok to pass a.b.c.d/x, iptables will ignore the host subnet bits
-			if err := setINC(thisV4.String(), otherV4.String(), enable); err != nil {
-				return err
-			}
-		}
-
-		if thisV6 != nil && otherV6 != nil && !types.CompareIPNet(thisV6, otherV6) {
-			if err := setINC(thisV6.String(), otherV6.String(), enable); err != nil {
+		if thisIface != otherIface {
+			if err := setINC(thisIface, otherIface, enable); err != nil {
 				return err
 			}
 		}
@@ -347,9 +338,11 @@ func (c *networkConfiguration) conflictsWithNetworks(id string, others []*bridge
 
 func (d *driver) configure(option map[string]interface{}) error {
 	var (
-		config                *configuration
-		err                   error
-		natChain, filterChain *iptables.ChainInfo
+		config         *configuration
+		err            error
+		natChain       *iptables.ChainInfo
+		filterChain    *iptables.ChainInfo
+		isolationChain *iptables.ChainInfo
 	)
 
 	genericData, ok := option[netlabel.GenericData]
@@ -378,7 +371,7 @@ func (d *driver) configure(option map[string]interface{}) error {
 	}
 
 	if config.EnableIPTables {
-		natChain, filterChain, err = setupIPChains(config)
+		natChain, filterChain, isolationChain, err = setupIPChains(config)
 		if err != nil {
 			return err
 		}
@@ -387,6 +380,7 @@ func (d *driver) configure(option map[string]interface{}) error {
 	d.Lock()
 	d.natChain = natChain
 	d.filterChain = filterChain
+	d.isolationChain = isolationChain
 	d.config = config
 	d.Unlock()
 

libnetwork/drivers/bridge/bridge_test.go

@@ -275,7 +275,7 @@ func TestCreateMultipleNetworks(t *testing.T) {
 	}
 
 	// Verify the network isolation rules are installed, each network subnet should appear 4 times
-	verifyV4INCEntries(d.networks, 4, t)
+	verifyV4INCEntries(d.networks, 6, t)
 
 	config4 := &networkConfiguration{BridgeName: "net_test_4"}
 	genericOption[netlabel.GenericData] = config4
@@ -284,10 +284,10 @@ func TestCreateMultipleNetworks(t *testing.T) {
 	}
 
 	// Now 6 times
-	verifyV4INCEntries(d.networks, 6, t)
+	verifyV4INCEntries(d.networks, 12, t)
 
 	d.DeleteNetwork("1")
-	verifyV4INCEntries(d.networks, 4, t)
+	verifyV4INCEntries(d.networks, 6, t)
 
 	d.DeleteNetwork("2")
 	verifyV4INCEntries(d.networks, 2, t)
@@ -300,19 +300,29 @@ func TestCreateMultipleNetworks(t *testing.T) {
 }
 
 func verifyV4INCEntries(networks map[string]*bridgeNetwork, numEntries int, t *testing.T) {
-	out, err := iptables.Raw("-L", "FORWARD")
+	out, err := iptables.Raw("-nvL", IsolationChain)
 	if err != nil {
 		t.Fatal(err)
 	}
-	for _, nw := range networks {
-		nt := types.GetIPNetCopy(nw.bridge.bridgeIPv4)
-		nt.IP = nt.IP.Mask(nt.Mask)
-		re := regexp.MustCompile(nt.String())
-		matches := re.FindAllString(string(out[:]), -1)
-		if len(matches) != numEntries {
-			t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables:\n%s", string(out[:]))
+
+	found := 0
+	for _, x := range networks {
+		for _, y := range networks {
+			if x == y {
+				continue
+			}
+			re := regexp.MustCompile(fmt.Sprintf("%s %s", x.config.BridgeName, y.config.BridgeName))
+			matches := re.FindAllString(string(out[:]), -1)
+			if len(matches) != 1 {
+				t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables:\n%s", string(out[:]))
+			}
+			found++
 		}
 	}
+
+	if found != numEntries {
+		t.Fatalf("Cannot find expected number (%d) of inter-network isolation rules in IP Tables:\n%s\nFound %d", numEntries, string(out[:]), found)
+	}
 }
 
 type testInterface struct {

libnetwork/drivers/bridge/setup_ip_tables.go

@@ -11,35 +11,52 @@ import (
 
 // DockerChain: DOCKER iptable chain name
 const (
-	DockerChain = "DOCKER"
+	DockerChain    = "DOCKER"
+	IsolationChain = "DOCKER-ISOLATION"
 )
 
-func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, error) {
+func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
 	// Sanity check.
 	if config.EnableIPTables == false {
-		return nil, nil, fmt.Errorf("Cannot create new chains, EnableIPTable is disabled")
+		return nil, nil, nil, fmt.Errorf("cannot create new chains, EnableIPTable is disabled")
 	}
 
 	hairpinMode := !config.EnableUserlandProxy
 
 	natChain, err := iptables.NewChain(DockerChain, iptables.Nat, hairpinMode)
 	if err != nil {
-		return nil, nil, fmt.Errorf("Failed to create NAT chain: %s", err.Error())
+		return nil, nil, nil, fmt.Errorf("failed to create NAT chain: %v", err)
 	}
 	defer func() {
 		if err != nil {
 			if err := iptables.RemoveExistingChain(DockerChain, iptables.Nat); err != nil {
-				logrus.Warnf("Failed on removing iptables NAT chain on cleanup: %v", err)
+				logrus.Warnf("failed on removing iptables NAT chain on cleanup: %v", err)
 			}
 		}
 	}()
 
-	filterChain, err := iptables.NewChain(DockerChain, iptables.Filter, hairpinMode)
+	filterChain, err := iptables.NewChain(DockerChain, iptables.Filter, false)
 	if err != nil {
-		return nil, nil, fmt.Errorf("Failed to create FILTER chain: %s", err.Error())
+		return nil, nil, nil, fmt.Errorf("failed to create FILTER chain: %v", err)
+	}
+	defer func() {
+		if err != nil {
+			if err := iptables.RemoveExistingChain(DockerChain, iptables.Filter); err != nil {
+				logrus.Warnf("failed on removing iptables FILTER chain on cleanup: %v", err)
+			}
+		}
+	}()
+
+	isolationChain, err := iptables.NewChain(IsolationChain, iptables.Filter, false)
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
 	}
 
-	return natChain, filterChain, nil
+	if err := addReturnRule(IsolationChain); err != nil {
+		return nil, nil, nil, err
+	}
+
+	return natChain, filterChain, isolationChain, nil
 }
 
 func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInterface) error {
@@ -72,7 +89,7 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt
 		return setupIPTablesInternal(config.BridgeName, maskedAddrv4, config.EnableICC, config.EnableIPMasquerade, hairpinMode, false)
 	})
 
-	natChain, filterChain, err := n.getDriverChains()
+	natChain, filterChain, _, err := n.getDriverChains()
 	if err != nil {
 		return fmt.Errorf("Failed to setup IP tables, cannot acquire chain info %s", err.Error())
 	}
@@ -86,6 +103,11 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt
 	if err != nil {
 		return fmt.Errorf("Failed to program FILTER chain: %s", err.Error())
 	}
+
+	if err := ensureJumpRule("FORWARD", IsolationChain); err != nil {
+		return err
+	}
+
 	n.registerIptCleanFunc(func() error {
 		return iptables.ProgramChain(filterChain, config.BridgeName, hairpinMode, false)
 	})
@@ -166,10 +188,8 @@ func programChainRule(rule iptRule, ruleDescr string, insert bool) error {
 	}
 
 	if condition {
-		if output, err := iptables.Raw(append(prefix, rule.args...)...); err != nil {
+		if err := iptables.RawCombinedOutput(append(prefix, rule.args...)...); err != nil {
 			return fmt.Errorf("Unable to %s %s rule: %s", operation, ruleDescr, err.Error())
-		} else if len(output) != 0 {
-			return &iptables.ChainError{Chain: rule.chain, Output: output}
 		}
 	}
 
@@ -190,20 +210,16 @@ func setIcc(bridgeIface string, iccEnable, insert bool) error {
 			iptables.Raw(append([]string{"-D", chain}, acceptArgs...)...)
 
 			if !iptables.Exists(table, chain, dropArgs...) {
-				if output, err := iptables.Raw(append([]string{"-A", chain}, dropArgs...)...); err != nil {
+				if err := iptables.RawCombinedOutput(append([]string{"-A", chain}, dropArgs...)...); err != nil {
 					return fmt.Errorf("Unable to prevent intercontainer communication: %s", err.Error())
-				} else if len(output) != 0 {
-					return fmt.Errorf("Error disabling intercontainer communication: %s", output)
 				}
 			}
 		} else {
 			iptables.Raw(append([]string{"-D", chain}, dropArgs...)...)
 
 			if !iptables.Exists(table, chain, acceptArgs...) {
-				if output, err := iptables.Raw(append([]string{"-I", chain}, acceptArgs...)...); err != nil {
+				if err := iptables.RawCombinedOutput(append([]string{"-I", chain}, acceptArgs...)...); err != nil {
 					return fmt.Errorf("Unable to allow intercontainer communication: %s", err.Error())
-				} else if len(output) != 0 {
-					return fmt.Errorf("Error enabling intercontainer communication: %s", output)
 				}
 			}
 		}
@@ -224,11 +240,11 @@ func setIcc(bridgeIface string, iccEnable, insert bool) error {
 }
 
 // Control Inter Network Communication. Install/remove only if it is not/is present.
-func setINC(network1, network2 string, enable bool) error {
+func setINC(iface1, iface2 string, enable bool) error {
 	var (
 		table = iptables.Filter
-		chain = "FORWARD"
-		args  = [2][]string{{"-s", network1, "-d", network2, "-j", "DROP"}, {"-s", network2, "-d", network1, "-j", "DROP"}}
+		chain = IsolationChain
+		args  = [2][]string{{"-i", iface1, "-o", iface2, "-j", "DROP"}, {"-i", iface2, "-o", iface1, "-j", "DROP"}}
 	)
 
 	if enable {
@@ -236,10 +252,8 @@ func setINC(network1, network2 string, enable bool) error {
 			if iptables.Exists(table, chain, args[i]...) {
 				continue
 			}
-			if output, err := iptables.Raw(append([]string{"-I", chain}, args[i]...)...); err != nil {
-				return fmt.Errorf("unable to add inter-network communication rule: %s", err.Error())
-			} else if len(output) != 0 {
-				return fmt.Errorf("error adding inter-network communication rule: %s", string(output))
+			if err := iptables.RawCombinedOutput(append([]string{"-I", chain}, args[i]...)...); err != nil {
+				return fmt.Errorf("unable to add inter-network communication rule: %v", err)
 			}
 		}
 	} else {
@@ -247,13 +261,51 @@ func setINC(network1, network2 string, enable bool) error {
 			if !iptables.Exists(table, chain, args[i]...) {
 				continue
 			}
-			if output, err := iptables.Raw(append([]string{"-D", chain}, args[i]...)...); err != nil {
-				return fmt.Errorf("unable to remove inter-network communication rule: %s", err.Error())
-			} else if len(output) != 0 {
-				return fmt.Errorf("error removing inter-network communication rule: %s", string(output))
+			if err := iptables.RawCombinedOutput(append([]string{"-D", chain}, args[i]...)...); err != nil {
+				return fmt.Errorf("unable to remove inter-network communication rule: %v", err)
 			}
 		}
 	}
 
 	return nil
 }
+
+func addReturnRule(chain string) error {
+	var (
+		table = iptables.Filter
+		args  = []string{"-j", "RETURN"}
+	)
+
+	if iptables.Exists(table, chain, args...) {
+		return nil
+	}
+
+	err := iptables.RawCombinedOutput(append([]string{"-I", chain}, args...)...)
+	if err != nil {
+		return fmt.Errorf("unable to add return rule in %s chain: %s", chain, err.Error())
+	}
+
+	return nil
+}
+
+// Ensure the jump rule is on top
+func ensureJumpRule(fromChain, toChain string) error {
+	var (
+		table = iptables.Filter
+		args  = []string{"-j", toChain}
+	)
+
+	if iptables.Exists(table, fromChain, args...) {
+		err := iptables.RawCombinedOutput(append([]string{"-D", fromChain}, args...)...)
+		if err != nil {
+			return fmt.Errorf("unable to remove jump to %s rule in %s chain: %s", toChain, fromChain, err.Error())
+		}
+	}
+
+	err := iptables.RawCombinedOutput(append([]string{"-I", fromChain}, args...)...)
+	if err != nil {
+		return fmt.Errorf("unable to insert jump to %s rule in %s chain: %s", toChain, fromChain, err.Error())
+	}
+
+	return nil
+}

libnetwork/drivers/bridge/setup_ip_tables_test.go

@@ -104,7 +104,7 @@ func assertIPTableChainProgramming(rule iptRule, descr string, t *testing.T) {
 func assertChainConfig(d *driver, t *testing.T) {
 	var err error
 
-	d.natChain, d.filterChain, err = setupIPChains(d.config)
+	d.natChain, d.filterChain, d.isolationChain, err = setupIPChains(d.config)
 	if err != nil {
 		t.Fatal(err)
 	}

libnetwork/drivers/overlay/filter.go

@@ -12,16 +12,6 @@ const globalChain = "DOCKER-OVERLAY"
 
 var filterOnce sync.Once
 
-func rawIPTables(args ...string) error {
-	if output, err := iptables.Raw(args...); err != nil {
-		return fmt.Errorf("unable to add overlay filter: %v", err)
-	} else if len(output) != 0 {
-		return fmt.Errorf("unable to add overlay filter: %s", string(output))
-	}
-
-	return nil
-}
-
 func chainExists(cname string) bool {
 	if _, err := iptables.Raw("-L", cname); err != nil {
 		return false
@@ -31,12 +21,14 @@ func chainExists(cname string) bool {
 }
 
 func setupGlobalChain() {
-	if err := rawIPTables("-N", globalChain); err != nil {
-		logrus.Debugf("could not create global overlay chain: %v", err)
+	if err := iptables.RawCombinedOutput("-N", globalChain); err != nil {
+		logrus.Errorf("could not create global overlay chain: %v", err)
+		return
 	}
 
-	if err := rawIPTables("-A", globalChain, "-j", "RETURN"); err != nil {
-		logrus.Debugf("could not install default return chain in the overlay global chain: %v", err)
+	if err := iptables.RawCombinedOutput("-A", globalChain, "-j", "RETURN"); err != nil {
+		logrus.Errorf("could not install default return chain in the overlay global chain: %v", err)
+		return
 	}
 }
 
@@ -49,21 +41,21 @@ func setNetworkChain(cname string, remove bool) error {
 	opt := "-N"
 	// In case of remove, make sure to flush the rules in the chain
 	if remove && exists {
-		if err := rawIPTables("-F", cname); err != nil {
+		if err := iptables.RawCombinedOutput("-F", cname); err != nil {
 			return fmt.Errorf("failed to flush overlay network chain %s rules: %v", cname, err)
 		}
 		opt = "-X"
 	}
 
 	if (!remove && !exists) || (remove && exists) {
-		if err := rawIPTables(opt, cname); err != nil {
+		if err := iptables.RawCombinedOutput(opt, cname); err != nil {
 			return fmt.Errorf("failed network chain operation %q for chain %s: %v", opt, cname, err)
 		}
 	}
 
 	if !remove {
 		if !iptables.Exists(iptables.Filter, cname, "-j", "DROP") {
-			if err := rawIPTables("-A", cname, "-j", "DROP"); err != nil {
+			if err := iptables.RawCombinedOutput("-A", cname, "-j", "DROP"); err != nil {
 				return fmt.Errorf("failed adding default drop rule to overlay network chain %s: %v", cname, err)
 			}
 		}
@@ -91,12 +83,12 @@ func setFilters(cname, brName string, remove bool) error {
 		for _, chain := range []string{"OUTPUT", "FORWARD"} {
 			exists := iptables.Exists(iptables.Filter, chain, "-j", globalChain)
 			if exists {
-				if err := rawIPTables("-D", chain, "-j", globalChain); err != nil {
+				if err := iptables.RawCombinedOutput("-D", chain, "-j", globalChain); err != nil {
 					return fmt.Errorf("failed to delete overlay hook in chain %s while moving the hook: %v", chain, err)
 				}
 			}
 
-			if err := rawIPTables("-I", chain, "-j", globalChain); err != nil {
+			if err := iptables.RawCombinedOutput("-I", chain, "-j", globalChain); err != nil {
 				return fmt.Errorf("failed to insert overlay hook in chain %s: %v", chain, err)
 			}
 		}
@@ -105,7 +97,7 @@ func setFilters(cname, brName string, remove bool) error {
 	// Insert/Delete the rule to jump to per-bridge chain
 	exists := iptables.Exists(iptables.Filter, globalChain, "-o", brName, "-j", cname)
 	if (!remove && !exists) || (remove && exists) {
-		if err := rawIPTables(opt, globalChain, "-o", brName, "-j", cname); err != nil {
+		if err := iptables.RawCombinedOutput(opt, globalChain, "-o", brName, "-j", cname); err != nil {
 			return fmt.Errorf("failed to add per-bridge filter rule for bridge %s, network chain %s: %v", brName, cname, err)
 		}
 	}
@@ -115,7 +107,7 @@ func setFilters(cname, brName string, remove bool) error {
 		return nil
 	}
 
-	if err := rawIPTables(opt, cname, "-i", brName, "-j", "ACCEPT"); err != nil {
+	if err := iptables.RawCombinedOutput(opt, cname, "-i", brName, "-j", "ACCEPT"); err != nil {
 		return fmt.Errorf("failed to add overlay filter rile for network chain %s, bridge %s: %v", cname, brName, err)
 	}
 

libnetwork/iptables/iptables.go

@@ -312,6 +312,7 @@ func Exists(table Table, chain string, rule ...string) bool {
 	// parse "iptables -S" for the rule (this checks rules in a specific chain
 	// in a specific table)
 	ruleString := strings.Join(rule, " ")
+	ruleString = chain + " " + ruleString
 	existingRules, _ := exec.Command(iptablesPath, "-t", string(table), "-S", chain).Output()
 
 	return strings.Contains(string(existingRules), ruleString)
@@ -351,3 +352,12 @@ func Raw(args ...string) ([]byte, error) {
 
 	return output, err
 }
+
+// RawCombinedOutput inernally calls the Raw function and returns a non nil
+// error if Raw returned a non nil error or a non empty output
+func RawCombinedOutput(args ...string) error {
+	if output, err := Raw(args...); err != nil || len(output) != 0 {
+		return fmt.Errorf("%s (%v)", string(output), err)
+	}
+	return nil
+}