From 06bdc095b65d4d35988a97153eeac28e81451faf Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Thu, 25 Apr 2024 09:28:19 -0400 Subject: [PATCH 1/7] Remove hub-template error handling The framework-addon now handles emitting compliance events for those errors for all template policy types. Refs: - https://issues.redhat.com/browse/ACM-10858 Signed-off-by: Justin Kulikauskas --- controllers/configurationpolicy_controller.go | 22 ----------- test/e2e/case13_templatization_test.go | 38 ------------------- 2 files changed, 60 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 265da875..539ef19c 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -971,28 +971,6 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi // process object templates for go template usage for i, rawData := range rawDataList { - // first check to make sure there are no hub-templates with delimiter - {{hub - // if one exists, it means the template resolution on the hub did not succeed. - if templates.HasTemplate(rawData, "{{hub", false) { - // check to see there is an annotation set to the hub error msg, - // if not ,set a generic msg - hubTemplatesErrMsg, ok := annotations["policy.open-cluster-management.io/hub-templates-error"] - if !ok || hubTemplatesErrMsg == "" { - // set a generic msg - hubTemplatesErrMsg = "Error occurred while processing hub-templates, " + - "check the policy events for more details." - } - - log.Info( - "An error occurred while processing hub-templates on the Hub cluster. Cannot process the policy.", - "message", hubTemplatesErrMsg, - ) - - addTemplateErrorViolation("Error processing hub templates", hubTemplatesErrMsg) - - return - } - if templates.HasTemplate(rawData, "", true) { log.V(1).Info("Processing policy templates") diff --git a/test/e2e/case13_templatization_test.go b/test/e2e/case13_templatization_test.go index 920d01ee..49493a01 100644 --- a/test/e2e/case13_templatization_test.go +++ b/test/e2e/case13_templatization_test.go @@ -461,44 +461,6 @@ var _ = Describe("Test templatization", Ordered, func() { return configmap }, defaultTimeoutSeconds, 1).ShouldNot(BeNil()) - By("Patch with invalid hub template") - utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p", - `[{ "op": "replace", - "path": "/spec/object-templates/0/objectDefinition/data/test", - "value": '{{hub "default" "e2esecret" dddddd vvvvv d hub}}' }]`, - "-n", testNamespace) - - By("By verifying that the configurationpolicy is NonCompliant") - Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, - case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds) - - return utils.GetComplianceState(managedPlc) - }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) - - By("By verifying that the configmap still exist ") - Consistently(func() interface{} { - configmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap, - case13PruneTmpErr+"-configmap", "default", true, defaultTimeoutSeconds) - - return configmap - }, defaultConsistentlyDuration, 1).ShouldNot(BeNil()) - - By("Change to valid configmap") - utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p", - `[{ "op": "replace", - "path": "/spec/object-templates/0/objectDefinition/data/test", - "value": "working" }]`, - "-n", testNamespace) - - By("By verifying that the configmap has no error") - Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, - case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds) - - return utils.GetComplianceState(managedPlc) - }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) - By("Patch with invalid managed template") utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p", `[{ "op": "replace", From c2694551b66ad85a05e8fd70c93ba4cfd781c26e Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Fri, 26 Apr 2024 15:20:25 -0400 Subject: [PATCH 2/7] Support templates in OperatorPolicy The Subscription and OperatorGroup fields can now utilize templates. The resolver uses the DynamicWatcher already used by the controller, so no additional plumbing was needed for new watches or in order to share a cache. Refs: - https://issues.redhat.com/browse/ACM-10858 Signed-off-by: Justin Kulikauskas --- controllers/operatorpolicy_controller.go | 57 ++++++++-- controllers/operatorpolicy_controller_test.go | 6 +- go.mod | 4 +- go.sum | 8 +- test/e2e/case38_install_operator_test.go | 102 ++++++++++++++++++ .../operator-policy-with-templates.yaml | 28 +++++ .../template-configmap.yaml | 7 ++ 7 files changed, 197 insertions(+), 15 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-with-templates.yaml create mode 100644 test/resources/case38_operator_install/template-configmap.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index c5b2e2ef..8586ecff 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -12,10 +12,12 @@ import ( "reflect" "regexp" "slices" + "strconv" "strings" operatorv1 "github.com/operator-framework/api/pkg/operators/v1" operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + templates "github.com/stolostron/go-template-utils/v4/pkg/templates" depclient "github.com/stolostron/kubernetes-dependency-watches/client" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -316,7 +318,24 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p validationErrors := make([]error, 0) - sub, subErr := buildSubscription(policy, r.DefaultNamespace) + disableTemplates := false + + if disableAnnotation, ok := policy.GetAnnotations()["policy.open-cluster-management.io/disable-templates"]; ok { + disableTemplates, _ = strconv.ParseBool(disableAnnotation) // on error, templates will not be disabled + } + + var tmplResolver *templates.TemplateResolver + + if !disableTemplates { + var err error + + tmplResolver, err = templates.NewResolverWithDynamicWatcher(r.DynamicWatcher, templates.Config{}) + if err != nil { + validationErrors = append(validationErrors, fmt.Errorf("unable to create template resolver: %w", err)) + } + } + + sub, subErr := buildSubscription(policy, r.DefaultNamespace, tmplResolver) if subErr == nil { err := r.applySubscriptionDefaults(ctx, sub) if err != nil { @@ -339,7 +358,7 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p opGroupNS = sub.Namespace } - opGroup, ogErr := buildOperatorGroup(policy, opGroupNS) + opGroup, ogErr := buildOperatorGroup(policy, opGroupNS, tmplResolver) if ogErr != nil { validationErrors = append(validationErrors, ogErr) } else { @@ -434,13 +453,26 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults( // If an error is returned, it will include details on why the policy spec if invalid and // why the desired subscription can't be determined. func buildSubscription( - policy *policyv1beta1.OperatorPolicy, defaultNS string, + policy *policyv1beta1.OperatorPolicy, defaultNS string, tmplResolver *templates.TemplateResolver, ) (*operatorv1alpha1.Subscription, error) { subscription := new(operatorv1alpha1.Subscription) + rawSub := policy.Spec.Subscription.Raw + + if tmplResolver != nil && templates.HasTemplate(rawSub, "", false) { + watcher := opPolIdentifier(policy.Namespace, policy.Name) + + resolvedTmpl, err := tmplResolver.ResolveTemplate(rawSub, nil, &templates.ResolveOptions{Watcher: &watcher}) + if err != nil { + return nil, fmt.Errorf("could not build subscription: %w", err) + } + + rawSub = resolvedTmpl.ResolvedJSON + } + sub := make(map[string]interface{}) - err := json.Unmarshal(policy.Spec.Subscription.Raw, &sub) + err := json.Unmarshal(rawSub, &sub) if err != nil { return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err) } @@ -510,7 +542,7 @@ func buildSubscription( // buildOperatorGroup bootstraps the OperatorGroup spec defined in the operator policy // with the apiversion and kind in preparation for resource creation func buildOperatorGroup( - policy *policyv1beta1.OperatorPolicy, namespace string, + policy *policyv1beta1.OperatorPolicy, namespace string, tmplResolver *templates.TemplateResolver, ) (*operatorv1.OperatorGroup, error) { operatorGroup := new(operatorv1.OperatorGroup) @@ -526,9 +558,22 @@ func buildOperatorGroup( return operatorGroup, nil } + rawOG := policy.Spec.OperatorGroup.Raw + + if tmplResolver != nil && templates.HasTemplate(rawOG, "", false) { + watcher := opPolIdentifier(policy.Namespace, policy.Name) + + resolvedTmpl, err := tmplResolver.ResolveTemplate(rawOG, nil, &templates.ResolveOptions{Watcher: &watcher}) + if err != nil { + return nil, fmt.Errorf("could not build operator group: %w", err) + } + + rawOG = resolvedTmpl.ResolvedJSON + } + opGroup := make(map[string]interface{}) - if err := json.Unmarshal(policy.Spec.OperatorGroup.Raw, &opGroup); err != nil { + if err := json.Unmarshal(rawOG, &opGroup); err != nil { return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err) } diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index 9e7f8941..d2d09241 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/controllers/operatorpolicy_controller_test.go @@ -43,7 +43,7 @@ func TestBuildSubscription(t *testing.T) { } // Check values are correctly bootstrapped to the Subscription - ret, err := buildSubscription(testPolicy, "my-operators") + ret, err := buildSubscription(testPolicy, "my-operators", nil) assert.Equal(t, err, nil) assert.Equal(t, ret.GroupVersionKind(), desiredGVK) assert.Equal(t, ret.ObjectMeta.Name, "my-operator") @@ -102,7 +102,7 @@ func TestBuildSubscriptionInvalidNames(t *testing.T) { } // Check values are correctly bootstrapped to the Subscription - _, err := buildSubscription(testPolicy, "my-operators") + _, err := buildSubscription(testPolicy, "my-operators", nil) assert.Equal(t, err.Error(), test.expected) }, ) @@ -138,7 +138,7 @@ func TestBuildOperatorGroup(t *testing.T) { } // Ensure OperatorGroup values are populated correctly - ret, err := buildOperatorGroup(testPolicy, "my-operators") + ret, err := buildOperatorGroup(testPolicy, "my-operators", nil) assert.Equal(t, err, nil) assert.Equal(t, ret.GroupVersionKind(), desiredGVK) assert.Equal(t, ret.ObjectMeta.GetGenerateName(), "my-operators-") diff --git a/go.mod b/go.mod index aecb97bc..5edf5db9 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/spf13/pflag v1.0.5 github.com/stolostron/go-log-utils v0.1.2 - github.com/stolostron/go-template-utils/v4 v4.0.0 + github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f github.com/stolostron/kubernetes-dependency-watches v0.5.2 github.com/stretchr/testify v1.8.4 golang.org/x/mod v0.16.0 @@ -56,7 +56,7 @@ require ( github.com/google/gofuzz v1.2.0 // indirect github.com/google/pprof v0.0.0-20240402174815-29b9bb013b0f // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect - github.com/google/uuid v1.3.1 // indirect + github.com/google/uuid v1.4.0 // indirect github.com/huandu/xstrings v1.4.0 // indirect github.com/imdario/mergo v1.0.0 // indirect github.com/josharian/intern v1.0.0 // indirect diff --git a/go.sum b/go.sum index 5ac352e0..597e04a6 100644 --- a/go.sum +++ b/go.sum @@ -69,8 +69,8 @@ github.com/google/pprof v0.0.0-20240402174815-29b9bb013b0f/go.mod h1:kf6iHlnVGwg github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4= github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= -github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/google/uuid v1.4.0 h1:MtMxsa51/r9yyhkyLsVeVt0B+BGQZzpQiTQ4eHZ8bc4= +github.com/google/uuid v1.4.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/huandu/xstrings v1.3.3/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE= github.com/huandu/xstrings v1.4.0 h1:D17IlohoQq4UcpqD7fDk80P7l+lwAmlFaBHgOipl2FU= github.com/huandu/xstrings v1.4.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE= @@ -142,8 +142,8 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stolostron/go-log-utils v0.1.2 h1:7l1aJWvBqU2+DUyimcslT5SJpdygVY/clRDmX5sO29c= github.com/stolostron/go-log-utils v0.1.2/go.mod h1:8zrB8UJmp1rXhv3Ck9bBl5SpNfKk3SApeElbo96YRtQ= -github.com/stolostron/go-template-utils/v4 v4.0.0 h1:gvSfhXIoymo5Ql9MH/ofTTOtBVkaUBq8HokCGR4xkkc= -github.com/stolostron/go-template-utils/v4 v4.0.0/go.mod h1:svIOPUJpG/ObRn3WwZMBGMEMsBgKH8LVfhsaIArgAAQ= +github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f h1:LKvxEJgNRLetVyMz8HdIRQg+nnojR9OHNL8QpNYiX5I= +github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f/go.mod h1:b93jKpuJtoePu9liTNcp7td+8Hvzk2lROeMqVLC42ks= github.com/stolostron/kubernetes-dependency-watches v0.5.2 h1:TtctOgPn+TYo1dJ+dr9TLR2rlpy5aFQ/1dzfcYZUDi4= github.com/stolostron/kubernetes-dependency-watches v0.5.2/go.mod h1:5nwtuleCNR9Mno7SJRz6B2BavvXHbfR4rIwzKLkk9G8= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 65bb33ad..5e0bc3e7 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -2653,4 +2653,106 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Expect(remBehavior).To(HaveKeyWithValue("customResourceDefinitions", "Keep")) }) }) + Describe("Testing templates in an OperatorPolicy", Ordered, func() { + const ( + opPolYAML = "../resources/case38_operator_install/operator-policy-with-templates.yaml" + opPolName = "oppol-with-templates" + configmapYAML = "../resources/case38_operator_install/template-configmap.yaml" + opGroupName = "scoped-operator-group" + subName = "project-quay" + ) + + BeforeAll(func() { + utils.Kubectl("create", "ns", opPolTestNS) + DeferCleanup(func() { + utils.Kubectl("delete", "ns", opPolTestNS) + }) + + utils.Kubectl("apply", "-f", configmapYAML, "-n", opPolTestNS) + + createObjWithParent(parentPolicyYAML, parentPolicyName, + opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy) + }) + + It("Should lookup values for operator policy resources when enforced", func(ctx SpecContext) { + // enforce the policy + utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) + + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "OperatorGroup", + APIVersion: "operators.coreos.com/v1", + Metadata: policyv1.ObjectMetadata{ + Name: opGroupName, + Namespace: opPolTestNS, + }, + }, + Compliant: "Compliant", + Reason: "Resource found as expected", + }}, + metav1.Condition{ + Type: "OperatorGroupCompliant", + Status: metav1.ConditionTrue, + Reason: "OperatorGroupMatches", + Message: "the OperatorGroup matches what is required by the policy", + }, + "the OperatorGroup required by the policy was created", + ) + + By("Verifying the targetNamespaces in the OperatorGroup") + og, err := clientManagedDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS). + Get(ctx, opGroupName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(og).NotTo(BeNil()) + + targetNamespaces, found, err := unstructured.NestedStringSlice(og.Object, "spec", "targetNamespaces") + Expect(err).NotTo(HaveOccurred()) + Expect(found).To(BeTrue()) + Expect(targetNamespaces).To(HaveExactElements("foo", "bar", opPolTestNS)) + + By("Verifying the Subscription channel") + sub, err := clientManagedDynamic.Resource(gvrSubscription).Namespace(opPolTestNS). + Get(ctx, subName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(sub).NotTo(BeNil()) + + channel, found, err := unstructured.NestedString(sub.Object, "spec", "channel") + Expect(err).NotTo(HaveOccurred()) + Expect(found).To(BeTrue()) + Expect(channel).To(Equal("fakechannel")) + }) + + It("Should update the subscription after the configmap is updated", func(ctx SpecContext) { + utils.Kubectl("patch", "configmap", "op-config", "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "replace", "path": "/data/channel", "value": "stable-3.8"}]`) + + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Name: subName, + Namespace: opPolTestNS, + }, + }, + Compliant: "Compliant", + Reason: "Resource found as expected", + }}, + metav1.Condition{ + Type: "SubscriptionCompliant", + Status: metav1.ConditionTrue, + Reason: "SubscriptionMatches", + Message: "the Subscription matches what is required by the policy", + }, + "the Subscription was updated to match the policy", + ) + }) + }) }) diff --git a/test/resources/case38_operator_install/operator-policy-with-templates.yaml b/test/resources/case38_operator_install/operator-policy-with-templates.yaml new file mode 100644 index 00000000..292b1eb0 --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-with-templates.yaml @@ -0,0 +1,28 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-with-templates + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: inform + severity: medium + complianceType: musthave + operatorGroup: # optional + name: scoped-operator-group + namespace: operator-policy-testns + targetNamespaces: '{{ (fromConfigMap "operator-policy-testns" "op-config" "namespaces") | toLiteral }}' + subscription: + channel: '{{ (lookup "v1" "ConfigMap" "operator-policy-testns" "op-config").data.channel }}' + name: project-quay + namespace: operator-policy-testns + installPlanApproval: Automatic + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: quay-operator.v3.8.1 diff --git a/test/resources/case38_operator_install/template-configmap.yaml b/test/resources/case38_operator_install/template-configmap.yaml new file mode 100644 index 00000000..1837feb1 --- /dev/null +++ b/test/resources/case38_operator_install/template-configmap.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: op-config +data: + channel: "fakechannel" + namespaces: '["foo", "bar", "operator-policy-testns"]' From 36689fc90e0a9882aa9519f3aa314cb5fcb34401 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 30 Apr 2024 15:21:20 -0400 Subject: [PATCH 3/7] Update dependency-watches library for DeepCopy The library now does the DeepCopy for us, to prevent accidental mutations to objects in the cache. Signed-off-by: Justin Kulikauskas --- controllers/operatorpolicy_controller.go | 21 ++++++++------------- go.mod | 4 ++-- go.sum | 8 ++++---- main.go | 6 +++++- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 8586ecff..012168d6 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -704,10 +704,8 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup( return nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err) } - merged := opGroup.DeepCopy() // Copy it so that the value in the cache is not changed - updateNeeded, skipUpdate, err := r.mergeObjects( - ctx, desiredUnstruct, merged, string(policy.Spec.ComplianceType), + ctx, desiredUnstruct, &opGroup, string(policy.Spec.ComplianceType), ) if err != nil { return nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err) @@ -747,7 +745,7 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup( desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion() - err = r.Update(ctx, merged) + err = r.Update(ctx, &opGroup) if err != nil { return nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err) } @@ -974,15 +972,13 @@ func (r *OperatorPolicyReconciler) musthaveSubscription( return nil, nil, false, fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err) } - merged := foundSub.DeepCopy() // Copy it so that the value in the cache is not changed - - updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, merged, string(policy.Spec.ComplianceType)) + updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, foundSub, string(policy.Spec.ComplianceType)) if err != nil { return nil, nil, false, fmt.Errorf("error checking if the Subscription needs an update: %w", err) } mergedSub := new(operatorv1alpha1.Subscription) - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(merged.Object, mergedSub); err != nil { + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(foundSub.Object, mergedSub); err != nil { return nil, nil, false, fmt.Errorf("error converting the retrieved Subscription to the go type: %w", err) } @@ -1040,14 +1036,14 @@ func (r *OperatorPolicyReconciler) musthaveSubscription( earlyConds = append(earlyConds, calculateComplianceCondition(policy)) } - err = r.Update(ctx, merged) + err = r.Update(ctx, foundSub) if err != nil { return mergedSub, nil, changed, fmt.Errorf("error updating the Subscription: %w", err) } - merged.SetGroupVersionKind(subscriptionGVK) // Update stripped this information + foundSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information - updateStatus(policy, updatedCond("Subscription"), updatedObj(merged)) + updateStatus(policy, updatedCond("Subscription"), updatedObj(foundSub)) return mergedSub, earlyConds, true, nil } @@ -1696,11 +1692,10 @@ func (r *OperatorPolicyReconciler) handleCatalogSource( if !isMissing { // CatalogSource is found, initiate health check - catalogSrcUnstruct := foundCatalogSrc.DeepCopy() catalogSrc := new(operatorv1alpha1.CatalogSource) err := runtime.DefaultUnstructuredConverter. - FromUnstructured(catalogSrcUnstruct.Object, catalogSrc) + FromUnstructured(foundCatalogSrc.Object, catalogSrc) if err != nil { return false, fmt.Errorf("error converting the retrieved CatalogSource to the Go type: %w", err) } diff --git a/go.mod b/go.mod index 5edf5db9..32231bb1 100644 --- a/go.mod +++ b/go.mod @@ -13,8 +13,8 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/spf13/pflag v1.0.5 github.com/stolostron/go-log-utils v0.1.2 - github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f - github.com/stolostron/kubernetes-dependency-watches v0.5.2 + github.com/stolostron/go-template-utils/v4 v4.1.0 + github.com/stolostron/kubernetes-dependency-watches v0.6.0 github.com/stretchr/testify v1.8.4 golang.org/x/mod v0.16.0 k8s.io/api v0.29.2 diff --git a/go.sum b/go.sum index 597e04a6..d460ad9b 100644 --- a/go.sum +++ b/go.sum @@ -142,10 +142,10 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stolostron/go-log-utils v0.1.2 h1:7l1aJWvBqU2+DUyimcslT5SJpdygVY/clRDmX5sO29c= github.com/stolostron/go-log-utils v0.1.2/go.mod h1:8zrB8UJmp1rXhv3Ck9bBl5SpNfKk3SApeElbo96YRtQ= -github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f h1:LKvxEJgNRLetVyMz8HdIRQg+nnojR9OHNL8QpNYiX5I= -github.com/stolostron/go-template-utils/v4 v4.0.2-0.20240426175705-e9547005fb4f/go.mod h1:b93jKpuJtoePu9liTNcp7td+8Hvzk2lROeMqVLC42ks= -github.com/stolostron/kubernetes-dependency-watches v0.5.2 h1:TtctOgPn+TYo1dJ+dr9TLR2rlpy5aFQ/1dzfcYZUDi4= -github.com/stolostron/kubernetes-dependency-watches v0.5.2/go.mod h1:5nwtuleCNR9Mno7SJRz6B2BavvXHbfR4rIwzKLkk9G8= +github.com/stolostron/go-template-utils/v4 v4.1.0 h1:i7S6gTmFixZB25zY03pR5rC30PVJGDFfFuLi15Oqq6g= +github.com/stolostron/go-template-utils/v4 v4.1.0/go.mod h1:pEezMPxvuvllc68oLUejh9tfM8r/Yi7s10ZlZhVkyM4= +github.com/stolostron/kubernetes-dependency-watches v0.6.0 h1:FxXsslLNH5hNojVvKX467iefCEwigdMftfmlYX/tddg= +github.com/stolostron/kubernetes-dependency-watches v0.6.0/go.mod h1:6v54aX8Bxx1m9YETZUxNGUrSHcRIArM/YnOqnUbIB/g= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c= diff --git a/main.go b/main.go index 5a660310..a17ad797 100644 --- a/main.go +++ b/main.go @@ -429,7 +429,11 @@ func main() { depReconciler, depEvents := depclient.NewControllerRuntimeSource() watcher, err := depclient.New(cfg, depReconciler, - &depclient.Options{DisableInitialReconcile: true, EnableCache: true}) + &depclient.Options{ + DisableInitialReconcile: true, + EnableCache: true, + ObjectCacheOptions: depclient.ObjectCacheOptions{UnsafeDisableDeepCopy: false}, + }) if err != nil { log.Error(err, "Unable to create dependency watcher") os.Exit(1) From bad4d202df6465303ed54c38e3274c00afce752c Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 30 Apr 2024 17:01:21 -0400 Subject: [PATCH 4/7] Use a newer quay version for operatorpolicy tests From some test runs, it seems like some of the 3.8.x releases do not always have the quayecosystems CRD, which makes the tests inconsistent. From what I can tell, *none* of the 3.10.x releases will have that CRD, so it should be consistent. Signed-off-by: Justin Kulikauskas --- test/e2e/case38_install_operator_test.go | 88 +++++++------------ .../operator-policy-mustnothave.yaml | 6 +- .../operator-policy-no-exist-enforce.yaml | 2 +- ...r-policy-no-group-enforce-one-version.yaml | 26 ++++++ .../operator-policy-no-group-enforce.yaml | 4 +- .../operator-policy-no-group.yaml | 4 +- .../operator-policy-validity-test.yaml | 4 +- .../operator-policy-with-group.yaml | 4 +- .../operator-policy-with-templates.yaml | 2 +- .../case38_operator_install/subscription.yaml | 4 +- 10 files changed, 73 insertions(+), 71 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 5e0bc3e7..70fc0375 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -734,8 +734,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }) Describe("Test health checks on OLM resources after OperatorPolicy operator installation", Ordered, func() { const ( - opPolYAML = "../resources/case38_operator_install/operator-policy-no-group-enforce.yaml" - opPolName = "oppol-no-group-enforce" + opPolYAML = "../resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml" + opPolName = "oppol-no-group-enforce-one-version" opPolNoExistYAML = "../resources/case38_operator_install/operator-policy-no-exist-enforce.yaml" opPolNoExistName = "oppol-no-exist-enforce" ) @@ -755,7 +755,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { It("Should generate conditions and relatedobjects of CSV", func(ctx SpecContext) { Eventually(func(ctx SpecContext) string { csv, _ := clientManagedDynamic.Resource(gvrClusterServiceVersion).Namespace(opPolTestNS). - Get(ctx, "quay-operator.v3.8.13", metav1.GetOptions{}) + Get(ctx, "quay-operator.v3.10.0", metav1.GetOptions{}) if csv == nil { return "" @@ -781,11 +781,11 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "ClusterServiceVersionCompliant", Status: metav1.ConditionTrue, Reason: "InstallSucceeded", - Message: "ClusterServiceVersion (quay-operator.v3.8.13) - install strategy completed with " + + Message: "ClusterServiceVersion (quay-operator.v3.10.0) - install strategy completed with " + "no errors", }, regexp.QuoteMeta( - "ClusterServiceVersion (quay-operator.v3.8.13) - install strategy completed with no errors", + "ClusterServiceVersion (quay-operator.v3.10.0) - install strategy completed with no errors", ), ) }) @@ -1407,16 +1407,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, Compliant: "Compliant", Reason: "Resource found as expected", - }, { - Object: policyv1.ObjectResource{ - Kind: "CustomResourceDefinition", - APIVersion: "apiextensions.k8s.io/v1", - Metadata: policyv1.ObjectMetadata{ - Name: "quayecosystems.redhatcop.redhat.io", - }, - }, - Compliant: "Compliant", - Reason: "Resource found as expected", }}, metav1.Condition{ Type: "CustomResourceDefinitionCompliant", @@ -1744,17 +1734,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { return crd }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) - By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) - // Revert to the original mustnothave policy utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", `[{"op": "replace", "path": "/spec/complianceType", "value": "mustnothave"},`+ @@ -1819,6 +1798,16 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, Compliant: "Compliant", Reason: "Resource found but will not be handled in mustnothave mode", + }, { + Object: policyv1.ObjectResource{ + Kind: "InstallPlan", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + }, + }, + Compliant: "Compliant", + Reason: "Resource found but will not be handled in mustnothave mode", }}, metav1.Condition{ Type: "InstallPlanCompliant", @@ -1846,24 +1835,14 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "ClusterServiceVersionCompliant", Status: metav1.ConditionFalse, Reason: "ClusterServiceVersionPresent", - Message: "the ClusterServiceVersion (quay-operator.v3.8.13) is present", + Message: "the ClusterServiceVersion (quay-operator.v3.10.0) is present", }, - regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.8.13) is present"), + regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.10.0) is present"), ) check( opPolName, true, []policyv1.RelatedObject{{ - Object: policyv1.ObjectResource{ - Kind: "CustomResourceDefinition", - APIVersion: "apiextensions.k8s.io/v1", - Metadata: policyv1.ObjectMetadata{ - Name: "quayecosystems.redhatcop.redhat.io", - }, - }, - Compliant: "NonCompliant", - Reason: "Resource found but should not exist", - }, { Object: policyv1.ObjectResource{ Kind: "CustomResourceDefinition", APIVersion: "apiextensions.k8s.io/v1", @@ -1955,16 +1934,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { opPolName, false, []policyv1.RelatedObject{{ - Object: policyv1.ObjectResource{ - Kind: "CustomResourceDefinition", - APIVersion: "apiextensions.k8s.io/v1", - Metadata: policyv1.ObjectMetadata{ - Name: "quayecosystems.redhatcop.redhat.io", - }, - }, - Reason: "The CustomResourceDefinition is attached to a mustnothave policy, but " + - "does not need to be removed", - }, { Object: policyv1.ObjectResource{ Kind: "CustomResourceDefinition", APIVersion: "apiextensions.k8s.io/v1", @@ -2002,12 +1971,10 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { keptChecks() By("Checking that certain (named) resources are still there") - utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.8.13", + utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.10.0", opPolTestNS, true, eventuallyTimeout) utils.GetWithTimeout(clientManagedDynamic, gvrSubscription, subName, opPolTestNS, true, eventuallyTimeout) - utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayecosystems.redhatcop.redhat.io", - "", true, eventuallyTimeout) utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayregistries.quay.redhat.com", "", true, eventuallyTimeout) }) @@ -2129,6 +2096,17 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, Compliant: "Compliant", Reason: "Resource found but will not be handled in mustnothave mode", + }, { + Object: policyv1.ObjectResource{ + Kind: "InstallPlan", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Name: installPlanName, + Namespace: opPolTestNS, + }, + }, + Compliant: "Compliant", + Reason: "Resource found but will not be handled in mustnothave mode", }}, metav1.Condition{ Type: "InstallPlanCompliant", @@ -2190,12 +2168,10 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }) It("Should report things as gone after the finalizers are removed", func() { By("Checking that certain (named) resources are not there, indicating the removal was completed") - utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.8.13", + utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.10.0", opPolTestNS, false, eventuallyTimeout) utils.GetWithTimeout(clientManagedDynamic, gvrSubscription, subName, opPolTestNS, false, eventuallyTimeout) - utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayecosystems.redhatcop.redhat.io", - "", false, eventuallyTimeout) utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayregistries.quay.redhat.com", "", false, eventuallyTimeout) @@ -2290,7 +2266,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Reason: "ClusterServiceVersionNotPresent", Message: "the ClusterServiceVersion is not present", }, - regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.8.13) was deleted"), + regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.10.0) was deleted"), ) check( opPolName, @@ -2728,7 +2704,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { It("Should update the subscription after the configmap is updated", func(ctx SpecContext) { utils.Kubectl("patch", "configmap", "op-config", "-n", opPolTestNS, "--type=json", "-p", - `[{"op": "replace", "path": "/data/channel", "value": "stable-3.8"}]`) + `[{"op": "replace", "path": "/data/channel", "value": "stable-3.10"}]`) check( opPolName, diff --git a/test/resources/case38_operator_install/operator-policy-mustnothave.yaml b/test/resources/case38_operator_install/operator-policy-mustnothave.yaml index 0396c68c..7f00835d 100644 --- a/test/resources/case38_operator_install/operator-policy-mustnothave.yaml +++ b/test/resources/case38_operator_install/operator-policy-mustnothave.yaml @@ -15,15 +15,15 @@ spec: severity: medium complianceType: mustnothave subscription: - channel: stable-3.8 + channel: stable-3.10 name: project-quay namespace: operator-policy-testns installPlanApproval: Manual source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.13 + startingCSV: quay-operator.v3.10.0 versions: - - quay-operator.v3.8.13 + - quay-operator.v3.10.0 removalBehavior: operatorGroups: DeleteIfUnused subscriptions: Delete diff --git a/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml b/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml index 13657edb..62d839ec 100644 --- a/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml +++ b/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml @@ -15,7 +15,7 @@ spec: severity: medium complianceType: musthave subscription: - channel: stable-3.8 + channel: stable-3.10 name: project-quay-does-not-exist namespace: operator-policy-testns installPlanApproval: Automatic diff --git a/test/resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml b/test/resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml new file mode 100644 index 00000000..ccd26352 --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml @@ -0,0 +1,26 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-no-group-enforce-one-version + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: enforce + severity: medium + complianceType: musthave + subscription: + channel: stable-3.10 + name: project-quay + namespace: operator-policy-testns + installPlanApproval: Automatic + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: quay-operator.v3.10.0 + versions: + - quay-operator.v3.10.0 diff --git a/test/resources/case38_operator_install/operator-policy-no-group-enforce.yaml b/test/resources/case38_operator_install/operator-policy-no-group-enforce.yaml index 23438359..3fb106bc 100644 --- a/test/resources/case38_operator_install/operator-policy-no-group-enforce.yaml +++ b/test/resources/case38_operator_install/operator-policy-no-group-enforce.yaml @@ -15,10 +15,10 @@ spec: severity: medium complianceType: musthave subscription: - channel: stable-3.8 + channel: stable-3.10 name: project-quay namespace: operator-policy-testns installPlanApproval: Automatic source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.13 + startingCSV: quay-operator.v3.10.0 diff --git a/test/resources/case38_operator_install/operator-policy-no-group.yaml b/test/resources/case38_operator_install/operator-policy-no-group.yaml index 89a49d3b..1509aea6 100644 --- a/test/resources/case38_operator_install/operator-policy-no-group.yaml +++ b/test/resources/case38_operator_install/operator-policy-no-group.yaml @@ -15,10 +15,10 @@ spec: severity: medium complianceType: musthave subscription: - channel: stable-3.8 + channel: stable-3.10 name: project-quay namespace: operator-policy-testns installPlanApproval: Automatic source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.1 + startingCSV: quay-operator.v3.10.0 diff --git a/test/resources/case38_operator_install/operator-policy-validity-test.yaml b/test/resources/case38_operator_install/operator-policy-validity-test.yaml index 12d4e0eb..4bf51998 100644 --- a/test/resources/case38_operator_install/operator-policy-validity-test.yaml +++ b/test/resources/case38_operator_install/operator-policy-validity-test.yaml @@ -22,10 +22,10 @@ spec: - operator-policy-testns subscription: actually: incorrect - channel: stable-3.8 + channel: stable-3.10 name: project-quay namespace: nonexist-testns installPlanApproval: Incorrect source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.1 + startingCSV: quay-operator.v3.10.0 diff --git a/test/resources/case38_operator_install/operator-policy-with-group.yaml b/test/resources/case38_operator_install/operator-policy-with-group.yaml index 70ddb8c6..cfc89a2f 100644 --- a/test/resources/case38_operator_install/operator-policy-with-group.yaml +++ b/test/resources/case38_operator_install/operator-policy-with-group.yaml @@ -20,10 +20,10 @@ spec: targetNamespaces: - operator-policy-testns subscription: - channel: stable-3.8 + channel: stable-3.10 name: project-quay namespace: operator-policy-testns installPlanApproval: Automatic source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.1 \ No newline at end of file + startingCSV: quay-operator.v3.10.0 \ No newline at end of file diff --git a/test/resources/case38_operator_install/operator-policy-with-templates.yaml b/test/resources/case38_operator_install/operator-policy-with-templates.yaml index 292b1eb0..de56c398 100644 --- a/test/resources/case38_operator_install/operator-policy-with-templates.yaml +++ b/test/resources/case38_operator_install/operator-policy-with-templates.yaml @@ -25,4 +25,4 @@ spec: installPlanApproval: Automatic source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.1 + startingCSV: quay-operator.v3.10.0 diff --git a/test/resources/case38_operator_install/subscription.yaml b/test/resources/case38_operator_install/subscription.yaml index c4592a1d..a4eddf43 100644 --- a/test/resources/case38_operator_install/subscription.yaml +++ b/test/resources/case38_operator_install/subscription.yaml @@ -3,9 +3,9 @@ kind: Subscription metadata: name: project-quay spec: - channel: stable-3.8 + channel: stable-3.10 name: project-quay installPlanApproval: Automatic source: operatorhubio-catalog sourceNamespace: olm - startingCSV: quay-operator.v3.8.1 + startingCSV: quay-operator.v3.10.0 From 4f306c4fc434a89cf36ac55ce10d5d62714b23e5 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 1 May 2024 10:04:07 -0400 Subject: [PATCH 5/7] Wait for actual ConstraintsNotSatisfiable reason This test started failing relatively often. Debugging revealed that the Subscription was getting into a ResolutionFailed=True condition, but that it did not have the expected ConstraintsNotSatisfiable reason; it looked like it was failing to resolve because the connection was refused to the catalog. Refs: - https://issues.redhat.com/browse/ACM-11414 Signed-off-by: Justin Kulikauskas --- test/e2e/case38_install_operator_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 70fc0375..6fa1bc84 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -1123,12 +1123,13 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { condType, _, _ := unstructured.NestedString(condMap, "type") if condType == "ResolutionFailed" { - return condMap["status"] + return condMap["reason"] } } return nil - }, olmWaitTimeout, 5, ctx).Should(Equal("True")) + }, olmWaitTimeout*2, 5, ctx).Should(Equal("ConstraintsNotSatisfiable")) + check( opPolName, true, From 0b7444843e6b7dba7b43d34f41293db2133dedf3 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 1 May 2024 12:03:45 -0400 Subject: [PATCH 6/7] Add checkCompliance function for debugging tests The full policy status is often helpful when understanding what might be unexpectedly happening in some of the operatorpolicy tests. Signed-off-by: Justin Kulikauskas --- test/e2e/case38_install_operator_test.go | 98 +++++++++--------------- 1 file changed, 38 insertions(+), 60 deletions(-) diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 6fa1bc84..72da6553 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -30,6 +30,36 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { olmWaitTimeout = 60 ) + // checks that the compliance state eventually matches what is desired + checkCompliance := func(polName string, ns string, timeoutSeconds int, comp policyv1.ComplianceState) { + var debugMessage string + + DeferCleanup(func() { + if CurrentSpecReport().Failed() { + GinkgoWriter.Println(debugMessage) + } + }) + + EventuallyWithOffset(1, func(g Gomega) { + unstructPolicy, err := clientManagedDynamic.Resource(gvrOperatorPolicy).Namespace(ns). + Get(context.TODO(), polName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + + unstructured.RemoveNestedField(unstructPolicy.Object, "metadata", "managedFields") + + policyJSON, err := json.MarshalIndent(unstructPolicy.Object, "", " ") + g.Expect(err).NotTo(HaveOccurred()) + + debugMessage = fmt.Sprintf("Debug info for failure.\npolicy JSON: %s", policyJSON) + + policy := policyv1beta1.OperatorPolicy{} + err = json.Unmarshal(policyJSON, &policy) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(policy.Status.ComplianceState).To(Equal(comp)) + }, timeoutSeconds, 3).Should(Succeed()) + } + // checks that the policy has the proper compliance, that the relatedObjects of a given // type exactly match the list given (no extras or omissions), that the condition is present, // and that an event was emitted that matches the given snippet. @@ -375,15 +405,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { It("Should be compliant when enforced", func() { By("Waiting for the operator policy " + opPolName + " to be compliant") - Eventually(func() string { - opPolicy := utils.GetWithTimeout( - clientManagedDynamic, gvrOperatorPolicy, opPolName, testNamespace, true, defaultTimeoutSeconds, - ) - - compliance, _, _ := unstructured.NestedString(opPolicy.Object, "status", "compliant") - - return compliance - }, defaultTimeoutSeconds*2, 1).Should(Equal("Compliant")) + // Wait for a while, because it might have upgrades that could take longer + checkCompliance(opPolName, testNamespace, olmWaitTimeout*2, policyv1.Compliant) }) }) @@ -1385,15 +1408,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) check( opPolName, @@ -1714,12 +1729,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { ) // The `check` function doesn't check that it is compliant, only that each piece is compliant - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - Expect(err).NotTo(HaveOccurred()) - Expect(found).To(BeTrue()) - Expect(compliance).To(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) }) It("Should be NonCompliant and report resources when the operator is installed", func(ctx SpecContext) { // Make it musthave and enforced, to install the operator @@ -2386,15 +2396,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) By("Verifying that an operator group exists") Eventually(func(g Gomega) []unstructured.Unstructured { @@ -2437,15 +2439,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) By("Verifying that an operator group exists") Eventually(func(g Gomega) []unstructured.Unstructured { @@ -2488,15 +2482,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) By("Verifying that an operator group exists") Eventually(func(g Gomega) []unstructured.Unstructured { @@ -2549,15 +2535,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) By("Waiting for the policy to become compliant, indicating the operator is installed") - Eventually(func(g Gomega) string { - pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName, - opPolTestNS, true, eventuallyTimeout) - compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue()) - - return compliance - }, olmWaitTimeout, 5, ctx).Should(Equal("Compliant")) + checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) By("Verifying that an operator group exists") Eventually(func(g Gomega) []unstructured.Unstructured { From 3b6ef6aaf321d50dff132c33100da9d5a02fc821 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 1 May 2024 16:59:50 -0400 Subject: [PATCH 7/7] More version fixes for tests This should help with some consistency. Signed-off-by: Justin Kulikauskas --- test/e2e/case38_install_operator_test.go | 30 ++++++------------- ...erator-policy-mustnothave-any-version.yaml | 28 +++++++++++++++++ .../operator-policy-no-group-one-version.yaml | 26 ++++++++++++++++ 3 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml create mode 100644 test/resources/case38_operator_install/operator-policy-no-group-one-version.yaml diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 72da6553..91b9b1a5 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -919,16 +919,12 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Reason: "UnsupportedOperatorGroup", }}, metav1.Condition{ - Type: "ClusterServiceVersionCompliant", - Status: metav1.ConditionFalse, - Reason: "UnsupportedOperatorGroup", - Message: "ClusterServiceVersion (etcdoperator.v0.9.2) - AllNamespaces InstallModeType not " + - "supported, cannot configure to watch all namespaces", + Type: "ClusterServiceVersionCompliant", + Status: metav1.ConditionFalse, + Reason: "UnsupportedOperatorGroup", + Message: "AllNamespaces InstallModeType not supported", }, - regexp.QuoteMeta( - "ClusterServiceVersion (etcdoperator.v0.9.2) - AllNamespaces InstallModeType not supported,"+ - " cannot configure to watch all namespaces", - ), + "AllNamespaces InstallModeType not supported", ) }) @@ -1357,7 +1353,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }) Describe("Testing CustomResourceDefinition reporting", Ordered, func() { const ( - opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml" + opPolYAML = "../resources/case38_operator_install/operator-policy-no-group-one-version.yaml" opPolName = "oppol-no-group" ) BeforeAll(func() { @@ -1407,9 +1403,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { return crd }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) - By("Waiting for the policy to become compliant, indicating the operator is installed") - checkCompliance(opPolName, opPolTestNS, olmWaitTimeout, policyv1.Compliant) - check( opPolName, false, @@ -2363,7 +2356,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }) Describe("Testing mustnothave behavior of operator groups in DeleteIfUnused mode", Ordered, func() { const ( - opPolYAML = "../resources/case38_operator_install/operator-policy-mustnothave.yaml" + opPolYAML = "../resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml" otherYAML = "../resources/case38_operator_install/operator-policy-authorino.yaml" opPolName = "oppol-mustnothave" subName = "project-quay" @@ -2554,13 +2547,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { utils.Kubectl("patch", "operatorpolicy", "oppol-authorino", "-n", opPolTestNS, "--type=json", "-p", `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) - By("Waiting for a CRD to appear, which should indicate the other operator was successfully installed.") - Eventually(func(ctx SpecContext) *unstructured.Unstructured { - crd, _ := clientManagedDynamic.Resource(gvrCRD).Get(ctx, - "authconfigs.authorino.kuadrant.io", metav1.GetOptions{}) - - return crd - }, olmWaitTimeout, 5, ctx).ShouldNot(BeNil()) + By("Waiting for the policy to become compliant, indicating the operator is installed") + checkCompliance("oppol-authorino", opPolTestNS, olmWaitTimeout, policyv1.Compliant) // revert main policy to mustnothave utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", diff --git a/test/resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml b/test/resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml new file mode 100644 index 00000000..179479f2 --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml @@ -0,0 +1,28 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-mustnothave + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: inform + severity: medium + complianceType: mustnothave + subscription: + channel: stable-3.10 + name: project-quay + namespace: operator-policy-testns + installPlanApproval: Manual + source: operatorhubio-catalog + sourceNamespace: olm + removalBehavior: + operatorGroups: DeleteIfUnused + subscriptions: Delete + clusterServiceVersions: Delete + customResourceDefinitions: Delete diff --git a/test/resources/case38_operator_install/operator-policy-no-group-one-version.yaml b/test/resources/case38_operator_install/operator-policy-no-group-one-version.yaml new file mode 100644 index 00000000..e5980d27 --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-no-group-one-version.yaml @@ -0,0 +1,26 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-no-group + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: inform + severity: medium + complianceType: musthave + subscription: + channel: stable-3.10 + name: project-quay + namespace: operator-policy-testns + installPlanApproval: Automatic + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: quay-operator.v3.10.0 + versions: + - quay-operator.v3.10.0