Use server scheme to handle credential request API group changes

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

internal/concierge/server/server.go

@@ -174,17 +174,17 @@ func getAggregatedAPIServerConfig(
 	startControllersPostStartHook func(context.Context),
 	apiGroupSuffix string,
 ) (*apiserver.Config, error) {
-	apiGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix)
+	loginConciergeAPIGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix)
 	if !ok {
 		return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, apiGroupSuffix)
 	}
 
-	scheme := getAggregatedAPIServerScheme(apiGroup)
+	scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix)
 	codecs := serializer.NewCodecFactory(scheme)
 
-	defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", apiGroup)
+	defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", loginConciergeAPIGroup)
 	groupVersion := schema.GroupVersion{
-		Group:   apiGroup,
+		Group:   loginConciergeAPIGroup,
 		Version: loginv1alpha1.SchemeGroupVersion.Version,
 	}
 
@@ -224,7 +224,7 @@ func getAggregatedAPIServerConfig(
 	return apiServerConfig, nil
 }
 
-func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme {
+func getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix string) *runtime.Scheme {
 	// standard set up of the server side scheme
 	scheme := runtime.NewScheme()
 
@@ -232,7 +232,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme {
 	metav1.AddToGroupVersion(scheme, metav1.Unversioned)
 
 	// nothing fancy is required if using the standard group
-	if apiGroup == loginv1alpha1.GroupName {
+	if loginConciergeAPIGroup == loginv1alpha1.GroupName {
 		utilruntime.Must(loginv1alpha1.AddToScheme(scheme))
 		utilruntime.Must(loginapi.AddToScheme(scheme))
 		return scheme
@@ -257,7 +257,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme {
 			panic(err) // programmer error, scheme internal code is broken
 		}
 		newGVK := schema.GroupVersionKind{
-			Group:   apiGroup,
+			Group:   loginConciergeAPIGroup,
 			Version: gvk.Version,
 			Kind:    gvk.Kind,
 		}
@@ -270,5 +270,44 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme {
 	utilruntime.Must(loginv1alpha1.RegisterConversions(scheme))
 	utilruntime.Must(loginv1alpha1.RegisterDefaults(scheme))
 
+	// we do not want to return errors from the scheme and instead would prefer to defer
+	// to the REST storage layer for consistency.  The simplest way to do this is to force
+	// a cache miss from the authenticator cache.  Kube API groups are validated via the
+	// IsDNS1123Subdomain func thus we can easily create a group that is guaranteed never
+	// to be in the authenticator cache.  Add a timestamp just to be extra sure.
+	const authenticatorCacheMissPrefix = "_INVALID_API_GROUP_"
+	authenticatorCacheMiss := authenticatorCacheMissPrefix + time.Now().UTC().String()
+
+	// we do not have any defaulting functions for *loginv1alpha1.TokenCredentialRequest
+	// today, but we may have some in the future.  Calling AddTypeDefaultingFunc overwrites
+	// any previously registered defaulting function.  Thus to make sure that we catch
+	// a situation where we add a defaulting func, we attempt to call it here with a nil
+	// *loginv1alpha1.TokenCredentialRequest.  This will do nothing when there is no
+	// defaulting func registered, but it will almost certainly panic if one is added.
+	scheme.Default((*loginv1alpha1.TokenCredentialRequest)(nil))
+
+	// on incoming requests, restore the authenticator API group to the standard group
+	// note that we are responsible for duplicating this logic for every external API version
+	scheme.AddTypeDefaultingFunc(&loginv1alpha1.TokenCredentialRequest{}, func(obj interface{}) {
+		credentialRequest := obj.(*loginv1alpha1.TokenCredentialRequest)
+
+		if credentialRequest.Spec.Authenticator.APIGroup == nil {
+			// force a cache miss because this is an invalid request
+			plog.Debug("invalid token credential request, nil group", "authenticator", credentialRequest.Spec.Authenticator)
+			credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss
+			return
+		}
+
+		restoredGroup, ok := groupsuffix.Unreplace(*credentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix)
+		if !ok {
+			// force a cache miss because this is an invalid request
+			plog.Debug("invalid token credential request, wrong group", "authenticator", credentialRequest.Spec.Authenticator)
+			credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss
+			return
+		}
+
+		credentialRequest.Spec.Authenticator.APIGroup = &restoredGroup
+	})
+
 	return scheme
 }

internal/concierge/server/server_test.go

@@ -13,12 +13,14 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login"
 	loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1"
+	"go.pinniped.dev/internal/groupsuffix"
 )
 
 const knownGoodUsage = `
@@ -124,13 +126,13 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) {
 	}
 
 	tests := []struct {
-		name     string
-		apiGroup string
-		want     map[schema.GroupVersionKind]reflect.Type
+		name           string
+		apiGroupSuffix string
+		want           map[schema.GroupVersionKind]reflect.Type
 	}{
 		{
-			name:     "regular api group",
-			apiGroup: "login.concierge.pinniped.dev",
+			name:           "regular api group",
+			apiGroupSuffix: "pinniped.dev",
 			want: map[schema.GroupVersionKind]reflect.Type{
 				// all the types that are in the aggregated API group
 
@@ -171,8 +173,8 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) {
 			},
 		},
 		{
-			name:     "other api group",
-			apiGroup: "login.concierge.walrus.tld",
+			name:           "other api group",
+			apiGroupSuffix: "walrus.tld",
 			want: map[schema.GroupVersionKind]reflect.Type{
 				// all the types that are in the aggregated API group
 
@@ -216,8 +218,46 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) {
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			scheme := getAggregatedAPIServerScheme(tt.apiGroup)
+			loginConciergeAPIGroup, ok := groupsuffix.Replace("login.concierge.pinniped.dev", tt.apiGroupSuffix)
+			require.True(t, ok)
+
+			scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, tt.apiGroupSuffix)
 			require.Equal(t, tt.want, scheme.AllKnownTypes())
+
+			// make a credential request like a client would send
+			authenticationConciergeAPIGroup := "authentication.concierge." + tt.apiGroupSuffix
+			credentialRequest := &loginv1alpha1.TokenCredentialRequest{
+				Spec: loginv1alpha1.TokenCredentialRequestSpec{
+					Authenticator: corev1.TypedLocalObjectReference{
+						APIGroup: &authenticationConciergeAPIGroup,
+					},
+				},
+			}
+
+			// run defaulting on it
+			scheme.Default(credentialRequest)
+
+			// make sure the group is restored if needed
+			require.Equal(t, "authentication.concierge.pinniped.dev", *credentialRequest.Spec.Authenticator.APIGroup)
+
+			// make a credential request in the standard group
+			defaultAuthenticationConciergeAPIGroup := "authentication.concierge.pinniped.dev"
+			defaultCredentialRequest := &loginv1alpha1.TokenCredentialRequest{
+				Spec: loginv1alpha1.TokenCredentialRequestSpec{
+					Authenticator: corev1.TypedLocalObjectReference{
+						APIGroup: &defaultAuthenticationConciergeAPIGroup,
+					},
+				},
+			}
+
+			// run defaulting on it
+			scheme.Default(defaultCredentialRequest)
+
+			if tt.apiGroupSuffix == "pinniped.dev" { // when using the standard group, this should just work
+				require.Equal(t, "authentication.concierge.pinniped.dev", *defaultCredentialRequest.Spec.Authenticator.APIGroup)
+			} else { // when using any other group, this should always be a cache miss
+				require.True(t, strings.HasPrefix(*defaultCredentialRequest.Spec.Authenticator.APIGroup, "_INVALID_API_GROUP_2"))
+			}
 		})
 	}
 }

internal/controller/authenticator/authncache/cache.go

@@ -6,7 +6,6 @@ package authncache
 
 import (
 	"context"
-	"fmt"
 	"sort"
 	"sync"
 
@@ -15,13 +14,12 @@ import (
 	"k8s.io/klog/v2"
 
 	loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login"
+	"go.pinniped.dev/internal/constable"
 	"go.pinniped.dev/internal/plog"
 )
 
-var (
-	// ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured.
-	ErrNoSuchAuthenticator = fmt.Errorf("no such authenticator")
-)
+// ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured.
+const ErrNoSuchAuthenticator = constable.Error("no such authenticator")
 
 // Cache implements the authenticator.Token interface by multiplexing across a dynamic set of authenticators
 // loaded from authenticator resources.