From fb5b5f44b4ae6b4307da16ad73e5c750e5a663aa Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 20 Feb 2024 10:46:06 -0500 Subject: [PATCH] Add more validation to the OperatorPolicy The `ValidPolicySpec` condition now reports when the subscription or operatorGroup in the policy have unknown fields. It also reports if the InstallPlanApproval value in the subscription is not correct. That condition also reports when the namespace for the subscription does not exist, and the controller now watches that namespace so that if it is created (or deleted), the policy will be reconciled and the status will be updated. Refs: - https://issues.redhat.com/browse/ACM-9993 Signed-off-by: Justin Kulikauskas (cherry picked from commit c4cfa083012ff35b0bdc85a3f46ab24b27cfb994) --- controllers/operatorpolicy_controller.go | 73 ++++++-- controllers/operatorpolicy_status.go | 4 +- test/e2e/case38_install_operator_test.go | 170 ++++++++++++++++-- .../operator-policy-validity-test.yaml | 31 ++++ 4 files changed, 249 insertions(+), 29 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-validity-test.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index b4383330..04aad038 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -4,6 +4,7 @@ package controllers import ( + "bytes" "context" "encoding/json" "errors" @@ -40,6 +41,11 @@ const ( ) var ( + namespaceGVK = schema.GroupVersionKind{ + Group: "", + Version: "v1", + Kind: "Namespace", + } subscriptionGVK = schema.GroupVersionKind{ Group: "operators.coreos.com", Version: "v1alpha1", @@ -223,6 +229,18 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p validationErrors = append(validationErrors, ogErr) } + watcher := opPolIdentifier(policy.Namespace, policy.Name) + + gotNamespace, err := r.DynamicWatcher.Get(watcher, namespaceGVK, "", opGroupNS) + if err != nil { + return sub, opGroup, fmt.Errorf("error getting operator namespace: %w", err) + } + + if gotNamespace == nil { + validationErrors = append(validationErrors, + fmt.Errorf("the operator namespace ('%v') does not exist", opGroupNS)) + } + return sub, opGroup, r.updateStatus(ctx, policy, validationCond(validationErrors)) } @@ -255,18 +273,35 @@ func buildSubscription( return nil, fmt.Errorf("the namespace '%v' used for the subscription is not a valid namespace identifier", ns) } - spec := new(operatorv1alpha1.SubscriptionSpec) + // This field is not actually in the subscription spec + delete(sub, "namespace") - err = json.Unmarshal(policy.Spec.Subscription.Raw, spec) + subSpec, err := json.Marshal(sub) if err != nil { return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err) } + // Use a decoder to find fields that were erroneously set by the user. + dec := json.NewDecoder(bytes.NewReader(subSpec)) + dec.DisallowUnknownFields() + + spec := new(operatorv1alpha1.SubscriptionSpec) + + if err := dec.Decode(spec); err != nil { + return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err) + } + subscription.SetGroupVersionKind(subscriptionGVK) subscription.ObjectMeta.Name = spec.Package subscription.ObjectMeta.Namespace = ns subscription.Spec = spec + // This is not validated by the CRD, so validate it here to prevent unexpected behavior. + if !(spec.InstallPlanApproval == "Manual" || spec.InstallPlanApproval == "Automatic") { + return nil, fmt.Errorf("the policy spec.subscription.installPlanApproval ('%v') is invalid: "+ + "must be 'Automatic' or 'Manual'", spec.InstallPlanApproval) + } + // If the policy is in `enforce` mode and the allowed CSVs are restricted, // the InstallPlanApproval will be set to Manual so that upgrades can be controlled. if policy.Spec.RemediationAction.IsEnforce() && len(policy.Spec.Versions) > 0 { @@ -302,7 +337,7 @@ func buildOperatorGroup( } if specifiedNS, ok := opGroup["namespace"].(string); ok && specifiedNS != "" { - if specifiedNS != namespace { + if specifiedNS != namespace && namespace != "" { return nil, fmt.Errorf("the namespace specified in spec.operatorGroup ('%v') must match "+ "the namespace used for the subscription ('%v')", specifiedNS, namespace) } @@ -313,9 +348,22 @@ func buildOperatorGroup( return nil, fmt.Errorf("name is required in spec.operatorGroup") } + // These fields are not actually in the operatorGroup spec + delete(opGroup, "name") + delete(opGroup, "namespace") + + opGroupSpec, err := json.Marshal(opGroup) + if err != nil { + return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err) + } + + // Use a decoder to find fields that were erroneously set by the user. + dec := json.NewDecoder(bytes.NewReader(opGroupSpec)) + dec.DisallowUnknownFields() + spec := new(operatorv1.OperatorGroupSpec) - if err := json.Unmarshal(policy.Spec.OperatorGroup.Raw, spec); err != nil { + if err := dec.Decode(spec); err != nil { return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err) } @@ -331,12 +379,14 @@ func (r *OperatorPolicyReconciler) handleOpGroup( ) error { watcher := opPolIdentifier(policy.Namespace, policy.Name) - if desiredOpGroup == nil { + if desiredOpGroup == nil || desiredOpGroup.Namespace == "" { // Note: existing related objects will not be removed by this status update err := r.updateStatus(ctx, policy, invalidCausingUnknownCond("OperatorGroup")) if err != nil { return fmt.Errorf("error updating the status when the OperatorGroup could not be determined: %w", err) } + + return nil } foundOpGroups, err := r.DynamicWatcher.List( @@ -830,7 +880,8 @@ func (r *OperatorPolicyReconciler) handleCSV(ctx context.Context, // need to report lack of existing CSV err := r.updateStatus(ctx, policy, noCSVCond, noExistingCSVObj) if err != nil { - return nil, fmt.Errorf("error updating the status for Deployments: %w", err) + return nil, fmt.Errorf("error updating the status for ClusterServiceVersion "+ + " with nonexistent Subscription: %w", err) } return nil, err @@ -842,7 +893,7 @@ func (r *OperatorPolicyReconciler) handleCSV(ctx context.Context, if sub.Status.InstalledCSV == "" { err := r.updateStatus(ctx, policy, noCSVCond, noExistingCSVObj) if err != nil { - return nil, fmt.Errorf("error updating the status for Deployments: %w", err) + return nil, fmt.Errorf("error updating the status for ClusterServiceVersion yet to be installed: %w", err) } return nil, err @@ -893,10 +944,10 @@ func (r *OperatorPolicyReconciler) handleDeployment( // need to report lack of existing Deployments err := r.updateStatus(ctx, policy, noDeploymentsCond, noExistingDeploymentObj) if err != nil { - return fmt.Errorf("error updating the status for Deployments: %w", err) + return fmt.Errorf("error updating the status for nonexistent Deployments: %w", err) } - return err + return nil } OpLog := ctrl.LoggerFrom(ctx) @@ -961,7 +1012,7 @@ func (r *OperatorPolicyReconciler) handleCatalogSource( // Note: existing related objects will not be removed by this status update err := r.updateStatus(ctx, policy, invalidCausingUnknownCond("CatalogSource")) if err != nil { - return fmt.Errorf("error updating the status when the could not be determined: %w", err) + return fmt.Errorf("error updating the status for CatalogSource with nonexistent Subscription: %w", err) } return nil @@ -1005,7 +1056,7 @@ func (r *OperatorPolicyReconciler) handleCatalogSource( isUnhealthy = (CatalogSrcState != CatalogSourceReady) } - err = r.updateStatus(ctx, policy, catalogSourceFindCond(isUnhealthy, isMissing), + err = r.updateStatus(ctx, policy, catalogSourceFindCond(isUnhealthy, isMissing, catalogName), catalogSourceObj(catalogName, catalogNS, isUnhealthy, isMissing)) if err != nil { return fmt.Errorf("error updating the status for a CatalogSource: %w", err) diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index 0bb0bcbb..33bccd62 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -643,7 +643,7 @@ var noDeploymentsCond = metav1.Condition{ // catalogSourceFindCond is a conditionally compliant condition with reason // based on the `isUnhealthy` and `isMissing` parameters -func catalogSourceFindCond(isUnhealthy bool, isMissing bool) metav1.Condition { +func catalogSourceFindCond(isUnhealthy bool, isMissing bool, name string) metav1.Condition { status := metav1.ConditionFalse reason := "CatalogSourcesFound" message := "CatalogSource was found" @@ -657,7 +657,7 @@ func catalogSourceFindCond(isUnhealthy bool, isMissing bool) metav1.Condition { if isMissing { status = metav1.ConditionTrue reason = "CatalogSourcesNotFound" - message = "CatalogSource was not found" + message = "CatalogSource '" + name + "' was not found" } return metav1.Condition{ diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 32ad3f12..bef42465 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -53,22 +53,24 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun g.Expect(policy.Status.ComplianceState).To(Equal(policyv1.NonCompliant)) } - matchingRelated := policy.Status.RelatedObjsOfKind(expectedRelatedObjs[0].Object.Kind) - g.Expect(matchingRelated).To(HaveLen(len(expectedRelatedObjs))) - - for _, expectedRelObj := range expectedRelatedObjs { - foundMatchingName := false - unnamed := expectedRelObj.Object.Metadata.Name == "" - - for _, actualRelObj := range matchingRelated { - if unnamed || actualRelObj.Object.Metadata.Name == expectedRelObj.Object.Metadata.Name { - foundMatchingName = true - g.Expect(actualRelObj.Compliant).To(Equal(expectedRelObj.Compliant)) - g.Expect(actualRelObj.Reason).To(Equal(expectedRelObj.Reason)) + if len(expectedRelatedObjs) != 0 { + matchingRelated := policy.Status.RelatedObjsOfKind(expectedRelatedObjs[0].Object.Kind) + g.Expect(matchingRelated).To(HaveLen(len(expectedRelatedObjs))) + + for _, expectedRelObj := range expectedRelatedObjs { + foundMatchingName := false + unnamed := expectedRelObj.Object.Metadata.Name == "" + + for _, actualRelObj := range matchingRelated { + if unnamed || actualRelObj.Object.Metadata.Name == expectedRelObj.Object.Metadata.Name { + foundMatchingName = true + g.Expect(actualRelObj.Compliant).To(Equal(expectedRelObj.Compliant)) + g.Expect(actualRelObj.Reason).To(Equal(expectedRelObj.Reason)) + } } - } - g.Expect(foundMatchingName).To(BeTrue()) + g.Expect(foundMatchingName).To(BeTrue()) + } } idx, actualCondition := policy.Status.GetCondition(expectedCondition.Type) @@ -809,9 +811,9 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun Type: "CatalogSourcesUnhealthy", Status: metav1.ConditionTrue, Reason: "CatalogSourcesNotFound", - Message: "CatalogSource was not found", + Message: "CatalogSource 'fakeName' was not found", }, - "CatalogSource was not found", + "CatalogSource 'fakeName' was not found", ) }) It("Should remain NonCompliant when CatalogSource fails", func() { @@ -1097,4 +1099,140 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun ) }) }) + Describe("Testing OperatorPolicy validation messages", Ordered, func() { + const ( + opPolYAML = "../resources/case38_operator_install/operator-policy-validity-test.yaml" + opPolName = "oppol-validity-test" + subName = "project-quay" + ) + + BeforeAll(func() { + utils.Kubectl("create", "ns", opPolTestNS) + DeferCleanup(func() { + utils.Kubectl("delete", "ns", opPolTestNS) + utils.Kubectl("delete", "ns", "nonexist-testns") + }) + + createObjWithParent(parentPolicyYAML, parentPolicyName, + opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy) + }) + + It("Should initially report unknown fields", func() { + check( + opPolName, + true, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: `spec.subscription is invalid: json: unknown field "actually"`, + }, + `the status of the Subscription could not be determined because the policy is invalid`, + ) + check( + opPolName, + true, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: `spec.operatorGroup is invalid: json: unknown field "foo"`, + }, + `the status of the OperatorGroup could not be determined because the policy is invalid`, + ) + }) + It("Should report about the invalid installPlanApproval value", func() { + // remove the "unknown" fields + utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "remove", "path": "/spec/operatorGroup/foo"}, `+ + `{"op": "remove", "path": "/spec/subscription/actually"}]`) + check( + opPolName, + true, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: "spec.subscription.installPlanApproval ('Incorrect') is invalid: " + + "must be 'Automatic' or 'Manual'", + }, + "NonCompliant", + ) + }) + It("Should report about the namespaces not matching", func() { + // Fix the `installPlanApproval` value + utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "replace", "path": "/spec/subscription/installPlanApproval", "value": "Automatic"}]`) + check( + opPolName, + true, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: "the namespace specified in spec.operatorGroup ('operator-policy-testns') must match " + + "the namespace used for the subscription ('nonexist-testns')", + }, + "NonCompliant", + ) + }) + It("Should report about the namespace not existing", func() { + // Fix the namespace mismatch by removing the operator group spec + utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "remove", "path": "/spec/operatorGroup"}]`) + check( + opPolName, + true, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Name: subName, + Namespace: "nonexist-testns", + }, + }, + Compliant: "NonCompliant", + Reason: "Resource not found but should exist", + }}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: "the operator namespace ('nonexist-testns') does not exist", + }, + "NonCompliant", + ) + }) + It("Should update the status after the namespace is created", func() { + utils.Kubectl("create", "namespace", "nonexist-testns") + check( + opPolName, + true, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Name: subName, + Namespace: "nonexist-testns", + }, + }, + Compliant: "NonCompliant", + Reason: "Resource not found but should exist", + }}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionTrue, + Reason: "PolicyValidated", + Message: "the policy spec is valid", + }, + "the policy spec is valid", + ) + }) + }) }) diff --git a/test/resources/case38_operator_install/operator-policy-validity-test.yaml b/test/resources/case38_operator_install/operator-policy-validity-test.yaml new file mode 100644 index 00000000..12d4e0eb --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-validity-test.yaml @@ -0,0 +1,31 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-validity-test + 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 + foo: bar + name: scoped-operator-group + namespace: operator-policy-testns + targetNamespaces: + - operator-policy-testns + subscription: + actually: incorrect + channel: stable-3.8 + name: project-quay + namespace: nonexist-testns + installPlanApproval: Incorrect + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: quay-operator.v3.8.1