Merge pull request #925 from tkashem/bz-1723818

Bug 1723818: CSV name change should not cause upgrade to fail

Author: openshift-merge-robot

pkg/controller/operators/olm/apiservices.go

@@ -1,6 +1,7 @@
 package olm
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -65,18 +66,6 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 	})
 
 	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) {
-		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 := desc.GetName()
@@ -92,8 +81,15 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 		}
 
 		// Check if the APIService is adoptable
-		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
-			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
+		adoptable, err := a.isAPIServiceAdoptable(csv, apiService)
+		if err != nil {
+			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Errorf("adoption check failed - %v", err)
+			errs = append(errs, err)
+			return utilerrors.NewAggregate(errs)
+		}
+
+		if !adoptable {
+			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Errorf("adoption failed")
 			err := olmerrors.NewUnadoptableError("", apiServiceName)
 			logger.WithError(err).Warn("found unadoptable apiservice")
 			errs = append(errs, err)
@@ -707,17 +703,12 @@ 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)
+		adoptable, err := a.isAPIServiceAdoptable(csv, apiService)
+		if err != nil {
+			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Errorf("adoption check failed - %v", err)
 		}
 
-		// check if the APIService is adoptable
-		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
-			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
+		if !adoptable{
 			return nil, fmt.Errorf("pre-existing APIService %s is not adoptable", apiServiceName)
 		}
 	}
@@ -759,3 +750,54 @@ func APIServiceNameToServiceName(apiServiceName string) string {
 	// Replace all '.'s with "-"s to convert to a DNS-1035 label
 	return strings.Replace(apiServiceName, ".", "-", -1)
 }
+
+func (a *Operator) isAPIServiceAdoptable(target *v1alpha1.ClusterServiceVersion, apiService *apiregistrationv1.APIService) (adoptable bool, err error) {
+	if apiService == nil || target == nil {
+		err = errors.New("invalid input")
+		return
+	}
+
+	labels := apiService.GetLabels()
+	ownerKind := labels[ownerutil.OwnerKind]
+	ownerName := labels[ownerutil.OwnerKey]
+	ownerNamespace := labels[ownerutil.OwnerNamespaceKey]
+
+	if ownerKind == "" || ownerNamespace == "" || ownerName == "" {
+		return
+	}
+
+	if err := ownerutil.InferGroupVersionKind(target); err != nil {
+		a.logger.Warn(err.Error())
+	}	
+
+	targetKind := target.GetObjectKind().GroupVersionKind().Kind
+	if ownerKind != targetKind {
+		return
+	}
+
+	// Get the CSV that target replaces
+	replacing, replaceGetErr := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(target.GetNamespace()).Get(target.Spec.Replaces)
+	if replaceGetErr != nil && !k8serrors.IsNotFound(replaceGetErr) && !k8serrors.IsGone(replaceGetErr) {
+		err = replaceGetErr
+		return
+	}
+
+	// Get the current owner CSV of the APIService
+	currentOwnerCSV, ownerGetErr := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ownerNamespace).Get(ownerName)
+	if ownerGetErr != nil && !k8serrors.IsNotFound(ownerGetErr) && !k8serrors.IsGone(ownerGetErr) {
+		err = ownerGetErr
+		return
+	}
+
+	owners := []ownerutil.Owner{target}
+	if replacing != nil {
+		owners = append(owners, replacing)
+	}
+	if currentOwnerCSV != nil && (
+		currentOwnerCSV.Status.Phase == v1alpha1.CSVPhaseReplacing || currentOwnerCSV.Status.Phase == v1alpha1.CSVPhaseDeleting) {
+		owners = append(owners, currentOwnerCSV)
+	}
+
+	adoptable = ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...)
+	return
+}

pkg/controller/operators/olm/operator.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"time"
 
+	log "github.com/sirupsen/logrus"
 	v1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
@@ -1410,17 +1411,6 @@ func (a *Operator) getReplacementChain(in *v1alpha1.ClusterServiceVersion, csvsI
 }
 
 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)
-	}
-
 	for _, desc := range csv.GetOwnedAPIServiceDescriptions() {
 		// Check if the APIService exists
 		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(desc.GetName())
@@ -1432,7 +1422,12 @@ func (a *Operator) apiServiceOwnerConflicts(csv *v1alpha1.ClusterServiceVersion)
 			continue
 		}
 
-		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
+		adoptable, err := a.isAPIServiceAdoptable(csv, apiService) 
+		if err != nil {
+			a.logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Errorf("adoption check failed - %v", err)
+		}
+
+		if !adoptable {
 			return ErrAPIServiceOwnerConflict
 		}
 	}