(fix)InstallPlan: Do not tranisition IP to failed on OG/SA failure

In #2077, a new phase `Failed` was introduced for InstallPlans, and failure in
detecting a valid OperatorGroup(OG) or a Service Account(SA) for the namespace
the InstallPlan was being created in would transition the InstallPlan to the
`Failed` state, i.e failure to detected these resources when the InstallPlan was
reconciled the first time was considered a permanant failure. This is a regression
from the previous behavior of InstallPlans where failure to detect OG/SA would
requeue the InstallPlan for reconciliation, so creating the required resources before
the retry limit of the informer queue was reached would transition the InstallPlan
from the `Installing` phase to the `Complete` phase(unless the bundle unpacking step
failed, in which case #2093 introduced transitioning the InstallPlan to the `Failed`
phase).

This regression introduced oddities for users who has infra built that applies a
set of manifests simultaneously to install an operator that includes a Subscription to
an operator (that creates InstallPlans) along with the required OG/SAs. In those cases,
whenever there was a delay in the reconciliation of the OG/SA, the InstallPlan would
be transitioned to a state of permanant faliure.

This PR:
* Removes the logic that transitioned the InstallPlan to `Failed`. Instead, the
InstallPlan will again be requeued for any reconciliation error.

* Introduces logic to bubble up reconciliation error through the InstallPlan's
status.Conditions, eg:

When no OperatorGroup is detected:

```
conditions:
    - lastTransitionTime: "2021-06-23T18:16:00Z"
      lastUpdateTime: "2021-06-23T18:16:16Z"
      message: attenuated service account query failed - no operator group found that
        is managing this namespace
      reason: InstallCheckFailed
      status: "False"
      type: Installed
```

Then when a valid OperatorGroup is created:

```
conditions:
    - lastTransitionTime: "2021-06-23T18:33:37Z"
      lastUpdateTime: "2021-06-23T18:33:37Z"
      status: "True"
      type: Installed
```

Author: anik120

doc/design/architecture.md

@@ -106,7 +106,7 @@ None --> Planning +------>------->------> Installing +---> Complete
 | RequiresApproval | occurs when using manual approval, will not transition phase until `approved` field is true    |
 | Installing       | resolved resources in the InstallPlan `Status` block are being created                      |
 | Complete         | all resolved resources in the `Status` block exist                                             |
-| Failed           | occurs when resources fail to install or there is an invalid OperatorGroup                     |
+| Failed           | occurs when bundle unpacking fails                     |
 
 ### Subscription Control Loop
 

pkg/controller/operators/catalog/operator.go

@@ -1389,25 +1389,23 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 	ref, err := querier()
 	if err != nil {
 
-		// Retry sync if non-fatal error
-		if !scoped.IsOperatorGroupError(err) {
-			syncError = fmt.Errorf("attenuated service account query failed - %v", err)
-			return
-		}
-
-		// Mark the InstallPlan as failed for a fatal Operator Group related error
-		logger.Infof("attenuated service account query failed - %v", err)
-		ipFailError := fmt.Errorf("invalid operator group - %v", err)
+		// Set status condition/message and retry sync if any error
+		ipFailError := fmt.Errorf("attenuated service account query failed - %v", err)
+		logger.Infof(ipFailError.Error())
 
-		if err := o.transitionInstallPlanToFailed(plan, logger, v1alpha1.InstallPlanReasonInstallCheckFailed, ipFailError.Error()); err != nil {
-			// retry for failure to update status
+		now := o.now()
+		out := plan.DeepCopy()
+		out.Status.SetCondition(v1alpha1.ConditionFailed(v1alpha1.InstallPlanInstalled,
+			v1alpha1.InstallPlanReasonInstallCheckFailed, ipFailError.Error(), &now))
+		_, err := o.client.OperatorsV1alpha1().InstallPlans(plan.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{})
+		if err != nil {
+			logger = logger.WithField("updateError", err.Error())
+			logger.Errorf("error updating InstallPlan status")
 			syncError = err
 			return
 		}
 
-		// Requeue subscription to propagate SubscriptionInstallPlanFailed condtion to subscription
-		o.requeueSubscriptionForInstallPlan(plan, logger)
-
+		syncError = ipFailError
 		return
 	}
 

pkg/controller/operators/catalog/operator_test.go

@@ -166,12 +166,12 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 	)
 
 	tests := []struct {
-		testName      string
-		err           error
-		in            *v1alpha1.InstallPlan
-		expectedPhase v1alpha1.InstallPlanPhase
-
-		clientObjs []runtime.Object
+		testName          string
+		err               error
+		in                *v1alpha1.InstallPlan
+		expectedPhase     v1alpha1.InstallPlanPhase
+		expectedCondition *v1alpha1.InstallPlanCondition
+		clientObjs        []runtime.Object
 	}{
 		{
 			testName:      "NoStatus",
@@ -182,16 +182,20 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 		{
 			// This checks that an installplan is marked as failed when no operatorgroup is present
 			testName:      "HasSteps/NoOperatorGroup",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
-			in:            ipWithSteps,
+			err:           fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "attenuated service account query failed - no operator group found that is managing this namespace"},
+			in: ipWithSteps,
 		},
 		{
 			// This checks that an installplan is marked as failed when multiple operator groups are present for the same namespace
 			testName:      "HasSteps/TooManyOperatorGroups",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
+			err:           fmt.Errorf("attenuated service account query failed - more than one operator group(s) are managing this namespace count=2"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
 			in:            ipWithSteps,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "attenuated service account query failed - more than one operator group(s) are managing this namespace count=2"},
 			clientObjs: []runtime.Object{
 				operatorGroup("og1", "sa", namespace,
 					&corev1.ObjectReference{
@@ -210,9 +214,11 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 		{
 			// This checks that an installplan is marked as failed when no service account is synced for the operator group, i.e the service account ref doesn't exist
 			testName:      "HasSteps/NonExistentServiceAccount",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
-			in:            ipWithSteps,
+			err:           fmt.Errorf("attenuated service account query failed - please make sure the service account exists. sa=sa1 operatorgroup=ns/og"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "attenuated service account query failed - please make sure the service account exists. sa=sa1 operatorgroup=ns/og"},
+			in: ipWithSteps,
 			clientObjs: []runtime.Object{
 				operatorGroup("og", "sa1", namespace, nil),
 			},
@@ -236,6 +242,11 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 			require.NoError(t, err)
 
 			require.Equal(t, tt.expectedPhase, ip.Status.Phase)
+
+			if tt.expectedCondition != nil {
+				require.True(t, hasExpectedCondition(ip, *tt.expectedCondition))
+			}
+
 		})
 	}
 }
@@ -1830,3 +1841,15 @@ func operatorGroup(ogName, saName, namespace string, saRef *corev1.ObjectReferen
 		},
 	}
 }
