Fix operatorpolicy confusing status when invalid

When the subscription section of the OperatorPolicy is invalid, the status might incorrectly indicate that other parts of the policy are invalid. This, combined with the many "... because the policy is invalid" clauses in the overall status, makes it very confusing to read and understand what parts might need to be fixed by the user.
Ref: https://issues.redhat.com/browse/ACM-16781
Signed-off-by: yiraeChristineKim <[email protected]>

Author: yiraeChristineKim

controllers/operatorpolicy_controller.go

@@ -489,9 +489,10 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 ) {
 	var returnedErr error
 	var tmplResolver *templates.TemplateResolver
+	var sub *operatorv1alpha1.Subscription
+	var opGroup *operatorv1.OperatorGroup
 
 	opLog := ctrl.LoggerFrom(ctx)
-	validationErrors := make([]error, 0)
 	disableTemplates := false
 
 	if disableAnnotation, ok := policy.GetAnnotations()["policy.open-cluster-management.io/disable-templates"]; ok {
@@ -505,12 +506,16 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 			r.DynamicWatcher, templates.Config{SkipBatchManagement: true},
 		)
 		if err != nil {
-			validationErrors = append(validationErrors, fmt.Errorf("unable to create template resolver: %w", err))
-		} else {
-			err := resolveVersionsTemplates(policy, tmplResolver)
-			if err != nil {
-				validationErrors = append(validationErrors, err)
-			}
+			newError := fmt.Errorf("unable to create template resolver: %w", err)
+
+			return sub, opGroup, updateStatus(policy, validationCond([]error{newError})), returnedErr
+		}
+
+		err = resolveVersionsTemplates(policy, tmplResolver)
+		if err != nil {
+			newError := fmt.Errorf("unable to create template resolver: %w", err)
+
+			return sub, opGroup, updateStatus(policy, validationCond([]error{newError})), returnedErr
 		}
 	} else {
 		opLog.V(1).Info("Templates disabled by annotation")
@@ -530,18 +535,22 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 				returnedErr = err
 			}
 
-			validationErrors = append(validationErrors, err)
+			updateStatus(policy, validationCond([]error{err}))
+
+			return sub, opGroup, true, returnedErr
 		}
 
 		if sub != nil && sub.Namespace == "" {
 			if r.DefaultNamespace != "" {
 				sub.Namespace = r.DefaultNamespace
 			} else {
-				validationErrors = append(validationErrors, errors.New("namespace is required in spec.subscription"))
+				newError := errors.New("namespace is required in spec.subscription")
+
+				return sub, opGroup, updateStatus(policy, validationCond([]error{newError})), returnedErr
 			}
 		}
 	} else {
-		validationErrors = append(validationErrors, subErr)
+		return sub, opGroup, updateStatus(policy, validationCond([]error{subErr})), returnedErr
 	}
 
 	opGroupNS := r.DefaultNamespace
@@ -551,23 +560,24 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 
 	opGroup, ogErr := buildOperatorGroup(policy, opGroupNS, tmplResolver)
 	if ogErr != nil {
-		validationErrors = append(validationErrors, ogErr)
-	} else {
-		watcher := opPolIdentifier(policy.Namespace, policy.Name)
+		return sub, opGroup, updateStatus(policy, validationCond([]error{ogErr})), returnedErr
+	}
 
-		gotNamespace, err := r.DynamicWatcher.Get(watcher, namespaceGVK, "", opGroupNS)
-		if err != nil {
-			return sub, opGroup, false, fmt.Errorf("error getting operator namespace: %w", err)
-		}
+	watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
-		if gotNamespace == nil && policy.Spec.ComplianceType.IsMustHave() {
-			validationErrors = append(validationErrors,
-				fmt.Errorf("the operator namespace ('%v') does not exist", opGroupNS))
-		}
+	gotNamespace, err := r.DynamicWatcher.Get(watcher, namespaceGVK, "", opGroupNS)
+	if err != nil {
+		return sub, opGroup, false, fmt.Errorf("error getting operator namespace: %w", err)
+	}
+
+	if gotNamespace == nil && policy.Spec.ComplianceType.IsMustHave() {
+		newError := fmt.Errorf("the operator namespace ('%v') does not exist", opGroupNS)
+
+		return sub, opGroup, updateStatus(policy, validationCond([]error{newError})), returnedErr
 	}
 
 	changed, overlapErr, apiErr := r.checkSubOverlap(ctx, policy, sub)
