Improve scalabiltiy of bridge network isolation rules

- This reduces complexity from O(N^2) to O(2N)

Signed-off-by: Alessandro Boch <aboch@docker.com>
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

libnetwork/drivers/bridge/bridge.go

@@ -137,15 +137,16 @@ type bridgeNetwork struct {
 }
 
 type driver struct {
-	config         *configuration
-	network        *bridgeNetwork
-	natChain       *iptables.ChainInfo
-	filterChain    *iptables.ChainInfo
-	isolationChain *iptables.ChainInfo
-	networks       map[string]*bridgeNetwork
-	store          datastore.DataStore
-	nlh            *netlink.Handle
-	configNetwork  sync.Mutex
+	config          *configuration
+	network         *bridgeNetwork
+	natChain        *iptables.ChainInfo
+	filterChain     *iptables.ChainInfo
+	isolationChain1 *iptables.ChainInfo
+	isolationChain2 *iptables.ChainInfo
+	networks        map[string]*bridgeNetwork
+	store           datastore.DataStore
+	nlh             *netlink.Handle
+	configNetwork   sync.Mutex
 	sync.Mutex
 }
 
@@ -266,15 +267,15 @@ func (n *bridgeNetwork) registerIptCleanFunc(clean iptableCleanFunc) {
 	n.iptCleanFuncs = append(n.iptCleanFuncs, clean)
 }
 
-func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
+func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
 	n.Lock()
 	defer n.Unlock()
 
 	if n.driver == nil {
-		return nil, nil, nil, types.BadRequestErrorf("no driver found")
+		return nil, nil, nil, nil, types.BadRequestErrorf("no driver found")
 	}
 
-	return n.driver.natChain, n.driver.filterChain, n.driver.isolationChain, nil
+	return n.driver.natChain, n.driver.filterChain, n.driver.isolationChain1, n.driver.isolationChain2, nil
 }
 
 func (n *bridgeNetwork) getNetworkBridgeName() string {
@@ -311,21 +312,9 @@ func (n *bridgeNetwork) isolateNetwork(others []*bridgeNetwork, enable bool) err
 		return nil
 	}
 
