Do not skip bridge creation if bridge exists

net/bridge.go

@@ -411,7 +411,7 @@ func configureIPTables(config *BridgeConfig) error {
 	}
 	if config.DockerBridgeName != "" {
 		if config.WeaveBridgeName != config.DockerBridgeName {
-			// This check is not ideal, as the rule should be at the very top
+			// 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 {
@@ -450,48 +450,46 @@ 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
-		// If WEAVE-NPC chain doesn't exist then next line will fail
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"); err != nil {
-			return err
-		}
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"); err != nil {
-			return err
-		}
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "DROP"); err != nil {
-			return err
-		}
+		// 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", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"},
+				{"-o", config.WeaveBridgeName, "-j", "DROP"},
+			}...)
 	} else {
 		// Work around the situation where there are no rules allowing traffic
 		// across our bridge. E.g. ufw
-		if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil {
-			return err
-		}
+		fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"})
 	}
 
 	if !config.NPC {
 		// Create a chain for allowing ingress traffic when the bridge is exposed
 		_ = ipt.NewChain("filter", "WEAVE-EXPOSE")
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"); err != nil {
-			return err
-		}
+		fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"})
 	}
 
 	// Forward from weave to the rest of the world
-	if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil {
-		return err
-	}
+	fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"})
 	// and allow replies back
-	if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"); err != nil {
+	fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"})
+
+	if err := ensureRules("filter", "FORWARD", fwdRules, ipt); err != nil {
 		return err
 	}
 
 	// create a chain for masquerading
-	if err = ipt.ClearChain("nat", "WEAVE"); err != nil {
-		return errors.Wrap(err, "clearing WEAVE chain")
-	}
+	//
+	// NB: we do not clear the chain to preserve existing rules
+	// inserted by "weave expose".
+	_ = ipt.NewChain("nat", "WEAVE")
+
 	return ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE")
 }
 
@@ -502,3 +500,40 @@ func linkSetUpByName(linkName string) error {
 	}
 	return netlink.LinkSetUp(link)
 }
+
+// ensureRules ensures the presence of given iptables rules.
+//
+// If any rule from the list is missing, the function deletes all given
+// rules and re-appends them to ensure the order of the rules.
+func ensureRules(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error {
+	allFound := true
+
+	for _, rs := range rulespecs {
+		found, err := ipt.Exists(table, chain, rs...)
+		if err != nil {
+			return errors.Wrapf(err, "ipt.Exists(%s, %s, %s)", table, chain, rs)
+		}
+		if !found {
+			allFound = false
+			break
+		}
+	}
+
+	// All rules exist, do nothing.
+	if allFound {
+		return nil
+	}
+
+	for _, rs := range rulespecs {
+		// If any is missing, then delete all, as we need to preserve the order of
+		// given rules. Ignore errors, as rule might not exist.
+		if !allFound {
+			ipt.Delete(table, chain, rs...)
+		}
+		if err := ipt.Append(table, chain, rs...); err != nil {
+			return errors.Wrapf(err, "ipt.Append(%s, %s, %s)", table, chain, rs)
+		}
+	}
+
+	return nil
+}