policy: Track policy rule labels from which map entries are derived from #10512

gandro · 2020-03-09T14:19:33Z

This PR allows us to track the policies for which a certain policy map entry has been created.

It is implemented by copying over the DerivedFromRules from the merged ingress/egress filters to the user-space representation of the policy map state. These entries are then moved over into the realizedPolicy of each endpoint when the policy maps are synced.

Since the order of the DerivedFromRules rules is not deterministic, we create a sorted copy of each LabelArrayList. Default entries such as AllowAnyIngress, AllowAnyEgress and AllowLocalHostIngress are annotated with artificial labels (of label source reserved).

Note to reviewer: This is the successor PR to #10293 - but with proper test coverage this time and hopefully addresses the feedback given in the other PR. Review per commit.

This change is

pkg/policy/mapstate.go

pkg/policy/distillery_test.go

gandro · 2020-03-09T14:20:48Z

test-me-please

coveralls · 2020-03-09T14:44:05Z

Coverage increased (+0.004%) to 45.713% when pulling 2442d14 on pr/gandro/track-source-policy-for-map-entry into a5c0488 on master.

pkg/labels/array_test.go

pkg/policy/mapstate.go

pkg/policy/l4.go

aanm · 2020-03-10T09:21:57Z

pkg/policy/l4.go

