Addresses the bug where updating the packageserver fails with a unable to adopt APIService status message. This is a result of ownership mismatches between the packageserver (owner) and the APIService (the owned object). This can be resolved by setting the actual k8s owner references or the label-based references that OLM uses. This commit ensures that ownership references for the APIService are cleaned up on OLM restart and that the olm.owner:packageserver label is set on the corresponding APIService object.

Author: exdx

cmd/olm/cleanup.go

@@ -3,17 +3,23 @@ package main
 import (
 	"time"
 
+	"k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
+
 	"github.com/sirupsen/logrus"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
+	apiregv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 const (
@@ -53,6 +59,10 @@ func cleanup(logger *logrus.Logger, c operatorclient.ClientInterface, crc versio
 	if err := cleanupOwnerReferences(c, crc); err != nil {
 		logger.WithError(err).Fatal("couldn't cleanup cross-namespace ownerreferences")
 	}
+
+	if err := ensureAPIServiceLabels(c.ApiregistrationV1Interface()); err != nil {
+		logger.WithError(err).Fatal("couldn't ensure APIService labels")
+	}
 }
 
 func waitForDelete(checkResource checkResourceFunc, deleteResource deleteResourceFunc) error {
@@ -171,6 +181,11 @@ func cleanupOwnerReferences(c operatorclient.ClientInterface, crc versioned.Inte
 		objs = append(objs, &obj)
 	}
 
+	apiServices, _ := c.ApiregistrationV1Interface().ApiregistrationV1().APIServices().List(listOpts)
+	for _, obj := range apiServices.Items {
+		objs = append(objs, &obj)
+	}
+
 	for _, obj := range objs {
 		if !removeBadRefs(obj) {
 			continue
@@ -206,6 +221,11 @@ func cleanupOwnerReferences(c operatorclient.ClientInterface, crc versioned.Inte
 				_, err = c.KubernetesInterface().RbacV1().RoleBindings(v.GetNamespace()).Update(v)
 				return err
 			}
+		case *apiregv1.APIService:
+			update = func() error {
+				_, err = c.ApiregistrationV1Interface().ApiregistrationV1().APIServices().Update(v)
+				return err
+			}
 		}
 
 		if err := retry.RetryOnConflict(retry.DefaultBackoff, update); err != nil {
@@ -239,3 +259,35 @@ func crossNamespaceOwnerReferenceRemoval(kind string, uidNamespaces map[types.UI
 		return
 	}
 }
+
+// ensureAPIServiceLabels checks the labels of existing APIService objects to ensure ownership is set correctly.
+// If the APIService label does not correspond to the expected packageserver owner the APIService labels are updated
+func ensureAPIServiceLabels(c clientset.Interface) error {
+	apiService, err := c.ApiregistrationV1().APIServices().Get(olm.PackageserverName, metav1.GetOptions{})
+	if err != nil && !k8serrors.IsNotFound(err) {
+		logrus.WithField("apiService", "packageserver").Debugf("get error: %s", err)
+		return err
+	}
+	if apiService == nil || k8serrors.IsNotFound(err) || apiService.Name == "" {
+		logrus.WithField("apiService", "packageserver").Debugf("not found")
+		return nil
+	}
+
+	if l := apiService.GetLabels(); l == nil {
+		apiService.Labels = make(map[string]string)
+	}
+
+	if apiService.Labels[ownerutil.OwnerKey] != ownerutil.OwnerPackageServer {
+		apiService.Labels[ownerutil.OwnerKey] = ownerutil.OwnerPackageServer
+		update := func() error {
+			_, err := c.ApiregistrationV1().APIServices().Update(apiService)
+			return err
+		}
+		if err := retry.RetryOnConflict(retry.DefaultBackoff, update); err != nil {
+			logrus.WithField("apiService", apiService.Name).Warnf("update error: %s", err)
+			return err
+		}
+	}
+
+	return nil
+}

cmd/olm/cleanup_test.go

@@ -10,11 +10,13 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
+	apiregv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 	apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	operatorsfake "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 func TestCrossOwnerReferenceRemoval(t *testing.T) {
@@ -352,8 +354,9 @@ func TestCleanupOwnerReferences(t *testing.T) {
 	clusterRoleKind := "ClusterRole"
 
 	type fields struct {
-		k8sObjs    []runtime.Object
-		clientObjs []runtime.Object
+		k8sObjs     []runtime.Object
+		clientObjs  []runtime.Object
+		apiServices []runtime.Object
 	}
 	type expected struct {
 		err                 error
@@ -362,6 +365,7 @@ func TestCleanupOwnerReferences(t *testing.T) {
 		clusterRoleBindings []rbacv1.ClusterRoleBinding
 		roles               []rbacv1.Role
 		roleBindings        []rbacv1.RoleBinding
+		apiServices         []apiregv1.APIService
 	}
 	tests := []struct {
 		description string
@@ -381,6 +385,9 @@ func TestCleanupOwnerReferences(t *testing.T) {
 					newClusterServiceVersion("ns-b", "csv-b", "csv-b-uid"),
 					newClusterServiceVersion("ns-a", "csv-a", "csv-a-uid", newOwnerReference(v1alpha1.ClusterServiceVersionKind, "csv-b-uid")),
 				},
+				apiServices: []runtime.Object{
+					newAPIService("apisvc", "apisvc-a-uid", nil, newOwnerReference(v1alpha1.ClusterServiceVersionKind, "csv-b-uid")),
+				},
 			},
 			expected: expected{
 				csvs: []v1alpha1.ClusterServiceVersion{
@@ -399,6 +406,9 @@ func TestCleanupOwnerReferences(t *testing.T) {
 				roleBindings: []rbacv1.RoleBinding{
 					*newRoleBinding("ns-a", "rb", "rb-a-uid"),
 				},
+				apiServices: []apiregv1.APIService{
+					*newAPIService("apisvc", "apisvc-a-uid", nil),
+				},
 			},
 		},
 		{
@@ -462,7 +472,7 @@ func TestCleanupOwnerReferences(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.description, func(t *testing.T) {
 			k8sClient := k8sfake.NewSimpleClientset(tt.fields.k8sObjs...)
-			c := operatorclient.NewClient(k8sClient, apiextensionsfake.NewSimpleClientset(), apiregistrationfake.NewSimpleClientset())
+			c := operatorclient.NewClient(k8sClient, apiextensionsfake.NewSimpleClientset(), apiregistrationfake.NewSimpleClientset(tt.fields.apiServices...))
 			crc := operatorsfake.NewSimpleClientset(tt.fields.clientObjs...)
 			require.Equal(t, tt.expected.err, cleanupOwnerReferences(c, crc))
 
@@ -486,10 +496,111 @@ func TestCleanupOwnerReferences(t *testing.T) {
 			roleBindings, err := c.KubernetesInterface().RbacV1().RoleBindings(metav1.NamespaceAll).List(listOpts)
 			require.NoError(t, err)
 			require.ElementsMatch(t, tt.expected.roleBindings, roleBindings.Items)
+
+			apiService, err := c.ApiregistrationV1Interface().ApiregistrationV1().APIServices().List(listOpts)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tt.expected.apiServices, apiService.Items)
 		})
 	}
 }
 
+func TestCheckAPIServiceLabels(t *testing.T) {
+	type fields struct {
+		apiServices []runtime.Object
+	}
+
+	type expected struct {
+		err         error
+		apiServices []apiregv1.APIService
+	}
+
+	tests := []struct {
+		description string
+		fields      fields
+		expected    expected
+	}{
+		{
+			description: "NoLabel/UpdateAPIService",
+			fields: fields{
+				apiServices: []runtime.Object{
+					newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{}),
+				},
+			},
+			expected: expected{
+				apiServices: []apiregv1.APIService{
+					*newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: ownerutil.OwnerPackageServer}),
+				},
+			},
+		},
+		{
+			description: "WrongLabel/UpdateAPIService",
+			fields: fields{
+				apiServices: []runtime.Object{
+					newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: "banana"}),
+				},
+			},
+			expected: expected{
+				apiServices: []apiregv1.APIService{
+					*newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: ownerutil.OwnerPackageServer}),
+				},
+			},
+		},
+		{
+			description: "CorrectLabel/NoUpdate",
+			fields: fields{
+				apiServices: []runtime.Object{
+					newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: ownerutil.OwnerPackageServer}),
+				},
+			},
+			expected: expected{
+				apiServices: []apiregv1.APIService{
+					*newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: ownerutil.OwnerPackageServer}),
+				},
+			},
+		},
+		{
+			description: "WrongAPIService/NoUpdate",
+			fields: fields{
+				apiServices: []runtime.Object{
+					newAPIService("banana", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: "banana"}),
+				},
+			},
+			expected: expected{
+				apiServices: []apiregv1.APIService{
+					*newAPIService("banana", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: "banana"}),
+				},
+			},
+		},
+		{
+			description: "NoLabels/Update",
+			fields: fields{
+				apiServices: []runtime.Object{
+					newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", nil),
+				},
+			},
+			expected: expected{
+				apiServices: []apiregv1.APIService{
+					*newAPIService("v1.packages.operators.coreos.com", "apisvc-a-uid", map[string]string{ownerutil.OwnerKey: ownerutil.OwnerPackageServer}),
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			listOpts := metav1.ListOptions{}
+			k8sClient := k8sfake.NewSimpleClientset()
+			c := operatorclient.NewClient(k8sClient, apiextensionsfake.NewSimpleClientset(), apiregistrationfake.NewSimpleClientset(tt.fields.apiServices...))
+			require.Equal(t, tt.expected.err, ensureAPIServiceLabels(c.ApiregistrationV1Interface()))
+
+			apiService, err := c.ApiregistrationV1Interface().ApiregistrationV1().APIServices().List(listOpts)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tt.expected.apiServices, apiService.Items)
+		})
+	}
+
+}
+
 func newOwnerReference(kind string, uid types.UID) metav1.OwnerReference {
 	return metav1.OwnerReference{
 		Kind: kind,
@@ -540,3 +651,12 @@ func newRoleBinding(namespace, name string, uid types.UID, ownerRefs ...metav1.O
 	roleBinding.SetOwnerReferences(ownerRefs)
 	return roleBinding
 }
+
+func newAPIService(name string, uid types.UID, labels map[string]string, ownerRefs ...metav1.OwnerReference) *apiregv1.APIService {
+	apiService := &apiregv1.APIService{}
+	apiService.SetUID(uid)
+	apiService.SetName(name)
+	apiService.SetOwnerReferences(ownerRefs)
+	apiService.SetLabels(labels)
+	return apiService
+}

pkg/controller/operators/olm/apiservices.go

@@ -32,6 +32,8 @@ const (
 	OLMCAHashAnnotationKey = "olmcahash"
 	// Organization is the organization name used in the generation of x509 certs
 	Organization = "Red Hat, Inc."
+	// Name of packageserver API service
+	PackageserverName = "v1.packages.operators.coreos.com"
 )
 
 func (a *Operator) shouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {

pkg/lib/ownerutil/util.go

@@ -16,9 +16,10 @@ import (
 )
 
 const (
-	OwnerKey          = "olm.owner"
-	OwnerNamespaceKey = "olm.owner.namespace"
-	OwnerKind         = "olm.owner.kind"
+	OwnerKey           = "olm.owner"
+	OwnerNamespaceKey  = "olm.owner.namespace"
+	OwnerKind          = "olm.owner.kind"
+	OwnerPackageServer = "packageserver"
 )
 
 var (