Mutation Objects #916

yanirq · 2020-10-28T11:42:56Z

What this PR does / why we need it:
Provided CRDs to support mutation: Assign and AssignMetadata CRDs
The PR includes CRDs + generated artifacts.

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 #907

Special notes for your reviewer:
deploy-mutation might be necessary in this PR to deploy mutation specific resources (using an additional overlay to kustomize)

maxsmythe

This is looking good! I left a few comments, though I realize this is WIP.

apis/mutations/v1alpha1/assign_types.go

maxsmythe · 2020-10-29T02:50:14Z

apis/mutations/v1alpha1/assign_types.go

+type Parameters struct {
+	PathTests []PathTests `json:"pathTests,omitempty"`
+	// IfIn Only mutate if the current value is in the supplied list
+	IfIn []string `json:"ifIn,omitempty"`


Do we want this to be a list of interface{}?

That would allow for testing more complex objects as well as unknown types (string vs integer etc.)

That being said, coercing the types is much less ambiguous, and numbers might need other comparators anyway (greaterThan, for example).

Personally I'm split. Starting with string seems safer, as we can always expand the set of valid types over time.

I'd go with the safer path for starters.

maxsmythe · 2020-10-29T02:51:33Z

apis/mutations/v1alpha1/assign_types.go

+	// IfNotIn Only mutate if the current value is NOT in the supplied list
+	IfNotIn []string `json:"ifNotIn,omitempty"`
+	// Value to assign
+	Value string `json:"value,omitempty"`


Value needs to be of type interface{} so users can assign arbitrary values to arbitrary points in an object's schema.

Changed Value type to runtime.RawExtension if that answers the way we add objects by definition
(both on assign_types.go and assignmetadata_types.go)

also - interface{} does not seem to be supported by controller-gen

I don't think we can use RawExtension as it looks like it expects to be wrapping an object:

https://godoc.org/k8s.io/apimachinery/pkg/runtime#RawExtension

How does controller-gen break if we try to use interface{}?

I played around a bit, it looks like json.RawMessage might work?

How does controller-gen break if we try to use interface{}?

See kubernetes-sigs/controller-tools#294 (comment)
The suggestion there is indeed use either json.RawMessage or RawExtension.

@fedepaol @mmirecki have you encounter an issue on the POC or current PRs with processing one of these types ? can RawExtension option to assign a full fledged sub-object meet our needs ?

We want to be able to pass both a full fledged object (i.e. for the sidecar injection example) or a scalar value (i.e. when changing the image pull policy).

This drives two different things:

What to put in the CRD manifest

How to map it in golang

Here we want a field that can be both an object, a scalar value (bool, integer, etc) or an array.
If we declare the field to be an object in the CRD, a string won't be accepted: Invalid value: "string": spec.foo in body must be of type object: "string" .

The option we have (I believe) is to use something like anyOf for alternative typed properties, or go with the object way and expect it to have a fixed property like "value: foo" in case the path we are aiming at is not an object (but this last one feels clunky).

The way we declare the CRD drives the go struct, having a single field to be either a json or a rawextensions (both would work), or a serie of typed alternative fields (with one of them being an object).

@maxsmythe @yanirq thoughts?

I don't think there's a way to use anyOf in controller-gen, which is unfortunate. The sub-object route seems like it would work.

What if we changed spec.parameters.value to be spec.parameters.assign.value

We could then have something like:

Parameters struct { // +kubebuilder:validation:XPreserveUnknownFields Assign runtime.RawExtension `json:"assign,omitempty"` }

It's a bit hacky to use RawExtension as this isn't a K8s object, but it saves us wrapping json.RawMessage in a struct and writing custom serializers like they did for runtime.RawExtension.

Fun finding: controller-runtime infers schema type from field type, so json.RawMessage is typed as a string (it is an alias for []byte), but runtime.RawExtension is an object because it is a struct enclosing a string.

@maxsmythe I changed the struct as suggested above.
One thought I had is to somehow override RawExtension.object to RawExtension.value just for semantic purposes (to get spec.parameters.assign.value) I don't think there is such an option but I won't mind to be enlightened.

You could create your own "RawExtension", though switching .Object to .Value wouldn't work.

You could follow the same pattern raw extension does (create your own struct that implements MarshalJSON() and UnmarshalJSON(), use json:"-", and then extend it how you want, including lazy-unmarshaling the JSON when a .Value() method is called.

apis/mutations/v1alpha1/assignmetadata_types.go

maxsmythe · 2020-10-29T02:54:25Z

apis/mutations/v1alpha1/assignmetadata_types.go

+}
+
+// AssignMetadataStatus defines the observed state of AssignMetadata
+type AssignMetadataStatus struct {


We'll need pod-specific status reporting ad some point, though that can be added on as a follow-on PR.

Follow-on PR sounds good.
I'll make sure to open an issue if need be.

+1 would be good to have an issue for this so we don't forget. Won't be necessary while we're just getting things running, but very necessary for users.

maxsmythe · 2020-10-29T02:54:54Z

apis/mutations/v1alpha1/assignmetadata_types.go

+}
+
+type MetadataParameters struct {
+	Value string `json:"value,omitempty"`


String makes sense here as annotations and labels can only be strings.

So something should be added/changed here ? or keep it as it is ?

this works as-is :)

maxsmythe · 2020-10-29T03:01:54Z

config/rbac/assignmetadata_viewer_role.yaml

+- apiGroups:
+  - mutations.gatekeeper.sh
+  resources:
+  - assignmetadata


I'm curious why one resource is "assigns" (plural) and the other is "assignmetadata" (singular)

Also, I don't see the CRD manifests?

assignmetadata now changed to plural

awesome, thanks!

codecov-io · 2020-10-29T21:22:34Z

Codecov Report

Merging #916 (8070c95) into master (dcbf36b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #916   +/-   ##
=======================================
  Coverage   43.46%   43.46%           
=======================================
  Files          47       47           
  Lines        3173     3173           
=======================================
  Hits         1379     1379           
  Misses       1598     1598           
  Partials      196      196

Flag	Coverage Δ
unittests	`43.46% <ø> (ø)`

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

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 dcbf36b...8070c95. Read the comment docs.

fedepaol · 2020-11-02T15:56:16Z

apis/mutations/v1alpha1/match.go

+	// APIGroups is the API groups the resources belong to. '*' is all groups.
+	// If '*' is present, the length of the slice must be one.
+	// Required.
+	APIGroups string `json:"apiGroups,omitempty" protobuf:"bytes,1,rep,name=apiGroups"`


I think both this and the next one must be []string to be aligned with Constraints.
@maxsmythe can you confirm?

Ah, you are correct, both of these should be []string

yanirq · 2020-11-03T13:32:33Z

@maxsmythe I covered all the comments.
Can you please take a look and see if the PR looks good ?

maxsmythe

Getting very close! Just minor cleanup-type stuff left IMO.

@ritazh @sozercan @shomron thoughts?

maxsmythe · 2020-11-03T22:44:48Z

Makefile

@@ -140,6 +140,9 @@ run: generate manifests
 install: manifests
 	kustomize build config/crd | kubectl apply -f -

+deploy-mutation: deploy
+	kustomize build --load_restrictor LoadRestrictionsNone config/overlays/mutation | kubectl apply -f -


Not a big fan of removing load restrictions, but it seems like this is probably the cleanest way of having dev CRDs, otherwise we'd need to change the file structure a bit, which may not play well with controller-gen.

TL;DR this works

maxsmythe · 2020-11-03T22:46:24Z

apis/mutations/v1alpha1/assign_types.go

+	IfIn []string `json:"ifIn,omitempty"`
+	// IfNotIn Only mutate if the current value is NOT in the supplied list
+	IfNotIn []string `json:"ifNotIn,omitempty"`
+	// Assign holds the value to be assigned


nit: Assign.value holds the value to be assigned

maxsmythe · 2020-11-03T22:53:37Z

config/crd/bases/mutations.gatekeeper.sh_assigns.yaml

+  names:
+    kind: Assign
+    listKind: AssignList
+    plural: assigns


Should the plural just be "assign"?

I'm a bit biased here as I really don't like K8s use of plurals, I think having multiple names causes more problems than it solves.

In any case whatever we choose should be consistent with assignmetadata

Changed plurals of assign and assignmetadata to be consistent (singular names).

Fun fact: controller-gen (flect) perceives assignmetadata plural as assignmetadata probably due to the ata suffix

Interesting. Programatically managing these grammar details seems like a hard thing to do.

config/rbac/mutation/assign_editor_role.yaml

config/rbac/mutation/assign_viewer_role.yaml

maxsmythe · 2020-11-03T22:56:09Z

config/rbac/mutation/assignmetadata_editor_role.yaml

@@ -0,0 +1,24 @@
+# permissions for end users to edit assignmetadatas.
+apiVersion: rbac.authorization.k8s.io/v1


None of these roles are necessary, they should all be folded up into the gatekeeper-manager role

maxsmythe · 2020-11-05T02:04:01Z

config/crd/patches/cainjection_in_assign.yaml

+# The following patch adds a directive for certmanager to inject CA into the CRD
+# CRD conversion requires k8s 1.13 or later.
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition


Did we remove permissions entirely?

I thought we were going to add them to the gatekeeper-manager role?

After digging deeper I learned that there is a convention where controller-gen generates these permissions and outputs them to role.yaml according to kubebuilder markers on the controllers themselves (and not on CRDs) as we can see in config and constraints for example.

I can add them manually to gatekeeper-manager role (which will be overridden and exclude these permission when we run deploy) or we can add them to the role.yaml file in the controller related PRs (I already added a comment for one here)

@maxsmythe was the permissions comment/question placed here on purpose ? (cainjection_in_assign.yaml) I'm trying to understand, out of placement context, if I missed anything else.

It was just placed randomly, I probably should have just added it to the bottom of the PR. Sorry.

You are correct. I'd forgotten that we were putting the permissions on the controller (which makes sense as that's the thing that actually needs the access).

maxsmythe

@ritazh @shomron This looks basically ready, once permissions are ironed out, WDYT?

maxsmythe

LGTM

yanirq · 2020-11-09T18:21:08Z

@maxsmythe PR re-based to resolve conflicts and allow merge.

maxsmythe · 2020-11-10T03:45:41Z

Thanks!

@ritazh @shomron @sozercan @brycecr

Last chance to comment before merging. I think I'll merge EOD tomorrow if no one has feedback.

ritazh · 2020-11-10T06:31:11Z

config/crd/kustomization.yaml

@@ -17,13 +17,17 @@ patchesStrategicMerge:
 #- patches/webhook_in_configs.yaml
 #- patches/webhook_in_constraintpodstatuses.yaml
 #- patches/webhook_in_constrainttemplatepodstatuses.yaml
+#- patches/webhook_in_assignmetadata.yaml


Do we want to add a TODO here to make sure we add the following under resources and remove the one under overlays in the future?

- bases/mutations.gatekeeper.sh_assignmetadata.yaml - bases/mutations.gatekeeper.sh_assign.yaml

gatekeeper/config/crd/kustomization.yaml

Line 4 in 611327a

resources:

ritazh

lgtm

Just one nit

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

yanirq force-pushed the mutation_crds branch 2 times, most recently from f04f608 to 987e34d Compare October 28, 2020 12:00

maxsmythe reviewed Oct 29, 2020

View reviewed changes

yanirq force-pushed the mutation_crds branch from 08d612a to aaa8538 Compare October 29, 2020 11:56

yanirq force-pushed the mutation_crds branch from 7fb8119 to 6d936d0 Compare October 29, 2020 21:27

yanirq changed the title ~~WIP: Mutation Objects~~ Mutation Objects Oct 29, 2020

mmirecki mentioned this pull request Oct 30, 2020

Add AssignMetadata controller #927

Merged

yanirq force-pushed the mutation_crds branch from 6d936d0 to 5d6e9be Compare November 1, 2020 18:04

fedepaol reviewed Nov 2, 2020

View reviewed changes

fedepaol mentioned this pull request Nov 2, 2020

Implement the match logic #932

Merged

maxsmythe reviewed Nov 3, 2020

View reviewed changes

mmirecki mentioned this pull request Nov 4, 2020

Write Mutation function for AssignMetadata #937

Merged

maxsmythe reviewed Nov 5, 2020

View reviewed changes

maxsmythe approved these changes Nov 6, 2020

View reviewed changes

mmirecki mentioned this pull request Nov 6, 2020

Add mutation cache to controller and webhook #945

Merged

yanirq force-pushed the mutation_crds branch from 00a0d44 to 611327a Compare November 7, 2020 09:59

ritazh reviewed Nov 10, 2020

View reviewed changes

ritazh approved these changes Nov 10, 2020

View reviewed changes

yanirq force-pushed the mutation_crds branch 2 times, most recently from 6a4782e to 8070c95 Compare November 10, 2020 11:48

yanirq added 5 commits November 11, 2020 01:54

mutation crds

d13db48

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

Change AssignMetadata to plural

8670308

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

adjust parameters field types

283c544

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

Overlay deployment for mutations

9c86d0a

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

Adjust parameters field

d72188d

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

yanirq added 3 commits November 11, 2020 01:54

Change Kinds fields to string slices

8a97049

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

Remove redundant rbac and fix plurals

51ea292

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

Add TODO for mutation resources

97bfc5c

Signed-off-by: Yanir Quinn <yquinn@redhat.com>

yanirq force-pushed the mutation_crds branch from 8070c95 to 97bfc5c Compare November 10, 2020 23:54

ritazh merged commit 9a28f98 into open-policy-agent:master Nov 11, 2020

roee88 mentioned this pull request Jan 19, 2021

Code generation from protos is broken fybrik/fybrik#291

Closed

		@@ -0,0 +1,24 @@
		# permissions for end users to edit assignmetadatas.
		apiVersion: rbac.authorization.k8s.io/v1

Mutation Objects #916

Mutation Objects #916

Conversation

yanirq commented Oct 28, 2020 • edited Loading

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

yanirq Oct 30, 2020 • edited Loading

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

yanirq Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 29, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanirq commented Nov 3, 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

yanirq Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

yanirq commented Nov 9, 2020

maxsmythe commented Nov 10, 2020

ritazh Nov 10, 2020 • edited Loading

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

yanirq commented Oct 28, 2020 •

edited

Loading

yanirq Oct 30, 2020 •

edited

Loading

yanirq Oct 29, 2020 •

edited

Loading

codecov-io commented Oct 29, 2020 •

edited

Loading

yanirq Nov 5, 2020 •

edited

Loading

ritazh Nov 10, 2020 •

edited

Loading