From eab6b4ecebee176ca2cffd71d1dd783649097cfd Mon Sep 17 00:00:00 2001 From: Mariam Fahmy Date: Fri, 15 Dec 2023 16:42:10 +0200 Subject: [PATCH] fix: updaterequests stuck in pending/fail infinite loop (#9119) * fix: updaterequests stuck in pending/fail infinite loop Signed-off-by: Mariam Fahmy * fix: prevent creating URs upon DELETE unless it is specified Signed-off-by: Mariam Fahmy * fix chainsaw test Signed-off-by: Mariam Fahmy --------- Signed-off-by: Mariam Fahmy --- pkg/webhooks/resource/generation/handler.go | 3 +- pkg/webhooks/resource/generation/utils.go | 10 ----- pkg/webhooks/resource/updaterequest.go | 13 +++++- pkg/webhooks/utils/match.go | 17 ++++++++ .../chainsaw-step-01-apply-1-4.yaml | 8 ++-- .../chainsaw-test.yaml | 41 +++++++++++++++++++ .../delete-trigger-namespace/configmap.yaml | 7 ++++ .../delete-trigger-namespace/namespace.yaml | 4 ++ .../patched-secret.yaml | 7 ++++ .../policy-ready.yaml | 9 ++++ .../delete-trigger-namespace/policy.yaml | 27 ++++++++++++ .../delete-trigger-namespace/secret.yaml | 7 ++++ 12 files changed, 136 insertions(+), 17 deletions(-) create mode 100644 pkg/webhooks/utils/match.go create mode 100755 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/chainsaw-test.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/configmap.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/namespace.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/patched-secret.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy-ready.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy.yaml create mode 100644 test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/secret.yaml diff --git a/pkg/webhooks/resource/generation/handler.go b/pkg/webhooks/resource/generation/handler.go index 4442cadac4..e51260ecb2 100644 --- a/pkg/webhooks/resource/generation/handler.go +++ b/pkg/webhooks/resource/generation/handler.go @@ -21,6 +21,7 @@ import ( "github.com/kyverno/kyverno/pkg/metrics" utils "github.com/kyverno/kyverno/pkg/utils/engine" webhookgenerate "github.com/kyverno/kyverno/pkg/webhooks/updaterequest" + webhookutils "github.com/kyverno/kyverno/pkg/webhooks/utils" admissionv1 "k8s.io/api/admission/v1" corev1listers "k8s.io/client-go/listers/core/v1" ) @@ -203,7 +204,7 @@ func (h *generationHandler) syncTriggerAction( rules := getAppliedRules(policy, failedRules) for _, rule := range rules { // fire generation on trigger deletion - if (request.Operation == admissionv1.Delete) && matchDeleteOperation(rule) { + if (request.Operation == admissionv1.Delete) && webhookutils.MatchDeleteOperation(rule) { h.log.V(4).Info("creating the UR to generate downstream on trigger's deletion", "operation", request.Operation, "rule", rule.Name) ur := buildURSpec(kyvernov1beta1.Generate, pKey, rule.Name, urSpec, false) ur.Context = buildURContext(request, policyContext) diff --git a/pkg/webhooks/resource/generation/utils.go b/pkg/webhooks/resource/generation/utils.go index 4d73a8389a..6b62218c90 100644 --- a/pkg/webhooks/resource/generation/utils.go +++ b/pkg/webhooks/resource/generation/utils.go @@ -4,7 +4,6 @@ import ( kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" "github.com/kyverno/kyverno/pkg/engine" - datautils "github.com/kyverno/kyverno/pkg/utils/data" admissionv1 "k8s.io/api/admission/v1" ) @@ -27,12 +26,3 @@ func buildURContext(request admissionv1.AdmissionRequest, policyContext *engine. }, } } - -func matchDeleteOperation(rule kyvernov1.Rule) bool { - ops := rule.MatchResources.GetOperations() - for _, rscFilters := range append(rule.MatchResources.All, rule.MatchResources.Any...) { - ops = append(ops, rscFilters.ResourceDescription.GetOperations()...) - } - - return datautils.SliceContains(ops, string(admissionv1.Delete)) -} diff --git a/pkg/webhooks/resource/updaterequest.go b/pkg/webhooks/resource/updaterequest.go index 3b037f76c7..7d5afb2cc6 100644 --- a/pkg/webhooks/resource/updaterequest.go +++ b/pkg/webhooks/resource/updaterequest.go @@ -8,10 +8,13 @@ import ( "github.com/go-logr/logr" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" + "github.com/kyverno/kyverno/pkg/autogen" "github.com/kyverno/kyverno/pkg/engine" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/event" + datautils "github.com/kyverno/kyverno/pkg/utils/data" "github.com/kyverno/kyverno/pkg/webhooks/resource/generation" + webhookutils "github.com/kyverno/kyverno/pkg/webhooks/utils" admissionv1 "k8s.io/api/admission/v1" ) @@ -36,12 +39,20 @@ func (h *resourceHandlers) handleMutateExisting(ctx context.Context, logger logr } logger.V(4).Info("update request for mutateExisting policy") + // skip rules that don't specify the DELETE operation in case the admission request is of type DELETE + var skipped []string + for _, rule := range autogen.ComputeRules(policy) { + if request.Operation == admissionv1.Delete && !webhookutils.MatchDeleteOperation(rule) { + skipped = append(skipped, rule.Name) + } + } + var rules []engineapi.RuleResponse policyContext := policyContext.WithPolicy(policy) engineResponse := h.engine.ApplyBackgroundChecks(ctx, policyContext) for _, rule := range engineResponse.PolicyResponse.Rules { - if rule.Status() == engineapi.RuleStatusPass { + if rule.Status() == engineapi.RuleStatusPass && !datautils.SliceContains(skipped, rule.Name()) { rules = append(rules, rule) } } diff --git a/pkg/webhooks/utils/match.go b/pkg/webhooks/utils/match.go new file mode 100644 index 0000000000..905bdf28cd --- /dev/null +++ b/pkg/webhooks/utils/match.go @@ -0,0 +1,17 @@ +package utils + +import ( + kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" + datautils "github.com/kyverno/kyverno/pkg/utils/data" + admissionv1 "k8s.io/api/admission/v1" +) + +// MatchDeleteOperation checks if the rule specifies the DELETE operation. +func MatchDeleteOperation(rule kyvernov1.Rule) bool { + ops := rule.MatchResources.GetOperations() + for _, rscFilters := range append(rule.MatchResources.All, rule.MatchResources.Any...) { + ops = append(ops, rscFilters.ResourceDescription.GetOperations()...) + } + + return datautils.SliceContains(ops, string(admissionv1.Delete)) +} diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/basic-delete/chainsaw-step-01-apply-1-4.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/basic-delete/chainsaw-step-01-apply-1-4.yaml index 2449c44fdb..9186c52791 100755 --- a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/basic-delete/chainsaw-step-01-apply-1-4.yaml +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/basic-delete/chainsaw-step-01-apply-1-4.yaml @@ -14,6 +14,8 @@ spec: - dictionary-2 namespaces: - staging-2 + operations: + - DELETE mutate: patchStrategicMerge: metadata: @@ -25,8 +27,4 @@ spec: name: test-secret-2 namespace: '{{ request.object.metadata.namespace }}' name: mutate-secret-on-configmap-delete - preconditions: - any: - - key: '{{ request.operation }}' - operator: Equals - value: DELETE + diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/chainsaw-test.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/chainsaw-test.yaml new file mode 100755 index 0000000000..58cf846a67 --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/chainsaw-test.yaml @@ -0,0 +1,41 @@ +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: delete-trigger-namespace +spec: + steps: + - name: step-01 + try: + - apply: + file: namespace.yaml + - name: step-02 + try: + - apply: + file: secret.yaml + - name: step-03 + try: + - apply: + file: policy.yaml + - assert: + file: policy-ready.yaml + - name: step-04 + try: + - apply: + file: configmap.yaml + - name: step-05 + try: + - assert: + file: patched-secret.yaml + - name: step-06 + try: + - delete: + ref: + apiVersion: v1 + kind: Namespace + name: staging + - name: step-07 + try: + - script: + content: "if kubectl get updaterequests -n kyverno 2>&1 | grep -q 'No resources found in kyverno namespace.'\nthen \n exit 0 \nelse \n exit + 1\nfi\n" diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/configmap.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/configmap.yaml new file mode 100644 index 0000000000..89d75e1e56 --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/configmap.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: dictionary-1 + namespace: staging +data: + key: "some value" diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/namespace.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/namespace.yaml new file mode 100644 index 0000000000..ee38adfbde --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/namespace.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: staging diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/patched-secret.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/patched-secret.yaml new file mode 100644 index 0000000000..e6eff71e9f --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/patched-secret.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: Secret +metadata: + labels: + foo: bar + name: secret-1 + namespace: staging diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy-ready.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy-ready.yaml new file mode 100644 index 0000000000..450edc769b --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy-ready.yaml @@ -0,0 +1,9 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: mutate-existing-secret +status: + conditions: + - reason: Succeeded + status: "True" + type: Ready diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy.yaml new file mode 100644 index 0000000000..0a4e8c2bea --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/policy.yaml @@ -0,0 +1,27 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: mutate-existing-secret +spec: + mutateExistingOnPolicyUpdate: true + rules: + - name: mutate-secret-on-configmap-event + match: + any: + - resources: + kinds: + - ConfigMap + names: + - dictionary-1 + namespaces: + - staging + mutate: + targets: + - apiVersion: v1 + kind: Secret + name: secret-1 + namespace: "{{ request.object.metadata.namespace }}" + patchStrategicMerge: + metadata: + labels: + foo: bar diff --git a/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/secret.yaml b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/secret.yaml new file mode 100644 index 0000000000..4aa4afb089 --- /dev/null +++ b/test/conformance/chainsaw/mutate/clusterpolicy/standard/existing/delete-trigger-namespace/secret.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: Secret +metadata: + name: secret-1 + namespace: staging +data: + key: dmFsdWUtMg0KDQo=