Use suggested namespace in package if not set

Previously, if the namespace was not set for a Subscription in an
OperatorPolicy, the default namespace on the controller (if set) would
be used in all cases. But many packages specify a suggested namespace in
their PackageManifests, which would be better to use, when provided.

Additionally, if the OperatorPolicy was already managing a subscription,
the namespace of that subscription will be used (if not set in the
policy). This may prevent some issues that could occur if the suggested
namespace for an operator changes at some point.

Refs:
 - https://issues.redhat.com/browse/ACM-12057

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

controllers/operatorpolicy_controller.go

@@ -396,9 +396,9 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 		opLog.V(1).Info("Templates disabled by annotation")
 	}
 
-	sub, subErr := buildSubscription(policy, r.DefaultNamespace, tmplResolver)
+	sub, subErr := buildSubscription(policy, tmplResolver)
 	if subErr == nil {
-		err := r.applySubscriptionDefaults(ctx, sub)
+		err := r.applySubscriptionDefaults(ctx, policy, sub)
 		if err != nil {
 			sub = nil
 
@@ -410,6 +410,14 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 
 			validationErrors = append(validationErrors, err)
 		}
+
+		if sub != nil && sub.Namespace == "" {
+			if r.DefaultNamespace != "" {
+				sub.Namespace = r.DefaultNamespace
+			} else {
+				validationErrors = append(validationErrors, errors.New("namespace is required in spec.subscription"))
+			}
+		}
 	} else {
 		validationErrors = append(validationErrors, subErr)
 	}
@@ -528,12 +536,13 @@ func (r *OperatorPolicyReconciler) checkSubOverlap(
 // applySubscriptionDefaults will set the subscription channel, source, and sourceNamespace when they are unset by
 // utilizing the PackageManifest API.
 func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
-	ctx context.Context, subscription *operatorv1alpha1.Subscription,
+	ctx context.Context, policy *policyv1beta1.OperatorPolicy, subscription *operatorv1alpha1.Subscription,
 ) error {
 	opLog := ctrl.LoggerFrom(ctx)
 	subSpec := subscription.Spec
 
-	defaultsNeeded := subSpec.Channel == "" || subSpec.CatalogSource == "" || subSpec.CatalogSourceNamespace == ""
+	defaultsNeeded := subSpec.Channel == "" || subSpec.CatalogSource == "" ||
+		subSpec.CatalogSourceNamespace == "" || subscription.Namespace == ""
 
 	if !defaultsNeeded {
 		return nil
@@ -595,6 +604,41 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
 		subSpec.CatalogSourceNamespace = catalogNamespace
 	}
 
+	if subscription.Namespace == "" {
+		// check for an already-known subscription to "adopt"
+		subs := policy.Status.RelatedObjsOfKind("Subscription")
+
+		if len(subs) == 1 {
+			// Note: RelatedObjsOfKind returns a map - in this case we just want the one object
+			for _, sub := range subs {
+				subscription.Namespace = sub.Object.Metadata.Namespace
+
+				return nil
+			}
+		}
+
+		// No known subscription - check for a suggested namespace in the PackageManifest
+		channels, _, _ := unstructured.NestedSlice(packageManifest.Object, "status", "channels")
+
+		for _, channel := range channels {
+			chanObj, ok := channel.(map[string]interface{})
+			if !ok {
+				continue
+			}
+
+			chanName, _, _ := unstructured.NestedString(chanObj, "name")
+			if chanName != subSpec.Channel {
+				continue
+			}
+
+			suggestedNS, _, _ := unstructured.NestedString(chanObj, "currentCSVDesc", "annotations",
+				"operatorframework.io/suggested-namespace")
+			subscription.Namespace = suggestedNS
+
+			return nil
+		}
+	}
+
 	return nil
 }
 
