client: cleanup leaked iptables rules #15407
Conversation
shoenig
force-pushed
the
b-force-iptables
branch
from
ddcc9f1
to
ca1b229
Compare
shoenig
force-pushed
the
b-force-iptables
branch
from
ca1b229
to
e3c9854
Compare
This PR adds a secondary path for cleaning up iptables created for an allocation when the normal CNI library fails to do so. This typically happens when the state of the pause container is unexpected - e.g. deleted out of band from Nomad. Before, the iptables rules would be leaked which could lead to unexpected nat routing behavior later on (in addition to leaked resources). With this change, we scan for the rules created on behalf of the allocation being GC'd and delete them. Fixes #6385
shoenig
force-pushed
the
b-force-iptables
branch
from
e3c9854
to
99fb7d6
Compare
shoenig
changed the title
|
Should we backport this? It's like on the fence between bug / feature. |
| var ( | ||
| // ipRuleRe is used to parse a postrouting iptables rule created by nomad, e.g. | ||
| // -A POSTROUTING -s 172.26.64.191/32 -m comment --comment "name: \"nomad\" id: \"6b235529-8111-4bbe-520b-d639b1d2a94e\"" -j CNI-50e58ea77dc52e0c731e3799 | ||
| ipRuleRe = regexp.MustCompile(`-A POSTROUTING -s (\S+) -m comment --comment "name: \\"nomad\\" id: \\"([[:xdigit:]-]+)\\"" -j (CNI-[[:xdigit:]]+)`) |
There was a problem hiding this comment.
My kingdom for an iptables library that parses the rules into a sensible struct instead of returning a list of strings! 😀
| // remove the jump rule | ||
| ok := true | ||
| if err = ipt.Delete(natTable, postRoutingChain, toDel...); err != nil { | ||
| c.logger.Warn("failed to remove iptables nat.POSTROUTING rule", "alloc_id", allocID, "chain", chainID, "error", err) | ||
| ok = false | ||
| } |
There was a problem hiding this comment.
Why not return the error here? If this fails, will we ever be able to clear and delete the chain?
There was a problem hiding this comment.
Yeah I dunno, my thinking is we're already in a "just close our eyes and try deleting stuff" state. If someone ever reports an error here we'll need client logs surrounding the delete command anyway; the error alone is next to useless.
shoenig
added
backport/1.2.x
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds a secondary path for cleaning up iptables created for an allocation
when the normal CNI library fails to do so. This typically happens when the state
of the pause container is unexpected - e.g. deleted out of band from Nomad. Before,
the iptables rules would be leaked which could lead to unexpected nat routing
behavior later on (in addition to leaked resources). With this change, we scan
for the rules created on behalf of the allocation being GC'd and delete them.
Fixes #6385