fix: avoid creating duplicate urs for background policies (kyverno#10431

) * feat: add generator abstraction Signed-off-by: ShutingZhao <[email protected]> * feat: replace urgenerator Signed-off-by: ShutingZhao <[email protected]> * fix: ko build Signed-off-by: ShutingZhao <[email protected]> * feat: load threshold from kyverno configmap Signed-off-by: ShutingZhao <[email protected]> * feat: add metadata client to get ur count Signed-off-by: ShutingZhao <[email protected]> * feat: add helm option to preserve configmap settings during upgrade Signed-off-by: ShutingZhao <[email protected]> * feat: add helm option to preserve configmap settings during upgrade 2 Signed-off-by: ShutingZhao <[email protected]> * chore: rename imports Signed-off-by: ShutingZhao <[email protected]> * chore: update codegen manifests Signed-off-by: ShutingZhao <[email protected]> * fix: handle nil value Signed-off-by: ShutingZhao <[email protected]> * fix: linter issue Signed-off-by: ShutingZhao <[email protected]> * chore: update threshold to 1000 Signed-off-by: ShutingZhao <[email protected]> * fix: avoid duplicate URs creation Signed-off-by: ShutingZhao <[email protected]> * fix: revert false changes Signed-off-by: ShutingZhao <[email protected]> * fix: simplify background applications Signed-off-by: ShutingZhao <[email protected]> --------- Signed-off-by: ShutingZhao <[email protected]>
MariamFahmy98 · Jun 12, 2024 · fe8c429 · fe8c429
1 parent 73e6aaa
commit fe8c429
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 38 deletions.
diff --git a/api/kyverno/v1beta1/updaterequest_types.go b/api/kyverno/v1beta1/updaterequest_types.go
@@ -48,6 +48,7 @@ type UpdateRequestStatus struct {
 // +kubebuilder:storageversion
 // +kubebuilder:subresource:status
 // +kubebuilder:printcolumn:name="Policy",type="string",JSONPath=".spec.policy"
+// +kubebuilder:printcolumn:name="Rule",type="string",JSONPath=".spec.rule"
 // +kubebuilder:printcolumn:name="RuleType",type="string",JSONPath=".spec.requestType"
 // +kubebuilder:printcolumn:name="ResourceKind",type="string",JSONPath=".spec.resource.kind"
 // +kubebuilder:printcolumn:name="ResourceName",type="string",JSONPath=".spec.resource.name"

diff --git a/charts/kyverno/charts/crds/templates/kyverno.io/kyverno.io_updaterequests.yaml b/charts/kyverno/charts/crds/templates/kyverno.io/kyverno.io_updaterequests.yaml
@@ -28,6 +28,9 @@ spec:
     - jsonPath: .spec.policy
       name: Policy
       type: string
+    - jsonPath: .spec.rule
+      name: Rule
+      type: string
     - jsonPath: .spec.requestType
       name: RuleType
       type: string

diff --git a/config/crds/kyverno/kyverno.io_updaterequests.yaml b/config/crds/kyverno/kyverno.io_updaterequests.yaml
@@ -22,6 +22,9 @@ spec:
     - jsonPath: .spec.policy
       name: Policy
       type: string
+    - jsonPath: .spec.rule
+      name: Rule
+      type: string
     - jsonPath: .spec.requestType
       name: RuleType
       type: string

diff --git a/config/install-latest-testing.yaml b/config/install-latest-testing.yaml
@@ -46169,6 +46169,9 @@ spec:
     - jsonPath: .spec.policy
       name: Policy
       type: string
+    - jsonPath: .spec.rule
+      name: Rule
+      type: string
     - jsonPath: .spec.requestType
       name: RuleType
       type: string

diff --git a/pkg/policy/generate.go b/pkg/policy/generate.go
@@ -13,6 +13,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/config"
 	"go.uber.org/multierr"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 func (pc *policyController) handleGenerate(policyKey string, policy kyvernov1.PolicyInterface) error {
@@ -38,14 +39,19 @@ func (pc *policyController) handleGenerate(policyKey string, policy kyvernov1.Po
 
 func (pc *policyController) handleGenerateForExisting(policy kyvernov1.PolicyInterface) error {
 	var errors []error
+	var triggers []*unstructured.Unstructured
+	ruleType := kyvernov1beta1.Generate
+	policyNew := policy.CreateDeepCopy()
+	policyNew.GetSpec().Rules = nil
+
 	for _, rule := range policy.GetSpec().Rules {
-		ruleType := kyvernov1beta1.Generate
-		triggers := getTriggers(pc.client, rule, policy.IsNamespaced(), policy.GetNamespace(), pc.log)
+		triggers = getTriggers(pc.client, rule, policy.IsNamespaced(), policy.GetNamespace(), pc.log)
+		policyNew.GetSpec().SetRules([]kyvernov1.Rule{rule})
 		for _, trigger := range triggers {
-			ur := newUR(policy, common.ResourceSpecFromUnstructured(*trigger), rule.Name, ruleType, false)
-			skip, err := pc.handleUpdateRequest(ur, trigger, rule, policy)
+			ur := newUR(policyNew, common.ResourceSpecFromUnstructured(*trigger), rule.Name, ruleType, false)
+			skip, err := pc.handleUpdateRequest(ur, trigger, rule.Name, policyNew)
 			if err != nil {
-				pc.log.Error(err, "failed to create new UR on policy update", "policy", policy.GetName(), "rule", rule.Name, "rule type", ruleType,
+				pc.log.Error(err, "failed to create new UR on policy update", "policy", policyNew.GetName(), "rule", rule.Name, "rule type", ruleType,
 					"target", fmt.Sprintf("%s/%s/%s/%s", trigger.GetAPIVersion(), trigger.GetKind(), trigger.GetNamespace(), trigger.GetName()))
 				errors = append(errors, err)
 				continue
@@ -55,7 +61,7 @@ func (pc *policyController) handleGenerateForExisting(policy kyvernov1.PolicyInt
 				continue
 			}
 
-			pc.log.V(4).Info("successfully created UR on policy update", "policy", policy.GetName(), "rule", rule.Name, "rule type", ruleType,
+			pc.log.V(4).Info("successfully created UR on policy update", "policy", policyNew.GetName(), "rule", rule.Name, "rule type", ruleType,
 				"target", fmt.Sprintf("%s/%s/%s/%s", trigger.GetAPIVersion(), trigger.GetKind(), trigger.GetNamespace(), trigger.GetName()))
 		}
 	}
@@ -112,15 +118,11 @@ func (pc *policyController) syncDataRulechanges(policy kyvernov1.PolicyInterface
 		labels := downstream.GetLabels()
 		trigger := generateutils.TriggerFromLabels(labels)
 		ur := newUR(policy, trigger, rule.Name, kyvernov1beta1.Generate, deleteDownstream)
-
-		created, err := pc.urGenerator.Generate(context.TODO(), pc.kyvernoClient, ur, pc.log)
+		created, err := pc.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Create(context.TODO(), ur, metav1.CreateOptions{})
 		if err != nil {
 			errorList = append(errorList, err)
 			continue
 		}
-		if created == nil {
-			continue
-		}
 		updated := created.DeepCopy()
 		updated.Status = newURStatus(downstream)
 		_, err = pc.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{})

diff --git a/pkg/policy/mutate.go b/pkg/policy/mutate.go
@@ -12,34 +12,39 @@ import (
 
 func (pc *policyController) handleMutate(policyKey string, policy kyvernov1.PolicyInterface) error {
 	logger := pc.log.WithName("handleMutate").WithName(policyKey)
-
 	logger.Info("update URs on policy event")
+
+	ruleType := kyvernov1beta1.Mutate
+	policyNew := policy.CreateDeepCopy()
+	policyNew.GetSpec().Rules = nil
+
 	for _, rule := range policy.GetSpec().Rules {
-		var ruleType kyvernov1beta1.RequestType
-		if rule.HasMutateExisting() {
-			ruleType = kyvernov1beta1.Mutate
-			triggers := getTriggers(pc.client, rule, policy.IsNamespaced(), policy.GetNamespace(), pc.log)
-			for _, trigger := range triggers {
-				murs := pc.listMutateURs(policyKey, trigger)
-				if murs != nil {
-					logger.V(4).Info("UR was created", "rule", rule.Name, "rule type", ruleType, "trigger", trigger.GetNamespace()+trigger.GetName())
-					continue
-				}
-
-				logger.Info("creating new UR for mutate")
-				ur := newUR(policy, backgroundcommon.ResourceSpecFromUnstructured(*trigger), rule.Name, ruleType, false)
-				skip, err := pc.handleUpdateRequest(ur, trigger, rule, policy)
-				if err != nil {
-					pc.log.Error(err, "failed to create new UR on policy update", "policy", policy.GetName(), "rule", rule.Name, "rule type", ruleType,
-						"target", fmt.Sprintf("%s/%s/%s/%s", trigger.GetAPIVersion(), trigger.GetKind(), trigger.GetNamespace(), trigger.GetName()))
-					continue
-				}
-				if skip {
-					continue
-				}
-				pc.log.V(2).Info("successfully created UR on policy update", "policy", policy.GetName(), "rule", rule.Name, "rule type", ruleType,
+		if !rule.HasMutateExisting() {
+			continue
+		}
+
+		policyNew.GetSpec().SetRules([]kyvernov1.Rule{rule})
+		triggers := getTriggers(pc.client, rule, policyNew.IsNamespaced(), policyNew.GetNamespace(), pc.log)
+		for _, trigger := range triggers {
+			murs := pc.listMutateURs(policyKey, trigger)
+			if murs != nil {
+				logger.V(4).Info("UR was created", "rule", rule.Name, "rule type", ruleType, "trigger", trigger.GetNamespace()+trigger.GetName())
+				continue
+			}
+
+			logger.Info("creating new UR for mutate")
+			ur := newUR(policy, backgroundcommon.ResourceSpecFromUnstructured(*trigger), rule.Name, ruleType, false)
+			skip, err := pc.handleUpdateRequest(ur, trigger, rule.Name, policyNew)
+			if err != nil {
+				pc.log.Error(err, "failed to create new UR on policy update", "policy", policyNew.GetName(), "rule", rule.Name, "rule type", ruleType,
 					"target", fmt.Sprintf("%s/%s/%s/%s", trigger.GetAPIVersion(), trigger.GetKind(), trigger.GetNamespace(), trigger.GetName()))
+				continue
+			}
+			if skip {
+				continue
 			}
+			pc.log.V(2).Info("successfully created UR on policy update", "policy", policyNew.GetName(), "rule", rule.Name, "rule type", ruleType,
+				"target", fmt.Sprintf("%s/%s/%s/%s", trigger.GetAPIVersion(), trigger.GetKind(), trigger.GetNamespace(), trigger.GetName()))
 		}
 	}
 	return nil

diff --git a/pkg/policy/policy_controller.go b/pkg/policy/policy_controller.go
@@ -396,11 +396,11 @@ func (pc *policyController) requeuePolicies() {
 	}
 }
 
-func (pc *policyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest, triggerResource *unstructured.Unstructured, rule kyvernov1.Rule, policy kyvernov1.PolicyInterface) (skip bool, err error) {
+func (pc *policyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest, triggerResource *unstructured.Unstructured, ruleName string, policy kyvernov1.PolicyInterface) (skip bool, err error) {
 	namespaceLabels := engineutils.GetNamespaceSelectorsFromNamespaceLister(triggerResource.GetKind(), triggerResource.GetNamespace(), pc.nsLister, pc.log)
 	policyContext, err := backgroundcommon.NewBackgroundContext(pc.log, pc.client, ur, policy, triggerResource, pc.configuration, pc.jp, namespaceLabels)
 	if err != nil {
-		return false, fmt.Errorf("failed to build policy context for rule %s: %w", rule.Name, err)
+		return false, fmt.Errorf("failed to build policy context for rule %s: %w", ruleName, err)
 	}
 
 	engineResponse := pc.engine.ApplyBackgroundChecks(context.TODO(), policyContext)
@@ -410,7 +410,11 @@ func (pc *policyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest
 
 	for _, ruleResponse := range engineResponse.PolicyResponse.Rules {
 		if ruleResponse.Status() != engineapi.RuleStatusPass {
-			pc.log.V(4).Info("skip creating URs on policy update", "policy", policy.GetName(), "rule", rule.Name, "rule.Status", ruleResponse.Status())
+			pc.log.V(4).Info("skip creating URs on policy update", "policy", policy.GetName(), "rule", ruleName, "rule.Status", ruleResponse.Status())
+			continue
+		}
+
+		if ruleResponse.Name() != ur.Spec.GetRuleName() {
 			continue
 		}