fix: process the matched resources only for mutate existing policies (#10164)

* fix: process the matched resources only for mutate existing policies Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com> * fix lint issue Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com> * chore: add unit tests Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com> --------- Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com> Co-authored-by: shuting <shuting@nirmata.com>
2024-12-14 11:57:48 +00:00 · 2024-05-20 19:40:53 +07:00 · 2024-05-20 19:40:53 +07:00 · 59ff771ae8
commit 59ff771ae8
parent 5534ac335a
5 changed files with 210 additions and 140 deletions
--- a/pkg/policy/generate.go
+++ b/pkg/policy/generate.go
@ -40,7 +40,7 @@ func (pc *policyController) handleGenerateForExisting(policy kyvernov1.PolicyInt
 	var errors []error
 	for _, rule := range policy.GetSpec().Rules {
 		ruleType := kyvernov1beta1.Generate
-		triggers := generateTriggers(pc.client, rule, pc.log)
+		triggers := getTriggers(pc.client, rule, policy.IsNamespaced(), policy.GetNamespace(), pc.log)
 		for _, trigger := range triggers {
 			ur := newUR(policy, common.ResourceSpecFromUnstructured(*trigger), rule.Name, ruleType, false)
 			skip, err := pc.handleUpdateRequest(ur, trigger, rule, policy)
--- a/pkg/policy/mutate.go
+++ b/pkg/policy/mutate.go
@ -18,7 +18,7 @@ func (pc *policyController) handleMutate(policyKey string, policy kyvernov1.Poli
 		var ruleType kyvernov1beta1.RequestType
 		if rule.HasMutateExisting() {
 			ruleType = kyvernov1beta1.Mutate
-			triggers := generateTriggers(pc.client, rule, pc.log)
+			triggers := getTriggers(pc.client, rule, policy.IsNamespaced(), policy.GetNamespace(), pc.log)
 			for _, trigger := range triggers {
 				murs := pc.listMutateURs(policyKey, trigger)
 				if murs != nil {
--- a/pkg/policy/policy_controller.go
+++ b/pkg/policy/policy_controller.go
@ -424,18 +424,58 @@ func (pc *policyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest
 	return false, err
 }

-func generateTriggers(client dclient.Interface, rule kyvernov1.Rule, log logr.Logger) []*unstructured.Unstructured {
-	list := &unstructured.UnstructuredList{}
+func getTriggers(client dclient.Interface, rule kyvernov1.Rule, isNamespacedPolicy bool, policyNamespace string, log logr.Logger) []*unstructured.Unstructured {
+	var resources []*unstructured.Unstructured

-	kinds := fetchUniqueKinds(rule)
+	appendResources := func(match kyvernov1.ResourceDescription) {
+		resources = append(resources, getResources(client, policyNamespace, isNamespacedPolicy, match, log)...)
+	}

-	for _, kind := range kinds {
-		mlist, err := client.ListResource(context.TODO(), "", kind, "", rule.MatchResources.Selector)
+	if !rule.MatchResources.ResourceDescription.IsEmpty() {
+		appendResources(rule.MatchResources.ResourceDescription)
+	}
+
+	for _, any := range rule.MatchResources.Any {
+		appendResources(any.ResourceDescription)
+	}
+
+	for _, all := range rule.MatchResources.All {
+		appendResources(all.ResourceDescription)
+	}
+
+	return resources
+}
+
+func getResources(client dclient.Interface, policyNs string, isNamespacedPolicy bool, match kyvernov1.ResourceDescription, log logr.Logger) []*unstructured.Unstructured {
+	var items []*unstructured.Unstructured
+
+	for _, kind := range match.Kinds {
+		group, version, kind, _ := kubeutils.ParseKindSelector(kind)
+
+		namespace := ""
+		if isNamespacedPolicy {
+			namespace = policyNs
+		}
+
+		groupVersion := ""
+		if group != "*" && version != "*" {
+			groupVersion = group + "/" + version
+		} else if version != "*" {
+			groupVersion = version
+		}
+
+		resources, err := client.ListResource(context.TODO(), groupVersion, kind, namespace, match.Selector)
 		if err != nil {
 			log.Error(err, "failed to list matched resource")
 			continue
 		}
-		list.Items = append(list.Items, mlist.Items...)
+
+		for i, res := range resources.Items {
+			if !resourceMatches(match, res, isNamespacedPolicy) {
+				continue
+			}
+			items = append(items, &resources.Items[i])
+		}
 	}
-	return convertlist(list.Items)
+	return items
 }
--- a/pkg/policy/utils.go
+++ b/pkg/policy/utils.go
@ -3,29 +3,28 @@ package policy
 import (
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/util/sets"
 )

-func fetchUniqueKinds(rule kyvernov1.Rule) []string {
-	kinds := sets.New(rule.MatchResources.Kinds...)
-
-	for _, any := range rule.MatchResources.Any {
-		kinds.Insert(any.Kinds...)
+func resourceMatches(match kyvernov1.ResourceDescription, res unstructured.Unstructured, isNamespacedPolicy bool) bool {
+	if match.Name != "" && res.GetName() != match.Name {
+		return false
 	}
-
-	for _, all := range rule.MatchResources.All {
-		kinds.Insert(all.Kinds...)
+	if len(match.Names) > 0 && !contains(match.Names, res.GetName()) {
+		return false
 	}
-
-	return kinds.UnsortedList()
+	if !isNamespacedPolicy && len(match.Namespaces) > 0 && !contains(match.Namespaces, res.GetNamespace()) {
+		return false
+	}
+	return true
 }

-func convertlist(ulists []unstructured.Unstructured) []*unstructured.Unstructured {
-	result := make([]*unstructured.Unstructured, 0, len(ulists))
-	for _, list := range ulists {
-		result = append(result, list.DeepCopy())
+func contains(slice []string, item string) bool {
+	for _, s := range slice {
+		if s == item {
+			return true
+		}
 	}
-	return result
+	return false
 }

 func castPolicy(p interface{}) kyvernov1.PolicyInterface {
--- a/pkg/policy/utils_test.go
+++ b/pkg/policy/utils_test.go
@ -1,148 +1,179 @@
 package policy

 import (
-	"fmt"
 	"testing"

 	kyverno "github.com/kyverno/kyverno/api/kyverno/v1"
-	kubeutils "github.com/kyverno/kyverno/pkg/utils/kube"
-	"gotest.tools/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )

-func Test_fetchUniqueKinds(t *testing.T) {
-
+func Test_resourceMatches(t *testing.T) {
 	tests := []struct {
-		name string
-		rule kyverno.Rule
-		want []string
+		name               string
+		match              kyverno.ResourceDescription
+		res                unstructured.Unstructured
+		isNamespacedPolicy bool
+		want               bool
 	}{
 		{
-			name: "Unique MatchResource kinds",
-			rule: kyverno.Rule{
-				MatchResources: kyverno.MatchResources{
-					ResourceDescription: kyverno.ResourceDescription{
-						Kinds: []string{"kind1", "kind2"},
+			name: "Matching resource based on its name",
+			match: kyverno.ResourceDescription{
+				Kinds: []string{"Pod"},
+				Names: []string{"my-pod", "test-pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name": "my-pod",
 					},
 				},
 			},
-			want: []string{"kind1", "kind2"},
-		},
-
-		{
-			name: "Any with same kind are valid",
-			rule: kyverno.Rule{
-				MatchResources: kyverno.MatchResources{
-					Any: []kyverno.ResourceFilter{
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind1", "kind2"},
-							},
-						},
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind1", "kind3"},
-							},
-						},
-					},
-				},
-			},
-			want: []string{"kind1", "kind2", "kind3"},
+			isNamespacedPolicy: false,
+			want:               true,
 		},
 		{
-			name: "Match with All and Any kind",
-			rule: kyverno.Rule{
-				MatchResources: kyverno.MatchResources{
-					All: []kyverno.ResourceFilter{
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind1"},
-							},
-						},
-					},
-					Any: []kyverno.ResourceFilter{
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind1", "kind2"},
-							},
-						},
+			name: "Non-matching resource based on its name",
+			match: kyverno.ResourceDescription{
+				Kinds: []string{"Pod"},
+				Names: []string{"test-pod", "test-pod-1"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name": "my-pod",
 					},
 				},
 			},
-			want: []string{"kind1", "kind2"},
+			isNamespacedPolicy: false,
+			want:               false,
 		},
 		{
-			name: "Match with different All and Any kind",
-			rule: kyverno.Rule{
-				MatchResources: kyverno.MatchResources{
-					All: []kyverno.ResourceFilter{
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind4", "kind5"},
-							},
-						},
-					},
-					Any: []kyverno.ResourceFilter{
-						{
-							ResourceDescription: kyverno.ResourceDescription{
-								Kinds: []string{"kind1", "kind2"},
-							},
-						},
+			name: "Matching resource based on its namespace",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "test-ns",
 					},
 				},
 			},
-			want: []string{"kind1", "kind2", "kind4", "kind5"},
+			isNamespacedPolicy: false,
+			want:               true,
+		},
+		{
+			name: "Non-matching resource based on its namespace",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "default",
+					},
+				},
+			},
+			isNamespacedPolicy: false,
+			want:               false,
+		},
+		{
+			name: "Matching resource with a namespaced policy",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "default",
+					},
+				},
+			},
+			isNamespacedPolicy: true,
+			want:               true,
+		},
+		{
+			name: "Matching resource based on its name and namespace",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+				Names:      []string{"my-pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "test-ns",
+					},
+				},
+			},
+			isNamespacedPolicy: false,
+			want:               true,
+		},
+		{
+			name: "Non-matching resource based on its name and namespace",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+				Names:      []string{"my-pod"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "default",
+					},
+				},
+			},
+			isNamespacedPolicy: false,
+			want:               false,
+		},
+		{
+			name: "Non-matching resource based on its name and namespace",
+			match: kyverno.ResourceDescription{
+				Namespaces: []string{"test-ns"},
+				Kinds:      []string{"Pod"},
+				Names:      []string{"test-pod-1", "test-pod-2"},
+			},
+			res: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Pod",
+					"metadata": map[string]interface{}{
+						"name":      "my-pod",
+						"namespace": "test-ns",
+					},
+				},
+			},
+			isNamespacedPolicy: false,
+			want:               false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			kinds := fetchUniqueKinds(tt.rule)
-			for _, want := range tt.want {
-				if !kubeutils.ContainsKind(kinds, want) {
-					assert.Error(t, fmt.Errorf("%s fails, expected %s", tt.name, want), "")
-				}
+			if got := resourceMatches(tt.match, tt.res, tt.isNamespacedPolicy); got != tt.want {
+				t.Errorf("resourceMatches() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }
-
-func Test_convertlist(t *testing.T) {
-	tests := []struct {
-		name   string
-		ulists []unstructured.Unstructured
-		want   []*unstructured.Unstructured
-	}{
-		{
-			name: "Convert list",
-			ulists: []unstructured.Unstructured{
-				{
-					Object: map[string]interface{}{
-						"kind": "kind1",
-					},
-				},
-				{
-					Object: map[string]interface{}{
-						"namespace": "ns-1",
-					},
-				},
-			},
-			want: []*unstructured.Unstructured{
-				{
-					Object: map[string]interface{}{
-						"kind": "kind1",
-					},
-				},
-				{
-					Object: map[string]interface{}{
-						"namespace": "ns-1",
-					},
-				},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			assert.DeepEqual(t, convertlist(tt.ulists), tt.want)
-		})
-	}
-}