Use API service as owner ref for cluster scoped resources

Signed-off-by: Monis Khan <mok@vmware.com>

internal/apiserviceref/apiserviceref.go

@@ -0,0 +1,36 @@
+// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package apiserviceref
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
+
+	"go.pinniped.dev/internal/kubeclient"
+	"go.pinniped.dev/internal/ownerref"
+)
+
+func New(apiServiceName string, opts ...kubeclient.Option) (kubeclient.Option, error) {
+	tempClient, err := kubeclient.New(opts...)
+	if err != nil {
+		return nil, fmt.Errorf("cannot create temp client: %w", err)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	apiService, err := tempClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiServiceName, metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("cannot get api service %s: %w", apiServiceName, err)
+	}
+
+	// work around stupid behavior of WithoutVersionDecoder.Decode
+	apiService.APIVersion, apiService.Kind = apiregistrationv1.SchemeGroupVersion.WithKind("APIService").ToAPIVersionAndKind()
+
+	return kubeclient.WithMiddleware(ownerref.New(apiService)), nil
+}

internal/controllermanager/prepare_controllers.go

@@ -18,6 +18,7 @@ import (
 	loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1"
 	pinnipedclientset "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned"
 	pinnipedinformers "go.pinniped.dev/generated/1.20/client/concierge/informers/externalversions"
+	"go.pinniped.dev/internal/apiserviceref"
 	"go.pinniped.dev/internal/config/concierge"
 	"go.pinniped.dev/internal/controller/apicerts"
 	"go.pinniped.dev/internal/controller/authenticator/authncache"
@@ -84,13 +85,25 @@ type Config struct {
 // Prepare the controllers and their informers and return a function that will start them when called.
 //nolint:funlen // Eh, fair, it is a really long function...but it is wiring the world...so...
 func PrepareControllers(c *Config) (func(ctx context.Context), error) {
+	groupName, ok := groupsuffix.Replace(loginv1alpha1.GroupName, c.APIGroupSuffix)
+	if !ok {
+		return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, c.APIGroupSuffix)
+	}
+	apiServiceName := loginv1alpha1.SchemeGroupVersion.Version + "." + groupName
+
 	dref, _, err := deploymentref.New(c.ServerInstallationInfo)
 	if err != nil {
 		return nil, fmt.Errorf("cannot create deployment ref: %w", err)
 	}
 
+	apiServiceRef, err := apiserviceref.New(apiServiceName)
+	if err != nil {
+		return nil, fmt.Errorf("cannot create API service ref: %w", err)
+	}
+
 	client, err := kubeclient.New(
-		dref,
+		dref,          // first try to use the deployment as an owner ref (for namespace scoped resources)
+		apiServiceRef, // fallback to our API service (for everything else we create)
 		kubeclient.WithMiddleware(groupsuffix.New(c.APIGroupSuffix)),
 	)
 	if err != nil {
@@ -112,12 +125,6 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) {
 		Name: c.NamesConfig.CredentialIssuer,
 	}
 
-	groupName, ok := groupsuffix.Replace(loginv1alpha1.GroupName, c.APIGroupSuffix)
-	if !ok {
-		return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, c.APIGroupSuffix)
-	}
-	apiServiceName := loginv1alpha1.SchemeGroupVersion.Version + "." + groupName
-
 	// Create controller manager.
 	controllerManager := controllerlib.
 		NewManager().

internal/deploymentref/deploymentref.go

@@ -19,7 +19,7 @@ import (
 
 // getTempClient is stubbed out for testing.
 //
-// We would normally inject a kubernetes.Interface into New(), but the client we want to create in
+// We would normally pass a kubernetes.Interface into New(), but the client we want to create in
 // the calling code depends on the return value of New() (i.e., on the kubeclient.Option for the
 // OwnerReference).
 //nolint: gochecknoglobals

test/integration/concierge_credentialissuerconfig_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 
 	configv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/config/v1alpha1"
 	"go.pinniped.dev/test/library"
@@ -20,6 +21,7 @@ func TestCredentialIssuer(t *testing.T) {
 	env := library.IntegrationEnv(t)
 	config := library.NewClientConfig(t)
 	client := library.NewConciergeClientset(t)
+	aggregatedClientset := library.NewAggregatedClientset(t)
 
 	library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "")
 
@@ -43,6 +45,23 @@ func TestCredentialIssuer(t *testing.T) {
 		}
 		require.Equal(t, env.ConciergeAppName, actualConfig.Labels["app"])
 
+		// verify owner ref is set
+		require.Len(t, actualConfig.OwnerReferences, 1)
+
+		apiService, err := aggregatedClientset.ApiregistrationV1().APIServices().Get(ctx, "v1alpha1.login.concierge."+env.APIGroupSuffix, metav1.GetOptions{})
+		require.NoError(t, err)
+
+		// work around stupid behavior of WithoutVersionDecoder.Decode
+		apiService.APIVersion, apiService.Kind = apiregistrationv1.SchemeGroupVersion.WithKind("APIService").ToAPIVersionAndKind()
+
+		ref := metav1.OwnerReference{
+			APIVersion: apiService.APIVersion,
+			Kind:       apiService.Kind,
+			Name:       apiService.Name,
+			UID:        apiService.UID,
+		}
+		require.Equal(t, ref, actualConfig.OwnerReferences[0])
+
 		// Verify the cluster strategy status based on what's expected of the test cluster's ability to share signing keys.
 		actualStatusStrategies := actualConfigList.Items[0].Status.Strategies
 		require.Len(t, actualStatusStrategies, 1)

test/integration/kubeclient_test.go

@@ -17,6 +17,7 @@ import (
 
 	conciergeconfigv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/config/v1alpha1"
 	supervisorconfigv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1"
+	"go.pinniped.dev/internal/apiserviceref"
 	"go.pinniped.dev/internal/groupsuffix"
 	"go.pinniped.dev/internal/kubeclient"
 	"go.pinniped.dev/internal/ownerref"
@@ -27,6 +28,7 @@ func TestKubeClientOwnerRef(t *testing.T) {
 	env := library.IntegrationEnv(t)
 
 	regularClient := library.NewKubernetesClientset(t)
+	regularAggregationClient := library.NewAggregatedClientset(t)
 
 	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
 	defer cancel()
@@ -73,9 +75,47 @@ func TestKubeClientOwnerRef(t *testing.T) {
 		UID:        parentSecret.UID,
 	}
 
+	parentAPIService, err := regularAggregationClient.ApiregistrationV1().APIServices().Create(
+		ctx,
+		&apiregistrationv1.APIService{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "v1.snorlax.dev",
+			},
+			Spec: apiregistrationv1.APIServiceSpec{
+				Version:              "v1",
+				Group:                "snorlax.dev",
+				GroupPriorityMinimum: 10_000,
+				VersionPriority:      500,
+			},
+		},
+		metav1.CreateOptions{},
+	)
+	require.NoError(t, err)
+	defer func() {
+		err := regularAggregationClient.ApiregistrationV1().APIServices().Delete(ctx, parentAPIService.Name, metav1.DeleteOptions{})
+		if errors.IsNotFound(err) {
+			return
+		}
+		require.NoError(t, err)
+	}()
+
+	// work around stupid behavior of WithoutVersionDecoder.Decode
+	parentAPIService.APIVersion, parentAPIService.Kind = apiregistrationv1.SchemeGroupVersion.WithKind("APIService").ToAPIVersionAndKind()
+
+	parentAPIServiceRef := metav1.OwnerReference{
+		APIVersion: parentAPIService.APIVersion,
+		Kind:       parentAPIService.Kind,
+		Name:       parentAPIService.Name,
+		UID:        parentAPIService.UID,
+	}
+
+	apiServiceRef, err := apiserviceref.New(parentAPIService.Name, kubeclient.WithConfig(library.NewClientConfig(t)))
+	require.NoError(t, err)
+
 	// create a client that should set an owner ref back to parent on create
 	ownerRefClient, err := kubeclient.New(
-		kubeclient.WithMiddleware(ownerref.New(parentSecret)),
+		kubeclient.WithMiddleware(ownerref.New(parentSecret)), // secret owner ref first when possible
+		apiServiceRef, // api service for everything else
 		kubeclient.WithMiddleware(groupsuffix.New(env.APIGroupSuffix)),
 		kubeclient.WithConfig(library.NewClientConfig(t)),
 	)
@@ -132,7 +172,7 @@ func TestKubeClientOwnerRef(t *testing.T) {
 	require.NotEqual(t, parentSecret.ResourceVersion, updatedParentSecret.ResourceVersion)
 	require.Len(t, updatedParentSecret.OwnerReferences, 0)
 
-	// delete the parent object
+	// delete the parent secret object
 	err = ownerRefSecrets.Delete(ctx, parentSecret.Name, metav1.DeleteOptions{})
 	require.NoError(t, err)
 
@@ -142,9 +182,7 @@ func TestKubeClientOwnerRef(t *testing.T) {
 		return err
 	})
 
-	// sanity check API service client - the middleware code shouldn't add an owner reference to this
+	// cluster scoped API service should be owned by the other one we created above
-	// APIService because the APIService is cluster-scoped and the parent object is namespace-scoped,
-	// which is invalid in Kubernetes
 	apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create(
 		ctx,
 		&apiregistrationv1.APIService{
@@ -162,10 +200,18 @@ func TestKubeClientOwnerRef(t *testing.T) {
 		metav1.CreateOptions{},
 	)
 	require.NoError(t, err)
-	hasNoOwnerRef(t, apiService)
+	hasOwnerRef(t, apiService, parentAPIServiceRef)
-	err = ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Delete(ctx, apiService.Name, metav1.DeleteOptions{})
+
+	// delete the parent API service object
+	err = ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Delete(ctx, parentAPIService.Name, metav1.DeleteOptions{})
 	require.NoError(t, err)
 
+	// the child object should be cleaned up on its own
+	isEventuallyDeleted(t, func() error {
+		_, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiService.Name, metav1.GetOptions{})
+		return err
+	})
+
 	// sanity check concierge client
 	credentialIssuer, err := ownerRefClient.PinnipedConcierge.ConfigV1alpha1().CredentialIssuers().Create(
 		ctx,
@@ -181,7 +227,7 @@ func TestKubeClientOwnerRef(t *testing.T) {
 		metav1.CreateOptions{},
 	)
 	require.NoError(t, err)
-	hasOwnerRef(t, credentialIssuer, ref)
+	hasOwnerRef(t, credentialIssuer, parentAPIServiceRef)
 	// this owner has already been deleted so the cred issuer should be immediately deleted
 	isEventuallyDeleted(t, func() error {
 		_, err := ownerRefClient.PinnipedConcierge.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuer.Name, metav1.GetOptions{})
@@ -246,13 +292,6 @@ func hasOwnerRef(t *testing.T, obj metav1.Object, ref metav1.OwnerReference) {
 	require.Equal(t, ref, ownerReferences[0])
 }
 
-func hasNoOwnerRef(t *testing.T, obj metav1.Object) {
-	t.Helper()
-
-	ownerReferences := obj.GetOwnerReferences()
-	require.Len(t, ownerReferences, 0)
-}
-
 func isEventuallyDeleted(t *testing.T, f func() error) {
 	t.Helper()