Mutation: conversion functions from assign / assignmetadata to Mutator #938

fedepaol · 2020-11-04T17:52:55Z

What this PR does / why we need it:

This implements MutatorFor plus the type specific objects implementing the interfaces.
Depends on other prs to get merged.

Fixes #936

Special notes for your reviewer:

maxsmythe · 2020-11-11T05:09:49Z

pkg/mutation/assign_mutator.go

+	return res
+}
+
+func mutatorForAssign(assign *mutationsv1.Assign) (MutatorWithSchema, error) {


We should return type assignMutator rather than the MutatorWithSchema interface, per golang conventions.

maxsmythe · 2020-11-11T05:15:21Z

pkg/mutation/assignmeta_mutator.go

+	return res
+}
+
+func mutatorForAssignMetadata(assignMeta *mutationsv1.AssignMetadata) (Mutator, error) {


same comment here WRT return type

maxsmythe · 2020-11-11T05:18:06Z

pkg/mutation/mutator.go

@@ -61,3 +66,19 @@ func MakeID(obj runtime.Object) (ID, error) {
 		Namespace: meta.GetNamespace(),
 	}, nil
 }
+
+// MutatorFor returns a mutator generated


I think we removed the MutatorFor function after refactoring the MutationSystem interface

Yes, was a bit torn about having a single entry point that dispatches to the right function or two type safe ones.
Will remove it

codecov-io · 2020-11-11T13:45:00Z

Codecov Report

Merging #938 (b497d52) into master (e47d95b) will increase coverage by 0.14%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   46.78%   46.93%   +0.14%     
==========================================
  Files          56       58       +2     
  Lines        3454     3541      +87     
==========================================
+ Hits         1616     1662      +46     
- Misses       1636     1669      +33     
- Partials      202      210       +8

Flag	Coverage Δ
unittests	`46.93% <62.50%> (+0.14%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/mutation/mutator.go	`77.77% <0.00%> (ø)`
pkg/mutation/assign_mutator.go	`59.18% <59.18%> (ø)`
pkg/mutation/assignmeta_mutator.go	`68.42% <68.42%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`55.70% <0.00%> (-1.63%)`	⬇️
pkg/readiness/object_tracker.go	`77.91% <0.00%> (-1.23%)`	⬇️
pkg/readiness/ready_tracker.go	`69.09% <0.00%> (-0.73%)`	⬇️

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 e47d95b...b497d52. Read the comment docs.

maxsmythe

Looks good, 2 nits on the tests

@ritazh @sozercan @shomron thoughts

maxsmythe · 2020-11-12T01:59:22Z

pkg/mutation/conversion_test.go

+				t.Fatal(tc.tname, "Failed to convert second to mutator", err)
+			}
+			hasDiff := firstMutator.HasDiff(secondMutator)
+			hasDiff1 := firstMutator.HasDiff(secondMutator)


should we swap caller/callee here?

maxsmythe · 2020-11-12T02:00:33Z

pkg/mutation/conversion_test.go

+	}
+	first.APIVersion, first.Kind = gvk.ToAPIVersionAndKind()
+
+	second := &mutationsv1.Assign{


You might be able to write: second := first.DeepCopy()

maxsmythe · 2020-11-12T02:00:51Z

pkg/mutation/conversion_test.go

+	}
+	first.APIVersion, first.Kind = gvk.ToAPIVersionAndKind()
+
+	second := &mutationsv1.AssignMetadata{


maxsmythe · 2020-11-12T02:01:08Z

pkg/mutation/conversion_test.go

+				t.Fatal(tc.tname, "Failed to convert second to mutator", err)
+			}
+			hasDiff := firstMutator.HasDiff(secondMutator)
+			hasDiff1 := firstMutator.HasDiff(secondMutator)


Swap call order?

fedepaol · 2020-11-12T17:33:23Z

@maxsmythe applied new changes

maxsmythe

LGTM!

@ritazh @sozercan @shomron

ritazh · 2020-11-18T00:46:54Z

pkg/mutation/assign_mutator.go

