Merge pull request #1039 from jpeeler/bug1749036

Bug 1749036: fix install behavior both during and post-install permission changes

Author: openshift-merge-robot

pkg/api/apis/operators/v1alpha1/clusterserviceversion_types.go

@@ -231,6 +231,7 @@ const (
 	CSVReasonRequirementsMet                             ConditionReason = "AllRequirementsMet"
 	CSVReasonOwnerConflict                               ConditionReason = "OwnerConflict"
 	CSVReasonComponentFailed                             ConditionReason = "InstallComponentFailed"
+	CSVReasonComponentFailedNoRetry                      ConditionReason = "InstallComponentFailedNoRetry"
 	CSVReasonInvalidStrategy                             ConditionReason = "InvalidInstallStrategy"
 	CSVReasonWaiting                                     ConditionReason = "InstallWaiting"
 	CSVReasonInstallSuccessful                           ConditionReason = "InstallSucceeded"
@@ -251,6 +252,7 @@ const (
 	CSVReasonTooManyOperatorGroups                       ConditionReason = "TooManyOperatorGroups"
 	CSVReasonInterOperatorGroupOwnerConflict             ConditionReason = "InterOperatorGroupOwnerConflict"
 	CSVReasonCannotModifyStaticOperatorGroupProvidedAPIs ConditionReason = "CannotModifyStaticOperatorGroupProvidedAPIs"
+	CSVReasonDetectedClusterChange                       ConditionReason = "DetectedClusterChange"
 )
 
 // Conditions appear in the status as a record of state transitions on the ClusterServiceVersion

pkg/controller/install/deployment.go

@@ -7,6 +7,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	rbac "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/diff"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
@@ -160,6 +161,9 @@ func (i *StrategyDeploymentInstaller) Install(s Strategy) error {
 	}
 
 	if err := i.installDeployments(strategy.DeploymentSpecs); err != nil {
+		if k8serrors.IsForbidden(err) {
+			return StrategyError{Reason: StrategyErrInsufficientPermissions, Message: fmt.Sprintf("install strategy failed: %s", err)}
+		}
 		return err
 	}
 

pkg/controller/install/errors.go

@@ -11,13 +11,15 @@ const (
 	StrategyErrReasonUnknown            = "Unknown"
 	StrategyErrBadPatch                 = "PatchUnsuccessful"
 	StrategyErrDeploymentUpdated        = "DeploymentUpdated"
+	StrategyErrInsufficientPermissions  = "InsufficentPermissions"
 )
 
 // unrecoverableErrors are the set of errors that mean we can't recover an install strategy
 var unrecoverableErrors = map[string]struct{}{
-	StrategyErrReasonInvalidStrategy: {},
-	StrategyErrReasonTimeout:         {},
-	StrategyErrBadPatch:              {},
+	StrategyErrReasonInvalidStrategy:   {},
+	StrategyErrReasonTimeout:           {},
+	StrategyErrBadPatch:                {},
+	StrategyErrInsufficientPermissions: {},
 }
 
 // StrategyError is used to represent error types for install strategies

pkg/controller/operators/catalog/installplan_sync.go

@@ -7,11 +7,10 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/runtime"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/scoped"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 // When a user adds permission to a ServiceAccount by creating or updating
@@ -26,7 +25,7 @@ func (o *Operator) triggerInstallPlanRetry(obj interface{}) (syncError error) {
 		return
 	}
 
-	related, _ := isObjectRBACRelated(obj)
+	related, _ := scoped.IsObjectRBACRelated(obj)
 	if !related {
 		return
 	}
@@ -75,26 +74,3 @@ func (o *Operator) triggerInstallPlanRetry(obj interface{}) (syncError error) {
 	syncError = utilerrors.NewAggregate(errs)
 	return
 }
-
-func isObjectRBACRelated(obj interface{}) (related bool, object runtime.Object) {
-	object, ok := obj.(runtime.Object)
-	if !ok {
-		return
-	}
-
-	if err := ownerutil.InferGroupVersionKind(object); err != nil {
-		return
-	}
-
-	kind := object.GetObjectKind().GroupVersionKind().Kind
-	switch kind {
-	case roleKind:
-		fallthrough
-	case roleBindingKind:
-		fallthrough
-	case serviceAccountKind:
-		related = true
-	}
-
-	return
-}

pkg/controller/operators/olm/operator.go

@@ -679,6 +679,29 @@ func (a *Operator) syncObject(obj interface{}) (syncError error) {
 		a.requeueCSVsByLabelSet(logger, labelSets...)
 	}
 
+	// Requeue CSVs that have the reason of `CSVReasonComponentFailedNoRetry` in the case of an RBAC change
+	var errs []error
+	related, _ := scoped.IsObjectRBACRelated(metaObj)
+	if related {
+		csvList := a.csvSet(metaObj.GetNamespace(), v1alpha1.CSVPhaseFailed)
+		for _, csv := range csvList {
+			if csv.Status.Reason != v1alpha1.CSVReasonComponentFailedNoRetry {
+				continue
+			}
+			csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonDetectedClusterChange, "Cluster resources changed state", a.now())
+			_, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).UpdateStatus(csv)
+			if err != nil {
+				errs = append(errs, err)
+				continue
+			}
+			if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {
+				errs = append(errs, err)
+			}
+			logger.Debug("Requeuing CSV due to detected RBAC change")
+		}
+	}
+
+	syncError = utilerrors.NewAggregate(errs)
 	return nil
 }
 
