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 <jkulikau@redhat.com> (cherry picked from commit c4cfa08)
stolostron · Feb 20, 2024 · 1ab13d2 · 1ab13d2
1 parent d614604
commit 1ab13d2
Show file tree

Hide file tree

Showing 4 changed files with 249 additions and 29 deletions.
diff --git 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
@@ -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
@@ -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",
+			)
+		})
+	})
 })