fix(csv): properly detect owner conflicts for crds and apiservices

- Use ownerlabels for APIService conflict detection
- Only detect CRD owner conflict when the potential conflict is not the
syncing CSV or the CSV it replaces

Author: njhale

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

@@ -113,6 +113,11 @@ type APIResourceReference struct {
 	Version string `json:"version"`
 }
 
+// GetName returns the name of an APIService as derived from its group and version.
+func (d APIServiceDescription) GetName() string {
+	return fmt.Sprintf("%s.%s", d.Version, d.Group)
+}
+
 // CustomResourceDefinitions declares all of the CRDs managed or required by
 // an operator being ran by ClusterServiceVersion.
 //

pkg/controller/operators/olm/apiservices.go

@@ -59,24 +59,28 @@ func (a *Operator) apiServiceResourceErrorActionable(err error) bool {
 
 // checkAPIServiceResources checks if all expected generated resources for the given APIService exist
 func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion, hashFunc certs.PEMHash) error {
+	logger := log.WithFields(log.Fields{
+		"csv":       csv.GetName(),
+		"namespace": csv.GetNamespace(),
+	})
+
 	errs := []error{}
 	owners := []ownerutil.Owner{csv}
+
 	// Get replacing CSV if exists
 	replacing, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(csv.GetNamespace()).Get(csv.Spec.Replaces)
-	if err != nil && k8serrors.IsNotFound(err) == false {
-		a.Log.Debugf("Replacement error regarding CSV (%v): %v", csv.GetName(), err)
-		errs = append(errs, err)
-		return utilerrors.NewAggregate(errs)
+	if err != nil && !k8serrors.IsNotFound(err) {
+		logger.WithError(err).Warn("could not get replacement csv")
+		return err
 	}
 	if replacing != nil {
 		owners = append(owners, replacing)
 	}
+
 	ruleChecker := install.NewCSVRuleChecker(a.lister.RbacV1().RoleLister(), a.lister.RbacV1().RoleBindingLister(), a.lister.RbacV1().ClusterRoleLister(), a.lister.RbacV1().ClusterRoleBindingLister(), csv)
 	for _, desc := range csv.GetOwnedAPIServiceDescriptions() {
-		apiServiceName := fmt.Sprintf("%s.%s", desc.Version, desc.Group)
-		logger := log.WithFields(log.Fields{
-			"csv":        csv.GetName(),
-			"namespace":  csv.GetNamespace(),
+		apiServiceName := desc.GetName()
+		logger := logger.WithFields(log.Fields{
 			"apiservice": apiServiceName,
 		})
 
@@ -88,7 +92,7 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 		}
 
 		// Check if the APIService is adoptable
-		if !ownerutil.OwnersIntersect(owners, apiService) {
+		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
 			err := olmerrors.NewUnadoptableError("", apiServiceName)
 			logger.WithError(err).Warn("found unadoptable apiservice")
 			errs = append(errs, err)
@@ -253,8 +257,7 @@ func (a *Operator) isAPIServiceAvailable(apiService *apiregistrationv1.APIServic
 
 func (a *Operator) areAPIServicesAvailable(csv *v1alpha1.ClusterServiceVersion) (bool, error) {
 	for _, desc := range csv.Spec.APIServiceDefinitions.Owned {
-		apiServiceName := fmt.Sprintf("%s.%s", desc.Version, desc.Group)
-		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
+		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(desc.GetName())
 		if k8serrors.IsNotFound(err) {
 			return false, nil
 		}
@@ -550,7 +553,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 	existingAuthDelegatorClusterRoleBinding, err := a.lister.RbacV1().ClusterRoleBindingLister().Get(authDelegatorClusterRoleBinding.GetName())
 	if err == nil {
 		// Check if the only owners are this CSV or in this CSV's replacement chain.
-		if ownerutil.AdoptableLabels(csv, existingAuthDelegatorClusterRoleBinding.GetLabels()) {
+		if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, csv) {
 			ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv)
 		}
 
@@ -593,7 +596,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 	existingAuthReaderRoleBinding, err := a.lister.RbacV1().RoleBindingLister().RoleBindings("kube-system").Get(authReaderRoleBinding.GetName())
 	if err == nil {
 		// Check if the only owners are this CSV or in this CSV's replacement chain.
-		if ownerutil.AdoptableLabels(csv, existingAuthReaderRoleBinding.GetLabels()) {
+		if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, csv) {
 			ownerutil.AddOwnerLabels(authReaderRoleBinding, csv)
 		}
 		// Attempt an update.
@@ -694,13 +697,15 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 		apiService.SetName(apiServiceName)
 	} else {
 		owners := []ownerutil.Owner{csv}
+
 		// Get replacing CSV
 		replaces, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(csv.GetNamespace()).Get(csv.Spec.Replaces)
 		if err == nil {
 			owners = append(owners, replaces)
 		}
+
 		// check if the APIService is adoptable
-		if !ownerutil.OwnersIntersect(owners, apiService) {
+		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
 			return nil, fmt.Errorf("pre-existing APIService %s is not adoptable", apiServiceName)
 		}
 	}

pkg/controller/operators/olm/operator.go

@@ -38,8 +38,8 @@ import (
 
 var (
 	ErrRequirementsNotMet      = errors.New("requirements were not met")
-	ErrCRDOwnerConflict        = errors.New("CRD owned by another ClusterServiceVersion")
-	ErrAPIServiceOwnerConflict = errors.New("APIService owned by another ClusterServiceVersion")
+	ErrCRDOwnerConflict        = errors.New("conflicting CRD owner in namespace")
+	ErrAPIServiceOwnerConflict = errors.New("unable to adopt APIService")
 )
 
 var timeNow = func() metav1.Time { return metav1.NewTime(time.Now().UTC()) }
@@ -849,16 +849,18 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check for CRD ownership conflicts
-		// TODO: find CSVs that provide any of those that out does across all namespaces
-		csvSet := a.csvSet(out.GetNamespace(), v1alpha1.CSVPhaseAny)
-		if syncError = a.crdOwnerConflicts(out, csvSet); syncError != nil {
-			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, fmt.Sprintf("crd owner conflict: %s", syncError), now, a.recorder)
+		if syncError = a.crdOwnerConflicts(out, a.csvSet(out.GetNamespace(), v1alpha1.CSVPhaseAny)); syncError != nil {
+			if syncError == ErrCRDOwnerConflict {
+				out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, syncError.Error(), now, a.recorder)
+			}
 			return
 		}
 
-		// check for APIServices ownership conflicts
-		if syncError = a.apiServiceOwnerConflicts(out, csvSet); syncError != nil {
-			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, fmt.Sprintf("apiService owner conflict: %s", syncError), now, a.recorder)
+		// Check for APIServices ownership conflicts
+		if syncError = a.apiServiceOwnerConflicts(out); syncError != nil {
+			if syncError == ErrAPIServiceOwnerConflict {
+				out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, syncError.Error(), now, a.recorder)
+			}
 			return
 		}
 
@@ -897,6 +899,11 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 
 		if installErr := a.updateInstallStatus(out, installer, strategy, v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonWaiting); installErr == nil {
 			logger.WithField("strategy", out.Spec.InstallStrategy.StrategyName).Infof("install strategy successful")
+		} else {
+			// Set phase to failed if it's been a long time since the last transition (5 minutes)
+			if metav1.Now().Sub(out.Status.LastTransitionTime.Time) >= 5*time.Minute {
+				out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonInstallCheckFailed, fmt.Sprintf("install timeout"), now, a.recorder)
+			}
 		}
 
 	case v1alpha1.CSVPhaseSucceeded:
@@ -981,9 +988,11 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check if any generated resources are missing and that OLM can action on them
-		if err := a.checkAPIServiceResources(out, certs.PEMSHA256); err != nil && a.apiServiceResourceErrorActionable(err) {
-			// Check if API services are adoptable. If not, keep CSV as Failed state
-			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonAPIServiceResourcesNeedReinstall, err.Error(), now, a.recorder)
+		if err := a.checkAPIServiceResources(out, certs.PEMSHA256); err != nil {
+			if a.apiServiceResourceErrorActionable(err) {
+				// Check if API services are adoptable. If not, keep CSV as Failed state
+				out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonAPIServiceResourcesNeedReinstall, err.Error(), now, a.recorder)
+			}
 			return
 		}
 
@@ -1009,20 +1018,20 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}
 
-		// if we can find a newer version that's successfully installed, we're safe to mark all intermediates
+		// If we can find a newer version that's successfully installed, we're safe to mark all intermediates
 		for _, csv := range a.findIntermediatesForDeletion(out) {
 			// we only mark them in this step, in case some get deleted but others fail and break the replacement chain
 			csv.SetPhaseWithEvent(v1alpha1.CSVPhaseDeleting, v1alpha1.CSVReasonReplaced, "has been replaced by a newer ClusterServiceVersion that has successfully installed.", now, a.recorder)
 
-			// ignore errors and success here; this step is just an optimization to speed up GC
+			// Ignore errors and success here; this step is just an optimization to speed up GC
 			_, _ = a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).UpdateStatus(csv)
 			err := a.csvQueueSet.Requeue(csv.GetName(), csv.GetNamespace())
 			if err != nil {
 				a.Log.Warn(err.Error())
 			}
 		}
 
-		// if there's no newer version, requeue for processing (likely will be GCable before resync)
+		// If there's no newer version, requeue for processing (likely will be GCable before resync)
 		err := a.csvQueueSet.Requeue(out.GetName(), out.GetNamespace())
 		if err != nil {
 			a.Log.Warn(err.Error())
@@ -1136,6 +1145,10 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst
 
 	if strategyErr != nil {
 		csv.SetPhaseWithEventIfChanged(requeuePhase, requeueConditionReason, fmt.Sprintf("installing: %s", strategyErr), now, a.recorder)
+		if err := a.csvQueueSet.Requeue(csv.GetName(), csv.GetNamespace()); err != nil {
+			a.Log.Warn(err.Error())
+		}
+
 		return strategyErr
 	}
 
@@ -1169,16 +1182,10 @@ func (a *Operator) parseStrategiesAndUpdateStatus(csv *v1alpha1.ClusterServiceVe
 	return installer, strategy, previousStrategy
 }
 
-func (a *Operator) crdOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion) error {
+func (a *Operator) crdOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvs map[string]*v1alpha1.ClusterServiceVersion) error {
 	for _, crd := range in.Spec.CustomResourceDefinitions.Owned {
-		for csvName, csv := range csvsInNamespace {
-			if csvName == in.GetName() {
-				continue
-			}
-			if csv.OwnsCRD(crd.Name) {
-				if in.Spec.Replaces == csvName {
-					return nil
-				}
+		for name, csv := range csvs {
+			if name != in.GetName() && in.Spec.Replaces != name && csv.OwnsCRD(crd.Name) {
 				return ErrCRDOwnerConflict
 			}
 		}
@@ -1187,25 +1194,34 @@ func (a *Operator) crdOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvsInN
 	return nil
 }
 
-func (a *Operator) apiServiceOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion) error {
-	owned := false
-	for _, api := range in.Spec.APIServiceDefinitions.Owned {
-		name := fmt.Sprintf("%s.%s", api.Version, api.Group)
-		for csvName, csv := range csvsInNamespace {
-			if csvName == in.GetName() {
-				continue
-			}
-			if csv.OwnsAPIService(name) {
-				owned = true
-			}
-			if owned && in.Spec.Replaces == csvName {
-				return nil
-			}
-		}
+func (a *Operator) apiServiceOwnerConflicts(csv *v1alpha1.ClusterServiceVersion) error {
+	// Get replacing CSV if exists
+	replacing, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(csv.GetNamespace()).Get(csv.Spec.Replaces)
+	if err != nil && !k8serrors.IsNotFound(err) && !k8serrors.IsGone(err) {
+		return err
+	}
+
+	owners := []ownerutil.Owner{csv}
+	if replacing != nil {
+		owners = append(owners, replacing)
 	}
-	if owned {
-		return ErrAPIServiceOwnerConflict
+
+	for _, desc := range csv.GetOwnedAPIServiceDescriptions() {
+		// Check if the APIService exists
+		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(desc.GetName())
+		if err != nil && !k8serrors.IsNotFound(err) && !k8serrors.IsGone(err) {
+			return err
+		}
+
+		if apiService == nil {
+			continue
+		}
+
+		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
+			return ErrAPIServiceOwnerConflict
+		}
 	}
+
 	return nil
 }
 

pkg/controller/registry/resolver/operators.go

@@ -2,12 +2,12 @@ package resolver
 
 import (
 	"fmt"
-	"strings"
 	"sort"
+	"strings"
 
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
 

pkg/lib/notify/notify.go

@@ -0,0 +1 @@
+package notify

pkg/lib/ownerutil/util.go

@@ -13,7 +13,6 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 )
 
 const (
@@ -143,46 +142,6 @@ func Adoptable(target Owner, owners []metav1.OwnerReference) bool {
 	return false
 }
 
-// OwnersIntersect checks to see if any member of targets exists in owners
-func OwnersIntersect(targets []Owner, apiservice *apiregistrationv1.APIService) bool {
-	ownerLabels := apiservice.GetLabels()
-	if len(ownerLabels) == 0 {
-		// Resources with no owners are not adoptable
-		log.Debugf("Did not contain labels for apiservice %v", apiservice.GetName())
-		return false
-	}
-
-	svcOwnerKind, ok := ownerLabels[OwnerKind]
-	if !ok {
-		log.Warnf("missing owner kind for apiservice %v", apiservice.GetName())
-		return false
-	}
-	svcOwnerNamespace, ok := ownerLabels[OwnerNamespaceKey]
-	if !ok {
-		log.Warnf("missing owner namespace for apiservice %v", apiservice.GetName())
-		return false
-	}
-	svcOwnerName, ok := ownerLabels[OwnerKey]
-	if !ok {
-		log.Warnf("missing owner name for apiservice %v", apiservice.GetName())
-		return false
-	}
-
-	for _, target := range targets {
-		if err := InferGroupVersionKind(target); err != nil {
-			log.Warnf("GVK infer failed for %v: %v", target, err)
-			return false
-		}
-		if svcOwnerKind == target.GetObjectKind().GroupVersionKind().Kind &&
-			svcOwnerNamespace == target.GetNamespace() &&
-			svcOwnerName == target.GetName() {
-			return true
-		}
-	}
-
-	return false
-}
-
 // AddNonBlockingOwner adds a nonblocking owner to the ownerref list.
 func AddNonBlockingOwner(object metav1.Object, owner Owner) {
 	ownerRefs := object.GetOwnerReferences()
@@ -257,17 +216,24 @@ func IsOwnedByKindLabel(object metav1.Object, ownerKind string) bool {
 	return object.GetLabels()[OwnerKind] == ownerKind
 }
 
-// AdoptableLabels determines if an OLM managed resource is adoptable based on its owner labels
-// Generally used for cross-namespace ownership and for Cluster -> Namespace scope
-func AdoptableLabels(target Owner, labels map[string]string) bool {
+// AdoptableLabels determines if an OLM managed resource is adoptable by any of the given targets based on its owner labels.
+// The checkName perimeter enables an additional check for name equality with the `olm.owner` label.
+// Generally used for cross-namespace ownership and for Cluster -> Namespace scope.
+func AdoptableLabels(labels map[string]string, checkName bool, targets ...Owner) bool {
 	if len(labels) == 0 {
 		// Resources with no owners are not adoptable
 		return false
 	}
 
-	if labels[OwnerKind] == target.GetObjectKind().GroupVersionKind().Kind &&
-		labels[OwnerNamespaceKey] == target.GetNamespace() {
-		return true
+	for _, target := range targets {
+		if err := InferGroupVersionKind(target); err != nil {
+			log.Warn(err.Error())
+		}
+		if labels[OwnerKind] == target.GetObjectKind().GroupVersionKind().Kind &&
+			labels[OwnerNamespaceKey] == target.GetNamespace() &&
+			(!checkName || labels[OwnerKey] == target.GetName()) {
+			return true
+		}
 	}
 
 	return false