🤖 Sync from open-cluster-management-io/config-policy-controller: #207

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)

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{

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",
+			)
+		})
+	})
 })