Add value testing to the assign mutator (#1113)

* Add locking, deepcopy path tester memoization

Signed-off-by: Max Smythe <smythe@google.com>

* Add path parsing error context

Signed-off-by: Max Smythe <smythe@google.com>

* Tester should also have a DeepCopy method

Signed-off-by: Max Smythe <smythe@google.com>

* Eager initialize path tester, guard against nil

Signed-off-by: Max Smythe <smythe@google.com>

* Add value test to assign mutator

Signed-off-by: Max Smythe <smythe@google.com>

apis/mutations/v1alpha1/assign_types.go

@@ -16,9 +16,11 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"encoding/json"
+
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation/path/tester"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
@@ -44,16 +46,17 @@ type ApplyTo struct {
 
 type Parameters struct {
 	PathTests []PathTest `json:"pathTests,omitempty"`
-	// IfIn Only mutate if the current value is in the supplied list
-	IfIn []string `json:"ifIn,omitempty"`
-	// IfNotIn Only mutate if the current value is NOT in the supplied list
-	IfNotIn []string `json:"ifNotIn,omitempty"`
+
+	// once https://github.com/kubernetes-sigs/controller-tools/pull/528
+	// is merged, we can use an actual object
+	AssignIf runtime.RawExtension `json:"assignIf,omitempty"`
+
 	// Assign.value holds the value to be assigned
 	// +kubebuilder:validation:XPreserveUnknownFields
 	Assign runtime.RawExtension `json:"assign,omitempty"`
 }
 
-// PathTests allows the user to customize how the mutation works if parent
+// PathTest allows the user to customize how the mutation works if parent
 // paths are missing. It traverses the list in order. All sub paths are
 // tested against the provided condition, if the test fails, the mutation is
 // not applied. All `subPath` entries must be a prefix of `location`. Any
@@ -99,3 +102,42 @@ type AssignList struct {
 func init() {
 	SchemeBuilder.Register(&Assign{}, &AssignList{})
 }
+
+// ValueTests returns tests that the mutator is expected
+// to run against the value
+func (a *Assign) ValueTests() (AssignIf, error) {
+	raw := a.Spec.Parameters.AssignIf
+	out := AssignIf{}
+	if len(raw.Raw) == 0 {
+		return out, nil
+	}
+	if err := json.Unmarshal(raw.Raw, &out); err != nil {
+		return AssignIf{}, err
+	}
+	return out, nil
+}
+
+// +kubebuilder:object:generate=false
+
+// AssignIf describes tests against the pre-existing value.
+// The object will be mutated only if assertions pass
+type AssignIf struct {
+	// In Asserts that the value is a member of the provided list before mutating
+	In []interface{} `json:"in,omitempty"`
+
+	// NotIn Asserts that the value is not a member of the provided list before mutating
+	NotIn []interface{} `json:"notIn,omitempty"`
+}
+
+func (a *AssignIf) DeepCopy() *AssignIf {
+	if a == nil {
+		return nil
+	}
+	in := runtime.DeepCopyJSONValue(a.In)
+	notIn := runtime.DeepCopyJSONValue(a.NotIn)
+
+	return &AssignIf{
+		In:    in.([]interface{}),
+		NotIn: notIn.([]interface{}),
+	}
+}

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

@@ -195,21 +195,13 @@ spec:
                   description: Assign.value holds the value to be assigned
                   type: object
                   x-kubernetes-preserve-unknown-fields: true
-                ifIn:
-                  description: IfIn Only mutate if the current value is in the supplied
-                    list
-                  items:
-                    type: string
-                  type: array
-                ifNotIn:
-                  description: IfNotIn Only mutate if the current value is NOT in
-                    the supplied list
-                  items:
-                    type: string
-                  type: array
+                assignIf:
+                  description: once https://github.com/kubernetes-sigs/controller-tools/pull/528
+                    is merged, we can use an actual object
+                  type: object
                 pathTests:
                   items:
-                    description: "PathTests allows the user to customize how the mutation
+                    description: "PathTest allows the user to customize how the mutation
                       works if parent paths are missing. It traverses the list in
                       order. All sub paths are tested against the provided condition,
                       if the test fails, the mutation is not applied. All `subPath`

pkg/mutation/assign_mutator.go

@@ -20,11 +20,12 @@ import (
 // AssignMutator is a mutator object built out of a
 // Assign instance.
 type AssignMutator struct {
-	id       types.ID
-	assign   *mutationsv1alpha1.Assign
-	path     *parser.Path
-	bindings []schema.Binding
-	tester   *patht.Tester
+	id        types.ID
+	assign    *mutationsv1alpha1.Assign
+	path      *parser.Path
+	bindings  []schema.Binding
+	tester    *patht.Tester
+	valueTest *mutationsv1alpha1.AssignIf
 }
 
 // AssignMutator implements mutatorWithSchema
@@ -40,8 +41,41 @@ func (m *AssignMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
 }
 
 func (m *AssignMutator) Mutate(obj *unstructured.Unstructured) error {
-	return mutate(m, m.tester, obj)
+	return mutate(m, m.tester, m.testValue, obj)
 }
+
+// valueTest returns true if it is okay for the mutation func to override the value
+func (m *AssignMutator) testValue(v interface{}, exists bool) bool {
+	if len(m.valueTest.In) != 0 {
+		ifInMatched := false
+		if !exists {
+			// a missing value cannot satisfy the "In" test
+			return false
+		}
+		for _, obj := range m.valueTest.In {
+			if cmp.Equal(v, obj) {
+				ifInMatched = true
+				break
+			}
+		}
+		if !ifInMatched {
+			return false
+		}
+	}
+
+	if !exists {
+		// a missing value cannot violate NotIn
+		return true
+	}
+
+	for _, obj := range m.valueTest.NotIn {
+		if cmp.Equal(v, obj) {
+			return false
+		}
+	}
+	return true
+}
+
 func (m *AssignMutator) ID() types.ID {
 	return m.id
 }
@@ -94,6 +128,7 @@ func (m *AssignMutator) DeepCopy() types.Mutator {
 	copy(res.path.Nodes, m.path.Nodes)
 	copy(res.bindings, m.bindings)
 	res.tester = m.tester.DeepCopy()
+	res.valueTest = m.valueTest.DeepCopy()
 	return res
 }
 
@@ -102,12 +137,12 @@ func (m *AssignMutator) DeepCopy() types.Mutator {
 func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error) {
 	id, err := types.MakeID(assign)
 	if err != nil {
-		return nil, errors.Wrap(err, "Failed to retrieve id for assign type")
+		return nil, errors.Wrap(err, "failed to retrieve id for assign type")
 	}
 
 	path, err := parser.Parse(assign.Spec.Location)
 	if err != nil {
-		return nil, errors.Wrap(err, "Failed to parse the location specified")
+		return nil, errors.Wrap(err, "failed to parse the location specified")
 	}
 
 	pathTests, err := gatherPathTests(assign)
@@ -122,13 +157,18 @@ func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error)
 	if err != nil {
 		return nil, err
 	}
+	valueTests, err := assign.ValueTests()
+	if err != nil {
+		return nil, err
+	}
 
 	return &AssignMutator{
-		id:       id,
-		assign:   assign.DeepCopy(),
-		bindings: applyToToBindings(assign.Spec.ApplyTo),
-		path:     path,
-		tester:   tester,
+		id:        id,
+		assign:    assign.DeepCopy(),
+		bindings:  applyToToBindings(assign.Spec.ApplyTo),
+		path:      path,
+		tester:    tester,
+		valueTest: &valueTests,
 	}, nil
 }
 
@@ -199,10 +239,10 @@ func IsValidAssign(assign *mutationsv1alpha1.Assign) error {
 	if err != nil {
 		return err
 	}
-	_, err = MutatorForAssign(assign)
-	if err != nil {
+	if _, err := MutatorForAssign(assign); err != nil {
 		return err
 	}
+
 	return nil
 }