(bug) CSV name change causes upgrade to fail

If a head CSV has a different name than the CSV it replaces, upgrade fails because olm fails to transfer ownership of the APIService from the previous CSV to the head CSV. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1723818
operator-framework · Jun 27, 2019 · 1d53fcc · 1d53fcc
1 parent 6bf64d0
commit 1d53fcc
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 22 deletions.
diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go
@@ -1,6 +1,7 @@
 package olm
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -92,7 +93,14 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 		}
 
 		// Check if the APIService is adoptable
-		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
+		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()}).Debug("adoption failed")
 			err := olmerrors.NewUnadoptableError("", apiServiceName)
 			logger.WithError(err).Warn("found unadoptable apiservice")
@@ -707,17 +715,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 +762,50 @@ 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
+	}
+
+	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
+}
diff --git a/pkg/controller/operators/olm/operator.go b/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"
@@ -1386,17 +1387,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())
@@ -1408,7 +1398,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
 		}
 	}