@@ -1096,6 +1119,12 @@ func (a *Operator) operatorGroupForCSV(csv *v1alpha1.ClusterServiceVersion, logg
 			if targetNamespaceList, err := a.getOperatorGroupTargets(operatorGroup); err == nil && len(targetNamespaceList) == 0 {
 				csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonNoTargetNamespaces, "no targetNamespaces are matched operatorgroups namespace selection", now, a.recorder)
 			}
+			logger.Debug("CSV not in operatorgroup, requeuing operator group")
+			// this requeue helps when an operator group has not annotated a CSV due to a permissions error
+			// but the permissions issue has now been resolved
+			if err := a.ogQueueSet.Requeue(operatorGroup.GetNamespace(), operatorGroup.GetName()); err != nil {
+				return nil, err
+			}
 			return nil, nil
 		}
 		logger.Info("csv in operatorgroup")
@@ -1119,6 +1148,12 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		"phase":     in.Status.Phase,
 	})
 
+	if in.Status.Reason == v1alpha1.CSVReasonComponentFailedNoRetry {
+		// will change phase out of failed in the event of an intentional requeue
+		logger.Debugf("skipping sync for CSV in failed-no-retry state")
+		return
+	}
+
 	out = in.DeepCopy()
 	now := a.now()
 
@@ -1308,6 +1343,11 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		if syncError = installer.Install(strategy); syncError != nil {
+			if install.IsErrorUnrecoverable(syncError) {
+				logger.Infof("Setting CSV reason to failed without retry: %v", syncError)
+				out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailedNoRetry, fmt.Sprintf("install strategy failed: %s", syncError), now, a.recorder)
+				return
+			}
 			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailed, fmt.Sprintf("install strategy failed: %s", syncError), now, a.recorder)
 			return
 		}

pkg/controller/operators/olm/operatorgroup.go

@@ -53,11 +53,34 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		"namespace":     op.GetNamespace(),
 	})
 
+	previousRef := op.Status.ServiceAccountRef.DeepCopy()
 	op, err := a.serviceAccountSyncer.SyncOperatorGroup(op)
 	if err != nil {
 		logger.Errorf("error updating service account - %v", err)
 		return err
 	}
+	if op.Status.ServiceAccountRef != previousRef {
+		crdList, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().List(labels.Everything())
+		if err != nil {
+			return err
+		}
+		for _, csv := range crdList {
+			if group, ok := csv.GetAnnotations()[v1.OperatorGroupAnnotationKey]; !ok || group != op.GetName() {
+				continue
+			}
+			if csv.Status.Reason == v1alpha1.CSVReasonComponentFailedNoRetry {
+				csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonDetectedClusterChange, "Cluster resources changed state", a.now())
+				_, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).UpdateStatus(csv)
+				if err != nil {
+					return err
+				}
+				if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {
+					return err
+				}
+				logger.Debug("Requeuing CSV due to detected service account change")
+			}
+		}
+	}
 
 	targetNamespaces, err := a.updateNamespaceList(op)
 	if err != nil {

pkg/lib/scoped/util.go

@@ -2,6 +2,15 @@ package scoped
 
 import (
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
+)
+
+const (
+	roleKind        = "Role"
+	roleBindingKind = "RoleBinding"
 )
 
 // IsServiceAccountToken returns true if the secret is a valid api token for the service account
@@ -24,3 +33,26 @@ func IsServiceAccountToken(secret *v1.Secret, sa *v1.ServiceAccount) bool {
 
 	return true
 }
+
+func IsObjectRBACRelated(obj interface{}) (related bool, object runtime.Object) {
+	object, ok := obj.(runtime.Object)
+	if !ok {
+		return
+	}
+
+	if err := ownerutil.InferGroupVersionKind(object); err != nil {
+		return
+	}
+
+	kind := object.GetObjectKind().GroupVersionKind().Kind
+	switch kind {
+	case roleKind:
+		fallthrough
+	case roleBindingKind:
+		fallthrough
+	case rbacv1.ServiceAccountKind:
+		related = true
+	}
+
+	return
+}