+
+func hasExpectedCondition(ip *v1alpha1.InstallPlan, expectedCondition v1alpha1.InstallPlanCondition) bool {
+
+	hasCondition := false
+	for _, cond := range ip.Status.Conditions {
+		if cond.Type == expectedCondition.Type && cond.Message == expectedCondition.Message && cond.Status == expectedCondition.Status {
+			hasCondition = true
+			break
+		}
+	}
+	return hasCondition
+}

test/e2e/installplan_e2e_test.go

@@ -3145,162 +3145,6 @@ var _ = Describe("Install Plan", func() {
 		require.Equal(GinkgoT(), 1, len(ips.Items), "If this test fails it should be taken seriously and not treated as a flake. \n%v", ips.Items)
 	})
 
-	It("should fail an InstallPlan when no OperatorGroup is present", func() {
-
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
-
-		c := newKubeClient()
-		crc := newCRClient()
-
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err := c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
-			Expect(err).NotTo(HaveOccurred())
-		}()
-
-		// Create InstallPlan
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
-
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).NotTo(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with phase %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
-	})
-
-	It("should fail an InstallPlan when multiple OperatorGroups are present", func() {
-
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
-
-		c := newKubeClient()
-		crc := newCRClient()
-
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err = c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
-			Expect(err).ToNot(HaveOccurred())
-		}()
-
-		// Create 2 operatorgroups in the same namespace
-		og1 := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og1",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces: []string{ns.GetName()},
-			},
-		}
-		og2 := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og2",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces: []string{ns.GetName()},
-			},
-		}
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og1, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og2, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
-
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).ToNot(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
-	})
-
-	It("should fail an InstallPlan when an OperatorGroup specifies an unsynced ServiceAccount with no ServiceAccountRef", func() {
-
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
-
-		c := newKubeClient()
-		crc := newCRClient()
-
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err = c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
-			Expect(err).ToNot(HaveOccurred())
-		}()
-
-		// OperatorGroup with serviceaccount specified
-		og := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces:   []string{ns.GetName()},
-				ServiceAccountName: "foobar",
-			},
-		}
-		og, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-		Expect(og).NotTo(BeNil())
-
-		// Update OperatorGroup status with namespace but no service account reference
-		now := metav1.Now()
-		og.Status = operatorsv1.OperatorGroupStatus{
-			Namespaces:        []string{ns.GetName()},
-			ServiceAccountRef: nil,
-			LastUpdated:       &now,
-		}
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
-
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).NotTo(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
-	})
-
 	When("waiting on the bundle unpacking job", func() {
 		var (
 			ns         *corev1.Namespace
@@ -3345,17 +3189,6 @@ var _ = Describe("Install Plan", func() {
 				return ctx.Ctx().Client().Create(context.Background(), og)
 			}, timeout, interval).Should(Succeed(), "could not create OperatorGroup")
 
-			// Wait for the OperatorGroup to be synced so the InstallPlan doesn't fail due to an invalid OperatorGroup
-			Eventually(
-				func() ([]string, error) {
-					err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(og), og)
-					ctx.Ctx().Logf("Waiting for OperatorGroup(%v) to be synced with status.namespaces: %v", og.Name, og.Status.Namespaces)
-					return og.Status.Namespaces, err
-				},
-				1*time.Minute,
-				interval,
-			).Should(ContainElement(ns.GetName()))
-
 			now := metav1.Now()
 			ip = &operatorsv1alpha1.InstallPlan{
 				ObjectMeta: metav1.ObjectMeta{
@@ -3639,18 +3472,6 @@ var _ = Describe("Install Plan", func() {
 		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
 		Expect(err).ToNot(HaveOccurred())
 
-		// Wait for the OperatorGroup to be synced and have a status.ServiceAccountRef
-		// before moving on. Otherwise the catalog operator treats it as an invalid OperatorGroup
-		// and fails the InstallPlan
-		Eventually(func() (*corev1.ObjectReference, error) {
-			outOG, err := crc.OperatorsV1().OperatorGroups(ns.GetName()).Get(context.TODO(), og.Name, metav1.GetOptions{})
-			if err != nil {
-				return nil, err
-			}
-			ctx.Ctx().Logf("[DEBUG] Operator Group Status: %+v\n", outOG.Status)
-			return outOG.Status.ServiceAccountRef, nil
-		}).ShouldNot(BeNil())
-
 		crd := apiextensionsv1.CustomResourceDefinition{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "ins" + ".cluster.com",
@@ -3895,18 +3716,6 @@ var _ = Describe("Install Plan", func() {
 		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
 		Expect(err).ToNot(HaveOccurred())
 
-		// Wait for the OperatorGroup to be synced and have a status.ServiceAccountRef
-		// before moving on. Otherwise the catalog operator treats it as an invalid OperatorGroup
-		// and fails the InstallPlan
-		Eventually(func() (*corev1.ObjectReference, error) {
-			outOG, err := crc.OperatorsV1().OperatorGroups(ns.GetName()).Get(context.TODO(), og.Name, metav1.GetOptions{})
-			if err != nil {
-				return nil, err
-			}
-			ctx.Ctx().Logf("[DEBUG] Operator Group Status: %+v\n", outOG.Status)
-			return outOG.Status.ServiceAccountRef, nil
-		}).ShouldNot(BeNil())
-
 		By("using the OLM client to install CRDs from the installplan and the scoped client for other resources")
 
 		crd := apiextensionsv1.CustomResourceDefinition{
@@ -4072,7 +3881,7 @@ func validateCRDVersions(t GinkgoTInterface, c operatorclient.ClientInterface, n
 
 func buildInstallPlanPhaseCheckFunc(phases ...operatorsv1alpha1.InstallPlanPhase) checkInstallPlanFunc {
 	return func(fip *operatorsv1alpha1.InstallPlan) bool {
-		ctx.Ctx().Logf("installplan is %s", fip.Status.Phase)
+		ctx.Ctx().Logf("installplan %v is in phase %v", fip.GetName(), fip.Status.Phase)
 		satisfiesAny := false
 		for _, phase := range phases {
 			satisfiesAny = satisfiesAny || fip.Status.Phase == phase
@@ -4298,34 +4107,3 @@ func newCSV(name, namespace, replaces string, version semver.Version, owned []ap
 
 	return csv
 }
-
-func newInstallPlanWithDummySteps(name, namespace string, phase operatorsv1alpha1.InstallPlanPhase) *operatorsv1alpha1.InstallPlan {
-	return &operatorsv1alpha1.InstallPlan{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: namespace,
-		},
-		Spec: operatorsv1alpha1.InstallPlanSpec{
-			ClusterServiceVersionNames: []string{"foobar"},
-			Approval:                   operatorsv1alpha1.ApprovalAutomatic,
-			Approved:                   true,
-		},
-		Status: operatorsv1alpha1.InstallPlanStatus{
-			CatalogSources: []string{"catalog"},
-			Phase:          phase,
-			Plan: []*operatorsv1alpha1.Step{
-				{
-					Resource: operatorsv1alpha1.StepResource{
-						CatalogSource:          "catalog",
-						CatalogSourceNamespace: namespace,
-						Group:                  "",
-						Version:                "v1",
-						Kind:                   "Foo",
-						Name:                   "bar",
-					},
-					Status: operatorsv1alpha1.StepStatusUnknown,
-				},
-			},
-		},
-	}
-}