Merge pull request #10792 from larsks/bug/10781

fix various problems with iptables.Exists

daemon/networkdriver/bridge/driver.go

@@ -284,10 +284,11 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error {
 	// Enable NAT
 
 	if ipmasq {
-		natArgs := []string{"POSTROUTING", "-t", "nat", "-s", addr.String(), "!", "-o", bridgeIface, "-j", "MASQUERADE"}
+		natArgs := []string{"-s", addr.String(), "!", "-o", bridgeIface, "-j", "MASQUERADE"}
 
-		if !iptables.Exists(natArgs...) {
-			if output, err := iptables.Raw(append([]string{"-I"}, natArgs...)...); err != nil {
+		if !iptables.Exists(iptables.Nat, "POSTROUTING", natArgs...) {
+			if output, err := iptables.Raw(append([]string{
+				"-t", string(iptables.Nat), "-I", "POSTROUTING"}, natArgs...)...); err != nil {
 				return fmt.Errorf("Unable to enable network bridge NAT: %s", err)
 			} else if len(output) != 0 {
 				return &iptables.ChainError{Chain: "POSTROUTING", Output: output}
@@ -296,28 +297,28 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error {
 	}
 
 	var (
-		args       = []string{"FORWARD", "-i", bridgeIface, "-o", bridgeIface, "-j"}
+		args       = []string{"-i", bridgeIface, "-o", bridgeIface, "-j"}
 		acceptArgs = append(args, "ACCEPT")
 		dropArgs   = append(args, "DROP")
 	)
 
 	if !icc {
-		iptables.Raw(append([]string{"-D"}, acceptArgs...)...)
+		iptables.Raw(append([]string{"-D", "FORWARD"}, acceptArgs...)...)
 
-		if !iptables.Exists(dropArgs...) {
+		if !iptables.Exists(iptables.Filter, "FORWARD", dropArgs...) {
 			log.Debugf("Disable inter-container communication")
-			if output, err := iptables.Raw(append([]string{"-I"}, dropArgs...)...); err != nil {
+			if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, dropArgs...)...); err != nil {
 				return fmt.Errorf("Unable to prevent intercontainer communication: %s", err)
 			} else if len(output) != 0 {
 				return fmt.Errorf("Error disabling intercontainer communication: %s", output)
 			}
 		}
 	} else {
-		iptables.Raw(append([]string{"-D"}, dropArgs...)...)
+		iptables.Raw(append([]string{"-D", "FORWARD"}, dropArgs...)...)
 
-		if !iptables.Exists(acceptArgs...) {
+		if !iptables.Exists(iptables.Filter, "FORWARD", acceptArgs...) {
 			log.Debugf("Enable inter-container communication")
-			if output, err := iptables.Raw(append([]string{"-I"}, acceptArgs...)...); err != nil {
+			if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, acceptArgs...)...); err != nil {
 				return fmt.Errorf("Unable to allow intercontainer communication: %s", err)
 			} else if len(output) != 0 {
 				return fmt.Errorf("Error enabling intercontainer communication: %s", output)
@@ -326,9 +327,9 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error {
 	}
 
 	// Accept all non-intercontainer outgoing packets
-	outgoingArgs := []string{"FORWARD", "-i", bridgeIface, "!", "-o", bridgeIface, "-j", "ACCEPT"}
-	if !iptables.Exists(outgoingArgs...) {
-		if output, err := iptables.Raw(append([]string{"-I"}, outgoingArgs...)...); err != nil {
+	outgoingArgs := []string{"-i", bridgeIface, "!", "-o", bridgeIface, "-j", "ACCEPT"}
+	if !iptables.Exists(iptables.Filter, "FORWARD", outgoingArgs...) {
+		if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, outgoingArgs...)...); err != nil {
 			return fmt.Errorf("Unable to allow outgoing packets: %s", err)
 		} else if len(output) != 0 {
 			return &iptables.ChainError{Chain: "FORWARD outgoing", Output: output}
@@ -336,10 +337,10 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error {
 	}
 
 	// Accept incoming packets for existing connections
-	existingArgs := []string{"FORWARD", "-o", bridgeIface, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}
+	existingArgs := []string{"-o", bridgeIface, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}
 
-	if !iptables.Exists(existingArgs...) {
-		if output, err := iptables.Raw(append([]string{"-I"}, existingArgs...)...); err != nil {
+	if !iptables.Exists(iptables.Filter, "FORWARD", existingArgs...) {
+		if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, existingArgs...)...); err != nil {
 			return fmt.Errorf("Unable to allow incoming packets: %s", err)
 		} else if len(output) != 0 {
 			return &iptables.ChainError{Chain: "FORWARD incoming", Output: output}

integration-cli/docker_cli_links_test.go

@@ -132,14 +132,14 @@ func TestLinksIpTablesRulesWhenLinkAndUnlink(t *testing.T) {
 	childIP := findContainerIP(t, "child")
 	parentIP := findContainerIP(t, "parent")
 
-	sourceRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"}
-	destinationRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"}
-	if !iptables.Exists(sourceRule...) || !iptables.Exists(destinationRule...) {
+	sourceRule := []string{"-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"}
+	destinationRule := []string{"-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"}
+	if !iptables.Exists("filter", "DOCKER", sourceRule...) || !iptables.Exists("filter", "DOCKER", destinationRule...) {
 		t.Fatal("Iptables rules not found")
 	}
 
 	dockerCmd(t, "rm", "--link", "parent/http")
-	if iptables.Exists(sourceRule...) || iptables.Exists(destinationRule...) {
+	if iptables.Exists("filter", "DOCKER", sourceRule...) || iptables.Exists("filter", "DOCKER", destinationRule...) {
 		t.Fatal("Iptables rules should be removed when unlink")
 	}
 

pkg/iptables/iptables.go

@@ -21,6 +21,7 @@ const (
 	Insert Action = "-I"
 	Nat    Table  = "nat"
 	Filter Table  = "filter"
+	Mangle Table  = "mangle"
 )
 
 var (
@@ -82,7 +83,7 @@ func NewChain(name, bridge string, table Table) (*Chain, error) {
 		preroute := []string{
 			"-m", "addrtype",
 			"--dst-type", "LOCAL"}
-		if !Exists(preroute...) {
+		if !Exists(Nat, "PREROUTING", preroute...) {
 			if err := c.Prerouting(Append, preroute...); err != nil {
 				return nil, fmt.Errorf("Failed to inject docker in PREROUTING chain: %s", err)
 			}
@@ -91,17 +92,17 @@ func NewChain(name, bridge string, table Table) (*Chain, error) {
 			"-m", "addrtype",
 			"--dst-type", "LOCAL",
 			"!", "--dst", "127.0.0.0/8"}
-		if !Exists(output...) {
+		if !Exists(Nat, "OUTPUT", output...) {
 			if err := c.Output(Append, output...); err != nil {
 				return nil, fmt.Errorf("Failed to inject docker in OUTPUT chain: %s", err)
 			}
 		}
 	case Filter:
-		link := []string{"FORWARD",
+		link := []string{
 			"-o", c.Bridge,
 			"-j", c.Name}
-		if !Exists(link...) {
-			insert := append([]string{string(Insert)}, link...)
+		if !Exists(Filter, "FORWARD", link...) {
+			insert := append([]string{string(Insert), "FORWARD"}, link...)
 			if output, err := Raw(insert...); err != nil {
 				return nil, err
 			} else if len(output) != 0 {
@@ -242,19 +243,25 @@ func (c *Chain) Remove() error {
 }
 
 // Check if a rule exists
-func Exists(args ...string) bool {
+func Exists(table Table, chain string, rule ...string) bool {
+	if string(table) == "" {
+		table = Filter
+	}
+
 	// iptables -C, --check option was added in v.1.4.11
 	// http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt
 
 	// try -C
 	// if exit status is 0 then return true, the rule exists
-	if _, err := Raw(append([]string{"-C"}, args...)...); err == nil {
+	if _, err := Raw(append([]string{
+		"-t", string(table), "-C", chain}, rule...)...); err == nil {
 		return true
 	}
 
-	// parse iptables-save for the rule
-	rule := strings.Replace(strings.Join(args, " "), "-t nat ", "", -1)
-	existingRules, _ := exec.Command("iptables-save").Output()
+	// parse "iptables -S" for the rule (this checks rules in a specific chain
+	// in a specific table)
+	rule_string := strings.Join(rule, " ")
+	existingRules, _ := exec.Command("iptables", "-t", string(table), "-S", chain).Output()
 
 	// regex to replace ips in rule
 	// because MASQUERADE rule will not be exactly what was passed
@@ -262,7 +269,7 @@ func Exists(args ...string) bool {
 
 	return strings.Contains(
 		re.ReplaceAllString(string(existingRules), "?"),
-		re.ReplaceAllString(rule, "?"),
+		re.ReplaceAllString(rule_string, "?"),
 	)
 }
 

pkg/iptables/iptables_test.go

@@ -39,8 +39,7 @@ func TestForward(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	dnatRule := []string{natChain.Name,
-		"-t", string(natChain.Table),
+	dnatRule := []string{
 		"!", "-i", filterChain.Bridge,
 		"-d", ip.String(),
 		"-p", proto,
@@ -49,12 +48,11 @@ func TestForward(t *testing.T) {
 		"--to-destination", dstAddr + ":" + strconv.Itoa(dstPort),
 	}
 
-	if !Exists(dnatRule...) {
+	if !Exists(natChain.Table, natChain.Name, dnatRule...) {
 		t.Fatalf("DNAT rule does not exist")
 	}
 
-	filterRule := []string{filterChain.Name,
-		"-t", string(filterChain.Table),
+	filterRule := []string{
 		"!", "-i", filterChain.Bridge,
 		"-o", filterChain.Bridge,
 		"-d", dstAddr,
@@ -63,12 +61,11 @@ func TestForward(t *testing.T) {
 		"-j", "ACCEPT",
 	}
 
-	if !Exists(filterRule...) {
+	if !Exists(filterChain.Table, filterChain.Name, filterRule...) {
 		t.Fatalf("filter rule does not exist")
 	}
 
-	masqRule := []string{"POSTROUTING",
-		"-t", string(natChain.Table),
+	masqRule := []string{
 		"-d", dstAddr,
 		"-s", dstAddr,
 		"-p", proto,
@@ -76,7 +73,7 @@ func TestForward(t *testing.T) {
 		"-j", "MASQUERADE",
 	}
 
-	if !Exists(masqRule...) {
+	if !Exists(natChain.Table, "POSTROUTING", masqRule...) {
 		t.Fatalf("MASQUERADE rule does not exist")
 	}
 }
@@ -94,8 +91,7 @@ func TestLink(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	rule1 := []string{filterChain.Name,
-		"-t", string(filterChain.Table),
+	rule1 := []string{
 		"-i", filterChain.Bridge,
 		"-o", filterChain.Bridge,
 		"-p", proto,
@@ -104,12 +100,11 @@ func TestLink(t *testing.T) {
 		"--dport", strconv.Itoa(port),
 		"-j", "ACCEPT"}
 
-	if !Exists(rule1...) {
+	if !Exists(filterChain.Table, filterChain.Name, rule1...) {
 		t.Fatalf("rule1 does not exist")
 	}
 
-	rule2 := []string{filterChain.Name,
-		"-t", string(filterChain.Table),
+	rule2 := []string{
 		"-i", filterChain.Bridge,
 		"-o", filterChain.Bridge,
 		"-p", proto,
@@ -118,7 +113,7 @@ func TestLink(t *testing.T) {
 		"--sport", strconv.Itoa(port),
 		"-j", "ACCEPT"}
 
-	if !Exists(rule2...) {
+	if !Exists(filterChain.Table, filterChain.Name, rule2...) {
 		t.Fatalf("rule2 does not exist")
 	}
 }
@@ -133,17 +128,16 @@ func TestPrerouting(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	rule := []string{"PREROUTING",
-		"-t", string(Nat),
+	rule := []string{
 		"-j", natChain.Name}
 
 	rule = append(rule, args...)
 
-	if !Exists(rule...) {
+	if !Exists(natChain.Table, "PREROUTING", rule...) {
 		t.Fatalf("rule does not exist")
 	}
 
-	delRule := append([]string{"-D"}, rule...)
+	delRule := append([]string{"-D", "PREROUTING", "-t", string(Nat)}, rule...)
 	if _, err = Raw(delRule...); err != nil {
 		t.Fatal(err)
 	}
@@ -159,17 +153,17 @@ func TestOutput(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	rule := []string{"OUTPUT",
-		"-t", string(natChain.Table),
+	rule := []string{
 		"-j", natChain.Name}
 
 	rule = append(rule, args...)
 
-	if !Exists(rule...) {
+	if !Exists(natChain.Table, "OUTPUT", rule...) {
 		t.Fatalf("rule does not exist")
 	}
 
-	delRule := append([]string{"-D"}, rule...)
+	delRule := append([]string{"-D", "OUTPUT", "-t",
+		string(natChain.Table)}, rule...)
 	if _, err = Raw(delRule...); err != nil {
 		t.Fatal(err)
 	}