Group common sub-sequences of policies into shared iptables chains #8098

fasaxc · 2023-10-10T10:03:58Z

Description

Group policies in iptables chains based on the selectors used in the policies.
For example, if there are three policies in a row that use the same selector,
put them in a group.
Render each group as its own chain, named using a hash of the group contents.
Share groups chains between endpoints that use the same group.
Change iptables refcounting to be recursive so that it can handle the new intermediate chain and still avoid programming unused policy chains.

Related issues/PRs

CORE-10015

Todos

Tests
Documentation
Release note

Release Note

Felix now breaks up "policy jump rules" into new iptables "policy group chains" by selector.  If two endpoints share a common sequence of policies they will share the same group chain, which reduces the number of rules that need to be programmed.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

- Recalculate groups on seelctor change. - Embed direction in group so that we get unique groups for each direction.

Combine ClearMark rules into one.

Various fixes.

Instead of refcounting blindly, trigger incref of children only when the chain itself becomes referenced. Trigger decref only for referenced chains. Only invalidate dataplane if changing a referenced chain.

nelljerram

Some small points, but all up to you and I don't think I will need to re-review. Please LMK if any of the points I've mentioned are not clear.

nelljerram · 2023-12-18T16:42:17Z

felix/proto/felixbackend.proto

@@ -244,6 +244,8 @@ message Policy {
  repeated Rule outbound_rules = 2;
  bool untracked = 3;
  bool pre_dnat = 4;
+
+  string original_selector = 6;


(Assume the numbering here is because there's an = 5 field in Enterprise.)

5 is used above for namespace.

nelljerram · 2023-12-18T16:49:38Z

felix/rules/policy.go

+		} else {
+			break
+		}
+	}


It would be marginally nicer if the coding here made it clear that these rules could not be further appended to. I'm aware that that is in fact the case - by looking at where ProtoRulesToIptablesRules is used - but I wonder if it might be worth changing the signature so that this function here returns the entire &iptables.Chain?

I coded this up but it made the tests uglier. Right now they can treat this as simple one-in-one-out function. Returning a chain means we have to pass in a chain name and then unpack the rules to test them.

nelljerram · 2023-12-18T16:55:21Z

felix/rules/rule_defs.go

+const (
+	PolicyDirectionIngress PolicyDirection = "ingress"
+	PolicyDirectionEgress  PolicyDirection = "egress"
+)


I'm amazed we don't have a definition like this somewhere already!

I couldn't find one!

Have changed these to match the terminology used in this package inbound/outbound

nelljerram · 2023-12-18T17:04:11Z

felix/rules/endpoints.go

@@ -254,14 +258,14 @@ func (r *DefaultRuleRenderer) HostEndpointToRawChains(

 func (r *DefaultRuleRenderer) HostEndpointToMangleIngressChains(
 	ifaceName string,
-	preDNATPolicyNames []string,
+	preDNATPolicis []*PolicyGroup,


sp, should be "...ies"

nelljerram · 2023-12-18T17:09:12Z

felix/rules/endpoints.go

+	polChainPrefix := PolicyInboundPfx
+	if group.Direction == PolicyDirectionEgress {
+		polChainPrefix = PolicyOutboundPfx
+	}


For host endpoints, are the "i" and "o" prefixes here the same way round as they currently are for policies applying to a host endpoint? (Might be a bit confusing if not.)

Oh, and same question (albeit not specifically this bit of code) for the policy group "gi" and "go" prefixes.

The pi and gi chains line up, as do the po and go chains. I.e. gi chains are groups of pi policies. Policy inbound/outbound aligns with the policy model direction. It's the to/from endpoint chains that are flipped between HEPs and WEPs.

nelljerram · 2023-12-18T18:03:25Z

felix/rules/endpoints.go

+	write(strconv.Itoa(len(g.PolicyNames)))
+	for _, name := range g.PolicyNames {
+		write(name)
+	}


Can you comment (and remind me) why we include tier and selector here, as well as the direction and policy names? If we had the same group in different tiers, couldn't we reuse the same group?

Tier: needed (in enterprise) to disambiguate tierA/policy1 vs tierB/policy1; different policies with the same policy name.

Direction and Selector I found useful to make the object self describing.

Tier needs to be in the hash. Direction and selector aren't strictly needed in the hash (I think) but I was finding it easier to prove to myself that all bases were covered if I included them rather than try to prove tat it works without them In an earlier iteration I didn't have the gi/go prefix, so the direction was needed.

nelljerram · 2023-12-19T12:13:45Z

felix/dataplane/linux/endpoint_mgr.go

+	ifaceNameToPolicyGroupChainNames map[string][]string /*chain name*/
+
+	activePolicySelectors map[proto.PolicyID]string
+	policyChainRefCounts  map[string] /*chain name*/ int


Worry that the separation of int here will be confusing.

nelljerram · 2023-12-19T12:23:18Z

felix/dataplane/linux/endpoint_mgr.go

+								break hepLoop
+							}
+						}
+					}


