ensure iptables chain creation is idempotent #408
Conversation
tgross
force-pushed
the
idempotent_chain_creation
branch
from
e0109e3 to
163447f
Compare
|
While it wasn't the intent of this PR, this may fix #370, which is bubbling up a similar error condition as we saw in hashicorp/nomad#6567 |
plugins/meta/firewall/iptables.go
Outdated
| @@ -43,7 +43,16 @@ func ensureChain(ipt *iptables.IPTables, table, chain string) error { | |||
| } | |||
| } | |||
|
|
|||
| return ipt.NewChain(table, chain) | |||
| err = ipt.NewChain(table, chain) | |||
There was a problem hiding this comment.
LGTM for your intention and implementation, but IMO, idempotency should be done in iptables package because decoupling is important for us... cc @squeed waiting for your suggestion...
There was a problem hiding this comment.
It could be done there, sure. But the iptables package is a fairly bare-bones wrapper around the command line tools. They also don't have any documentation promising idempotency, whereas containernetworking/plugins does.
tgross
force-pushed
the
idempotent_chain_creation
branch
from
163447f to
3d0ddef
Compare
tgross
force-pushed
the
idempotent_chain_creation
branch
2 times, most recently
from
0d001e3 to
81426da
Compare
|
I think at this point I've resolved the items from code review, but I think there's still the open question to @squeed about whether this should be handled upstream in go-iptables instead. |
Concurrent use of the `portmap` and `firewall` plugins can result in errors during iptables chain creation: - The `portmap` plugin has a time-of-check-time-of-use race where it checks for existence of the chain but the operation isn't atomic. - The `firewall` plugin doesn't check for existing chains and just returns an error. This commit makes both operations idempotent by creating the chain and then discarding the error if it's caused by the chain already existing. It also factors the chain creation out into `pkg/utils` as a site for future refactoring work. Signed-off-by: Tim Gross <tim@0x74696d.com>
tgross
force-pushed
the
idempotent_chain_creation
branch
from
81426da to
58dd90b
Compare
|
/lgtm |
|
lgtm! |
Concurrent use of the
portmapandfirewallplugins can result in errors during iptables chain creation:portmapplugin has a time-of-check-time-of-use race where it checks for existence of the chain but the operation isn't atomic.firewallplugin doesn't check for existing chains and just returns an error.This commit makes both operations idempotent by creating the chain and then discarding the error if it's caused by the chain already existing. I've added a test for idempotent creation in the
firewallplugin but theportmapplugin already had one so I've left that in place.