From 264eaec049ee2facbc9f7c2aa19a476b75082ff4 Mon Sep 17 00:00:00 2001 From: shuting Date: Wed, 1 Mar 2023 23:49:05 +0800 Subject: [PATCH] fix: remove timestamp checks for the clone rule (#6439) * remove timestamp checks Signed-off-by: ShutingZhao * add kuttl tests Signed-off-by: ShutingZhao --------- Signed-off-by: ShutingZhao --- .../kubectl-kyverno/utils/common/common.go | 2 +- pkg/background/common/context.go | 20 +++--- pkg/background/generate/generate.go | 62 ++++++------------- pkg/background/mutate/mutate.go | 2 +- pkg/policy/policy_controller.go | 2 +- .../01-assert.yaml | 9 +++ .../01-manifests.yaml | 30 +++++++++ .../02-assert.yaml | 5 ++ .../02-ns.yaml | 4 ++ .../README.md | 11 ++++ .../01-assert.yaml | 10 +++ .../01-manifests.yaml | 36 +++++++++++ .../02-assert.yaml | 5 ++ .../02-configmap.yaml | 9 +++ .../README.md | 11 ++++ 15 files changed, 163 insertions(+), 55 deletions(-) create mode 100644 test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-assert.yaml create mode 100644 test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-manifests.yaml create mode 100644 test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-assert.yaml create mode 100644 test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-ns.yaml create mode 100644 test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/README.md create mode 100644 test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-assert.yaml create mode 100644 test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-manifests.yaml create mode 100644 test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-assert.yaml create mode 100644 test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-configmap.yaml create mode 100644 test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/README.md diff --git a/cmd/cli/kubectl-kyverno/utils/common/common.go b/cmd/cli/kubectl-kyverno/utils/common/common.go index bded6663d1..24bdebd428 100644 --- a/cmd/cli/kubectl-kyverno/utils/common/common.go +++ b/cmd/cli/kubectl-kyverno/utils/common/common.go @@ -1124,7 +1124,7 @@ func handleGeneratePolicy(generateResponse *engineapi.EngineResponse, policyCont var newRuleResponse []engineapi.RuleResponse for _, rule := range generateResponse.PolicyResponse.Rules { - genResource, _, err := c.ApplyGeneratePolicy(log.Log, &policyContext, gr, []string{rule.Name}) + genResource, err := c.ApplyGeneratePolicy(log.Log, &policyContext, gr, []string{rule.Name}) if err != nil { rule.Status = engineapi.RuleStatusError return nil, err diff --git a/pkg/background/common/context.go b/pkg/background/common/context.go index f778d91174..fac5bbd031 100644 --- a/pkg/background/common/context.go +++ b/pkg/background/common/context.go @@ -21,25 +21,25 @@ func NewBackgroundContext(dclient dclient.Interface, ur *kyvernov1beta1.UpdateRe cfg config.Configuration, namespaceLabels map[string]string, logger logr.Logger, -) (*engine.PolicyContext, bool, error) { +) (*engine.PolicyContext, error) { ctx := context.NewContext() var new, old unstructured.Unstructured var err error if ur.Spec.Context.AdmissionRequestInfo.AdmissionRequest != nil { if err := ctx.AddRequest(ur.Spec.Context.AdmissionRequestInfo.AdmissionRequest); err != nil { - return nil, false, fmt.Errorf("failed to load request in context: %w", err) + return nil, fmt.Errorf("failed to load request in context: %w", err) } new, old, err = admissionutils.ExtractResources(nil, ur.Spec.Context.AdmissionRequestInfo.AdmissionRequest) if err != nil { - return nil, false, fmt.Errorf("failed to load request in context: %w", err) + return nil, fmt.Errorf("failed to load request in context: %w", err) } if !reflect.DeepEqual(new, unstructured.Unstructured{}) { if !check(&new, trigger) { err := fmt.Errorf("resources don't match") - return nil, false, fmt.Errorf("resource %v: %w", ur.Spec.GetResource().String(), err) + return nil, fmt.Errorf("resource %v: %w", ur.Spec.GetResource().String(), err) } } } @@ -49,27 +49,27 @@ func NewBackgroundContext(dclient dclient.Interface, ur *kyvernov1beta1.UpdateRe } if trigger == nil { - return nil, false, fmt.Errorf("trigger resource does not exist") + return nil, fmt.Errorf("trigger resource does not exist") } err = ctx.AddResource(trigger.Object) if err != nil { - return nil, false, fmt.Errorf("failed to load resource in context: %w", err) + return nil, fmt.Errorf("failed to load resource in context: %w", err) } err = ctx.AddOldResource(old.Object) if err != nil { - return nil, false, fmt.Errorf("failed to load resource in context: %w", err) + return nil, fmt.Errorf("failed to load resource in context: %w", err) } err = ctx.AddUserInfo(ur.Spec.Context.UserRequestInfo) if err != nil { - return nil, false, fmt.Errorf("failed to load SA in context: %w", err) + return nil, fmt.Errorf("failed to load SA in context: %w", err) } err = ctx.AddServiceAccount(ur.Spec.Context.UserRequestInfo.AdmissionUserInfo.Username) if err != nil { - return nil, false, fmt.Errorf("failed to load UserInfo in context: %w", err) + return nil, fmt.Errorf("failed to load UserInfo in context: %w", err) } if err := ctx.AddImageInfos(trigger, cfg); err != nil { @@ -83,7 +83,7 @@ func NewBackgroundContext(dclient dclient.Interface, ur *kyvernov1beta1.UpdateRe WithAdmissionInfo(ur.Spec.Context.UserRequestInfo). WithNamespaceLabels(namespaceLabels) - return policyContext, false, nil + return policyContext, nil } func check(admissionRsc, existingRsc *unstructured.Unstructured) bool { diff --git a/pkg/background/generate/generate.go b/pkg/background/generate/generate.go index cc6d063229..5d3c916cf8 100644 --- a/pkg/background/generate/generate.go +++ b/pkg/background/generate/generate.go @@ -90,7 +90,6 @@ func (c *GenerateController) ProcessUR(ur *kyvernov1beta1.UpdateRequest) error { var err error var resource *unstructured.Unstructured var genResources []kyvernov1.ResourceSpec - var precreatedResource bool logger.Info("start processing UR", "ur", ur.Name, "resourceVersion", ur.GetResourceVersion()) // 1 - Check if the trigger exists @@ -127,7 +126,7 @@ func (c *GenerateController) ProcessUR(ur *kyvernov1beta1.UpdateRequest) error { // 2 - Apply the generate policy on the resource namespaceLabels := engineutils.GetNamespaceSelectorsFromNamespaceLister(resource.GetKind(), resource.GetNamespace(), c.nsLister, logger) - genResources, precreatedResource, err = c.applyGenerate(*resource, *ur, namespaceLabels) + genResources, err = c.applyGenerate(*resource, *ur, namespaceLabels) if err != nil { // Need not update the status when policy doesn't apply on resource, because all the update requests are removed by the cleanup controller if strings.Contains(err.Error(), doesNotApply) { @@ -141,36 +140,36 @@ func (c *GenerateController) ProcessUR(ur *kyvernov1beta1.UpdateRequest) error { } // 4 - Update Status - return updateStatus(c.statusControl, *ur, err, genResources, precreatedResource) + return updateStatus(c.statusControl, *ur, err, genResources) } const doesNotApply = "policy does not apply to resource" -func (c *GenerateController) applyGenerate(resource unstructured.Unstructured, ur kyvernov1beta1.UpdateRequest, namespaceLabels map[string]string) ([]kyvernov1.ResourceSpec, bool, error) { +func (c *GenerateController) applyGenerate(resource unstructured.Unstructured, ur kyvernov1beta1.UpdateRequest, namespaceLabels map[string]string) ([]kyvernov1.ResourceSpec, error) { logger := c.log.WithValues("name", ur.GetName(), "policy", ur.Spec.GetPolicyKey(), "resource", ur.Spec.GetResource().String()) logger.V(3).Info("applying generate policy rule") policy, err := c.getPolicySpec(ur) if err != nil && !apierrors.IsNotFound(err) { logger.Error(err, "error in fetching policy") - return nil, false, err + return nil, err } if ur.Spec.DeleteDownstream || apierrors.IsNotFound(err) { err = c.deleteDownstream(policy, &ur) - return nil, false, err + return nil, err } - policyContext, precreatedResource, err := common.NewBackgroundContext(c.client, &ur, policy, &resource, c.configuration, namespaceLabels, logger) + policyContext, err := common.NewBackgroundContext(c.client, &ur, policy, &resource, c.configuration, namespaceLabels, logger) if err != nil { - return nil, precreatedResource, err + return nil, err } // check if the policy still applies to the resource engineResponse := c.engine.GenerateResponse(context.Background(), policyContext, ur) if len(engineResponse.PolicyResponse.Rules) == 0 { logger.V(4).Info(doesNotApply) - return nil, false, errors.New(doesNotApply) + return nil, errors.New(doesNotApply) } var applicableRules []string @@ -226,15 +225,11 @@ func (c *GenerateController) getPolicySpec(ur kyvernov1beta1.UpdateRequest) (kyv return npolicyObj, nil } -func updateStatus(statusControl common.StatusControlInterface, ur kyvernov1beta1.UpdateRequest, err error, genResources []kyvernov1.ResourceSpec, precreatedResource bool) error { +func updateStatus(statusControl common.StatusControlInterface, ur kyvernov1beta1.UpdateRequest, err error, genResources []kyvernov1.ResourceSpec) error { if err != nil { if _, err := statusControl.Failed(ur.GetName(), err.Error(), genResources); err != nil { return err } - } else if precreatedResource { - if _, err := statusControl.Skip(ur.GetName(), genResources); err != nil { - return err - } } else { if _, err := statusControl.Success(ur.GetName(), genResources); err != nil { return err @@ -243,7 +238,7 @@ func updateStatus(statusControl common.StatusControlInterface, ur kyvernov1beta1 return nil } -func (c *GenerateController) ApplyGeneratePolicy(log logr.Logger, policyContext *engine.PolicyContext, ur kyvernov1beta1.UpdateRequest, applicableRules []string) (genResources []kyvernov1.ResourceSpec, processExisting bool, err error) { +func (c *GenerateController) ApplyGeneratePolicy(log logr.Logger, policyContext *engine.PolicyContext, ur kyvernov1beta1.UpdateRequest, applicableRules []string) (genResources []kyvernov1.ResourceSpec, err error) { // Get the response as the actions to be performed on the resource // - - substitute values policy := policyContext.Policy() @@ -265,17 +260,7 @@ func (c *GenerateController) ApplyGeneratePolicy(log logr.Logger, policyContext } startTime := time.Now() - processExisting = false var genResource []kyvernov1.ResourceSpec - - if len(rule.MatchResources.Kinds) > 0 { - if len(rule.MatchResources.Annotations) == 0 && rule.MatchResources.Selector == nil { - rcreationTime := resource.GetCreationTimestamp() - pcreationTime := policy.GetCreationTimestamp() - processExisting = rcreationTime.Before(&pcreationTime) - } - } - if applyRules == kyvernov1.ApplyOne && applyCount > 0 { break } @@ -283,33 +268,26 @@ func (c *GenerateController) ApplyGeneratePolicy(log logr.Logger, policyContext // add configmap json data to context if err := c.engine.ContextLoader(policyContext.Policy(), rule)(context.TODO(), rule.Context, policyContext.JSONContext()); err != nil { log.Error(err, "cannot add configmaps to context") - return nil, processExisting, err + return nil, err } if rule, err = variables.SubstituteAllInRule(log, policyContext.JSONContext(), rule); err != nil { log.Error(err, "variable substitution failed for rule %s", rule.Name) - return nil, processExisting, err + return nil, err } - if policy.GetSpec().IsGenerateExistingOnPolicyUpdate() || !processExisting { - genResource, err = applyRule(log, c.client, rule, resource, jsonContext, policy, ur) - if err != nil { - log.Error(err, "failed to apply generate rule", "policy", policy.GetName(), - "rule", rule.Name, "resource", resource.GetName(), "suggestion", "users need to grant Kyverno's service account additional privileges") - return nil, processExisting, err - } - ruleNameToProcessingTime[rule.Name] = time.Since(startTime) - genResources = append(genResources, genResource...) + genResource, err = applyRule(log, c.client, rule, resource, jsonContext, policy, ur) + if err != nil { + log.Error(err, "failed to apply generate rule", "policy", policy.GetName(), + "rule", rule.Name, "resource", resource.GetName(), "suggestion", "users need to grant Kyverno's service account additional privileges") + return nil, err } - - if policy.GetSpec().IsGenerateExistingOnPolicyUpdate() { - processExisting = false - } - + ruleNameToProcessingTime[rule.Name] = time.Since(startTime) + genResources = append(genResources, genResource...) applyCount++ } - return genResources, processExisting, nil + return genResources, nil } func getResourceInfo(object map[string]interface{}) (kind, name, namespace, apiversion string, err error) { diff --git a/pkg/background/mutate/mutate.go b/pkg/background/mutate/mutate.go index 8346335a25..a1cf53fe0f 100644 --- a/pkg/background/mutate/mutate.go +++ b/pkg/background/mutate/mutate.go @@ -92,7 +92,7 @@ func (c *MutateExistingController) ProcessUR(ur *kyvernov1beta1.UpdateRequest) e } namespaceLabels := engineutils.GetNamespaceSelectorsFromNamespaceLister(trigger.GetKind(), trigger.GetNamespace(), c.nsLister, logger) - policyContext, _, err := common.NewBackgroundContext(c.client, ur, policy, trigger, c.configuration, namespaceLabels, logger) + policyContext, err := common.NewBackgroundContext(c.client, ur, policy, trigger, c.configuration, namespaceLabels, logger) if err != nil { logger.WithName(rule.Name).Error(err, "failed to build policy context") errs = append(errs, err) diff --git a/pkg/policy/policy_controller.go b/pkg/policy/policy_controller.go index d7f44f3bc6..5d01bb8c06 100644 --- a/pkg/policy/policy_controller.go +++ b/pkg/policy/policy_controller.go @@ -407,7 +407,7 @@ func (pc *PolicyController) requeuePolicies() { func (pc *PolicyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest, triggerResource *unstructured.Unstructured, rule kyvernov1.Rule, policy kyvernov1.PolicyInterface) (skip bool, err error) { namespaceLabels := engineutils.GetNamespaceSelectorsFromNamespaceLister(triggerResource.GetKind(), triggerResource.GetNamespace(), pc.nsLister, pc.log) - policyContext, _, err := backgroundcommon.NewBackgroundContext(pc.client, ur, policy, triggerResource, pc.configHandler, namespaceLabels, pc.log) + policyContext, err := backgroundcommon.NewBackgroundContext(pc.client, ur, policy, triggerResource, pc.configHandler, namespaceLabels, pc.log) if err != nil { return false, fmt.Errorf("failed to build policy context for rule %s: %w", rule.Name, err) } diff --git a/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-assert.yaml b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-assert.yaml new file mode 100644 index 0000000000..ec2273bcbd --- /dev/null +++ b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-assert.yaml @@ -0,0 +1,9 @@ +apiVersion: kyverno.io/v2beta1 +kind: ClusterPolicy +metadata: + name: cpol-clone-sync-create-source-after-policy +status: + conditions: + - reason: Succeeded + status: "True" + type: Ready diff --git a/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-manifests.yaml b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-manifests.yaml new file mode 100644 index 0000000000..aca73c7197 --- /dev/null +++ b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/01-manifests.yaml @@ -0,0 +1,30 @@ +apiVersion: kyverno.io/v2beta1 +kind: ClusterPolicy +metadata: + name: cpol-clone-sync-create-source-after-policy +spec: + rules: + - name: clone-secret + match: + any: + - resources: + kinds: + - Namespace + generate: + apiVersion: v1 + kind: Secret + name: regcred + namespace: "{{request.object.metadata.name}}" + synchronize: true + clone: + namespace: default + name: regcred +--- +apiVersion: v1 +data: + foo: YmFy +kind: Secret +metadata: + name: regcred + namespace: default +type: Opaque diff --git a/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-assert.yaml b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-assert.yaml new file mode 100644 index 0000000000..7cc4b1fa3b --- /dev/null +++ b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-assert.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: Secret +metadata: + name: regcred + namespace: cpol-clone-sync-create-source-after-policy-ns \ No newline at end of file diff --git a/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-ns.yaml b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-ns.yaml new file mode 100644 index 0000000000..cbb32084c6 --- /dev/null +++ b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/02-ns.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: cpol-clone-sync-create-source-after-policy-ns \ No newline at end of file diff --git a/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/README.md b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/README.md new file mode 100644 index 0000000000..374df95f8e --- /dev/null +++ b/test/conformance/kuttl/generate/clusterpolicy/cornercases/cpol-clone-sync-create-source-after-policy/README.md @@ -0,0 +1,11 @@ +## Description + +This is a corner case test to ensure a clone rule is applied when the source is created after the ClusterPolicy. + +## Expected Behavior + +If the downstream resource is created, the test passes. If it is not created, the test fails. + +## Reference Issue(s) + +https://github.com/kyverno/kyverno/issues/5411 \ No newline at end of file diff --git a/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-assert.yaml b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-assert.yaml new file mode 100644 index 0000000000..92ae0cc530 --- /dev/null +++ b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-assert.yaml @@ -0,0 +1,10 @@ +apiVersion: kyverno.io/v2beta1 +kind: Policy +metadata: + name: pol-clone-sync-create-source-after-policy + namespace: pol-clone-sync-create-source-after-policy-ns +status: + conditions: + - reason: Succeeded + status: "True" + type: Ready diff --git a/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-manifests.yaml b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-manifests.yaml new file mode 100644 index 0000000000..5ae065d89e --- /dev/null +++ b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/01-manifests.yaml @@ -0,0 +1,36 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: pol-clone-sync-create-source-after-policy-ns +--- +apiVersion: kyverno.io/v2beta1 +kind: Policy +metadata: + name: pol-clone-sync-create-source-after-policy + namespace: pol-clone-sync-create-source-after-policy-ns +spec: + rules: + - name: clone-secret + match: + any: + - resources: + kinds: + - ConfigMap + generate: + apiVersion: v1 + kind: Secret + name: mynewsecret + namespace: pol-clone-sync-create-source-after-policy-ns + synchronize: true + clone: + namespace: pol-clone-sync-create-source-after-policy-ns + name: regcred +--- +apiVersion: v1 +data: + foo: YmFy +kind: Secret +metadata: + name: regcred + namespace: pol-clone-sync-create-source-after-policy-ns +type: Opaque \ No newline at end of file diff --git a/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-assert.yaml b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-assert.yaml new file mode 100644 index 0000000000..c6722dee3e --- /dev/null +++ b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-assert.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: Secret +metadata: + name: mynewsecret + namespace: pol-clone-sync-create-source-after-policy-ns \ No newline at end of file diff --git a/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-configmap.yaml b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-configmap.yaml new file mode 100644 index 0000000000..387481152a --- /dev/null +++ b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/02-configmap.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: mycm + namespace: pol-clone-sync-create-source-after-policy-ns +data: + food: cheese + day: monday + color: red \ No newline at end of file diff --git a/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/README.md b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/README.md new file mode 100644 index 0000000000..374df95f8e --- /dev/null +++ b/test/conformance/kuttl/generate/policy/cornercases/pol-clone-sync-create-source-after-policy/README.md @@ -0,0 +1,11 @@ +## Description + +This is a corner case test to ensure a clone rule is applied when the source is created after the ClusterPolicy. + +## Expected Behavior + +If the downstream resource is created, the test passes. If it is not created, the test fails. + +## Reference Issue(s) + +https://github.com/kyverno/kyverno/issues/5411 \ No newline at end of file