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

@@ -70,7 +70,10 @@ 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 {
+	err := ipt.ClearChain(c.table, c.name)
+	eerr, eok := err.(*iptables.Error)
+	// #418 don't fail if the error is because the chain doesn't exist
+	if err != nil && !eerr.IsNotExist() {
 		return err
 	}
 
@@ -97,10 +100,15 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 		}
 	}
 
-	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,8 @@ import (
 	"fmt"
 	"math/rand"
 	"runtime"
+	"sync"
+	"time"
 
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/testutils"
@@ -32,6 +34,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 +44,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 +198,48 @@ 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)
+			}()
+			// wait 1 ms until next execution
+			time.Sleep(1 * time.Millisecond)
+		}
+		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")
+			}
+		}
+
+	})
 })