FFFF
Skip to content

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fcrisciani merged 3 commits into from
Apr 2, 2018
Merged

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

Show file tree
Hide file tree
Changes from all c 10BC0 ommits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c73b1c
Improve scalabiltiy of bridge network isolation rules
aboch Nov 2, 2016
b6503b8
bridge: fix handling errors during setupIPChains()
AkihiroSuda Mar 22, 2018
9be4c10
bridge: fix lint issue
AkihiroSuda Apr 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 12 additions & 25 deletions drivers/bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ type driver struct {
network *bridgeNetwork
natChain *iptables.ChainInfo
filterChain *iptables.ChainInfo
isolationChain *iptables.ChainInfo
isolationChain1 *iptables.ChainInfo
isolationChain2 *iptables.ChainInfo
networks map[string]*bridgeNetwork
store datastore.DataStore
nlh *netlink.Handle
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -311,24 +312,8 @@ 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
}
}
}

return nil
// Install the rules to isolate this network against each of the other networks
return setINC(thisConfig.BridgeName, enable)
}

func (d *driver) configure(option map[string]interface{}) error {
Expand All @@ -337,7 +322,8 @@ func (d *driver) configure(option map[string]interface{}) error {
err error
natChain *iptables.ChainInfo
filterChain *iptables.ChainInfo
isolationChain *iptables.ChainInfo
isolationChain1 *iptables.ChainInfo
isolationChain2 *iptables.ChainInfo
)

genericData, ok := option[netlabel.GenericData]
Expand Down Expand Up @@ -365,7 +351,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
}
Expand All @@ -384,7 +370,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()

Expand Down
54 changes: 26 additions & 28 deletions drivers/bridge/bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,69 +425,67 @@ 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
if err := d.CreateNetwork("3", 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 4 times
verifyV4INCEntries(d.networks, 6, t)
verifyV4INCEntries(d.networks, t)

config4 := &networkConfiguration{BridgeName: "net_test_4"}
genericOption[netlabel.GenericData] = config4
if err := d.CreateNetwork("4", genericOption, nil, getIPv4Data(t, ""), nil); err != nil {
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)
}

found := 0
for _, x := range networks {
for _, y := range networks {
if x == y {
continue
out2, err := iptables.Raw("-S", IsolationChain2)
if err != nil {
t.Fatal(err)
}
re := regexp.MustCompile(x.config.BridgeName + " " + y.config.BridgeName)
matches := re.FindAllString(string(out[:]), -1)

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:\n%s", string(out[:]))
}
found++
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)
}
}

Expand Down Expand Up @@ -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 {
Expand Down
117 changes: 82 additions & 35 deletions drivers/bridge/setup_ip_tables.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,51 +13,84 @@ import (
// DockerChain: DOCKER iptable chain name
const (
DockerChain = "DOCKER"
IsolationChain = "DOCKER-ISOLATION"
// 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 %s: %v", DockerChain, 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 %s on cleanup: %v", DockerChain, err)
}
}
}()

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 %s: %v", DockerChain, 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)
logrus.Warnf("failed on removing iptables FILTER chain %s on cleanup: %v", DockerChain, err)
}
}
}()

isolationChain, err := iptables.NewChain(IsolationChain, iptables.Filter, false)
isolationChain1, err := iptables.NewChain(IsolationChain1, iptables.Filter, false)
if err != nil {
return nil, nil, nil, fmt.E DDB1 rrorf("failed to create FILTER isolation chain: %v", err)
return nil, nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
}
defer func() {
if err != nil {
if err := iptables.RemoveExistingChain(IsolationChain1, iptables.Filter); err != nil {
logrus.Warnf("failed on removing iptables FILTER chain %s on cleanup: %v", IsolationChain1, 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)
}
defer func() {
if err != nil {
if err := iptables.RemoveExistingChain(IsolationChain2, iptables.Filter); err != nil {
logrus.Warnf("failed on removing iptables FILTER chain %s on cleanup: %v", IsolationChain2, err)
}
}
}()

if err := iptables.AddReturnRule(IsolationChain1); err != nil {
return nil, nil, nil, nil, err
}

if err := iptables.AddReturnRule(IsolationChain); err != nil {
return nil, nil, nil, err
if err := iptables.AddReturnRule(IsolationChain2); err != nil {
return nil, nil, nil, nil, err
}

return natChain, filterChain, isolationChain, nil
return natChain, filterChain, isolationChain1, isolationChain2, nil
}

func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInterface) error {
Expand Down Expand Up @@ -94,7 +127,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())
}
Expand All @@ -117,7 +150,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
Expand Down Expand Up @@ -245,42 +278,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)
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)
}
} 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)
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)
Expand All @@ -290,8 +337,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
Expand Down
Loading
0