Support for loading all rules in one command

[caseydavenport]

I've been working with knftables for prototyping Calico's nftables network policy implementation and have found it innefficient to list rules from each chain individually, where Calico usually wants to cache the full set of rules to more effeciently calculate diffs in future updates.

This change allows calls to ListRules without specifying a chain name to return the full set of rules in the table in a single call.

[caseydavenport]

@danwinship @aojea hoping to get your thoughts on this one. I could also reasonably see this functionality being exposed through a different function on the interface (e.g., ListTable) if that's more appealing.

[aojea]

this seems a bit of a stretch for an API, since now it list chains and rules based on a special input

I'm not quite familiar with the implementation, @danwinship should know better, but does the other generic methods in this file List(ctx context.Context, objectType string) ([]string, error) or ListElements do not solve the problem?
If not, should we add a new method?

[danwinship]

this seems a bit of a stretch for an API

The List APIs are all kind of a mess anyway and at some point I want to redo all of them.

Though part of the reason I haven't yet is because I keep ending up not actually needing to use ListRules anyway, and feel like maybe it's an antipattern for nftables usage? (At least sometimes... it's generally better in nftables is to have mostly-static rules and dynamic sets/maps, and in that case, there's no need to do partial updates to the rules because there aren't many of them; if you think they're wrong, just overwrite all of them. Of course, kube-proxy doesn't currently live up to this ideal; it has static rules for the top-level services chain, but still has dynamic rules per-endpoint.)

But anyway, I don't object to this patch

Calico usually wants to cache the full set of rules to more effeciently calculate diffs in future updates.

Right. IPTables kube-proxy sort of does this too (nftables kube-proxy doesn't yet), but it doesn't bother with actually looking at the rules, it just computes diffs between "what I assume the rules must look like based on my previous state" and "what I want the rules to look like based on my current state", and does a partial update based on that, and if it fails, it just overwrites everything, ensuring the expected state will be correct next time. I'm expecting to do it the same way in the nftables backend.

(Of course, part of the reason iptables does it that way is because iptables-save is almost as slow as iptables-restore, so rewriting all of the rules from scratch isn't really any slower than doing a list followed by a small incremental update. I'm not sure how the timing works out in the nftables case.)

The other reason for not listing the rules is because it's sometimes hard to match up the rules as returned from nft list with the rules you actually created, whether due to version skew, canonical representation, nft optimizing your rules for you, etc. Though given how ListRules currently works in knftables I guess you're not looking at the actual rule text at all? (Though, on that note, would this be useful to you?)

[danwinship]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caseydavenport, danwinship

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[caseydavenport]

@danwinship thanks!

Though given how ListRules currently works in knftables I guess you're not looking at the actual rule text at all?

Yeah, that's right - Calico hashes its own "canonical" representation of the rule and stores that in a comment, so it can read back the comments and compare hashes to determine if a rule matches what it expects without caring that the actual rule text returned may not match.

and feel like maybe it's an antipattern for nftables usage

Yep, one of the things on my roadmap is to leverage maps more but for now I've largely been porting existing iptables patterns in order to get an MVP working quicker.

(Though, on that note, would this be useful to you?)

I am not immediately aware of a reason Calico would need that, but will let you know if that changes :)