-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client: cleanup leaked iptables rules #15407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:improvement | ||
client: detect and cleanup leaked iptables rules | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,12 +12,14 @@ import ( | |
"math/rand" | ||
"os" | ||
"path/filepath" | ||
"regexp" | ||
"sort" | ||
"strings" | ||
"time" | ||
|
||
cni "github.com/containerd/go-cni" | ||
cnilibrary "github.com/containernetworking/cni/libcni" | ||
"github.com/coreos/go-iptables/iptables" | ||
log "github.com/hashicorp/go-hclog" | ||
"github.com/hashicorp/nomad/nomad/structs" | ||
"github.com/hashicorp/nomad/plugins/drivers" | ||
|
@@ -226,7 +228,101 @@ func (c *cniNetworkConfigurator) Teardown(ctx context.Context, alloc *structs.Al | |
return err | ||
} | ||
|
||
return c.cni.Remove(ctx, alloc.ID, spec.Path, cni.WithCapabilityPortMap(getPortMapping(alloc, c.ignorePortMappingHostIP))) | ||
if err := c.cni.Remove(ctx, alloc.ID, spec.Path, cni.WithCapabilityPortMap(getPortMapping(alloc, c.ignorePortMappingHostIP))); err != nil { | ||
// create a real handle to iptables | ||
ipt, iptErr := iptables.New() | ||
if iptErr != nil { | ||
return fmt.Errorf("failed to detect iptables: %w", iptErr) | ||
} | ||
// most likely the pause container was removed from underneath nomad | ||
return c.forceCleanup(ipt, alloc.ID) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// IPTables is a subset of iptables.IPTables | ||
type IPTables interface { | ||
List(table, chain string) ([]string, error) | ||
Delete(table, chain string, rule ...string) error | ||
ClearAndDeleteChain(table, chain string) error | ||
} | ||
|
||
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:]]+)`) | ||
) | ||
|
||
// forceCleanup is the backup plan for removing the iptables rule and chain associated with | ||
// an allocation that was using bridge networking. The cni library refuses to handle a | ||
// dirty state - e.g. the pause container is removed out of band, and so we must cleanup | ||
// iptables ourselves to avoid leaking rules. | ||
func (c *cniNetworkConfigurator) forceCleanup(ipt IPTables, allocID string) error { | ||
const ( | ||
natTable = "nat" | ||
postRoutingChain = "POSTROUTING" | ||
commentFmt = `--comment "name: \"nomad\" id: \"%s\""` | ||
) | ||
|
||
// list the rules on the POSTROUTING chain of the nat table | ||
rules, err := ipt.List(natTable, postRoutingChain) | ||
if err != nil { | ||
return fmt.Errorf("failed to list iptables rules: %w", err) | ||
} | ||
|
||
// find the POSTROUTING rule associated with our allocation | ||
matcher := fmt.Sprintf(commentFmt, allocID) | ||
var ruleToPurge string | ||
for _, rule := range rules { | ||
if strings.Contains(rule, matcher) { | ||
ruleToPurge = rule | ||
break | ||
} | ||
} | ||
|
||
// no rule found for our allocation, just give up | ||
if ruleToPurge == "" { | ||
return fmt.Errorf("failed to find postrouting rule for alloc %s", allocID) | ||
} | ||
|
||
// re-create the rule we need to delete, as tokens | ||
subs := ipRuleRe.FindStringSubmatch(ruleToPurge) | ||
if len(subs) != 4 { | ||
return fmt.Errorf("failed to parse postrouting rule for alloc %s", allocID) | ||
} | ||
cidr := subs[1] | ||
id := subs[2] | ||
chainID := subs[3] | ||
toDel := []string{ | ||
`-s`, | ||
cidr, | ||
`-m`, | ||
`comment`, | ||
`--comment`, | ||
`name: "nomad" id: "` + id + `"`, | ||
`-j`, | ||
chainID, | ||
} | ||
|
||
// 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 | ||
} | ||
Comment on lines
+308
to
+313
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// remote the associated chain | ||
if err = ipt.ClearAndDeleteChain(natTable, chainID); err != nil { | ||
c.logger.Warn("failed to remove iptables nat chain", "chain", chainID, "error", err) | ||
ok = false | ||
} | ||
|
||
if !ok { | ||
return fmt.Errorf("failed to cleanup iptables rules for alloc %s", allocID) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (c *cniNetworkConfigurator) ensureCNIInitialized() error { | ||
|
@@ -240,7 +336,7 @@ func (c *cniNetworkConfigurator) ensureCNIInitialized() error { | |
// getPortMapping builds a list of portMapping structs that are used as the | ||
// portmapping capability arguments for the portmap CNI plugin | ||
func getPortMapping(alloc *structs.Allocation, ignoreHostIP bool) []cni.PortMapping { | ||
ports := []cni.PortMapping{} | ||
var ports []cni.PortMapping | ||
|
||
if len(alloc.AllocatedResources.Shared.Ports) == 0 && len(alloc.AllocatedResources.Shared.Networks) > 0 { | ||
for _, network := range alloc.AllocatedResources.Shared.Networks { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My kingdom for an iptables library that parses the rules into a sensible struct instead of returning a list of strings! 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