From d3c2b32b7719159408374fc18b97afb4de073f58 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Tue, 25 Jun 2019 21:39:01 -0400 Subject: [PATCH] (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 --- pkg/controller/operators/olm/apiservices.go | 88 +++++++++++++++------ pkg/controller/operators/olm/operator.go | 19 ++--- 2 files changed, 72 insertions(+), 35 deletions(-) diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go index 11a6a0ae1b..2c13e356a4 100644 --- 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" @@ -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 +} \ No newline at end of file diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 48142093ca..9367dc5535 100644 --- 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 } }