catalog: don't treat permissions errors as terminal

Signed-off-by: Steve Kuznetsov <[email protected]>

Author: stevekuznetsov

pkg/controller/operators/catalog/operator.go

@@ -2036,13 +2036,15 @@ func transitionInstallPlanState(log logrus.FieldLogger, transitioner installPlan
 		}
 		log.Debug("attempting to install")
 		if err := transitioner.ExecutePlan(out); err != nil {
-			if now.Sub(out.Status.StartTime.Time) >= timeout {
+			if apierrors.IsForbidden(err) || now.Sub(out.Status.StartTime.Time) < timeout {
+				// forbidden problems are never terminal since we don't know when a user might provide
+				// the service account they specified with more permissions
+				out.Status.Message = fmt.Sprintf("retrying execution due to error: %s", err.Error())
+			} else {
 				out.Status.SetCondition(v1alpha1.ConditionFailed(v1alpha1.InstallPlanInstalled,
 					v1alpha1.InstallPlanReasonComponentFailed, err.Error(), &now))
 				out.Status.Phase = v1alpha1.InstallPlanPhaseFailed
 				out.Status.Message = err.Error()
-			} else {
-				out.Status.Message = fmt.Sprintf("retrying execution due to error: %s", err.Error())
 			}
 			return out, err
 		} else if !out.Status.NeedsRequeue() {

test/e2e/installplan_e2e_test.go

@@ -3638,11 +3638,9 @@ var _ = Describe("Install Plan", func() {
 		}
 		Expect(ctx.Ctx().Client().Status().Update(context.Background(), newPlan)).To(Succeed())
 
-		newKey := client.ObjectKeyFromObject(newPlan)
-
-		Eventually(func() (*operatorsv1alpha1.InstallPlan, error) {
-			return newPlan, ctx.Ctx().Client().Get(context.Background(), newKey, newPlan)
-		}).Should(HavePhase(operatorsv1alpha1.InstallPlanPhaseFailed))
+		ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource "services" in API group`)
+		_, err = fetchInstallPlanWithNamespace(GinkgoT(), crc, newPlan.Name, newPlan.Namespace, ipPhaseCheckerFunc)
+		require.NoError(GinkgoT(), err)
 
 		Expect(client.IgnoreNotFound(ctx.Ctx().Client().Delete(context.Background(), &crd))).To(Succeed())
 		Eventually(func() error {
@@ -3928,6 +3926,19 @@ func validateCRDVersions(t GinkgoTInterface, c operatorclient.ClientInterface, n
 	require.Equal(t, 0, len(expectedVersions), "Actual CRD versions do not match expected")
 }
 
+func buildInstallPlanMessageCheckFunc(substring string) checkInstallPlanFunc {
+	var lastMessage string
+	lastTime := time.Now()
+	return func(fip *operatorsv1alpha1.InstallPlan) bool {
+		if fip.Status.Message != lastMessage {
+			ctx.Ctx().Logf("waiting %s for installplan %s/%s to have message substring %s, have message %s", time.Since(lastTime), fip.Namespace, fip.Name, substring, fip.Status.Phase)
+			lastMessage = fip.Status.Message
+			lastTime = time.Now()
+		}
+		return strings.Contains(fip.Status.Message, substring)
+	}
+}
+
 func buildInstallPlanPhaseCheckFunc(phases ...operatorsv1alpha1.InstallPlanPhase) checkInstallPlanFunc {
 	var lastPhase operatorsv1alpha1.InstallPlanPhase
 	lastTime := time.Now()

test/e2e/user_defined_sa_test.go

@@ -83,7 +83,7 @@ var _ = Describe("User defined service account", func() {
 
 		By("We expect the InstallPlan to be in status: Failed.")
 		ipName := subscription.Status.Install.Name
-		ipPhaseCheckerFunc := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseFailed)
+		ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource`)
 		ipGot, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ipName, generatedNamespace.GetName(), ipPhaseCheckerFunc)
 		require.NoError(GinkgoT(), err)
 
@@ -186,12 +186,11 @@ var _ = Describe("User defined service account", func() {
 		require.NoError(GinkgoT(), err)
 		require.NotNil(GinkgoT(), subscription)
 
-		By("We expect the InstallPlan to be in status: Failed.")
+		By("We expect the InstallPlan to expose the permissions error.")
 		ipNameOld := subscription.Status.InstallPlanRef.Name
-		ipPhaseCheckerFunc := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseFailed)
-		ipGotOld, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ipNameOld, generatedNamespace.GetName(), ipPhaseCheckerFunc)
+		ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource "clusterserviceversions" in API group "operators.coreos.com" in the namespace`)
+		_, err = fetchInstallPlanWithNamespace(GinkgoT(), crc, ipNameOld, generatedNamespace.GetName(), ipPhaseCheckerFunc)
 		require.NoError(GinkgoT(), err)
-		require.Equal(GinkgoT(), v1alpha1.InstallPlanPhaseFailed, ipGotOld.Status.Phase)
 
 		By("Grant permission now and this should trigger an retry of InstallPlan.")
 		cleanupPerm := grantPermission(GinkgoT(), c, generatedNamespace.GetName(), saName)