Preserve conflicting schemas and report conflicts

pkg/mutation/match/match.go

@@ -2,15 +2,15 @@ package match
 
 import (
 	"errors"
-	"fmt"
 	"strings"
 
 	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.
@@ -22,6 +22,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 {
@@ -48,12 +67,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")
 	}
@@ -130,7 +144,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
 			}
 		}
@@ -143,7 +157,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)),