Merge pull request #421 from aojea/portmapErrors2

Portmap doesn't fail if chain doesn't exist

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/alexflint/go-filemutex v0.0.0-20171022225611-72bdc8eae2ae
 	github.com/buger/jsonparser v0.0.0-20180808090653-f4dd9f5a6b44
 	github.com/containernetworking/cni v0.7.1
-	github.com/coreos/go-iptables v0.4.2
+	github.com/coreos/go-iptables v0.4.5
 	github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7
 	github.com/d2g/dhcp4 v0.0.0-20170904100407-a1d1b6c41b1c
 	github.com/d2g/dhcp4client v1.0.0

go.sum

@@ -10,6 +10,10 @@ github.com/containernetworking/cni v0.7.1 h1:fE3r16wpSEyaqY4Z4oFrLMmIGfBYIKpPrHK
 github.com/containernetworking/cni v0.7.1/go.mod h1:LGwApLUm2FpoOfxTDEeq8T9ipbpZ61X79hmU3w8FmsY=
 github.com/coreos/go-iptables v0.4.2 h1:KH0EwId05JwWIfb96gWvkiT2cbuOu8ygqUaB+yPAwIg=
 github.com/coreos/go-iptables v0.4.2/go.mod h1:/mVI274lEDI2ns62jHCDnCyBF9Iwsmekav8Dbxlm1MU=
+github.com/coreos/go-iptables v0.4.4 h1:5oOUvU7Fk53Hn/rkdJ0zcYGCffotqXpyi4ADCkO1TJ8=
+github.com/coreos/go-iptables v0.4.4/go.mod h1:/mVI274lEDI2ns62jHCDnCyBF9Iwsmekav8Dbxlm1MU=
+github.com/coreos/go-iptables v0.4.5 h1:DpHb9vJrZQEFMcVLFKAAGMUVX0XoRC0ptCthinRYm38=
+github.com/coreos/go-iptables v0.4.5/go.mod h1:/mVI274lEDI2ns62jHCDnCyBF9Iwsmekav8Dbxlm1MU=
 github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7 h1:u9SHYsPQNyt5tgDm3YN7+9dYrpK96E5wFilTFWIDZOM=
 github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
 github.com/d2g/dhcp4 v0.0.0-20170904100407-a1d1b6c41b1c h1:Xo2rK1pzOm0jO6abTPIQwbAmqBIOj132otexc1mmzFc=

pkg/utils/iptables.go

@@ -62,3 +62,60 @@ func ChainExists(ipt *iptables.IPTables, table, chain string) (bool, error) {
 	}
 	return false, nil
 }
+
+// DeleteRule idempotently delete the iptables rule in the specified table/chain.
+// It does not return an error if the referring chain doesn't exist
+func DeleteRule(ipt *iptables.IPTables, table, chain string, rulespec ...string) error {
+	if ipt == nil {
+		return errors.New("failed to ensure iptable chain: IPTables was nil")
+	}
+	if err := ipt.Delete(table, chain, rulespec...); err != nil {
+		eerr, eok := err.(*iptables.Error)
+		switch {
+		case eok && eerr.IsNotExist():
+			// swallow here, the chain was already deleted
+			return nil
+		case eok && eerr.ExitStatus() == 2:
+			// swallow here, invalid command line parameter because the referring rule is missing
+			return nil
+		default:
+			return fmt.Errorf("Failed to delete referring rule %s %s: %v", table, chain, err)
+		}
+	}
+	return nil
+}
+
+// DeleteChain idempotently deletes the specified table/chain.
+// It does not return an errors if the chain does not exist
+func DeleteChain(ipt *iptables.IPTables, table, chain string) error {
+	if ipt == nil {
+		return errors.New("failed to ensure iptable chain: IPTables was nil")
+	}
+
+	err := ipt.DeleteChain(table, chain)
+	eerr, eok := err.(*iptables.Error)
+	switch {
+	case eok && eerr.IsNotExist():
+		// swallow here, the chain was already deleted
+		return nil
+	default:
+		return err
+	}
+}
+
+// ClearChain idempotently clear the iptables rules in the specified table/chain.
+// If the chain does not exist, a new one will be created
+func ClearChain(ipt *iptables.IPTables, table, chain string) error {
+	if ipt == nil {
+		return errors.New("failed to ensure iptable chain: IPTables was nil")
+	}
+	err := ipt.ClearChain(table, chain)
+	eerr, eok := err.(*iptables.Error)
+	switch {
+	case eok && eerr.IsNotExist():
+		// swallow here, the chain was already deleted
+		return EnsureChain(ipt, table, chain)
+	default:
+		return err
+	}
+}