+
+import (
+	"github.com/google/go-cmp/cmp"
+	mutationsv1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"


To be consistent, let's use mutationsv1alpha1

ritazh · 2020-11-18T00:51:41Z

pkg/mutation/assign_mutator.go

+		id:       m.id,
+		assign:   m.assign.DeepCopy(),
+		path:     make([]path.Entry, len(m.path)),
+		bindings: make([]SchemaBinding, 0),


should this also be initialized with len(m.bindings)

ritazh · 2020-11-18T00:55:08Z

pkg/mutation/assignmeta_mutator.go

+
+import (
+	"github.com/google/go-cmp/cmp"
+	mutationsv1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"


mutationsv1alpha1

ritazh · 2020-11-18T01:00:10Z

pkg/mutation/assignmeta_mutator.go

+func MutatorForAssignMetadata(assignMeta *mutationsv1.AssignMetadata) (*AssignMetadataMutator, error) {
+	id, err := MakeID(assignMeta)
+	if err != nil {
+		return nil, errors.Wrapf(err, "Failed to retrieve id for assign type")


Suggested change

return nil, errors.Wrapf(err, "Failed to retrieve id for assign type")

return nil, errors.Wrapf(err, "Failed to retrieve id for assignMetadata type")

ritazh · 2020-11-18T01:00:43Z

pkg/mutation/conversion_test.go

+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	mutationsv1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"


mutationsv1alpha1

ritazh · 2020-11-18T01:11:02Z

pkg/mutation/conversion_test.go

+
+	for _, tc := range table {
+		t.Run(tc.tname, func(t *testing.T) {
+			secondAssign := second.DeepCopy()


nit: secondAssignMeta

ritazh · 2020-11-18T01:12:43Z

@fedepaol just few nits. mostly looks good!

fedepaol · 2020-11-18T11:31:51Z

@fedepaol just few nits. mostly looks good!

I think I applied all the suggestions, thanks for the review!

ritazh

lgtm

ritazh

Changes from recently merged PRs will need to be reflected in this PR now

We expose a generic MutatorFor(o Object) to build a mutator instance that wraps the cr. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol · 2020-11-19T08:30:15Z

Changes from recently merged PRs will need to be reflected in this PR now

Done

maxsmythe · 2020-11-20T10:32:01Z

It looks like linting is failing?

fedepaol · 2020-11-20T10:36:14Z

It looks like linting is failing?

Fixing it

Location parser is merged. Here we fill the path field for both the mutators. We also add a new test validating that the conversion fails if an invalid location is specified. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol · 2020-11-20T11:01:03Z

Done, sorry. I took the change too lightly.
Here I integrate the parsing, and add a new test that validates that the conversion fails if we pass an invalid path.

fedepaol · 2020-11-20T12:50:20Z

@maxsmythe I also added a minimal verification for assignmeta.

This is under the idea that we want to limit the mutation to labels and annotations, but we can make the constraint lighter and have it validate that the path is a valid metadata path.

maxsmythe · 2020-11-24T04:39:40Z

Thanks for also checking the metadata prefix!

The changes LGTM. @ritazh, it looks like all your comments were addressed. Does this LGTY?

maxsmythe · 2020-11-24T04:40:06Z

Also, @fedepaol, if this looks good to Rita tomorrow, are you ready for this to merge?

ritazh · 2020-11-24T06:54:41Z

pkg/mutation/assignmeta_mutator.go

+		},
+	}
+
+	annotationValidiSubPath = []parser.Node{


nit: is annotationValidiSubPath intentional or a typo?

Was a typo, fixed it

ritazh

lgtm

just one nit

This checks that the only valid location for assignmetadata is related to labels or annotations. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol · 2020-11-24T07:14:25Z

Also, @fedepaol, if this looks good to Rita tomorrow, are you ready for this to merge?

It is, I fixed the nit.

maxsmythe · 2020-11-25T02:10:22Z

Awesome! Merging in

mmirecki mentioned this pull request Nov 6, 2020

Add mutation cache to controller and webhook #945

Merged

maxsmythe reviewed Nov 11, 2020

View reviewed changes

fedepaol force-pushed the mutationfor branch from f7fc400 to 4b72d64 Compare November 11, 2020 13:14

fedepaol changed the title ~~WIP Mutation: conversion functions from assign / assignmetadata to Mutator~~ Mutation: conversion functions from assign / assignmetadata to Mutator Nov 11, 2020

fedepaol force-pushed the mutationfor branch from 4b72d64 to af23697 Compare November 11, 2020 13:42

fedepaol force-pushed the mutationfor branch 2 times, most recently from 630eb87 to 9789800 Compare November 11, 2020 13:51

maxsmythe requested changes Nov 12, 2020

View reviewed changes

fedepaol force-pushed the mutationfor branch from 9789800 to ee582b2 Compare November 12, 2020 17:32

maxsmythe approved these changes Nov 13, 2020

View reviewed changes

ritazh reviewed Nov 18, 2020

View reviewed changes

fedepaol force-pushed the mutationfor branch from ee582b2 to d98f7af Compare November 18, 2020 11:31

maxsmythe requested a review from ritazh November 18, 2020 20:11

ritazh approved these changes Nov 18, 2020

View reviewed changes

ritazh requested changes Nov 19, 2020

View reviewed changes

Conversion functions from assign / assignmetadata to Mutator.

52f6017

We expose a generic MutatorFor(o Object) to build a mutator instance that wraps the cr. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the mutationfor branch from b3af09f to 52f6017 Compare November 19, 2020 07:06

Update to location parser updates.

4b72de6

Location parser is merged. Here we fill the path field for both the mutators. We also add a new test validating that the conversion fails if an invalid location is specified. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

mmirecki mentioned this pull request Nov 20, 2020

Add AssignMetadata controller #927

Merged

fedepaol mentioned this pull request Nov 20, 2020

Implement mutation validation #978

Merged

maxsmythe requested a review from ritazh November 24, 2020 04:40

ritazh reviewed Nov 24, 2020

View reviewed changes

ritazh approved these changes Nov 24, 2020

View reviewed changes

Implement assignmetadata path validation.

c09743a

This checks that the only valid location for assignmetadata is related to labels or annotations. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the mutationfor branch from 34e6bb5 to c09743a Compare November 24, 2020 07:14

Merge branch 'master' into mutationfor

b497d52

maxsmythe merged commit 964864d into open-policy-agent:master Nov 25, 2020

	return nil, errors.Wrapf(err, "Failed to retrieve id for assign type")
	return nil, errors.Wrapf(err, "Failed to retrieve id for assignMetadata type")

Mutation: conversion functions from assign / assignmetadata to Mutator #938

Mutation: conversion functions from assign / assignmetadata to Mutator #938

Conversation

fedepaol commented Nov 4, 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

codecov-io commented Nov 11, 2020 • edited Loading

Codecov Report

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

fedepaol commented Nov 12, 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

ritazh commented Nov 18, 2020

fedepaol commented Nov 18, 2020

ritazh left a comment

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

fedepaol commented Nov 19, 2020

maxsmythe commented Nov 20, 2020

fedepaol commented Nov 20, 2020

fedepaol commented Nov 20, 2020

fedepaol commented Nov 20, 2020

maxsmythe commented Nov 24, 2020

maxsmythe commented Nov 24, 2020

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

fedepaol commented Nov 24, 2020

maxsmythe commented Nov 25, 2020

codecov-io commented Nov 11, 2020 •

edited

Loading