fix: rework ns check, refactor: bubble up match err for mut (#2812)

Signed-off-by: Alex Pana <[email protected]> Signed-off-by: alex <[email protected]> Co-authored-by: Davis Haba <[email protected]> Co-authored-by: Rita Zhang <[email protected]>
open-policy-agent · Aug 8, 2023 · 5c49294 · 5c49294
1 parent 1e68843
commit 5c49294
Show file tree

Hide file tree

Showing 21 changed files with 305 additions and 35 deletions.
diff --git a/pkg/expansion/fixtures/fixtures.go b/pkg/expansion/fixtures/fixtures.go
@@ -133,6 +133,33 @@ spec:
         - "/bin/sh"
 `
 
+	DeploymentNginxWithNs = `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+  labels:
+    app: nginx
+  namespace: not-default
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx:1.14.2
+        ports:
+        - containerPort: '80'
+        args:
+        - "/bin/sh"
+`
+
 	DeploymentNoGVK = `
 metadata:
   name: nginx-deployment
@@ -193,6 +220,25 @@ spec:
     - containerPort: '80'
 `
 
+	PodImagePullMutateWithNs = `
+apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    app: nginx
+  name: nginx-deployment-pod
+  namespace: not-default
+spec:
+  containers:
+  - args:
+    - "/bin/sh"
+    image: nginx:1.14.2
+    imagePullPolicy: Always
+    name: nginx
+    ports:
+    - containerPort: '80'
+`
+
 	PodMutateImage = `
 apiVersion: v1
 kind: Pod
@@ -252,6 +298,33 @@ spec:
     kinds:
       - apiGroups: []
         kinds: []
+`
+
+	AssignPullImageWithNsSelector = `
+apiVersion: mutations.gatekeeper.sh/v1alpha1
+kind: Assign
+metadata:
+  name: always-pull-image
+spec:
+  applyTo:
+  - groups: [""]
+    kinds: ["Pod"]
+    versions: ["v1"]
+  location: "spec.containers[name: *].imagePullPolicy"
+  parameters:
+    assign:
+      value: "Always"
+  match:
+    source: "Generated"
+    scope: Namespaced
+    kinds:
+      - apiGroups: []
+        kinds: []
+    namespaceSelector:
+      matchExpressions:
+        - key: admission.gatekeeper.sh/ignore
+          operator: DoesNotExist
+
 `
 
 	AssignPullImageSourceAll = `

diff --git a/pkg/expansion/system.go b/pkg/expansion/system.go
@@ -208,10 +208,6 @@ func (s *System) expand(base *mutationtypes.Mutable) ([]*Resultant, error) {
 }
 
 func expandResource(obj *unstructured.Unstructured, ns *corev1.Namespace, template *expansionunversioned.ExpansionTemplate) (*unstructured.Unstructured, error) {
-	if ns == nil {
-		return nil, fmt.Errorf("cannot expand resource with nil namespace")
-	}
-
 	srcPath := template.Spec.TemplateSource
 	if srcPath == "" {
 		return nil, fmt.Errorf("cannot expand resource using a template with no source")
@@ -232,7 +228,20 @@ func expandResource(obj *unstructured.Unstructured, ns *corev1.Namespace, templa
 	resource := &unstructured.Unstructured{}
 	resource.SetUnstructuredContent(src)
 	resource.SetGroupVersionKind(resultantGVK)
-	resource.SetNamespace(ns.Name)
+	if ns != nil {
+		resource.SetNamespace(ns.Name)
+	} else {
+		nsFromUn, found, err := unstructured.NestedString(obj.Object, "metadata", "namespace")
+		if err != nil {
+			return nil, fmt.Errorf("could not extract namespace field %q in parent resource %s", srcPath, obj.GetName())
+		}
+
+		if found {
+			resource.SetNamespace(nsFromUn)
+		}
+		// if not found, then the resulting resource may be cluster scoped.
+	}
+
 	resource.SetName(mockNameForResource(obj, resultantGVK))
 
 	return resource, nil

diff --git a/pkg/expansion/system_test.go b/pkg/expansion/system_test.go
@@ -82,17 +82,32 @@ func TestExpand(t *testing.T) {
 			},
 		},
 		{
-			name:      "expand with nil namespace returns error",
-			generator: fixtures.LoadFixture(fixtures.DeploymentNginx, t),
+			name:      "expand with nil namespace returns error bc the namespace selector errs out",
+			generator: fixtures.LoadFixture(fixtures.DeploymentNginxWithNs, t),
 			ns:        nil,
 			mutators: []types.Mutator{
-				fixtures.LoadAssign(fixtures.AssignPullImage, t),
+				fixtures.LoadAssign(fixtures.AssignPullImageWithNsSelector, t),
 			},
 			templates: []*expansionunversioned.ExpansionTemplate{
 				fixtures.LoadTemplate(fixtures.TempExpDeploymentExpandsPods, t),
 			},
 			expectErr: true,
 		},
+		{
+			name:      "expand with nil namespace does not error out if no namespace selectors",
+			generator: fixtures.LoadFixture(fixtures.DeploymentNginxWithNs, t),
+			ns:        &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "not-default"}},
+			mutators: []types.Mutator{
+				fixtures.LoadAssign(fixtures.AssignPullImage, t),
+			},
+			templates: []*expansionunversioned.ExpansionTemplate{
+				fixtures.LoadTemplate(fixtures.TempExpDeploymentExpandsPods, t),
+			},
+			expectErr: false,
+			want: []*Resultant{
+				{Obj: fixtures.LoadFixture(fixtures.PodImagePullMutateWithNs, t), EnforcementAction: "", TemplateName: "expand-deployments"},
+			},
+		},
 		{
 			name:      "1 mutator source All deployment expands pod and mutates",
 			generator: fixtures.LoadFixture(fixtures.DeploymentNginx, t),

diff --git a/pkg/gator/expand/expand.go b/pkg/gator/expand/expand.go
@@ -4,9 +4,9 @@ import (
 	"fmt"
 
 	"github.com/open-policy-agent/gatekeeper/v3/apis/expansion/unversioned"
-	expansionv1 "github.com/open-policy-agent/gatekeeper/v3/apis/expansion/v1alpha1"
+	expansionv1alpha1 "github.com/open-policy-agent/gatekeeper/v3/apis/expansion/v1alpha1"
 	mutationsunversioned "github.com/open-policy-agent/gatekeeper/v3/apis/mutations/unversioned"
-	mutationsv1 "github.com/open-policy-agent/gatekeeper/v3/apis/mutations/v1alpha1"
+	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/v3/apis/mutations/v1alpha1"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/expansion"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/mutation"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/mutation/mutators/assign"
@@ -180,7 +180,6 @@ func (er *Expander) add(u *unstructured.Unstructured) error {
 	case isNamespace(u):
 		err = er.addNamespace(u)
 	}
-
 	if err == nil {
 		// Any resource can technically be a generator
 		er.objects = append(er.objects, u)
@@ -208,14 +207,14 @@ func (er *Expander) addNamespace(u *unstructured.Unstructured) error {
 }
 
 func isExpansion(u *unstructured.Unstructured) bool {
-	return u.GroupVersionKind().Group == expansionv1.GroupVersion.Group && u.GetKind() == "ExpansionTemplate"
+	return u.GroupVersionKind().Group == expansionv1alpha1.GroupVersion.Group && u.GetKind() == "ExpansionTemplate"
 }
 
 func isMutator(obj *unstructured.Unstructured) bool {
 	if _, exists := mutatorKinds[obj.GetKind()]; !exists {
 		return false
 	}
-	return obj.GroupVersionKind().Group == mutationsv1.GroupVersion.Group
+	return obj.GroupVersionKind().Group == mutationsv1alpha1.GroupVersion.Group
 }
 
 func isNamespace(obj *unstructured.Unstructured) bool {

diff --git a/pkg/mutation/mutators/assign/assign_mutator.go b/pkg/mutation/mutators/assign/assign_mutator.go
@@ -36,12 +36,12 @@ type Mutator struct {
 // Mutator implements mutatorWithSchema.
 var _ schema.MutatorWithSchema = &Mutator{}
 
-func (m *Mutator) Matches(mutable *types.Mutable) bool {
+func (m *Mutator) Matches(mutable *types.Mutable) (bool, error) {
 	res, err := core.MatchWithApplyTo(mutable, m.assign.Spec.ApplyTo, &m.assign.Spec.Match)
 	if err != nil {
 		log.Error(err, "Matches failed for assign", "assign", m.assign.Name)
 	}
-	return res
+	return res, err
 }
 
 func (m *Mutator) TerminalType() parser.NodeType {

diff --git a/pkg/mutation/mutators/assign/assign_mutator_test.go b/pkg/mutation/mutators/assign/assign_mutator_test.go
@@ -775,7 +775,8 @@ func TestApplyTo(t *testing.T) {
 				Version: test.version,
 				Kind:    test.kind,
 			})
-			matches := mutator.Matches(&types.Mutable{Object: obj, Source: types.SourceTypeDefault})
+			matches, err := mutator.Matches(&types.Mutable{Object: obj, Source: types.SourceTypeDefault})
+			require.NoError(t, err)
 			if matches != test.matchExpected {
 				t.Errorf("Matches() = %t, expected %t", matches, test.matchExpected)
 			}

diff --git a/pkg/mutation/mutators/assignimage/assignimage_mutator.go b/pkg/mutation/mutators/assignimage/assignimage_mutator.go
@@ -34,12 +34,12 @@ type Mutator struct {
 // Mutator implements mutatorWithSchema.
 var _ schema.MutatorWithSchema = &Mutator{}
 
-func (m *Mutator) Matches(mutable *types.Mutable) bool {
+func (m *Mutator) Matches(mutable *types.Mutable) (bool, error) {
 	res, err := core.MatchWithApplyTo(mutable, m.assignImage.Spec.ApplyTo, &m.assignImage.Spec.Match)
 	if err != nil {
 		log.Error(err, "Matches failed for assign image", "assignImage", m.assignImage.Name)
 	}
-	return res
+	return res, err
 }
 
 func (m *Mutator) TerminalType() parser.NodeType {

diff --git a/pkg/mutation/mutators/assignmeta/assignmeta_mutator.go b/pkg/mutation/mutators/assignmeta/assignmeta_mutator.go
@@ -54,7 +54,7 @@ type Mutator struct {
 // Mutator implements mutator.
 var _ types.Mutator = &Mutator{}
 
-func (m *Mutator) Matches(mutable *types.Mutable) bool {
+func (m *Mutator) Matches(mutable *types.Mutable) (bool, error) {
 	target := &match.Matchable{
 		Object:    mutable.Object,
 		Namespace: mutable.Namespace,
@@ -63,9 +63,8 @@ func (m *Mutator) Matches(mutable *types.Mutable) bool {
 	matches, err := match.Matches(&m.assignMetadata.Spec.Match, target)
 	if err != nil {
 		log.Error(err, "Matches failed for assign metadata", "assignMeta", m.assignMetadata.Name)
-		return false
 	}
-	return matches
+	return matches, err
 }
 
 func (m *Mutator) Mutate(mutable *types.Mutable) (bool, error) {

diff --git a/pkg/mutation/mutators/modifyset/modify_set_mutator.go b/pkg/mutation/mutators/modifyset/modify_set_mutator.go
@@ -36,12 +36,12 @@ type Mutator struct {
 // Mutator implements mutatorWithSchema.
 var _ schema.MutatorWithSchema = &Mutator{}
 
-func (m *Mutator) Matches(mutable *types.Mutable) bool {
+func (m *Mutator) Matches(mutable *types.Mutable) (bool, error) {
 	res, err := core.MatchWithApplyTo(mutable, m.modifySet.Spec.ApplyTo, &m.modifySet.Spec.Match)
 	if err != nil {
 		log.Error(err, "Matches failed for modify set", "modifyset", m.modifySet.Name)
 	}
-	return res
+	return res, err
 }
 
 func (m *Mutator) TerminalType() parser.NodeType {

diff --git a/pkg/mutation/mutators/testhelpers/dummy_mutator.go b/pkg/mutation/mutators/testhelpers/dummy_mutator.go
@@ -36,13 +36,13 @@ func (d *DummyMutator) Path() parser.Path {
 	return d.path
 }
 
-func (d *DummyMutator) Matches(mutable *types.Mutable) bool {
+func (d *DummyMutator) Matches(mutable *types.Mutable) (bool, error) {
 	m := &match.Matchable{Object: mutable.Object, Namespace: mutable.Namespace}
 	matches, err := match.Matches(&d.match, m)
 	if err != nil {
-		return false
+		return false, err
 	}
-	return matches
+	return matches, nil
 }
 
 func (d *DummyMutator) Mutate(mutable *types.Mutable) (bool, error) {

diff --git a/pkg/mutation/schema/schema_test.go b/pkg/mutation/schema/schema_test.go
@@ -34,7 +34,7 @@ func newFakeMutator(id types.ID, pathStr string, usesExternalData bool, bindings
 	}
 }
 
-func (m *fakeMutator) Matches(*types.Mutable) bool {
+func (m *fakeMutator) Matches(*types.Mutable) (bool, error) {
 	panic("should not be called")
 }
 

diff --git a/pkg/mutation/system.go b/pkg/mutation/system.go
@@ -183,7 +183,12 @@ func (s *System) mutate(mutable *types.Mutable) (int, error) {
 			}
 
 			mutator := s.mutatorsMap[id]
-			if mutator.Matches(mutable) {
+			matches, err := mutator.Matches(mutable)
+			if err != nil {
+				return iteration, matchesErr(err, mutator.ID(), mutable.Object)
+			}
+
+			if matches {
 				mutated, err := mutator.Mutate(mutable)
 				if mutated {
 					appliedMutations = append(appliedMutations, mutator)
@@ -244,3 +249,12 @@ func mutateErr(err error, uid uuid.UUID, mID types.ID, obj *unstructured.Unstruc
 		obj.GetNamespace(),
 		obj.GetName())
 }
+
+func matchesErr(err error, mID types.ID, obj *unstructured.Unstructured) error {
+	return errors.Wrapf(err, "matching for mutator %v failed for %s %s %s %s",
+		mID,
+		obj.GroupVersionKind().Group,
+		obj.GroupVersionKind().Kind,
+		obj.GetNamespace(),
+		obj.GetName())
+}
diff --git a/pkg/mutation/system_errors_test.go b/pkg/mutation/system_errors_test.go
@@ -38,8 +38,8 @@ type errorMutator struct {
 
 var _ types.Mutator = &errorMutator{}
 
-func (e errorMutator) Matches(*types.Mutable) bool {
-	return true
+func (e errorMutator) Matches(*types.Mutable) (bool, error) {
+	return true, nil
 }
 
 func (e errorMutator) Mutate(*types.Mutable) (bool, error) {

diff --git a/pkg/mutation/system_test.go b/pkg/mutation/system_test.go
@@ -36,8 +36,8 @@ type fakeMutator struct {
 	Default       *types.Anything
 }
 
-func (m *fakeMutator) Matches(*types.Mutable) bool {
-	return true // always matches
+func (m *fakeMutator) Matches(*types.Mutable) (bool, error) {
+	return true, nil // always matches
 }
 
 func (m *fakeMutator) Mutate(mutable *types.Mutable) (bool, error) {

diff --git a/pkg/mutation/types/mutator.go b/pkg/mutation/types/mutator.go
@@ -48,7 +48,7 @@ func IsValidSource(src SourceType) bool {
 // Mutator represent a mutation object.
 type Mutator interface {
 	// Matches tells if the given object is eligible for this mutation.
-	Matches(mutable *Mutable) bool
+	Matches(mutable *Mutable) (bool, error)
 	// Mutate applies the mutation to the given object
 	Mutate(mutable *Mutable) (bool, error)
 	// MustTerminate returns true if the mutator requires its path to terminate

diff --git a/pkg/target/matcher.go b/pkg/target/matcher.go
@@ -56,7 +56,7 @@ func matchAny(m *Matcher, ns *corev1.Namespace, source types.SourceType, objs ..
 		}
 		matched, err := match.Matches(m.match, t)
 		if err != nil {
-			return false, fmt.Errorf("%w: %w", ErrMatching, err)
+			return false, fmt.Errorf("%w: %v :%w", ErrMatching, obj.GetName(), err)
 		}
 
 		if matched {

diff --git a/test/gator/expand/fixtures/expand-with-missing-ns/input/manifest.yaml b/test/gator/expand/fixtures/expand-with-missing-ns/input/manifest.yaml
@@ -38,3 +38,6 @@ spec:
         args:
         - "/bin/sh"
 ---
+# while this is missing the "my-ns" namespace
+# since there are no namespace selectors
+# this test will pass.
diff --git a/test/gator/expand/test.bats b/test/gator/expand/test.bats
@@ -52,8 +52,10 @@ test_dir () {
  test_dir "expand-with-ns" 0
 }
 
+# the matching system will not err out when the namespace for
+# a resource is not defined but we DON'T use a namespace selector.
 @test "generator with a custom namespace but namespace config missing" {
- test_dir "expand-with-missing-ns" 1
+ test_dir "expand-with-missing-ns" 0
 }
 
 @test "expand into 2 resultants and write to file" {