Fix scope selector for mutators (#1110)

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

apis/mutations/v1alpha1/match.go

@@ -22,7 +22,7 @@ import (
 
 type Match struct {
 	Kinds              []Kinds                            `json:"kinds,omitempty"`
-	Scope              apiextensionsv1beta1.ResourceScope `json:"scope" protobuf:"bytes,4,opt,name=scope,casttype=ResourceScope"`
+	Scope              apiextensionsv1beta1.ResourceScope `json:"scope,omitempty"`
 	Namespaces         []string                           `json:"namespaces,omitempty"`
 	ExcludedNamespaces []string                           `json:"excludedNamespaces,omitempty"`
 	LabelSelector      *metav1.LabelSelector              `json:"labelSelector,omitempty"`

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

@@ -186,8 +186,6 @@ spec:
                   description: ResourceScope is an enum defining the different scopes
                     available to a custom resource
                   type: string
-              required:
-              - scope
               type: object
             parameters:
               properties:

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

@@ -165,8 +165,6 @@ spec:
                   description: ResourceScope is an enum defining the different scopes
                     available to a custom resource
                   type: string
-              required:
-              - scope
               type: object
             parameters:
               properties:

pkg/mutation/match.go

@@ -1,6 +1,7 @@
 package mutation
 
 import (
+	"errors"
 	"fmt"
 
 	mutationsv1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
@@ -18,7 +19,11 @@ import (
 func Matches(match mutationsv1.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)
+		return false, fmt.Errorf("accessor failed for %s", obj.GetObjectKind().GroupVersionKind().Kind)
+	}
+
+	if isNamespace(obj) && ns == nil {
+		return false, errors.New("invalid call to Matches(), ns must not be nil for Namespace objects")
 	}
 
 	foundMatch := false
@@ -59,57 +64,61 @@ func Matches(match mutationsv1.Match, obj runtime.Object, ns *corev1.Namespace)
 		return false, nil
 	}
 
+	clusterScoped := ns == nil || isNamespace(obj)
+
 	if match.Scope == apiextensionsv1beta1.ClusterScoped &&
-		meta.GetNamespace() != "" {
+		!clusterScoped {
 		return false, nil
 	}
 
 	if match.Scope == apiextensionsv1beta1.NamespaceScoped &&
-		meta.GetNamespace() == "" {
+		clusterScoped {
 		return false, nil
 	}
 
-	found := false
-	for _, n := range match.Namespaces {
-		if meta.GetNamespace() == n {
-			found = true
-			break
+	if ns != nil {
+		found := false
+		for _, n := range match.Namespaces {
+			if ns.Name == n {
+				found = true
+				break
+			}
 		}
-	}
-	if !found && len(match.Namespaces) > 0 {
-		return false, nil
-	}
-
-	for _, n := range match.ExcludedNamespaces {
-		if meta.GetNamespace() == n {
+		if !found && len(match.Namespaces) > 0 {
 			return false, nil
 		}
-	}
-	if match.LabelSelector != nil {
-		selector, err := metav1.LabelSelectorAsSelector(match.LabelSelector)
-		if err != nil {
-			return false, err
+
+		for _, n := range match.ExcludedNamespaces {
+			if ns.Name == n {
+				return false, nil
+			}
 		}
-		if !selector.Matches(labels.Set(meta.GetLabels())) {
-			return false, nil
+		if match.LabelSelector != nil {
+			selector, err := metav1.LabelSelectorAsSelector(match.LabelSelector)
+			if err != nil {
+				return false, err
+			}
+			if !selector.Matches(labels.Set(meta.GetLabels())) {
+				return false, nil
+			}
 		}
-	}
 
-	if match.NamespaceSelector != nil {
-		selector, err := metav1.LabelSelectorAsSelector(match.NamespaceSelector)
-		if err != nil {
-			return false, err
-		}
+		if match.NamespaceSelector != nil {
+			selector, err := metav1.LabelSelectorAsSelector(match.NamespaceSelector)
+			if err != nil {
+				return false, err
+			}
 
-		switch {
-		case isNamespace(obj): // if the object is a namespace, namespace selector matches against the object
-			if !selector.Matches(labels.Set(meta.GetLabels())) {
+			switch {
+			case isNamespace(obj): // if the object is a namespace, namespace selector matches against the object
+				if !selector.Matches(labels.Set(meta.GetLabels())) {
+					return false, nil
+				}
+			case clusterScoped:
+				// cluster scoped, matches by default
+			case !selector.Matches(labels.Set(ns.Labels)):
 				return false, nil
 			}
-		case meta.GetNamespace() == "":
-			// cluster scoped, matches by default
-		case !selector.Matches(labels.Set(ns.Labels)):
-			return false, nil
 		}
 	}
 

pkg/mutation/match_test.go

@@ -1,6 +1,7 @@
 package mutation_test
 
 import (
+	"encoding/json"
 	"testing"
 
 	configv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/config/v1alpha1"
@@ -127,7 +128,7 @@ func TestMatch(t *testing.T) {
 			match: mutationsv1.Match{
 				Namespaces: []string{"nonmatching", "namespace"},
 			},
-			namespace:   &corev1.Namespace{},
+			namespace:   &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "namespace"}},
 			shouldMatch: true,
 		},
 		{
@@ -177,7 +178,7 @@ func TestMatch(t *testing.T) {
 				},
 				Scope: apiextensionsv1beta1.NamespaceScoped,
 			},
-			namespace:   &corev1.Namespace{},
+			namespace:   nil,
 			shouldMatch: false,
 		},
 		{
@@ -321,7 +322,7 @@ func TestMatch(t *testing.T) {
 			shouldMatch: true,
 		},
 		{
-			tname: "namespace selector is applied to the namespace, and does not matches",
+			tname: "namespace selector is applied to the namespace, and does not match",
 			toMatch: makeNamespace("namespace", func(o *unstructured.Unstructured) {
 				meta, _ := meta.Accessor(o)
 				meta.SetLabels(map[string]string{
@@ -341,7 +342,21 @@ func TestMatch(t *testing.T) {
 	}
 	for _, tc := range table {
 		t.Run(tc.tname, func(t *testing.T) {
-			matches, err := mutation.Matches(tc.match, tc.toMatch, tc.namespace)
+			ns := tc.namespace
+			nsgk := schema.GroupKind{Group: "", Kind: "Namespace"}
+			if tc.toMatch.GetObjectKind().GroupVersionKind().GroupKind() == nsgk {
+				b, err := json.Marshal(tc.toMatch.Object)
+				if err != nil {
+					t.Fatal(err)
+				}
+				ns = &corev1.Namespace{}
+				if err := json.Unmarshal(b, ns); err != nil {
+					t.Fatal(err)
+				}
+			}
+			// namespace is not populated in the object metadata for mutation requests
+			tc.toMatch.SetNamespace("")
+			matches, err := mutation.Matches(tc.match, tc.toMatch, ns)
 			if err != nil {
 				t.Error("Match failed for ", tc.tname)
 			}
@@ -379,7 +394,7 @@ func makeNamespace(name string, options ...func(*unstructured.Unstructured)) *un
 	namespace := &corev1.Namespace{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Namespace",
-			APIVersion: "",
+			APIVersion: "v1",
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,

pkg/webhook/mutation.go

@@ -193,6 +193,8 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req admission.Reque
 				return admission.Errored(int32(http.StatusInternalServerError), err), nil
 			}
 		}
+	} else {
+		ns = nil
 	}
 	obj := unstructured.Unstructured{}
 	err := obj.UnmarshalJSON(req.Object.Raw)