pkg/utils/iptables_test.go

@@ -67,12 +67,31 @@ var _ = Describe("chain tests", func() {
 		cleanup()
 	})
 
-	It("creates chains idempotently", func() {
+	Describe("EnsureChain", func() {
-		err := EnsureChain(ipt, TABLE, testChain)
+		It("creates chains idempotently", func() {
-		Expect(err).NotTo(HaveOccurred())
+			err := EnsureChain(ipt, TABLE, testChain)
+			Expect(err).NotTo(HaveOccurred())
 
-		// Create it again!
+			// Create it again!
-		err = EnsureChain(ipt, TABLE, testChain)
+			err = EnsureChain(ipt, TABLE, testChain)
-		Expect(err).NotTo(HaveOccurred())
+			Expect(err).NotTo(HaveOccurred())
+		})
 	})
+
+	Describe("DeleteChain", func() {
+		It("delete chains idempotently", func() {
+			// Create chain
+			err := EnsureChain(ipt, TABLE, testChain)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Delete chain
+			err = DeleteChain(ipt, TABLE, testChain)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Delete it again!
+			err = DeleteChain(ipt, TABLE, testChain)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
 })

plugins/meta/portmap/chain.go

@@ -70,7 +70,7 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 	// flush the chain
 	// This will succeed *and create the chain* if it does not exist.
 	// If the chain doesn't exist, the next checks will fail.
-	if err := ipt.ClearChain(c.table, c.name); err != nil {
+	if err := utils.ClearChain(ipt, c.table, c.name); err != nil {
 		return err
 	}
 
@@ -90,17 +90,15 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 				}
 				chainParts = chainParts[2:] // List results always include an -A CHAINNAME
 
-				if err := ipt.Delete(c.table, entryChain, chainParts...); err != nil {
+				if err := utils.DeleteRule(ipt, c.table, entryChain, chainParts...); err != nil {
-					return fmt.Errorf("Failed to delete referring rule %s %s: %v", c.table, entryChainRule, err)
+					return err
 				}
+
 			}
 		}
 	}
 
-	if err := ipt.DeleteChain(c.table, c.name); err != nil {
+	return utils.DeleteChain(ipt, c.table, c.name)
-		return err
-	}
-	return nil
 }
 
 // insertUnique will add a rule to a chain if it does not already exist.

plugins/meta/portmap/chain_test.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"math/rand"
 	"runtime"
+	"sync"
 
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/testutils"
@@ -32,6 +33,7 @@ const TABLE = "filter" // We'll monkey around here
 var _ = Describe("chain tests", func() {
 	var testChain chain
 	var ipt *iptables.IPTables
+	var testNs ns.NetNS
 	var cleanup func()
 
 	BeforeEach(func() {
@@ -41,7 +43,7 @@ var _ = Describe("chain tests", func() {
 		currNs, err := ns.GetCurrentNS()
 		Expect(err).NotTo(HaveOccurred())
 
-		testNs, err := testutils.NewNS()
+		testNs, err = testutils.NewNS()
 		Expect(err).NotTo(HaveOccurred())
 
 		tlChainName := fmt.Sprintf("cni-test-%d", rand.Intn(10000000))
@@ -195,4 +197,38 @@ var _ = Describe("chain tests", func() {
 			}
 		}
 	})
+
+	It("deletes chains idempotently in parallel", func() {
+		defer cleanup()
+		// number of parallel executions
+		N := 10
+		var wg sync.WaitGroup
+		err := testChain.setup(ipt)
+		Expect(err).NotTo(HaveOccurred())
+		errCh := make(chan error, N)
+		for i := 0; i < N; i++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				// teardown chain
+				errCh <- testNs.Do(func(ns.NetNS) error {
+					return testChain.teardown(ipt)
+				})
+			}()
+		}
+		wg.Wait()
+		close(errCh)
+		for err := range errCh {
+			Expect(err).NotTo(HaveOccurred())
+		}
+
+		chains, err := ipt.ListChains(TABLE)
+		Expect(err).NotTo(HaveOccurred())
+		for _, chain := range chains {
+			if chain == testChain.name {
+				Fail("Chain was not deleted")
+			}
+		}
+
+	})
 })

plugins/meta/portmap/portmap_integ_test.go

@@ -96,119 +96,133 @@ var _ = Describe("portmap integration tests", func() {
 		}
 	})
 
