Add test for when no SA token is cached in impersonator_test.go

internal/concierge/impersonator/impersonator_test.go

@@ -58,7 +58,6 @@ import (
 	"go.pinniped.dev/internal/tokenclient"
 )
 
-// TODO: add a test without a token?
 func TestImpersonator(t *testing.T) {
 	const (
 		priorityLevelConfigurationsVersion = "v1beta3"
@@ -94,6 +93,7 @@ func TestImpersonator(t *testing.T) {
 		kubeAPIServerStatusCode         int
 		kubeAPIServerHealthz            http.Handler
 		anonymousAuthDisabled           bool
+		noServiceAcctTokenInCache       bool // when true, no available service account token for the impersonator to use
 		wantKubeAPIServerRequestHeaders http.Header
 		wantError                       string
 		wantConstructionError           string
@@ -658,6 +658,19 @@ func TestImpersonator(t *testing.T) {
 			wantError:                `an error on the server ("Internal Server Error: \"/api/v1/namespaces\": requested [{UID  008  authentication.k8s.io/v1  }] without impersonating a user") has prevented the request from succeeding (get namespaces)`,
 			wantAuthorizerAttributes: []authorizer.AttributesRecord{},
 		},
+		{
+			name:                            "when there is no service account token cached for the impersonator to use to call the KAS",
+			clientCert:                      newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}),
+			noServiceAcctTokenInCache:       true,
+			wantKubeAPIServerRequestHeaders: nil, // no request should have been made to the KAS on behalf of the user
+			wantError:                       `an error on the server ("") has prevented the request from succeeding (get namespaces)`,
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -673,7 +686,9 @@ func TestImpersonator(t *testing.T) {
 			require.NoError(t, err)
 
 			// After failing to start and after shutdown, the impersonator port should be available again.
-			defer requireCanBindToPort(t, port)
+			t.Cleanup(func() {
+				requireCanBindToPort(t, port)
+			})
 
 			if tt.kubeAPIServerStatusCode == 0 {
 				tt.kubeAPIServerStatusCode = http.StatusOK
@@ -845,10 +860,13 @@ func TestImpersonator(t *testing.T) {
 				return kubeclient.Secure(config)
 			}
 
+			serviceTokenCache := tokenclient.NewExpiringSingletonTokenCache()
+			if !tt.noServiceAcctTokenInCache {
+				serviceTokenCache.Set("some-service-account-token", 1*time.Hour)
+			}
+
 			// Create an impersonator.  Use an invalid port number to make sure our listener override works.
-			cache := tokenclient.NewExpiringSingletonTokenCache()
-			cache.Set("some-service-account-token", 1*time.Hour)
-			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, cache, &testKubeAPIServerKubeconfig, recOpts, recConfig)
+			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, serviceTokenCache, &testKubeAPIServerKubeconfig, recOpts, recConfig)
 			if len(tt.wantConstructionError) > 0 {
 				require.EqualError(t, constructionErr, tt.wantConstructionError)
 				require.Nil(t, runner)
@@ -866,6 +884,13 @@ func TestImpersonator(t *testing.T) {
 				errCh <- stopErr
 			}()
 
+			// Stop the impersonator server at the end of the test, even if it fails.
+			t.Cleanup(func() {
+				close(stopCh)
+				exitErr := <-errCh
+				require.NoError(t, exitErr)
+			})
+
 			// Create a kubeconfig to talk to the impersonator as a client.
 			clientKubeconfig := &rest.Config{
 				Host: "https://127.0.0.1:" + strconv.Itoa(port),
@@ -919,6 +944,12 @@ func TestImpersonator(t *testing.T) {
 			// of the original request mutated by the impersonator.  Otherwise the headers should be nil.
 			require.Equal(t, tt.wantKubeAPIServerRequestHeaders, testKubeAPIServerSawHeaders)
 
+			// The rest of the test doesn't make sense for when there is no service account token available in the cache.
+			// In this case, the impersonator cannot make any calls to the Kube API server on behalf of any user.
+			if tt.noServiceAcctTokenInCache {
+				return
+			}
+
 			// these authorization checks are caused by the anonymous auth checks below
 			tt.wantAuthorizerAttributes = append(tt.wantAuthorizerAttributes,
 				authorizer.AttributesRecord{
@@ -1021,11 +1052,6 @@ func TestImpersonator(t *testing.T) {
 			_, errBadCert := tcrBadCert.PinnipedConcierge.LoginV1alpha1().TokenCredentialRequests().Create(ctx, &loginv1alpha1.TokenCredentialRequest{}, metav1.CreateOptions{})
 			require.True(t, errors.IsUnauthorized(errBadCert), errBadCert)
 			require.EqualError(t, errBadCert, "Unauthorized")
-
-			// Stop the impersonator server.
-			close(stopCh)
-			exitErr := <-errCh
-			require.NoError(t, exitErr)
 		})
 	}
 }

internal/concierge/impersonator/roundtripper.go

@@ -4,11 +4,13 @@
 package impersonator
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 
 	utilnet "k8s.io/apimachinery/pkg/util/net"
 
+	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/tokenclient"
 )
 
@@ -25,10 +27,17 @@ func (rt *authorizationRoundTripper) WrappedRoundTripper() http.RoundTripper {
 
 func (rt *authorizationRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	req = utilnet.CloneRequest(req)
+
 	token := rt.cache.Get()
+
 	if token == "" {
-		return nil, fmt.Errorf("no token available")
+		plog.Error("could not RoundTrip impersonation proxy request to API server",
+			errors.New("no service account token available in in-memory cache"))
+
+		return nil, fmt.Errorf("no impersonator service account token available")
 	}
+
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+
 	return rt.base.RoundTrip(req)
 }

internal/concierge/impersonator/roundtripper_test.go

@@ -69,7 +69,7 @@ func TestRoundTrip(t *testing.T) {
 		{
 			name:      "no token available",
 			token:     "", // since the cache always returns a non-pointer string, this indicates empty
-			wantError: "no token available",
+			wantError: "no impersonator service account token available",
 		},
 	} {
 		tt := tt

test/integration/concierge_impersonation_proxy_test.go

@@ -2050,7 +2050,6 @@ func createServiceAccountToken(ctx context.Context, t *testing.T, adminClient ku
 			Delete(context.Background(), serviceAccount.Name, metav1.DeleteOptions{}))
 	})
 
-	// TODO: What is this used for?
 	secret, err := adminClient.CoreV1().Secrets(namespaceName).Create(ctx, &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "int-test-service-account-token-",

test/integration/concierge_whoami_test.go

@@ -89,7 +89,6 @@ func TestWhoAmI_ServiceAccount_Legacy_Parallel(t *testing.T) {
 	}, metav1.CreateOptions{})
 	require.NoError(t, err)
 
-	// TODO: What is this used for?
 	secret, err := kubeClient.Secrets(ns.Name).Create(ctx, &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "test-whoami-",