Merge pull request #758 from njhale/fix-conflict-detection

fix(csv): properly detect apiservice and crd conflicts

go.sum

@@ -112,7 +112,7 @@ github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoA
 github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
 github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f h1:ShTPMJQes6tubcjzGMODIVG5hlrCeImaBnZzKF2N8SM=
 github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
-github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c=
+github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:BWIsLfhgKhV5g/oF34aRjniBHLTZe5DNekSjbAjIS6c=
 github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs=
 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho=
 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk=
@@ -132,7 +132,6 @@ github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NH
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/jonboulle/clockwork v0.1.0 h1:VKV+ZcuP6l3yW9doeqz6ziZGgcynBVQO+obU0+0hcPo=
 github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo=
-github.com/json-iterator/go v1.1.5 h1:gL2yXlmiIo4+t+y32d4WGwOjKGYcGOuyrg46vadswDE=
 github.com/json-iterator/go v1.1.5/go.mod h1:+SdeFBvtyEkXs7REEP0seUULqWtbJapLOCVDaaPEHmU=
 github.com/json-iterator/go v1.1.6 h1:MrUvLMLTMxbqFJ9kzlvat/rYZqZnW3u4wkLzWTaFwKs=
 github.com/json-iterator/go v1.1.6/go.mod h1:+SdeFBvtyEkXs7REEP0seUULqWtbJapLOCVDaaPEHmU=
@@ -322,5 +321,5 @@ k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd h1:ggv/Vfza0i5xuhUZyYyxcc
 k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd/go.mod h1:BXM9ceUBTj2QnfH2MK1odQs778ajze1RxcmP6S8RVVc=
 k8s.io/kubernetes v1.11.7-beta.0.0.20181219023948-b875d52ea96d/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk=
 k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk=
-k8s.io/kubernetes v1.11.9-beta.0.0.20190311041124-ede55fd57298 h1:CMsSK4yqJD0IfftutHd0XjYoJTsrtx983MpdKFd8c74=
+k8s.io/kubernetes v1.11.9-beta.0.0.20190311041124-ede55fd57298 h1:vVWtHOzTn3+uHbF0nZUlTrwFfbxPn2ZM3chxhAIUzg0=
 k8s.io/kubernetes v1.11.9-beta.0.0.20190311041124-ede55fd57298/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk=

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

@@ -30,6 +30,16 @@ var uncopiableReasons = map[ConditionReason]struct{}{
 	CSVReasonCannotModifyStaticOperatorGroupProvidedAPIs: {},
 }
 
+// SetPhaseWithEventIfChanged emits a Kubernetes event with details of a phase change and sets the current phase if phase, reason, or message would changed
+func (c *ClusterServiceVersion) SetPhaseWithEventIfChanged(phase ClusterServiceVersionPhase, reason ConditionReason, message string, now metav1.Time, recorder record.EventRecorder) {
+	if c.Status.Phase == phase && c.Status.Reason == reason && c.Status.Message == message {
+		return
+	}
+
+	c.SetPhaseWithEvent(phase, reason, message, now, recorder)
+}
+
+// SetPhaseWithEvent generates a Kubernetes event with details about the phase change and sets the current phase
 func (c *ClusterServiceVersion) SetPhaseWithEvent(phase ClusterServiceVersionPhase, reason ConditionReason, message string, now metav1.Time, recorder record.EventRecorder) {
 	var eventtype string
 	if phase == CSVPhaseFailed {

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

@@ -11,6 +11,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 
@@ -41,42 +42,45 @@ func (a *Operator) shouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
 	return false
 }
 
-// apiServiceResourceErrorsActionable returns true if OLM can do something about any one
+// apiServiceResourceErrorActionable returns true if OLM can do something about any one
 // of the apiService errors in errs; otherwise returns false
 //
 // This method can be used to determine if a CSV in a failed state due to APIService
 // issues can resolve them by reinstalling
-func (a *Operator) apiServiceResourceErrorsActionable(errs []error) bool {
-	for _, err := range errs {
-		switch err.(type) {
-		case olmerrors.UnadoptableError:
-			return false
-		}
-	}
+func (a *Operator) apiServiceResourceErrorActionable(err error) bool {
+	filtered := utilerrors.FilterOut(err, func(e error) bool {
+		_, unadoptable := e.(olmerrors.UnadoptableError)
+		return !unadoptable
+	})
+	actionable := filtered == nil
 
-	return true
+	return actionable
 }
 
 // checkAPIServiceResources checks if all expected generated resources for the given APIService exist
-func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion, hashFunc certs.PEMHash) []error {
+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
-	replacement, 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 errs
+	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 replacement != nil {
-		owners = append(owners, replacement)
+	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,11 +92,11 @@ 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)
-			return errs
+			return utilerrors.NewAggregate(errs)
 		}
 
 		serviceName := APIServiceNameToServiceName(apiServiceName)
@@ -239,7 +243,7 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 		}
 	}
 
-	return errs
+	return utilerrors.NewAggregate(errs)
 }
 
 func (a *Operator) isAPIServiceAvailable(apiService *apiregistrationv1.APIService) bool {
@@ -251,10 +255,9 @@ func (a *Operator) isAPIServiceAvailable(apiService *apiregistrationv1.APIServic
 	return false
 }
 
-func (a *Operator) areAPIServicesAvailable(descs []v1alpha1.APIServiceDescription) (bool, error) {
-	for _, desc := range descs {
-		apiServiceName := fmt.Sprintf("%s.%s", desc.Version, desc.Group)
-		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
+func (a *Operator) areAPIServicesAvailable(csv *v1alpha1.ClusterServiceVersion) (bool, error) {
+	for _, desc := range csv.Spec.APIServiceDefinitions.Owned {
+		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)
 		}
 	}