-	// This needs to be done using Ginkgo's asynchronous testing mode.
+	Describe("Creating an interface in a namespace with the ptp plugin", func() {
-	It("forwards a TCP port on ipv4", func(done Done) {
+		// This needs to be done using Ginkgo's asynchronous testing mode.
-		var err error
+		It("forwards a TCP port on ipv4", func(done Done) {
-		hostPort := rand.Intn(10000) + 1025
+			var err error
-		runtimeConfig := libcni.RuntimeConf{
+			hostPort := rand.Intn(10000) + 1025
-			ContainerID: fmt.Sprintf("unit-test-%d", hostPort),
+			runtimeConfig := libcni.RuntimeConf{
-			NetNS:       targetNS.Path(),
+				ContainerID: fmt.Sprintf("unit-test-%d", hostPort),
-			IfName:      "eth0",
+				NetNS:       targetNS.Path(),
-			CapabilityArgs: map[string]interface{}{
+				IfName:      "eth0",
-				"portMappings": []map[string]interface{}{
+				CapabilityArgs: map[string]interface{}{
-					{
+					"portMappings": []map[string]interface{}{
-						"hostPort":      hostPort,
+						{
-						"containerPort": containerPort,
+							"hostPort":      hostPort,
-						"protocol":      "tcp",
+							"containerPort": containerPort,
+							"protocol":      "tcp",
+						},
 					},
 				},
-			},
-		}
-
-		// Make delete idempotent, so we can clean up on failure
-		netDeleted := false
-		deleteNetwork := func() error {
-			if netDeleted {
-				return nil
 			}
-			netDeleted = true
-			return cniConf.DelNetworkList(context.TODO(), configList, &runtimeConfig)
-		}
 
-		// we'll also manually check the iptables chains
+			// Make delete idempotent, so we can clean up on failure
-		ipt, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
+			netDeleted := false
-		Expect(err).NotTo(HaveOccurred())
+			deleteNetwork := func() error {
-		dnatChainName := genDnatChain("cni-portmap-unit-test", runtimeConfig.ContainerID).name
+				if netDeleted {
-
+					return nil
-		// Create the network
+				}
-		resI, err := cniConf.AddNetworkList(context.TODO(), configList, &runtimeConfig)
+				netDeleted = true
-		Expect(err).NotTo(HaveOccurred())
+				return cniConf.DelNetworkList(context.TODO(), configList, &runtimeConfig)
-		defer deleteNetwork()
-
-		// Undo Docker's forwarding policy
-		cmd := exec.Command("iptables", "-t", "filter",
-			"-P", "FORWARD", "ACCEPT")
-		cmd.Stderr = GinkgoWriter
-		err = cmd.Run()
-		Expect(err).NotTo(HaveOccurred())
-
-		// Check the chain exists
-		_, err = ipt.List("nat", dnatChainName)
-		Expect(err).NotTo(HaveOccurred())
-
-		result, err := current.GetResult(resI)
-		Expect(err).NotTo(HaveOccurred())
-		var contIP net.IP
-
-		for _, ip := range result.IPs {
-			intfIndex := *ip.Interface
-			if result.Interfaces[intfIndex].Sandbox == "" {
-				continue
 			}
-			contIP = ip.Address.IP
-		}
-		if contIP == nil {
-			Fail("could not determine container IP")
-		}
 
-		hostIP := getLocalIP()
+			// we'll also manually check the iptables chains
-		fmt.Fprintf(GinkgoWriter, "hostIP: %s:%d, contIP: %s:%d\n",
+			ipt, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
-			hostIP, hostPort, contIP, containerPort)
+			Expect(err).NotTo(HaveOccurred())
+			dnatChainName := genDnatChain("cni-portmap-unit-test", runtimeConfig.ContainerID).name
 
-		// dump iptables-save output for debugging
+			// Create the network
-		cmd = exec.Command("iptables-save")
+			resI, err := cniConf.AddNetworkList(context.TODO(), configList, &runtimeConfig)
-		cmd.Stderr = GinkgoWriter
+			Expect(err).NotTo(HaveOccurred())
-		cmd.Stdout = GinkgoWriter
+			defer deleteNetwork()
-		Expect(cmd.Run()).To(Succeed())
 
-		// Sanity check: verify that the container is reachable directly
+			// Undo Docker's forwarding policy
-		contOK := testEchoServer(contIP.String(), containerPort, "")
+			cmd := exec.Command("iptables", "-t", "filter",
+				"-P", "FORWARD", "ACCEPT")
+			cmd.Stderr = GinkgoWriter
+			err = cmd.Run()
+			Expect(err).NotTo(HaveOccurred())
 
-		// Verify that a connection to the forwarded port works
+			// Check the chain exists
-		dnatOK := testEchoServer(hostIP, hostPort, "")
+			_, err = ipt.List("nat", dnatChainName)
+			Expect(err).NotTo(HaveOccurred())
 
-		// Verify that a connection to localhost works
+			result, err := current.GetResult(resI)
-		snatOK := testEchoServer("127.0.0.1", hostPort, "")
+			Expect(err).NotTo(HaveOccurred())
+			var contIP net.IP
 
-		// verify that hairpin works
+			for _, ip := range result.IPs {
-		hairpinOK := testEchoServer(hostIP, hostPort, targetNS.Path())
+				intfIndex := *ip.Interface
+				if result.Interfaces[intfIndex].Sandbox == "" {
+					continue
+				}
+				contIP = ip.Address.IP
+			}
+			if contIP == nil {
+				Fail("could not determine container IP")
+			}
 
-		// Cleanup
+			hostIP := getLocalIP()
-		session.Terminate()
+			fmt.Fprintf(GinkgoWriter, "hostIP: %s:%d, contIP: %s:%d\n",
-		err = deleteNetwork()
+				hostIP, hostPort, contIP, containerPort)
-		Expect(err).NotTo(HaveOccurred())
 
-		// Verify iptables rules are gone
+			// dump iptables-save output for debugging
-		_, err = ipt.List("nat", dnatChainName)
+			cmd = exec.Command("iptables-save")
-		Expect(err).To(MatchError(ContainSubstring("iptables: No chain/target/match by that name.")))
+			cmd.Stderr = GinkgoWriter
+			cmd.Stdout = GinkgoWriter
+			Expect(cmd.Run()).To(Succeed())
 
-		// Check that everything succeeded *after* we clean up the network
+			// dump ip routes output for debugging
-		if !contOK {
+			cmd = exec.Command("ip", "route")
-			Fail("connection direct to " + contIP.String() + " failed")
+			cmd.Stderr = GinkgoWriter
-		}
+			cmd.Stdout = GinkgoWriter
-		if !dnatOK {
+			Expect(cmd.Run()).To(Succeed())
-			Fail("Connection to " + hostIP + " was not forwarded")
-		}
-		if !snatOK {
-			Fail("connection to 127.0.0.1 was not forwarded")
-		}
-		if !hairpinOK {
-			Fail("Hairpin connection failed")
-		}
 
-		close(done)
+			// dump ip addresses output for debugging
+			cmd = exec.Command("ip", "addr")
+			cmd.Stderr = GinkgoWriter
+			cmd.Stdout = GinkgoWriter
+			Expect(cmd.Run()).To(Succeed())
 
-	}, TIMEOUT*9)
+			// Sanity check: verify that the container is reachable directly
+			contOK := testEchoServer(contIP.String(), containerPort, "")
+
+			// Verify that a connection to the forwarded port works
+			dnatOK := testEchoServer(hostIP, hostPort, "")
+
+			// Verify that a connection to localhost works
+			snatOK := testEchoServer("127.0.0.1", hostPort, "")
+
+			// verify that hairpin works
+			hairpinOK := testEchoServer(hostIP, hostPort, targetNS.Path())
+
+			// Cleanup
+			session.Terminate()
+			err = deleteNetwork()
+			Expect(err).NotTo(HaveOccurred())
+
+			// Verify iptables rules are gone
+			_, err = ipt.List("nat", dnatChainName)
+			Expect(err).To(MatchError(ContainSubstring("iptables: No chain/target/match by that name.")))
+
+			// Check that everything succeeded *after* we clean up the network
+			if !contOK {
+				Fail("connection direct to " + contIP.String() + " failed")
+			}
+			if !dnatOK {
+				Fail("Connection to " + hostIP + " was not forwarded")
+			}
+			if !snatOK {
+				Fail("connection to 127.0.0.1 was not forwarded")
+			}
+			if !hairpinOK {
+				Fail("Hairpin connection failed")
+			}
+
+			close(done)
+
+		}, TIMEOUT*9)
+	})
 })
 
 // testEchoServer returns true if we found an echo server on the port

vendor/github.com/coreos/go-iptables/iptables/iptables.go

@@ -48,9 +48,13 @@ func (e *Error) Error() string {
 
 // IsNotExist returns true if the error is due to the chain or rule not existing
 func (e *Error) IsNotExist() bool {
-	return e.ExitStatus() == 1 &&
+	if e.ExitStatus() != 1 {
-		(e.msg == fmt.Sprintf("%s: Bad rule (does a matching rule exist in that chain?).\n", getIptablesCommand(e.proto)) ||
+		return false
-			e.msg == fmt.Sprintf("%s: No chain/target/match by that name.\n", getIptablesCommand(e.proto)))
+	}
+	cmdIptables := getIptablesCommand(e.proto)
+	msgNoRuleExist := fmt.Sprintf("%s: Bad rule (does a matching rule exist in that chain?).\n", cmdIptables)
+	msgNoChainExist := fmt.Sprintf("%s: No chain/target/match by that name.\n", cmdIptables)
+	return strings.Contains(e.msg, msgNoRuleExist) || strings.Contains(e.msg, msgNoChainExist)
 }
 
 // Protocol to differentiate between IPv4 and IPv6
@@ -101,7 +105,13 @@ func NewWithProtocol(proto Protocol) (*IPTables, error) {
 		return nil, err
 	}
 	vstring, err := getIptablesVersionString(path)
+	if err != nil {
+		return nil, fmt.Errorf("could not get iptables version: %v", err)
+	}
 	v1, v2, v3, mode, err := extractIptablesVersion(vstring)
+	if err != nil {
+		return nil, fmt.Errorf("failed to extract iptables version from [%s]: %v", vstring, err)
+	}
 
 	checkPresent, waitPresent, randomFullyPresent := getIptablesCommandSupport(v1, v2, v3)
 
@@ -348,18 +358,6 @@ func (ipt *IPTables) executeList(args []string) ([]string, error) {
 		rules = rules[:len(rules)-1]
 	}
 
-	// nftables mode doesn't return an error code when listing a non-existent
-	// chain. Patch that up.
-	if len(rules) == 0 && ipt.mode == "nf_tables" {
-		v := 1
-		return nil, &Error{
-			cmd:        exec.Cmd{Args: args},
-			msg:        fmt.Sprintf("%s: No chain/target/match by that name.\n", getIptablesCommand(ipt.proto)),
-			proto:      ipt.proto,
-			exitStatus: &v,
-		}
-	}
-
 	for i, rule := range rules {
 		rules[i] = filterRuleOutput(rule)
 	}
@@ -437,6 +435,7 @@ func (ipt *IPTables) runWithOutput(args []string, stdout io.Writer) error {
 		}
 		ul, err := fmu.tryLock()
 		if err != nil {
+			syscall.Close(fmu.fd)
 			return err
 		}
 		defer ul.Unlock()

vendor/modules.txt

@@ -32,7 +32,7 @@ github.com/containernetworking/cni/pkg/skel
 github.com/containernetworking/cni/pkg/version
 github.com/containernetworking/cni/pkg/types/020
 github.com/containernetworking/cni/libcni
-# github.com/coreos/go-iptables v0.4.2
+# github.com/coreos/go-iptables v0.4.5
 github.com/coreos/go-iptables/iptables
 # github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7
 github.com/coreos/go-systemd/activation