Implement the match logic #932

fedepaol · 2020-11-02T18:17:02Z

What this PR does / why we need it:

Implement both a Matches function and an AppliesTo function that checks if the given object matches a Match / ApplyTo.
This tells if a given mutation will need to be applied to a given object.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #929

Special notes for your reviewer:

Built on top of
#926
#916

The last commit is the relevant one, namely with match.go / match_test.go files.

This assumes that the Match structure will need to be changed according to https://github.com/open-policy-agent/gatekeeper/pull/916/files#r516071473

maxsmythe · 2020-11-03T04:05:08Z

Thanks! I'll take a look at this once we merge some of the other PRs, it's getting a bit hard to track which diff belongs to which PR.

fedepaol · 2020-11-04T08:58:15Z

Thanks! I'll take a look at this once we merge some of the other PRs, it's getting a bit hard to track which diff belongs to which PR.

Actually (not asking to), but you could do by reviewing only the last commit d6cff6b

maxsmythe · 2020-11-05T01:44:32Z

oh, nice! I'll take a look

maxsmythe

LGTM @ritazh @shomron thoughts?

fedepaol · 2020-11-11T10:12:00Z

@maxsmythe @ritazh @shomron rebased, ready for 👀

maxsmythe

Thanks! Some comments around the label selector logic

maxsmythe · 2020-11-12T02:06:41Z

pkg/mutation/match.go

+	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


nit: merge import blocks

maxsmythe · 2020-11-12T02:10:49Z

pkg/mutation/match.go

