Adopt existing sub when packagemanifest not found

Previously, if the pacakagemanifest was missing, perhaps due to the
operator no longer being available in the catalog, the operator policy
would be marked as invalid, and not report on the potentially helpful
status of the subscription. Now the existing subscription can be used
for the default values, and a more helpful status can be reported.

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

Signed-off-by: Justin Kulikauskas <[email protected]>

Author: JustinKuli

controllers/operatorpolicy_controller.go

@@ -571,7 +571,8 @@ func (r *OperatorPolicyReconciler) checkSubOverlap(
 }
 
 // applySubscriptionDefaults will set the subscription channel, source, and sourceNamespace when they are unset by
-// utilizing the PackageManifest API.
+// utilizing the PackageManifest API. If the PackageManifest can not be found, and a subscription already exists for the
+// operator, then information from the found subscription will be used.
 func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
 	ctx context.Context, policy *policyv1beta1.OperatorPolicy, subscription *operatorv1alpha1.Subscription,
 ) error {
@@ -594,10 +595,14 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
 	)
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
-			return fmt.Errorf(
-				"%wthe subscription defaults could not be determined because the PackageManifest was not found",
-				ErrPackageManifest,
-			)
+			if !r.usingExistingSubIfFound(policy, subscription) {
+				return fmt.Errorf(
+					"%wthe subscription defaults could not be determined because the PackageManifest was not found",
+					ErrPackageManifest,
+				)
+			}
+
+			return nil
 		}
 
 		log.Error(err, "Failed to get the PackageManifest", "name", subSpec.Package)
@@ -679,6 +684,55 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
 	return nil
 }
 
+// usingExistingSubIfFound attempts to find an existing subscription for the operator, and uses values
+// from that object to help build the desired subscription. It is meant to be used when the PackageManifest
+// for the operator is not found. It returns true when a subscription was found and used successfully.
+func (r *OperatorPolicyReconciler) usingExistingSubIfFound(
+	policy *policyv1beta1.OperatorPolicy, subscription *operatorv1alpha1.Subscription,
+) bool {
+	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
+			}
+		}
+	}
+
+	if subscription.Namespace != "" {
+		watcher := opPolIdentifier(policy.Namespace, policy.Name)
+
+		gotSub, err := r.DynamicWatcher.Get(watcher, subscriptionGVK, subscription.Namespace, subscription.Name)
+		if err != nil || gotSub == nil {
+			return false
+		}
+
+		foundSub := new(operatorv1alpha1.Subscription)
+		if err := runtime.DefaultUnstructuredConverter.FromUnstructured(gotSub.Object, foundSub); err != nil {
+			return false
+		}
+
+		if subscription.Spec.Channel == "" {
+			subscription.Spec.Channel = foundSub.Spec.Channel
+		}
+
+		if subscription.Spec.CatalogSource == "" {
+			subscription.Spec.CatalogSource = foundSub.Spec.CatalogSource
+		}
+
+		if subscription.Spec.CatalogSourceNamespace == "" {
+			subscription.Spec.CatalogSourceNamespace = foundSub.Spec.CatalogSourceNamespace
+		}
+
+		return true
+	}
+
+	return false
+}
+
 // buildSubscription bootstraps the subscription spec defined in the operator policy
 // with the apiversion and kind in preparation for resource creation.
 // If an error is returned, it will include details on why the policy spec if invalid and

test/e2e/case38_install_operator_test.go

@@ -246,6 +246,19 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			createObjWithParent(parentPolicyYAML, parentPolicyName,
 				opPolYAML, testNamespace, gvrPolicy, gvrOperatorPolicy)
 		})
+		AfterAll(func(ctx context.Context) {
+			By("Fixing the catalog source")
+			KubectlTarget("patch", "catalogsource", "operatorhubio-catalog", "--namespace=olm", "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/image", "value": "quay.io/operatorhubio/catalog:latest"}]`)
+
+			By("Waiting for a packagemanifest to reappear")
+			Eventually(func() error {
+				_, err := targetK8sDynamic.Resource(gvrPackageManifest).Namespace("default").Get(
+					ctx, "airflow-helm-operator", metav1.GetOptions{})
+
+				return err
+			}, olmWaitTimeout*2, 3).Should(Succeed())
+		})
 
 		It("Should create the Subscription with default values", func(ctx context.Context) {
 			By("Verifying the policy is compliant")
@@ -284,6 +297,34 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			sourceNamespace, _, _ := unstructured.NestedString(sub.Object, "spec", "sourceNamespace")
 			Expect(sourceNamespace).To(Equal("olm"))
 		})
+
+		It("Should adopt an existing subscription if removed from the catalog", func(ctx context.Context) {
+			By("Breaking the catalog source")
+			KubectlTarget("patch", "catalogsource", "operatorhubio-catalog", "--namespace=olm", "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/image", "value": "quay.io/operatorhubio/catalog:broken"}]`)
+
+			By("ensuring the packagemanifest is gone")
+			Eventually(func() error {
+				_, err := targetK8sDynamic.Resource(gvrPackageManifest).Namespace("default").Get(
+					ctx, "airflow-helm-operator", metav1.GetOptions{})
+
+				return err
+			}, olmWaitTimeout, 3).ShouldNot(Succeed())
+
+			By("Checking the validation condition")
+			check(
+				opPolName,
+				false,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:    "ValidPolicySpec",
+					Status:  metav1.ConditionTrue,
+					Reason:  "PolicyValidated",
+					Message: `the policy spec is valid`,
+				},
+				`the policy spec is valid`,
+			)
+		})
 	})
 
 	Describe("Testing an operator policy with invalid partial defaults", Ordered, func() {

test/e2e/e2e_suite_test.go

@@ -53,6 +53,7 @@ var (
 	gvrOperatorGroup            schema.GroupVersionResource
 	gvrInstallPlan              schema.GroupVersionResource
 	gvrClusterServiceVersion    schema.GroupVersionResource
+	gvrPackageManifest          schema.GroupVersionResource
 	defaultImageRegistry        string
 	IsHosted                    bool
 	targetK8sClient             kubernetes.Interface
@@ -122,6 +123,11 @@ var _ = BeforeSuite(func() {
 		Version:  "v1alpha1",
 		Resource: "clusterserviceversions",
 	}
+	gvrPackageManifest = schema.GroupVersionResource{
+		Group:    "packages.operators.coreos.com",
+		Version:  "v1",
+		Resource: "packagemanifests",
+	}
 	gvrAPIService = schema.GroupVersionResource{
 		Group:    "apiregistration.k8s.io",
 		Version:  "v1",