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