From e6d912d94d30f3910537195d26e80bf343a32819 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 11 Jun 2024 16:44:24 -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. 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 --- controllers/operatorpolicy_controller.go | 68 +++++++++++++++---- controllers/operatorpolicy_controller_test.go | 4 +- test/e2e/case38_install_operator_test.go | 17 +++-- .../operator-policy-all-defaults.yaml | 3 +- 4 files changed, 70 insertions(+), 22 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 3bb94c80..0ca663c7 100644 --- a/controllers/operatorpolicy_controller.go +++ b/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 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 e7e3fe8f..57db832c 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/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()) 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