-	// Install the rules to isolate this networks against each of the other networks
-	for _, o := range others {
-		o.Lock()
-		otherConfig := o.config
-		o.Unlock()
-
-		if otherConfig.Internal {
-			continue
-		}
-
-		if thisConfig.BridgeName != otherConfig.BridgeName {
-			if err := setINC(thisConfig.BridgeName, otherConfig.BridgeName, enable); err != nil {
-				return err
-			}
-		}
+	// Install the rules to isolate this network against each of the other networks
+	if err := setINC(thisConfig.BridgeName, enable); err != nil {
+		return err
 	}
 
 	return nil
@@ -333,11 +322,12 @@ func (n *bridgeNetwork) isolateNetwork(others []*bridgeNetwork, enable bool) err
 
 func (d *driver) configure(option map[string]interface{}) error {
 	var (
-		config         *configuration
-		err            error
-		natChain       *iptables.ChainInfo
-		filterChain    *iptables.ChainInfo
-		isolationChain *iptables.ChainInfo
+		config          *configuration
+		err             error
+		natChain        *iptables.ChainInfo
+		filterChain     *iptables.ChainInfo
+		isolationChain1 *iptables.ChainInfo
+		isolationChain2 *iptables.ChainInfo
 	)
 
 	genericData, ok := option[netlabel.GenericData]
@@ -365,7 +355,7 @@ func (d *driver) configure(option map[string]interface{}) error {
 			}
 		}
 		removeIPChains()
-		natChain, filterChain, isolationChain, err = setupIPChains(config)
+		natChain, filterChain, isolationChain1, isolationChain2, err = setupIPChains(config)
 		if err != nil {
 			return err
 		}
@@ -384,7 +374,8 @@ func (d *driver) configure(option map[string]interface{}) error {
 	d.Lock()
 	d.natChain = natChain
 	d.filterChain = filterChain
-	d.isolationChain = isolationChain
+	d.isolationChain1 = isolationChain1
+	d.isolationChain2 = isolationChain2
 	d.config = config
 	d.Unlock()
 

libnetwork/drivers/bridge/bridge_test.go

@@ -425,14 +425,15 @@ func TestCreateMultipleNetworks(t *testing.T) {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
+	verifyV4INCEntries(d.networks, t)
+
 	config2 := &networkConfiguration{BridgeName: "net_test_2"}
 	genericOption[netlabel.GenericData] = config2
 	if err := d.CreateNetwork("2", genericOption, nil, getIPv4Data(t, ""), nil); err != nil {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
-	// Verify the network isolation rules are installed, each network subnet should appear 2 times
-	verifyV4INCEntries(d.networks, 2, t)
+	verifyV4INCEntries(d.networks, t)
 
 	config3 := &networkConfiguration{BridgeName: "net_test_3"}
 	genericOption[netlabel.GenericData] = config3
@@ -440,8 +441,7 @@ func TestCreateMultipleNetworks(t *testing.T) {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
-	// Verify the network isolation rules are installed, each network subnet should appear 4 times
-	verifyV4INCEntries(d.networks, 6, t)
+	verifyV4INCEntries(d.networks, t)
 
 	config4 := &networkConfiguration{BridgeName: "net_test_4"}
 	genericOption[netlabel.GenericData] = config4
@@ -449,45 +449,43 @@ func TestCreateMultipleNetworks(t *testing.T) {
 		t.Fatalf("Failed to create bridge: %v", err)
 	}
 
-	// Now 6 times
-	verifyV4INCEntries(d.networks, 12, t)
+	verifyV4INCEntries(d.networks, t)
 
 	d.DeleteNetwork("1")
-	verifyV4INCEntries(d.networks, 6, t)
+	verifyV4INCEntries(d.networks, t)
 
 	d.DeleteNetwork("2")
-	verifyV4INCEntries(d.networks, 2, t)
+	verifyV4INCEntries(d.networks, t)
 
 	d.DeleteNetwork("3")
-	verifyV4INCEntries(d.networks, 0, t)
+	verifyV4INCEntries(d.networks, t)
 
 	d.DeleteNetwork("4")
-	verifyV4INCEntries(d.networks, 0, t)
+	verifyV4INCEntries(d.networks, t)
 }
 
-func verifyV4INCEntries(networks map[string]*bridgeNetwork, numEntries int, t *testing.T) {
-	out, err := iptables.Raw("-nvL", IsolationChain)
+// Verify the network isolation rules are installed for each network
+func verifyV4INCEntries(networks map[string]*bridgeNetwork, t *testing.T) {
+	out1, err := iptables.Raw("-S", IsolationChain1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	out2, err := iptables.Raw("-S", IsolationChain2)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	found := 0
-	for _, x := range networks {
-		for _, y := range networks {
-			if x == y {
-				continue
-			}
-			re := regexp.MustCompile(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++
+	for _, n := range networks {
+		re := regexp.MustCompile(fmt.Sprintf("-i %s ! -o %s -j %s", n.config.BridgeName, n.config.BridgeName, IsolationChain2))
+		matches := re.FindAllString(string(out1[:]), -1)
+		if len(matches) != 1 {
+			t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables for network %s:\n%s.", n.id, string(out1[:]))
+		}
+		re = regexp.MustCompile(fmt.Sprintf("-o %s -j DROP", n.config.BridgeName))
+		matches = re.FindAllString(string(out2[:]), -1)
+		if len(matches) != 1 {
+			t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables for network %s:\n%s.", n.id, string(out2[:]))
 		}
-	}
-
-	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)
 	}
 }
 
@@ -996,9 +994,9 @@ func TestCleanupIptableRules(t *testing.T) {
 	bridgeChain := []iptables.ChainInfo{
 		{Name: DockerChain, Table: iptables.Nat},
 		{Name: DockerChain, Table: iptables.Filter},
-		{Name: IsolationChain, Table: iptables.Filter},
+		{Name: IsolationChain1, Table: iptables.Filter},
 	}
-	if _, _, _, err := setupIPChains(&configuration{EnableIPTables: true}); err != nil {
+	if _, _, _, _, err := setupIPChains(&configuration{EnableIPTables: true}); err != nil {
 		t.Fatalf("Error setting up ip chains: %v", err)
 	}
 	for _, chainInfo := range bridgeChain {

libnetwork/drivers/bridge/setup_ip_tables.go

@@ -12,21 +12,31 @@ import (
 
 // DockerChain: DOCKER iptable chain name
 const (
-	DockerChain    = "DOCKER"
-	IsolationChain = "DOCKER-ISOLATION"
+	DockerChain = "DOCKER"
+	// Isolation between bridge networks is achieved in two stages by means
+	// of the following two chains in the filter table. The first chain matches
+	// on the source interface being a bridge network's bridge and the
+	// destination being a different interface. A positive match leads to the
+	// second isolation chain. No match returns to the parent chain. The second
+	// isolation chain matches on destination interface being a bridge network's
+	// bridge. A positive match identifies a packet originated from one bridge
+	// network's bridge destined to another bridge network's bridge and will
+	// result in the packet being dropped. No match returns to the parent chain.
+	IsolationChain1 = "DOCKER-ISOLATION-STAGE-1"
+	IsolationChain2 = "DOCKER-ISOLATION-STAGE-2"
 )
 
-func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
+func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) {
 	// Sanity check.
 	if config.EnableIPTables == false {
-		return nil, nil, nil, errors.New("cannot create new chains, EnableIPTable is disabled")
+		return nil, nil, nil, nil, errors.New("cannot create new chains, EnableIPTable is disabled")
 	}
 
 	hairpinMode := !config.EnableUserlandProxy
 
 	natChain, err := iptables.NewChain(DockerChain, iptables.Nat, hairpinMode)
 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("failed to create NAT chain: %v", err)
+		return nil, nil, nil, nil, fmt.Errorf("failed to create NAT chain: %v", err)
 	}
 	defer func() {
 		if err != nil {
@@ -38,7 +48,7 @@ func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainI
 
 	filterChain, err := iptables.NewChain(DockerChain, iptables.Filter, false)
 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("failed to create FILTER chain: %v", err)
+		return nil, nil, nil, nil, fmt.Errorf("failed to create FILTER chain: %v", err)
 	}
 	defer func() {
 		if err != nil {
@@ -48,16 +58,25 @@ func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainI
 		}
 	}()
 
-	isolationChain, err := iptables.NewChain(IsolationChain, iptables.Filter, false)
+	isolationChain1, err := iptables.NewChain(IsolationChain1, iptables.Filter, false)
 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
+		return nil, nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
 	}
 
-	if err := iptables.AddReturnRule(IsolationChain); err != nil {
-		return nil, nil, nil, err
+	isolationChain2, err := iptables.NewChain(IsolationChain2, iptables.Filter, false)
+	if err != nil {
+		return nil, nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
 	}
 
-	return natChain, filterChain, isolationChain, nil
+	if err := iptables.AddReturnRule(IsolationChain1); err != nil {
+		return nil, nil, nil, nil, err
+	}
+
+	if err := iptables.AddReturnRule(IsolationChain2); err != nil {
+		return nil, nil, nil, nil, err
+	}
+
+	return natChain, filterChain, isolationChain1, isolationChain2, nil
 }
 
 func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInterface) error {
@@ -94,7 +113,7 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt
 		n.registerIptCleanFunc(func() error {
 			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())
 		}
@@ -117,7 +136,7 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt
 	}
 
 	d.Lock()
-	err = iptables.EnsureJumpRule("FORWARD", IsolationChain)
+	err = iptables.EnsureJumpRule("FORWARD", IsolationChain1)
 	d.Unlock()
 	if err != nil {
 		return err
@@ -245,42 +264,56 @@ func setIcc(bridgeIface string, iccEnable, insert bool) error {
 	return nil
 }
 
-// Control Inter Network Communication. Install/remove only if it is not/is present.
-func setINC(iface1, iface2 string, enable bool) error {
+// Control Inter Network Communication. Install[Remove] only if it is [not] present.
+func setINC(iface string, enable bool) error {
 	var (
-		table = iptables.Filter
-		chain = IsolationChain
-		args  = [2][]string{{"-i", iface1, "-o", iface2, "-j", "DROP"}, {"-i", iface2, "-o", iface1, "-j", "DROP"}}
+		action    = iptables.Insert
+		actionMsg = "add"
+		chains    = []string{IsolationChain1, IsolationChain2}
+		rules     = [][]string{
+			{"-i", iface, "!", "-o", iface, "-j", IsolationChain2},
+			{"-o", iface, "-j", "DROP"},
+		}
 	)
 
-	if enable {
-		for i := 0; i < 2; i++ {
-			if iptables.Exists(table, chain, args[i]...) {
-				continue
-			}
-			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 {
-		for i := 0; i < 2; i++ {
-			if !iptables.Exists(table, chain, args[i]...) {
-				continue
-			}
-			if err := iptables.RawCombinedOutput(append([]string{"-D", chain}, args[i]...)...); err != nil {
-				return fmt.Errorf("unable to remove inter-network communication rule: %v", err)
+	if !enable {
+		action = iptables.Delete
+		actionMsg = "remove"
+	}
+
+	for i, chain := range chains {
+		if err := iptables.ProgramRule(iptables.Filter, chain, action, rules[i]); err != nil {
+			msg := fmt.Sprintf("unable to %s inter-network communication rule: %v", actionMsg, err)
+			if enable {
+				if i == 1 {
+					// Rollback the rule installed on first chain
+					if err2 := iptables.ProgramRule(iptables.Filter, chains[0], iptables.Delete, rules[0]); err2 != nil {
+						logrus.Warn("Failed to rollback iptables rule after failure (%v): %v", err, err2)
+					}
+				}
+				return fmt.Errorf(msg)
 			}
+			logrus.Warn(msg)
 		}
 	}
 
 	return nil
 }
 
+// Obsolete chain from previous docker versions
+const oldIsolationChain = "DOCKER-ISOLATION"
+
 func removeIPChains() {
+	// Remove obsolete rules from default chains
+	iptables.ProgramRule(iptables.Filter, "FORWARD", iptables.Delete, []string{"-j", oldIsolationChain})
+
+	// Remove chains
 	for _, chainInfo := range []iptables.ChainInfo{
 		{Name: DockerChain, Table: iptables.Nat},
 		{Name: DockerChain, Table: iptables.Filter},
-		{Name: IsolationChain, Table: iptables.Filter},
+		{Name: IsolationChain1, Table: iptables.Filter},
+		{Name: IsolationChain2, Table: iptables.Filter},
+		{Name: oldIsolationChain, Table: iptables.Filter},
 	} {
 		if err := chainInfo.Remove(); err != nil {
 			logrus.Warnf("Failed to remove existing iptables entries in table %s chain %s : %v", chainInfo.Table, chainInfo.Name, err)
@@ -290,8 +323,8 @@ func removeIPChains() {
 
 func setupInternalNetworkRules(bridgeIface string, addr net.Addr, icc, insert bool) error {
 	var (
-		inDropRule  = iptRule{table: iptables.Filter, chain: IsolationChain, args: []string{"-i", bridgeIface, "!", "-d", addr.String(), "-j", "DROP"}}
-		outDropRule = iptRule{table: iptables.Filter, chain: IsolationChain, args: []string{"-o", bridgeIface, "!", "-s", addr.String(), "-j", "DROP"}}
+		inDropRule  = iptRule{table: iptables.Filter, chain: IsolationChain1, args: []string{"-i", bridgeIface, "!", "-d", addr.String(), "-j", "DROP"}}
+		outDropRule = iptRule{table: iptables.Filter, chain: IsolationChain1, args: []string{"-o", bridgeIface, "!", "-s", addr.String(), "-j", "DROP"}}
 	)
 	if err := programChainRule(inDropRule, "DROP INCOMING", insert); err != nil {
 		return err

libnetwork/drivers/bridge/setup_ip_tables_test.go

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