Skip to content

Commit

Permalink
Portmap doesn't fail if chain doesn't exist
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
aojea committed Dec 9, 2019
1 parent 6551165 commit d8e28fc
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 3 deletions.
14 changes: 11 additions & 3 deletions plugins/meta/portmap/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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.
Expand Down
32 changes: 32 additions & 0 deletions plugins/meta/portmap/chain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,36 @@ var _ = Describe("chain tests", func() {
}
}
})

It("deletes chains idempotently in parallel", func() {
defer cleanup()
// number of parallel executions
N := 10
err := testChain.setup(ipt)
Expect(err).NotTo(HaveOccurred())

errCh := make(chan error, N)
for i := 0; i < N; i++ {
go func() {
errCh <- testChain.teardown(ipt)
}()
}
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")
}
}

})
})

0 comments on commit d8e28fc

Please sign in to comment.