From 195b1382ba16797c1f8fbb7aa9baa8dae3210cab Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Mon, 10 Jun 2024 19:13:34 -0400 Subject: [PATCH] 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 --- controllers/operatorpolicy_controller.go | 51 ++++++++++++++----- controllers/operatorpolicy_controller_test.go | 4 +- test/e2e/case38_install_operator_test.go | 17 +++++-- .../operator-policy-all-defaults.yaml | 3 +- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index b92bdc0f..fe85888d 100644 --- a/controllers/operatorpolicy_controller.go +++ b/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 diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index 677bb3ad..e772ac87 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/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) }, ) diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 861ab470..233f32a2 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/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()) diff --git a/test/resources/case38_operator_install/operator-policy-all-defaults.yaml b/test/resources/case38_operator_install/operator-policy-all-defaults.yaml index a677e9c0..a8a80e30 100644 --- a/test/resources/case38_operator_install/operator-policy-all-defaults.yaml +++ b/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