Commit addresses the issue where updating the packageserver fails wit…

…h 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.

cmd/olm/cleanup.go

@@ -1,24 +1,30 @@
 package main
 
 import (
+	"fmt"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
+	"k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
 	"time"
 
 	"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/lib/operatorclient"
 )
 
 const (
-	pollInterval = 1 * time.Second
-	pollDuration = 5 * time.Minute
+	pollInterval      = 1 * time.Second
+	pollDuration      = 5 * time.Minute
+	packageserverName = "v1.packages.operators.coreos.com"
 )
 
 type checkResourceFunc func() error
@@ -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,30 @@ 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(packageserverName, metav1.GetOptions{})
+	if err != nil && !k8serrors.IsNotFound(err) {
+		return err
+	}
+
+	labels := apiService.GetLabels()
+	if labels == nil {
+		return fmt.Errorf("APIService has no labels set")
+	}
+
+	if 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 {
+			return err
+		}
+	}
+
+	return nil
+}

cmd/olm/cleanup_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -10,6 +11,7 @@ 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"
@@ -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,85 @@ 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}),
+				},
+			},
+		},
+	}
+
+	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 +625,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/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 (