@@ -358,12 +356,13 @@ func (l4 *L4Filter) IdentitySelectionUpdated(selector CachedSelector, selections
 	// that we could not push updates on a stale policy.
 	l4Policy := (*L4Policy)(atomic.LoadPointer(&l4.policy))
 	if l4Policy != nil {
+		derivedFrom := l4.DerivedFromRules.DeepCopy().Sort()


Why do we need to:

DeepCopy()?

Sort the rules?

Can't both of these be avoided if we simply insert the labels by order when l4.DerivedFromRules is created / appended?

They need to be sorted because the insert is not deterministic (see commit message) and we need to be able to have them sorted so we can compare them here:

cilium/pkg/policy/mapstate.go

Line 114 in b50f082

return e.ProxyPort == o.ProxyPort && e.DerivedFromRules.Equals(o.DerivedFromRules)

They need to be copied to avoid accidentally sorting the original array. As an added bonus, sorting them also makes the unit tests much easier to read.

Edit: I think I slightly misread your comment. Ideally yes, we would have them sorted properly when we insert a new rule. However, given that the insert order is not deterministic, I think it's cheaper to do the sorting here instead of trying to use a heap or something.

But here we are doing a DeepCopy of the full array plus sorting it. Which operation will be performed more frequently? This one or when we insert a new label into o.DerivedFromRules?

My understanding of the policy code is still somewhat limited, so I might be wrong. But it is my understanding that inserting a new label is much more frequent, as it is called for each rule of each policy applicable to the endpoint when the endpoint is regenerated. While this code here is only called once for each endpoint regeneration.

Edit: Note that the MapStateEntry.DerivedFromRules is never modified after the entry is created here. Only L4Filter.DerivedFromRules is being modified when multiple rules are being merged.

If I'm not mistaken (/cc @jrajahalme) IdentitySelectionUpdated() can be invoked any time any new identity appears anywhere in the cluster. In large-scale clusters with constant churn, this may happen quite frequently.

I see, that's a good pointer. That's a case then where we indeed should probably sort L4Filter.DerivedFromRules once we are certain that there are no rules which can be added to the L4Filter anymore. I will have to look a bit more deeply where that place would be.

@joestringer is correct, typically policy computations are less frequent than map state generation due to identities being created either for new endpoints or newly resolved DNS names.

@gandro L4Filter.attach() is called once when the merging is over to "attach" the computed policy to the filter. Sorting in the beginning of this function is likely the right thing to do:

func (l4 *L4Filter) attach(ctx PolicyContext, l4Policy *L4Policy) { // sort here ...

Thank you all for the feedback, I think that helped me quite understand the code quite a bit better! I have pushed a new version where we sort the labels in L4Filter.attach. This also allows me to avoid having to create a copy, since this call seems to happen after any modifications to the DerivedFromRules.

`Equals` is more idiomatic than `Same` and consistent with Label.Equals. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

These functions are used to create canonical representations of the LabelArrayList that can be used to check if two LabelArrayList contain the same set of labels. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This commit allows us to track the policies for which a certain policy map entry has been created. It is implemented by copying over the `DerivedFromRules` from the merged ingress/egress filters to the user-space representation of the policy map state. These entries are then moved over into the `realizedPolicy` of each endpoint when the policy maps are synced. Since the order of the `DerivedFromRules` rules is not deterministic, sort each LabelArrayList when the computed policy is attached. Default entries such as `AllowAnyIngress`, `AllowAnyEgress` and `AllowLocalHostIngress` are annotated with artificial labels (of label source `reserved`). Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This function allows callers to get the list of policies which caused a certain policy map entry to be added for a given endpoint. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro · 2020-03-16T09:13:50Z

test-me-please

Edit: Hit https://github.com/cilium/cilium/projects/97#card-33577672

gandro · 2020-03-17T10:42:19Z

test-me-please

gandro added pending-review sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels Mar 9, 2020

gandro requested review from a team as code owners March 9, 2020 14:19

gandro requested a review from a team March 9, 2020 14:19

maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 9, 2020

houndci-bot reviewed Mar 9, 2020

View reviewed changes

gandro force-pushed the pr/gandro/track-source-policy-for-map-entry branch from 83a6de7 to b50f082 Compare March 9, 2020 14:20

aanm requested changes Mar 10, 2020

View reviewed changes

gandro force-pushed the pr/gandro/track-source-policy-for-map-entry branch from b50f082 to b883a53 Compare March 10, 2020 12:22

gandro added 4 commits March 16, 2020 09:59

labels: Rename LabelArray.Same to LabelArray.Equals

31715fa

`Equals` is more idiomatic than `Same` and consistent with Label.Equals. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

labels: Add Sort and Equals to LabelArrayList

7c69d9d

These functions are used to create canonical representations of the LabelArrayList that can be used to check if two LabelArrayList contain the same set of labels. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

endpoint: Add GetRealizedPolicyRuleLabelsForKey

2442d14

This function allows callers to get the list of policies which caused a certain policy map entry to be added for a given endpoint. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro force-pushed the pr/gandro/track-source-policy-for-map-entry branch from b883a53 to 2442d14 Compare March 16, 2020 09:05

gandro requested a review from aanm March 17, 2020 10:15

aanm approved these changes Mar 17, 2020

View reviewed changes

joestringer approved these changes Mar 18, 2020

View reviewed changes

aanm merged commit a62c369 into master Mar 18, 2020

1.8.0 automation moved this from In progress to Merged Mar 18, 2020

aanm deleted the pr/gandro/track-source-policy-for-map-entry branch March 18, 2020 09:54

gandro mentioned this pull request Apr 24, 2024

Add metadata to annotation-derived l7 policy #32166

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: Track policy rule labels from which map entries are derived from #10512

policy: Track policy rule labels from which map entries are derived from #10512

gandro commented Mar 9, 2020 •

edited by tgraf

gandro commented Mar 9, 2020

coveralls commented Mar 9, 2020 •

edited

aanm Mar 10, 2020

gandro Mar 10, 2020 •

edited

aanm Mar 10, 2020

gandro Mar 10, 2020 •

edited

joestringer Mar 10, 2020

gandro Mar 10, 2020

jrajahalme Mar 13, 2020 •

edited

gandro Mar 16, 2020

gandro commented Mar 16, 2020 •

edited

gandro commented Mar 17, 2020

policy: Track policy rule labels from which map entries are derived from #10512

policy: Track policy rule labels from which map entries are derived from #10512

Conversation

gandro commented Mar 9, 2020 • edited by tgraf

gandro commented Mar 9, 2020

coveralls commented Mar 9, 2020 • edited

aanm Mar 10, 2020

Choose a reason for hiding this comment

gandro Mar 10, 2020 • edited

Choose a reason for hiding this comment

aanm Mar 10, 2020

Choose a reason for hiding this comment

gandro Mar 10, 2020 • edited

Choose a reason for hiding this comment

joestringer Mar 10, 2020

Choose a reason for hiding this comment

gandro Mar 10, 2020

Choose a reason for hiding this comment

jrajahalme Mar 13, 2020 • edited

Choose a reason for hiding this comment

gandro Mar 16, 2020

Choose a reason for hiding this comment

gandro commented Mar 16, 2020 • edited

gandro commented Mar 17, 2020

gandro commented Mar 9, 2020 •

edited by tgraf

coveralls commented Mar 9, 2020 •

edited

gandro Mar 10, 2020 •

edited

gandro Mar 10, 2020 •

edited

jrajahalme Mar 13, 2020 •

edited

gandro commented Mar 16, 2020 •

edited