Portmap doesn't fail if chain doesn't exist

It turns out that the portmap plugin is not idempotent if its
executed in parallel.
The errors are caused due to a race of different instantiations
deleting the chains.
This patch does that the portmap plugin doesn't fail if the
errors are because the chain doesn't exist on teardown.

xref: containernetworking#418

Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>

plugins/meta/portmap/chain.go

@@ -71,7 +71,14 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 	// 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 {
-		return err
+		eerr, eok := err.(*iptables.Error)
+		switch {
+		case eok && eerr.IsNotExist():
+			// swallow here, the chain was already deleted
+			return nil
+		default:
+			return err
+		}
 	}
 
 	for _, entryChain := range c.entryChains {
@@ -91,16 +98,28 @@ 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 {
-					return fmt.Errorf("Failed to delete referring rule %s %s: %v", c.table, entryChainRule, err)
+					eerr, eok := err.(*iptables.Error)
+					switch {
+					case eok && eerr.IsNotExist():
+						// swallow here, the chain was already deleted
+						continue
+					default:
+						return fmt.Errorf("Failed to delete referring rule %s %s: %v", c.table, entryChainRule, err)
+					}
 				}
 			}
 		}
 	}
 
-	if err := ipt.DeleteChain(c.table, c.name); err != nil {
+	err := ipt.DeleteChain(c.table, c.name)
+	eerr, eok := err.(*iptables.Error)
+	switch {
+	case eok && eerr.IsNotExist():
+		// #418 don't fail if the chain doesn't exist
+		return nil
+	default:
 		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,46 @@ 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()
+				// Use the testing namespace
+				runtime.LockOSThread()
+				defer runtime.UnlockOSThread()
+				err = testNs.Set()
+				Expect(err).NotTo(HaveOccurred())
+				// teardown chain
+				errCh <- testChain.teardown(ipt)
+			}()
+		}
+		wg.Wait()
+		close(errCh)
+		errs := 0
+		for err := range errCh {
+			if err != nil {
+				errs++
+			}
+		}
+
+		Expect(errs).Should(BeNumerically("==", 0))
+
+		chains, err := ipt.ListChains(TABLE)
+		Expect(err).NotTo(HaveOccurred())
+		for _, chain := range chains {
+			if chain == testChain.name {
+				Fail("Chain was not deleted")
+			}
+		}
+
+	})
 })