Possibly worth a common utility definition here:

tierUsesDirtyPolicy := func(t *proto.TierInfo) bool { for _, pols := range [][]string{t.IngressPolicies, t.EgressPolicies} { for _, p := range pols { polID := proto.PolicyID{ Tier: t.Name, Name: p, } if m.dirtyPolicyIDs.Contains(polID) { return true } } } return false }

nelljerram · 2023-12-19T12:34:37Z

felix/dataplane/linux/endpoint_mgr.go

@@ -975,6 +1080,11 @@ func (m *endpointManager) updateHostEndpoints() {
 		}
 	}

+	ifaceNameToPolicyGroups := map[string][]*rules.PolicyGroup{}


I'm not yet entirely following what this is for. Does it matter that different subsets of these groups will need to be programmed in different iptables tables?

Oh, I see, the groups are going to be unconditionally programmed in all tables. I guess that's OK, albeit a touch inelegant.

Ah yes, I just copied the old inelegant solution for policies. Could do a bit better here because we do know which groups go in which tables; just seemed like the number of tracking maps would explode a bit

I would personally be fine with leaving this to a future tidy up, as this PR is already a good unit on its own.

nelljerram · 2023-12-19T12:44:20Z

felix/dataplane/linux/endpoint_mgr.go

+		}],
+	}
+	groups := []*rules.PolicyGroup{group}
+	for _, name := range names[1:] {


Is names[1:] ok if names only has 1 entry?

Yes, I double checked.

…rojectcalico#8098) Group policies in iptables chains based on the selectors used in the policies. For example, if there are three policies in a row that use the same selector, put them in a group. - Add protobuf field for policy selector pass through from calc graph. - Add policy group calculation to endpoint manager. Does refcounting to manage shared policy group chains. - Add policy group chain rendering to rule renderer. Take advantage of extra indirection to implement pass rules more efficiently (return on pass or accept). - Make iptables reference counting recursive. Instead of refcounting blindly, trigger incref of children only when the chain itself becomes referenced. Trigger decref only for referenced chains. Only invalidate dataplane if changing a referenced chain. CORE-10015

marvin-tigera added this to the Calico v3.27.0 milestone Oct 10, 2023

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Oct 10, 2023

mgleung modified the milestones: Calico v3.27.0, Calico v3.28.0 Nov 15, 2023

fasaxc force-pushed the policy-grouping branch 2 times, most recently from bd5397a to 663aac1 Compare December 6, 2023 15:24

fasaxc changed the title ~~[WIP] Policy grouping~~ [WIP] Group common sub-sequences of policies into shared iptables chains Dec 6, 2023

fasaxc added 6 commits December 15, 2023 13:48

Add protobuf field for policy selector.

745e115

WiP on grouping policies into chains.

4f676f4

Remove spammy info log.

03807af

Various fixes/improvements.

bd76097

- Recalculate groups on seelctor change. - Embed direction in group so that we get unique groups for each direction.

Get existing tests passing.

018210c

Add policy groups ot HEPs.

67894db

fasaxc force-pushed the policy-grouping branch from db1c91a to 67894db Compare December 15, 2023 13:50

fasaxc added 3 commits December 15, 2023 14:08

Cleanups.

bd801a8

UTs for policy group rules.

11244a0

Combine ClearMark rules into one.

Remove unneccessary types.

b3b5a9a

fasaxc force-pushed the policy-grouping branch from 7db0210 to 0a95fd4 Compare December 15, 2023 17:31

UT for policy group rule rendering.

703c1cc

Various fixes.

fasaxc force-pushed the policy-grouping branch from 0a95fd4 to 703c1cc Compare December 18, 2023 09:56

Tweak iptables logging/validation to reflect ref counting.

ec98062

fasaxc force-pushed the policy-grouping branch from dd2b132 to eeb4b46 Compare December 18, 2023 11:33

Make iptables reference counting recursive.

b360b6a

Instead of refcounting blindly, trigger incref of children only when the chain itself becomes referenced. Trigger decref only for referenced chains. Only invalidate dataplane if changing a referenced chain.

fasaxc force-pushed the policy-grouping branch from eeb4b46 to b360b6a Compare December 18, 2023 14:39

fasaxc added docs-not-required Docs not required for this change and removed docs-pr-required Change is not yet documented labels Dec 18, 2023

fasaxc added 3 commits December 18, 2023 15:07

Markups.

6a8bbc9

Fix policy grouping alisaing bug and add UT.

4128d52

UT for WEP policy chain grouping.

a50b023

fasaxc added 5 commits December 19, 2023 09:55

Fix HEP marking on policy change. Add tests.

da9f13b

Fix HEP marking on policy change. Add tests.

c45a3ca

Fix formating.

6367438

Fix policy sync tests, broken by new field.

779f2fe

Fix lint.

5638ba5

fasaxc changed the title ~~[WIP] Group common sub-sequences of policies into shared iptables chains~~ Group common sub-sequences of policies into shared iptables chains Dec 19, 2023

fasaxc marked this pull request as ready for review December 19, 2023 11:59

fasaxc requested a review from a team as a code owner December 19, 2023 11:59

nelljerram approved these changes Dec 19, 2023

View reviewed changes

Code review markups.

d7d5d78

fasaxc merged commit e83ae81 into projectcalico:master Dec 20, 2023
2 checks passed

fasaxc mentioned this pull request Dec 20, 2023

Add FV for policy grouping. #8363

Merged

3 tasks

fasaxc mentioned this pull request Feb 12, 2024

Fix that PolicyGroup UID wasn't cached. #8502

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group common sub-sequences of policies into shared iptables chains #8098

Group common sub-sequences of policies into shared iptables chains #8098

fasaxc commented Oct 10, 2023 •

edited

Loading

nelljerram left a comment

nelljerram Dec 18, 2023

fasaxc Dec 19, 2023

nelljerram Dec 18, 2023

fasaxc Dec 19, 2023

nelljerram Dec 18, 2023

fasaxc Dec 19, 2023

fasaxc Dec 19, 2023

nelljerram Dec 18, 2023

nelljerram Dec 18, 2023

nelljerram Dec 18, 2023

fasaxc Dec 19, 2023

nelljerram Dec 18, 2023

fasaxc Dec 19, 2023

nelljerram Dec 19, 2023

nelljerram Dec 19, 2023

nelljerram Dec 19, 2023

nelljerram Dec 19, 2023

fasaxc Dec 19, 2023

nelljerram Dec 19, 2023

nelljerram Dec 19, 2023

fasaxc Dec 19, 2023

Group common sub-sequences of policies into shared iptables chains #8098

Group common sub-sequences of policies into shared iptables chains #8098

Conversation

fasaxc commented Oct 10, 2023 • edited Loading

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasaxc commented Oct 10, 2023 •

edited

Loading