+			return false, nil
+		}
+	}
+	if match.LabelSelector != nil {


Should we also match the label selector against old object?

It seems weird, but that's how MutatingWebhookConfiguration's logic works:

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#mutatingwebhook-v1-admissionregistration-k8s-io

I assumed it was only against the old object, as it's the criteria for applying a given mutation.

Given all the stability premises and the fact that we are only adding (and not changing) labels, I think it's safe to assume that if the old object matches the selector, the new one will. If only the new one will, it will be a matter of another round of mutation before the stability.

I agree the old object/new object premise doesn't really work for mutation. It's a bit weird to be deviating from K8s convention and what we do with admission control, but I think it makes sense.

@shomron @ritazh @sozercan @brycecr thoughts?

This is the last bit, I think apart from this discussion this is good to go.

Adding the above people to the review, hopefully that kick-starts the discussion.

+1 on not deviating from K8s mutatingwebhookconfig logic to avoid surprises to users

we are only adding (and not changing) labels

what if users want to match label on the old object then mutate the object to delete that label? is that a supported scenario for MVP?

Just to make sure I understood.
Here defining "the old" vs "the new" is a bit tricky, as we apply a bunch of mutations at the same time, and we can't just refer to the old as "before all mutations" and new as "after all mutations" because the decision of whether applying a mutation or not is based on the match logic. I think what we do here is to test only against "the old" object, with respect to the single mutation.

See

gatekeeper/pkg/mutation/system.go

Line 85 in 624bfcc

if m.Matches(obj, ns) {

In that case, what is referred as old vs new? And what should happen if we find out that after a mutation the object does not match anymore?
Is it fine to assume that we match against the old, we mutate and then we "rollback" the mutation (i.e restoring a backup of the object) if match of the single mutation fails afterwards?

Re: deleting labels, this is not possible with the current design as we are only adding non-existing labels / annotations. This is needed for making sure the mutation converges.

Given all the reasons above, +1 on not testing OldObject for now

Re: deleting labels, this is not possible with the current design as we are only adding non-existing labels / annotations. This is needed for making sure the mutation converges.

Thanks!

Lemme make sure I'm getting this straight:
If we also match against newObject, then, if a mutation were to add a label to a resource, then all instances of that resource type would match. The other angle on that would be that you couldn't add a label to a subset of objects of one GVK by selecting by label.

I think I don't grok the purpose of applying to both old and new objects unless you allow the mutation webhook to create or delete objects. It seems unexpected at best.

I'm in favor of shirking the k8s standard here and doing what makes more common sense, but I suspect I am missing something, too.

One point of confusion is OldObject has a specific meaning, defined here:

https://github.com/kubernetes/api/blob/4a626d306b987a4096cf0784ec01af1be2f6d67f/admission/v1beta1/types.go#L101-L106

I don't think that changes much, if any, of the above analysis, but has been the source of a bit of confusion.

maxsmythe · 2020-11-12T02:13:30Z

pkg/mutation/match.go

+		}
+	}
+
+	if match.NamespaceSelector != nil {


For MutatingWebhookConfigurations:

Cluster scoped resources always match an NS selector

If the object itself is an NS, the namespace selector is evaluated against the object's labels

Unclear whether the old object + new object matching logic also applies here

The namespace remains the same, so there won't be difference in new vs old.

Implemented the first two points and added tests.

The namespace would remain the same unless the object itself is a namespace.

Adding a Match function that accepts a Match instance, an object to be matched against and its related namespace. Adding a AppliesTo function that accepts a ApplyTo slice and an object to be matched against. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol · 2020-11-25T09:41:44Z

Rebased and integrated the two mutators' Matches method with the Matches function.
Also, changed the Matches() method of the mutator interface to accept runtime.Object as metav1.Object do not have kind / group accessors.

maxsmythe

LGTM

@shomron @ritazh @sozercan PTAL

maxsmythe · 2020-11-25T19:11:24Z

pkg/mutation/match.go

+			return false, nil
+		}
+	}
+	if match.LabelSelector != nil {


See the notes in the discussion on the meeting minutes:

https://docs.google.com/document/d/1A1-Q-1OMw3QODs1wT6eqfLTagcGmgzAJAjJihiO3T48/edit#heading=h.5gv5xh7vxp5i

TL;DR my opinion is we not test OldObject for mutation for now, and re-visit the question before graduating from alpha, so we can make this decision based off real usage data.

Each mutator matches uses the Matches function to check if the given object matches the mutator. This also includes a change in the mutator interface as metav1.Object do not have accessors for kind / group. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

codecov-io · 2020-11-27T22:13:37Z

Codecov Report

Merging #932 (55aaaff) into master (964864d) will increase coverage by 0.86%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##           master     #932      +/-   ##
==========================================
+ Coverage   46.99%   47.86%   +0.86%     
==========================================
  Files          58       59       +1     
  Lines        3541     3623      +82     
==========================================
+ Hits         1664     1734      +70     
- Misses       1668     1678      +10     
- Partials      209      211       +2

Flag	Coverage Δ
unittests	`47.86% <80.95%> (+0.86%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/mutation/assign_mutator.go	`54.71% <0.00%> (-4.47%)`	⬇️
pkg/mutation/assignmeta_mutator.go	`61.90% <0.00%> (-6.52%)`	⬇️
pkg/mutation/mutation.go	`100.00% <ø> (ø)`
pkg/mutation/mutator.go	`77.77% <ø> (ø)`
pkg/mutation/match.go	`91.89% <91.89%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`55.70% <0.00%> (-0.66%)`	⬇️
pkg/readiness/ready_tracker.go	`69.81% <0.00%> (+0.72%)`	⬆️
pkg/readiness/object_tracker.go	`79.14% <0.00%> (+1.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 964864d...55aaaff. Read the comment docs.

fedepaol · 2020-11-30T17:50:41Z

Added this last change https://github.com/open-policy-agent/gatekeeper/compare/379dc934fa7a12f7a4a99f080aa3c211a5a5197f..55aaaffc6c91e4ce33787d62e55d26be8a416251

Apart from this, based on the previous discussions I think this one is ready to go.

ritazh · 2020-11-30T23:11:21Z

pkg/mutation/match.go

+		}
+		for _, g := range apply.Versions {
+			if g == gvk.Version {
+				matchesVersion = true


how do we handle when there are multiple versions of the same group kind in the cluster? this here would resolve to mismatch but the apiserver would consider it a match.

applyTo should be a strict check, because mutations create implicit schema.

Note that this check is for the mutation.spec.applyTo field, not spec.match.kinds

Not sure I got the point.
ApplyTo allows to specify multiple versions, which would then match (or, apply) to different versions of the same group.

Gotcha! Let's make sure we document this. To support multiple versions of the same schema, user needs to add another gvk to the applyTo list in the mutation object.

ritazh

lgtm

fedepaol changed the title ~~Matchlogic~~ Implement the match logic Nov 2, 2020

maxsmythe approved these changes Nov 5, 2020

View reviewed changes

fedepaol force-pushed the matchlogic branch from d6cff6b to 555006e Compare November 11, 2020 10:10

maxsmythe requested changes Nov 12, 2020

View reviewed changes

fedepaol force-pushed the matchlogic branch 2 times, most recently from f24dfd1 to a22459c Compare November 12, 2020 17:04

maxsmythe requested review from brycecr, ritazh, shomron and sozercan November 20, 2020 10:33

mmirecki mentioned this pull request Nov 20, 2020

Write Mutation function for AssignMetadata #937

Merged

fedepaol force-pushed the matchlogic branch from a22459c to 379dc93 Compare November 25, 2020 09:33

maxsmythe approved these changes Nov 25, 2020

View reviewed changes

fedepaol force-pushed the matchlogic branch from 379dc93 to 55aaaff Compare November 27, 2020 22:11

ritazh reviewed Nov 30, 2020

View reviewed changes

ritazh approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into matchlogic

5ca2951

maxsmythe merged commit 0af9133 into open-policy-agent:master Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the match logic #932

Implement the match logic #932

fedepaol commented Nov 2, 2020

maxsmythe commented Nov 3, 2020

fedepaol commented Nov 4, 2020

maxsmythe commented Nov 5, 2020

maxsmythe left a comment

fedepaol commented Nov 11, 2020

maxsmythe left a comment

maxsmythe Nov 12, 2020

maxsmythe Nov 12, 2020

fedepaol Nov 12, 2020

maxsmythe Nov 13, 2020

fedepaol Nov 19, 2020

maxsmythe Nov 20, 2020

ritazh Nov 25, 2020

fedepaol Nov 25, 2020

ritazh Nov 25, 2020

brycecr Nov 25, 2020

maxsmythe Nov 25, 2020

maxsmythe Nov 12, 2020

fedepaol Nov 12, 2020

fedepaol Nov 12, 2020

maxsmythe Nov 20, 2020

fedepaol commented Nov 25, 2020

maxsmythe left a comment

maxsmythe Nov 25, 2020

codecov-io commented Nov 27, 2020

fedepaol commented Nov 30, 2020

ritazh Nov 30, 2020

maxsmythe Nov 30, 2020

fedepaol Nov 30, 2020

ritazh Nov 30, 2020

ritazh left a comment

Implement the match logic #932

Implement the match logic #932

Conversation

fedepaol commented Nov 2, 2020

maxsmythe commented Nov 3, 2020

fedepaol commented Nov 4, 2020

maxsmythe commented Nov 5, 2020

maxsmythe left a comment

Choose a reason for hiding this comment

fedepaol commented Nov 11, 2020

maxsmythe 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

fedepaol commented Nov 25, 2020

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 27, 2020

Codecov Report

fedepaol commented Nov 30, 2020

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

ritazh left a comment

Choose a reason for hiding this comment