Preserve conflicting schemas and report conflicts

When multiple Pods are running mutators, it is possible that they will
ingest mutators in different orders. Before this PR, we would
essentially randomly pick which mutator to honor and discard future
mutators which conflicted.

Fixes: #1216

Signed-off-by: Will Beason <willbeason@google.com>

pkg/mutation/match/match.go

@@ -2,14 +2,14 @@ package match
 
 import (
 	"errors"
-	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // ApplyTo determines what GVKs items the mutation should apply to.
@@ -21,6 +21,25 @@ type ApplyTo struct {
 	Versions []string `json:"versions,omitempty"`
 }
 
+// Flatten returns the set of GroupVersionKinds this ApplyTo matches.
+// The GVKs are not guaranteed to be sorted or unique.
+func (a ApplyTo) Flatten() []schema.GroupVersionKind {
+	var result []schema.GroupVersionKind
+	for _, group := range a.Groups {
+		for _, version := range a.Versions {
+			for _, kind := range a.Kinds {
+				gvk := schema.GroupVersionKind{
+					Group:   group,
+					Version: version,
+					Kind:    kind,
+				}
+				result = append(result, gvk)
+			}
+		}
+	}
+	return result
+}
+
 // Match selects objects to apply mutations to.
 // +kubebuilder:object:generate=true
 type Match struct {
@@ -47,12 +66,7 @@ type Kinds struct {
 
 // Matches verifies if the given object belonging to the given namespace
 // matches the current mutator.
-func Matches(match *Match, obj runtime.Object, ns *corev1.Namespace) (bool, error) {
-	meta, err := meta.Accessor(obj)
-	if err != nil {
-		return false, fmt.Errorf("accessor failed for %s", obj.GetObjectKind().GroupVersionKind().Kind)
-	}
-
+func Matches(match *Match, obj client.Object, ns *corev1.Namespace) (bool, error) {
 	if isNamespace(obj) && ns == nil {
 		return false, errors.New("invalid call to Matches(), ns must not be nil for Namespace objects")
 	}
@@ -129,7 +143,7 @@ func Matches(match *Match, obj runtime.Object, ns *corev1.Namespace) (bool, erro
 			if err != nil {
 				return false, err
 			}
-			if !selector.Matches(labels.Set(meta.GetLabels())) {
+			if !selector.Matches(labels.Set(obj.GetLabels())) {
 				return false, nil
 			}
 		}
@@ -142,7 +156,7 @@ func Matches(match *Match, obj runtime.Object, ns *corev1.Namespace) (bool, erro
 
 			switch {
 			case isNamespace(obj): // if the object is a namespace, namespace selector matches against the object
-				if !selector.Matches(labels.Set(meta.GetLabels())) {
+				if !selector.Matches(labels.Set(obj.GetLabels())) {
 					return false, nil
 				}
 			case clusterScoped:

pkg/mutation/match/match_test.go

@@ -22,6 +22,48 @@ func TestMatch(t *testing.T) {
 		namespace   *corev1.Namespace
 		shouldMatch bool
 	}{
+		{
+			tname:   "match empty group kinds",
+			toMatch: makeObject("kind", "group", "namespace", "name"),
+			match: Match{
+				Kinds: []Kinds{
+					{
+						Kinds:     []string{},
+						APIGroups: []string{},
+					},
+				},
+			},
+			namespace:   &corev1.Namespace{},
+			shouldMatch: true,
+		},
+		{
+			tname:   "match empty kinds",
+			toMatch: makeObject("kind", "group", "namespace", "name"),
+			match: Match{
+				Kinds: []Kinds{
+					{
+						Kinds:     []string{},
+						APIGroups: []string{"*"},
+					},
+				},
+			},
+			namespace:   &corev1.Namespace{},
+			shouldMatch: true,
+		},
+		{
+			tname:   "don't match empty kinds in other group",
+			toMatch: makeObject("kind", "group", "namespace", "name"),
+			match: Match{
+				Kinds: []Kinds{
+					{
+						Kinds:     []string{},
+						APIGroups: []string{"rbac"},
+					},
+				},
+			},
+			namespace:   &corev1.Namespace{},
+			shouldMatch: false,
+		},
 		{
 			tname:   "match kind with *",
 			toMatch: makeObject("kind", "group", "namespace", "name"),

pkg/mutation/mutators/assign_mutator.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"sort"
 
 	"github.com/google/go-cmp/cmp"
 	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
@@ -17,7 +18,8 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
+	runtimeschema "k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -28,18 +30,20 @@ var (
 // 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
+	id     types.ID
+	assign *mutationsv1alpha1.Assign
+	path   parser.Path
+
+	// bindings are the set of GVKs this Mutator applies to.
+	bindings  []runtimeschema.GroupVersionKind
 	tester    *patht.Tester
 	valueTest *mutationsv1alpha1.AssignIf
 }
 
 // AssignMutator implements mutatorWithSchema
 var _ schema.MutatorWithSchema = &AssignMutator{}
 
-func (m *AssignMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
+func (m *AssignMutator) Matches(obj client.Object, ns *corev1.Namespace) bool {
 	if !match.AppliesTo(m.assign.Spec.ApplyTo, obj) {
 		return false
 	}
@@ -91,7 +95,7 @@ func (m *AssignMutator) ID() types.ID {
 	return m.id
 }
 
-func (m *AssignMutator) SchemaBindings() []schema.Binding {
+func (m *AssignMutator) SchemaBindings() []runtimeschema.GroupVersionKind {
 	return m.bindings
 }
 
@@ -134,10 +138,11 @@ func (m *AssignMutator) DeepCopy() types.Mutator {
 		path: parser.Path{
 			Nodes: make([]parser.Node, len(m.path.Nodes)),
 		},
-		bindings: make([]schema.Binding, len(m.bindings)),
+		bindings: make([]runtimeschema.GroupVersionKind, len(m.bindings)),
 	}
 	copy(res.path.Nodes, m.path.Nodes)
 	copy(res.bindings, m.bindings)
+	fmt.Println(len(res.bindings))
 	res.tester = m.tester.DeepCopy()
 	res.valueTest = m.valueTest.DeepCopy()
 	return res
@@ -194,20 +199,21 @@ func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error)
 	if err != nil {
 		return nil, err
 	}
-	applyTos := applyToToBindings(assign.Spec.ApplyTo)
-	if len(applyTos) == 0 {
-		return nil, fmt.Errorf("applyTo required for Assign mutator %s", assign.GetName())
-	}
-	for _, applyTo := range applyTos {
+	for _, applyTo := range assign.Spec.ApplyTo {
 		if len(applyTo.Groups) == 0 || len(applyTo.Versions) == 0 || len(applyTo.Kinds) == 0 {
 			return nil, fmt.Errorf("invalid applyTo for Assign mutator %s, all of group, version and kind must be specified", assign.GetName())
 		}
 	}
 
+	gvks := getSortedGVKs(assign.Spec.ApplyTo)
+	if len(gvks) == 0 {
+		return nil, fmt.Errorf("applyTo required for Assign mutator %s", assign.GetName())
+	}
+
 	return &AssignMutator{
 		id:        id,
 		assign:    assign.DeepCopy(),
-		bindings:  applyTos,
+		bindings:  gvks,
 		path:      path,
 		tester:    tester,
 		valueTest: &valueTests,
@@ -227,28 +233,6 @@ func gatherPathTests(assign *mutationsv1alpha1.Assign) ([]patht.Test, error) {
 	return pathTests, nil
 }
 
-func applyToToBindings(applyTos []match.ApplyTo) []schema.Binding {
-	res := []schema.Binding{}
-	for _, applyTo := range applyTos {
-		binding := schema.Binding{
-			Groups:   make([]string, len(applyTo.Groups)),
-			Kinds:    make([]string, len(applyTo.Kinds)),
-			Versions: make([]string, len(applyTo.Versions)),
-		}
-		for i, g := range applyTo.Groups {
-			binding.Groups[i] = g
-		}
-		for i, k := range applyTo.Kinds {
-			binding.Kinds[i] = k
-		}
-		for i, v := range applyTo.Versions {
-			binding.Versions[i] = v
-		}
-		res = append(res, binding)
-	}
-	return res
-}
-
 // IsValidAssign returns an error if the given assign object is not
 // semantically valid
 func IsValidAssign(assign *mutationsv1alpha1.Assign) error {
@@ -329,3 +313,24 @@ func validateObjectAssignedToList(p parser.Path, value interface{}, assignName s
 
 	return nil
 }
+
+func getSortedGVKs(bindings []match.ApplyTo) []runtimeschema.GroupVersionKind {
+	// deduplicate GVKs
+	gvksMap := map[runtimeschema.GroupVersionKind]struct{}{}
+	for _, binding := range bindings {
+		for _, gvk := range binding.Flatten() {
+			gvksMap[gvk] = struct{}{}
+		}
+	}
+
+	var gvks []runtimeschema.GroupVersionKind
+	for gvk := range gvksMap {
+		gvks = append(gvks, gvk)
+	}
+
+	// we iterate over the map in a stable order so that
+	// unit tests won't be flaky.
+	sort.Slice(gvks, func(i, j int) bool { return gvks[i].String() < gvks[j].String() })
+
+	return gvks
+}

pkg/mutation/mutators/assignmeta_mutator.go

@@ -15,7 +15,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
@@ -49,7 +49,7 @@ type AssignMetadataMutator struct {
 // assignMetadataMutator implements mutator
 var _ types.Mutator = &AssignMetadataMutator{}
 
-func (m *AssignMetadataMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
+func (m *AssignMetadataMutator) Matches(obj client.Object, ns *corev1.Namespace) bool {
 	matches, err := match.Matches(&m.assignMetadata.Spec.Match, obj, ns)
 	if err != nil {
 		log.Error(err, "AssignMetadataMutator.Matches failed", "assignMeta", m.assignMetadata.Name)

pkg/mutation/mutators/conversion_test.go

@@ -7,7 +7,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation/match"
-	mschema "github.com/open-policy-agent/gatekeeper/pkg/mutation/schema"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -46,21 +45,23 @@ func TestAssignToMutator(t *testing.T) {
 	}
 
 	bindings := mutatorWithSchema.SchemaBindings()
-	expectedBindings := []mschema.Binding{
-		{
-			Groups:   []string{"group1", "group2"},
-			Kinds:    []string{"kind1", "kind2", "kind3"},
-			Versions: []string{"version1"},
-		},
-		{
-			Groups:   []string{"group3", "group4"},
-			Kinds:    []string{"kind4", "kind2", "kind3"},
-			Versions: []string{"version1"},
-		},
+	expectedBindings := []schema.GroupVersionKind{
+		{Group: "group1", Version: "version1", Kind: "kind1"},
+		{Group: "group1", Version: "version1", Kind: "kind2"},
+		{Group: "group1", Version: "version1", Kind: "kind3"},
+		{Group: "group2", Version: "version1", Kind: "kind1"},
+		{Group: "group2", Version: "version1", Kind: "kind2"},
+		{Group: "group2", Version: "version1", Kind: "kind3"},
+		{Group: "group3", Version: "version1", Kind: "kind2"},
+		{Group: "group3", Version: "version1", Kind: "kind3"},
+		{Group: "group3", Version: "version1", Kind: "kind4"},
+		{Group: "group4", Version: "version1", Kind: "kind2"},
+		{Group: "group4", Version: "version1", Kind: "kind3"},
+		{Group: "group4", Version: "version1", Kind: "kind4"},
 	}
 
-	if !cmp.Equal(bindings, expectedBindings) {
-		t.Errorf("Bindings are not as expected: %s", cmp.Diff(bindings, expectedBindings))
+	if diff := cmp.Diff(expectedBindings, bindings); diff != "" {
+		t.Errorf("Bindings are not as expected: %s", diff)
 	}
 }
 

pkg/mutation/mutators/testhelpers/dummy_mutator.go

@@ -10,7 +10,7 @@ import (
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation/types"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var _ types.Mutator = &DummyMutator{}
@@ -43,7 +43,7 @@ func (d *DummyMutator) Path() parser.Path {
 	return d.path
 }
 
-func (d *DummyMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
+func (d *DummyMutator) Matches(obj client.Object, ns *corev1.Namespace) bool {
 	matches, err := match.Matches(&d.match, obj, ns)
 	if err != nil {
 		return false

pkg/mutation/path/parser/node.go

@@ -23,8 +23,6 @@ import (
 type NodeType string
 
 const (
-	// PathNode is a string segment of a path.
-	PathNode NodeType = "Path"
 	// ListNode is an array element of a path.
 	ListNode NodeType = "List"
 	// ObjectNode is the final Node in a path, what is being referenced.
@@ -45,17 +43,6 @@ type Path struct {
 	Nodes []Node
 }
 
-var _ Node = Path{}
-
-func (r Path) Type() NodeType {
-	return PathNode
-}
-
-func (r Path) DeepCopyNode() Node {
-	rout := r.DeepCopy()
-	return &rout
-}
-
 func (r Path) DeepCopy() Path {
 	out := Path{
 		Nodes: make([]Node, len(r.Nodes)),