@@ -603,7 +647,7 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
 // If an error is returned, it will include details on why the policy spec if invalid and
 // why the desired subscription can't be determined.
 func buildSubscription(
-	policy *policyv1beta1.OperatorPolicy, defaultNS string, tmplResolver *templates.TemplateResolver,
+	policy *policyv1beta1.OperatorPolicy, tmplResolver *templates.TemplateResolver,
 ) (*operatorv1alpha1.Subscription, error) {
 	subscription := new(operatorv1alpha1.Subscription)
 
@@ -639,16 +683,12 @@ func buildSubscription(
 	}
 
 	ns, ok := sub["namespace"].(string)
-	if !ok {
-		if defaultNS == "" {
-			return nil, fmt.Errorf("namespace is required in spec.subscription")
+	if ok {
+		if validationErrs := validation.IsDNS1123Label(ns); len(validationErrs) != 0 {
+			return nil, fmt.Errorf(
+				"the namespace '%v' used for the subscription is not a valid namespace identifier", ns,
+			)
 		}
-
-		ns = defaultNS
-	}
-
-	if validationErrs := validation.IsDNS1123Label(ns); len(validationErrs) != 0 {
-		return nil, fmt.Errorf("the namespace '%v' used for the subscription is not a valid namespace identifier", ns)
 	}
 
 	// This field is not actually in the subscription spec

controllers/operatorpolicy_controller_test.go

@@ -43,7 +43,7 @@ func TestBuildSubscription(t *testing.T) {
 	}
 
 	// Check values are correctly bootstrapped to the Subscription
-	ret, err := buildSubscription(testPolicy, "my-operators", nil)
+	ret, err := buildSubscription(testPolicy, nil)
 	assert.Equal(t, err, nil)
 	assert.Equal(t, ret.GroupVersionKind(), desiredGVK)
 	assert.Equal(t, ret.ObjectMeta.Name, "my-operator")
@@ -103,7 +103,7 @@ func TestBuildSubscriptionInvalidNames(t *testing.T) {
 				}
 
 				// Check values are correctly bootstrapped to the Subscription
-				_, err := buildSubscription(testPolicy, "my-operators", nil)
+				_, err := buildSubscription(testPolicy, nil)
 				assert.Equal(t, err.Error(), test.expected)
 			},
 		)

test/e2e/case38_install_operator_test.go

@@ -212,11 +212,20 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 
 	Describe("Testing an all default operator policy", Ordered, func() {
 		const (
-			opPolYAML = "../resources/case38_operator_install/operator-policy-all-defaults.yaml"
-			opPolName = "oppol-all-defaults"
-			subName   = "project-quay"
+			opPolYAML   = "../resources/case38_operator_install/operator-policy-all-defaults.yaml"
+			opPolName   = "oppol-all-defaults"
+			subName     = "airflow-helm-operator"
+			suggestedNS = "airflow-helm"
 		)
 		BeforeAll(func() {
+			DeferCleanup(func() {
+				utils.Kubectl("delete", "ns", suggestedNS, "--ignore-not-found")
+
+				if IsHosted {
+					KubectlTarget("delete", "ns", suggestedNS, "--ignore-not-found")
+				}
+			})
+
 			preFunc()
 
 			createObjWithParent(parentPolicyYAML, parentPolicyName,
@@ -246,7 +255,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			)
 
 			By("Verifying the subscription has the correct defaults")
-			sub, err := targetK8sDynamic.Resource(gvrSubscription).Namespace(opPolTestNS).
+			sub, err := targetK8sDynamic.Resource(gvrSubscription).Namespace(suggestedNS).
 				Get(ctx, subName, metav1.GetOptions{})
 			Expect(err).ToNot(HaveOccurred())
 

test/resources/case38_operator_install/operator-policy-all-defaults.yaml

@@ -18,6 +18,5 @@ spec:
   severity: medium
   complianceType: musthave
   subscription:
-    name: project-quay
-    namespace: operator-policy-testns
+    name: airflow-helm-operator
   upgradeApproval: Automatic