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.

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

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

controllers/operatorpolicy_controller.go

@@ -400,7 +400,7 @@ 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)
  if err != nil {
@@ -414,6 +414,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)
  }
@@ -573,7 +581,8 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
  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
@@ -635,6 +644,28 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
  subSpec.CatalogSourceNamespace = catalogNamespace
  }
 
+ if subscription.Namespace == "" {
+ 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
+
+ break
+ }
+ }
+
  return nil
 }
 
@@ -643,7 +674,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)
 
@@ -679,16 +710,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,15 +212,24 @@ 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() {
  preFunc()
 
  createObjWithParent(parentPolicyYAML, parentPolicyName,
  opPolYAML, testNamespace, gvrPolicy, gvrOperatorPolicy)
+
+ DeferCleanup(func() {
+ utils.Kubectl("delete", "ns", suggestedNS, "--ignore-not-found")
+
+ if IsHosted {
+ KubectlTarget("delete", "ns", suggestedNS, "--ignore-not-found")
+ }
+ })
  })
 
  It("Should create the Subscription with default values", func(ctx context.Context) {
@@ -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