From 9a9c06966b9ccd16d74d293f869afeeb746d988e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 20 Jan 2018 12:47:11 +0100 Subject: [PATCH 1/4] Bring the weave bridge isolation iptables rule to the top Make the rule creation to use the same mechanism (via ensureRules) as other rules from filter/FORWARD. Otherwise, the rule is preceded by "-j WEAVE-EXPOSE" rule which can break the subnet isolation. --- net/bridge.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index c084d7cd..b2183103 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -409,20 +409,13 @@ func configureIPTables(config *BridgeConfig) error { if err != nil { return errors.Wrap(err, "creating iptables object") } + + // The order among weave filter/FORWARD rules is important! + fwdRules := make([][]string, 0) + if config.DockerBridgeName != "" { if config.WeaveBridgeName != config.DockerBridgeName { - // This is not ideal, as it does not check whether the rule is at the top - // of the chain. - found, err := ipt.Exists("filter", "FORWARD", "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") - if err != nil { - return err - } - if !found { - err := ipt.Insert("filter", "FORWARD", 1, "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") - if err != nil { - return err - } - } + fwdRules = append(fwdRules, []string{"-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP"}) } dockerBridgeIP, err := FindBridgeIP(config.DockerBridgeName, nil) @@ -450,9 +443,6 @@ func configureIPTables(config *BridgeConfig) error { } } - // The order among weave filter/FORWARD rules is important! - fwdRules := make([][]string, 0) - if config.NPC { // Steer traffic via the NPC _ = ipt.NewChain("filter", "WEAVE-NPC") // ignore error because it might already exist From 439b4ad190d08f1c067554f7a7ed49e91747bae3 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 20 Jan 2018 12:51:31 +0100 Subject: [PATCH 2/4] Improve naming of ensureRulesAtTop function --- net/bridge.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index b2183103..403d43ed 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -470,7 +470,7 @@ func configureIPTables(config *BridgeConfig) error { // and allow replies back fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}) - if err := ensureRules("filter", "FORWARD", fwdRules, ipt); err != nil { + if err := ensureRulesAtTop("filter", "FORWARD", fwdRules, ipt); err != nil { return err } @@ -491,11 +491,11 @@ func linkSetUpByName(linkName string) error { return netlink.LinkSetUp(link) } -// ensureRules ensures the presence of given iptables rules. +// ensureRulesAtTop ensures the presence of given iptables rules. // // If any rule from the list is missing, the function deletes all given -// rules and re-inserts them to ensure the order of the rules. -func ensureRules(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error { +// rules and re-inserts them at the top of the chain to ensure the order of the rules. +func ensureRulesAtTop(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error { allFound := true for _, rs := range rulespecs { From 9a5514ffe03cc06e7c709c361bf69145d435254d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 20 Jan 2018 12:57:11 +0100 Subject: [PATCH 3/4] Add comment about the FORWARD rule order Should help debugging. --- net/bridge.go | 4 +++- test/191_create_bridge_2_test.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 403d43ed..21719aa2 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -449,7 +449,9 @@ func configureIPTables(config *BridgeConfig) error { // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, [][]string{ - {"-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"}, + {"-o", config.WeaveBridgeName, + "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", + "-j", "WEAVE-NPC"}, {"-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"}, {"-o", config.WeaveBridgeName, "-j", "DROP"}, }...) diff --git a/test/191_create_bridge_2_test.sh b/test/191_create_bridge_2_test.sh index fa26c780..abca8a9b 100755 --- a/test/191_create_bridge_2_test.sh +++ b/test/191_create_bridge_2_test.sh @@ -48,7 +48,7 @@ IPT_BEFORE=$(mktemp) IPT_AFTER=$(mktemp) run_on $HOST1 "sudo iptables-save | grep -i weave | grep -v '\[.*:.*\]' > $IPT_BEFORE" -run_on $HOST1 sudo iptables -t filter -D FORWARD -o weave -j WEAVE-NPC +run_on $HOST1 "sudo iptables -t filter -D FORWARD -o weave -m comment --comment \"NOTE: this must go before '-j KUBE-FORWARD'\" -j WEAVE-NPC" run_on $HOST1 sudo iptables -t nat -D POSTROUTING -j WEAVE kill_weaver # should re-create the bridge and iptables friends From 0da2cbf84e951c45eaa2360e95cb1a9cb15579df Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 29 Jan 2018 13:45:45 +0100 Subject: [PATCH 4/4] Update build-tools to include the defunct process fix --- tools | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools b/tools index 74dc626b..ca69607f 160000 --- a/tools +++ b/tools @@ -1 +1 @@ -Subproject commit 74dc626b6de3ffb38591510f7cb7bc2db33743c4 +Subproject commit ca69607f7936e186cee43bb4bcaede979de701e2