diff --git a/plugins/meta/portmap/chain.go b/plugins/meta/portmap/chain.go index 8e8dbe3fa..968f03ed3 100644 --- a/plugins/meta/portmap/chain.go +++ b/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. diff --git a/plugins/meta/portmap/chain_test.go b/plugins/meta/portmap/chain_test.go index 93e4be13e..98034201c 100644 --- a/plugins/meta/portmap/chain_test.go +++ b/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") + } + } + + }) })