diff --git a/internal/apiserviceref/apiserviceref.go b/internal/apiserviceref/apiserviceref.go new file mode 100644 index 00000000..b50b1d33 --- /dev/null +++ b/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 +} diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 6e3e0b75..5f6c6d16 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/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(). diff --git a/internal/deploymentref/deploymentref.go b/internal/deploymentref/deploymentref.go index b5fa2c9f..0c550a04 100644 --- a/internal/deploymentref/deploymentref.go +++ b/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 diff --git a/test/integration/concierge_credentialissuerconfig_test.go b/test/integration/concierge_credentialissuerconfig_test.go index 583a0250..4f241d5b 100644 --- a/test/integration/concierge_credentialissuerconfig_test.go +++ b/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) diff --git a/test/integration/kubeclient_test.go b/test/integration/kubeclient_test.go index c8580e14..0448c4f6 100644 --- a/test/integration/kubeclient_test.go +++ b/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 - // APIService because the APIService is cluster-scoped and the parent object is namespace-scoped, - // which is invalid in Kubernetes + // cluster scoped API service should be owned by the other one we created above 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) - err = ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Delete(ctx, apiService.Name, metav1.DeleteOptions{}) + hasOwnerRef(t, apiService, parentAPIServiceRef) + + // 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()