-	if apiErr != nil && returnedErr == nil {
+	if apiErr != nil {
 		returnedErr = apiErr
 	}
 
@@ -577,10 +587,12 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 		sub = nil
 		opGroup = nil
 
-		validationErrors = append(validationErrors, overlapErr)
+		updateStatus(policy, validationCond([]error{overlapErr}))
+
+		return sub, opGroup, true, returnedErr
 	}
 
-	changed = updateStatus(policy, validationCond(validationErrors)) || changed
+	changed = changed || updateStatus(policy, validationCond([]error{}))
 
 	return sub, opGroup, changed, returnedErr
 }

controllers/operatorpolicy_controller_test.go

@@ -14,10 +14,230 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/yaml"
+	dynamicfake "k8s.io/client-go/dynamic/fake"
 
 	policyv1beta1 "open-cluster-management.io/config-policy-controller/api/v1beta1"
 )
 
+func TestBuildResources_SubscriptionErrorUpdatesStatus(t *testing.T) {
+	t.Parallel()
+
+	r := &OperatorPolicyReconciler{}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-policy",
+			Namespace: "default",
+			Annotations: map[string]string{
+				"policy.open-cluster-management.io/disable-templates": "true",
+			},
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			Severity:          "low",
+			RemediationAction: "inform",
+			ComplianceType:    "musthave",
+			Subscription: runtime.RawExtension{
+				Raw: []byte(`{
+					"namespace": "default",
+					"source": "my-catalog",
+					"sourceNamespace": "my-ns",
+					"name": "",
+					"channel": "stable"
+				}`),
+			},
+			UpgradeApproval: "None",
+		},
+	}
+
+	_, _, changed, returnedErr := r.buildResources(t.Context(), policy)
+	assert.True(t, changed, "expected status to be updated")
+	assert.NoError(t, returnedErr)
+
+	_, cond := policy.Status.GetCondition(validPolicyConditionType)
+	assert.Equal(t, metav1.ConditionFalse, cond.Status)
+	assert.Equal(t, "InvalidPolicySpec", cond.Reason)
+	assert.Equal(t, "name is required in spec.subscription", cond.Message)
+}
+
+func TestBuildResources_subInstallPlanApprovalErrorUpdatesStatus(t *testing.T) {
+	t.Parallel()
+
+	r := &OperatorPolicyReconciler{}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-policy",
+			Namespace: "default",
+			Annotations: map[string]string{
+				"policy.open-cluster-management.io/disable-templates": "true",
+			},
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			Severity:          "low",
+			RemediationAction: "inform",
+			ComplianceType:    "musthave",
+			Subscription: runtime.RawExtension{
+				Raw: []byte(`{
+					"namespace": "default",
+					"source": "my-catalog",
+					"sourceNamespace": "my-ns",
+					"name": "my-operator",
+					"channel": "stable",
+					"installPlanApproval": "ERR"
+				}`),
+			},
+			UpgradeApproval: "None",
+		},
+	}
+
+	_, _, changed, returnedErr := r.buildResources(t.Context(), policy)
+	assert.True(t, changed, "expected status to be updated")
+	assert.NoError(t, returnedErr)
+
+	_, cond := policy.Status.GetCondition(validPolicyConditionType)
+	assert.Equal(t, metav1.ConditionFalse, cond.Status)
+	assert.Equal(t, "InvalidPolicySpec", cond.Reason)
+	assert.Equal(t, "installPlanApproval is prohibited in spec.subscription", cond.Message)
+}
+
+func TestBuildResources_SubDefaultsPkgManifestNotFoundUpdatesStatusAndReturnsErr(t *testing.T) {
+	// To test line 542
+	t.Parallel()
+
+	r := &OperatorPolicyReconciler{
+		// Use a fake dynamic client without any objects so PackageManifest GET returns NotFound
+		DynamicClient: dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()),
+	}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-policy",
+			Namespace: "default",
+			Annotations: map[string]string{
+				"policy.open-cluster-management.io/disable-templates": "true",
+			},
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			Severity:          "low",
+			RemediationAction: "inform",
+			ComplianceType:    "musthave",
+			Subscription: runtime.RawExtension{
+				// Omit namespace key entirely so buildSubscription succeeds with empty namespace,
+				// and omit source/sourceNamespace so defaultsNeeded = true (triggers PackageManifest lookup).
+				Raw: []byte(`{
+					"name": "my-operator",
+					"channel": "stable",
+					"startingCSV": "my-operator-v1"
+				}`),
+			},
+			UpgradeApproval: "None",
+		},
+	}
+
+	sub, opGroup, changed, returnedErr := r.buildResources(t.Context(), policy)
+	assert.True(t, changed, "expected status to be updated")
+	assert.ErrorIs(t, returnedErr, ErrPackageManifest, "expected returned error to wrap ErrPackageManifest")
+
+	_, cond := policy.Status.GetCondition(validPolicyConditionType)
+	assert.Equal(t, metav1.ConditionFalse, cond.Status)
+	assert.Equal(t, "InvalidPolicySpec", cond.Reason)
+	assert.Equal(t,
+		"the subscription defaults could not be determined because the PackageManifest was not found",
+		cond.Message)
+	assert.Nil(t, sub, "expected subscription to be nil")
+	assert.Nil(t, opGroup, "expected operator group to be nil")
+}
+
+func TestBuildResources_OperatorGroupErrorUpdatesStatus(t *testing.T) {
+	t.Parallel()
+
+	r := &OperatorPolicyReconciler{}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-policy",
+			Namespace: "default",
+			Annotations: map[string]string{
+				"policy.open-cluster-management.io/disable-templates": "true",
+			},
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			Severity:          "low",
+			RemediationAction: "inform",
+			ComplianceType:    "musthave",
+			Subscription: runtime.RawExtension{
+				Raw: []byte(`{
+					"namespace": "default",
+					"source": "my-catalog",
+					"sourceNamespace": "my-ns",
+					"name": "my-operator",
+					"channel": "stable"
+				}`),
+			},
+			// Invalid operatorGroup (missing name) should trigger a validation error
+			OperatorGroup: &runtime.RawExtension{
+				Raw: []byte(`{}`),
+			},
+			UpgradeApproval: "None",
+		},
+	}
+
+	_, _, changed, returnedErr := r.buildResources(t.Context(), policy)
+	assert.True(t, changed, "expected status to be updated")
+	assert.NoError(t, returnedErr)
+
+	_, cond := policy.Status.GetCondition(validPolicyConditionType)
+	assert.Equal(t, metav1.ConditionFalse, cond.Status)
+	assert.Equal(t, "InvalidPolicySpec", cond.Reason)
+	assert.Equal(t, "name is required in spec.operatorGroup", cond.Message)
+}
+
+func TestBuildResources_TemplateResolverCreationErrorUpdatesStatus(t *testing.T) {
+	t.Parallel()
+
+	r := &OperatorPolicyReconciler{
+		DynamicWatcher: nil,
+	}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-policy",
+			Namespace: "default",
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			Severity:          "low",
+			RemediationAction: "inform",
+			ComplianceType:    "musthave",
+			// Include an obviously bad template so template resolution would fail if reached.
+			// In this test, resolver creation itself is expected to fail earlier.
+			Versions: []string{"{{ .badTemplate "},
+			Subscription: runtime.RawExtension{
+				Raw: []byte(`{
+					"namespace": "default",
+					"name": "my-operator",
+					"channel": "stable",
+					"startingCSV": "my-operator-v1"
+				}`),
+			},
+			UpgradeApproval: "None",
+		},
+	}
+
+	sub, opGroup, changed, returnedErr := r.buildResources(t.Context(), policy)
+	assert.True(t, changed, "expected status to be updated")
+	assert.NoError(t, returnedErr)
+	assert.Nil(t, sub, "expected subscription to be nil on early return")
+	assert.Nil(t, opGroup, "expected operator group to be nil on early return")
+
+	_, cond := policy.Status.GetCondition(validPolicyConditionType)
+	assert.Equal(t, metav1.ConditionFalse, cond.Status)
+	assert.Equal(t, "InvalidPolicySpec", cond.Reason)
+	assert.Equal(t,
+		"unable to create template resolver: could not resolve the version template: "+
+			"failed to parse the template JSON string [\"{{ .badTemplate \"]: template: tmpl:1: "+
+			"unterminated character constant", cond.Message)
+}
+
 func TestBuildSubscription(t *testing.T) {
 	testPolicy := &policyv1beta1.OperatorPolicy{
 		ObjectMeta: metav1.ObjectMeta{