Merge pull request #775 from enj/enj/i/dynamiccert_no_unload

impersonatorconfig: only unload dynamiccert when proxy is disabled

internal/controller/apicerts/certs_expirer_test.go

@@ -20,9 +20,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	kubeinformers "k8s.io/client-go/informers"
-	"k8s.io/client-go/kubernetes"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
-	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	kubetesting "k8s.io/client-go/testing"
 
 	"go.pinniped.dev/internal/controllerlib"
@@ -253,7 +251,8 @@ func TestExpirerControllerSync(t *testing.T) {
 				0,
 			)
 
-			trackDeleteClient := &clientWrapper{Interface: kubeAPIClient, opts: &[]metav1.DeleteOptions{}}
+			opts := &[]metav1.DeleteOptions{}
+			trackDeleteClient := testutil.NewDeleteOptionsRecorder(kubeAPIClient, opts)
 
 			c := NewCertsExpirerController(
 				namespace,
@@ -297,44 +296,16 @@ func TestExpirerControllerSync(t *testing.T) {
 			require.Equal(t, exActions, acActions)
 
 			if test.wantDelete {
-				require.Len(t, *trackDeleteClient.opts, 1)
+				require.Len(t, *opts, 1)
 				require.Equal(t, metav1.DeleteOptions{
 					Preconditions: &metav1.Preconditions{
 						UID:             &testUID,
 						ResourceVersion: &testRV,
 					},
-				}, (*trackDeleteClient.opts)[0])
+				}, (*opts)[0])
 			} else {
-				require.Len(t, *trackDeleteClient.opts, 0)
+				require.Len(t, *opts, 0)
 			}
 		})
 	}
 }
-
-type clientWrapper struct {
-	kubernetes.Interface
-	opts *[]metav1.DeleteOptions
-}
-
-func (c *clientWrapper) CoreV1() corev1client.CoreV1Interface {
-	return &coreWrapper{CoreV1Interface: c.Interface.CoreV1(), opts: c.opts}
-}
-
-type coreWrapper struct {
-	corev1client.CoreV1Interface
-	opts *[]metav1.DeleteOptions
-}
-
-func (c *coreWrapper) Secrets(namespace string) corev1client.SecretInterface {
-	return &secretsWrapper{SecretInterface: c.CoreV1Interface.Secrets(namespace), opts: c.opts}
-}
-
-type secretsWrapper struct {
-	corev1client.SecretInterface
-	opts *[]metav1.DeleteOptions
-}
-
-func (s *secretsWrapper) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error {
-	*s.opts = append(*s.opts, opts)
-	return s.SecretInterface.Delete(ctx, name, opts)
-}

internal/controller/impersonatorconfig/impersonator_config.go

@@ -183,8 +183,6 @@ func (c *impersonatorConfigController) Sync(syncCtx controllerlib.Context) error
 			Message:        err.Error(),
 			LastUpdateTime: metav1.NewTime(c.clock.Now()),
 		}
-		// The impersonator is not ready, so clear the signer CA from the dynamic provider.
-		c.clearSignerCA()
 	}
 
 	err = utilerrors.NewAggregate([]error{err, issuerconfig.Update(
@@ -281,28 +279,33 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 
 	nameInfo, err := c.findDesiredTLSCertificateName(impersonationSpec)
 	if err != nil {
-		// Unexpected error while determining the name that should go into the certs, so clear any existing certs.
-		c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
 		return nil, err
 	}
 
 	var impersonationCA *certauthority.CA
-	if c.shouldHaveTLSSecret(impersonationSpec) {
+	if c.shouldHaveImpersonator(impersonationSpec) {
 		if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
 			return nil, err
 		}
 		if err = c.ensureTLSSecret(ctx, nameInfo, impersonationCA); err != nil {
 			return nil, err
 		}
-	} else if err = c.ensureTLSSecretIsRemoved(ctx); err != nil {
+	} else {
+		if err = c.ensureTLSSecretIsRemoved(ctx); err != nil {
 			return nil, err
 		}
+		c.clearTLSSecret()
+	}
 
 	credentialIssuerStrategyResult := c.doSyncResult(nameInfo, impersonationSpec, impersonationCA)
 
-	if err = c.loadSignerCA(credentialIssuerStrategyResult.Status); err != nil {
+	if c.shouldHaveImpersonator(impersonationSpec) {
+		if err = c.loadSignerCA(); err != nil {
 			return nil, err
 		}
+	} else {
+		c.clearSignerCA()
+	}
 
 	return credentialIssuerStrategyResult, nil
 }
@@ -350,20 +353,16 @@ func (c *impersonatorConfigController) shouldHaveClusterIPService(config *v1alph
 	return c.shouldHaveImpersonator(config) && config.Service.Type == v1alpha1.ImpersonationProxyServiceTypeClusterIP
 }
 
-func (c *impersonatorConfigController) shouldHaveTLSSecret(config *v1alpha1.ImpersonationProxySpec) bool {
+func (c *impersonatorConfigController) serviceExists(serviceName string) (bool, *v1.Service, error) {
-	return c.shouldHaveImpersonator(config)
+	service, err := c.servicesInformer.Lister().Services(c.namespace).Get(serviceName)
-}
-
-func (c *impersonatorConfigController) serviceExists(serviceName string) (bool, error) {
-	_, err := c.servicesInformer.Lister().Services(c.namespace).Get(serviceName)
 	notFound := k8serrors.IsNotFound(err)
 	if notFound {
-		return false, nil
+		return false, nil, nil
 	}
 	if err != nil {
-		return false, err
+		return false, nil, err
 	}
-	return true, nil
+	return true, service, nil
 }
 
 func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, error) {
@@ -477,7 +476,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 }
 
 func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error {
-	running, err := c.serviceExists(c.generatedLoadBalancerServiceName)
+	running, service, err := c.serviceExists(c.generatedLoadBalancerServiceName)
 	if err != nil {
 		return err
 	}
@@ -488,7 +487,12 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.C
 	c.infoLog.Info("deleting load balancer for impersonation proxy",
 		"service", klog.KRef(c.namespace, c.generatedLoadBalancerServiceName),
 	)
-	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{})
+	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{
+		Preconditions: &metav1.Preconditions{
+			UID:             &service.UID,
+			ResourceVersion: &service.ResourceVersion,
+		},
+	})
 	return utilerrors.FilterOut(err, k8serrors.IsNotFound)
 }
 
@@ -517,7 +521,7 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx conte
 }
 
 func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx context.Context) error {
-	running, err := c.serviceExists(c.generatedClusterIPServiceName)
+	running, service, err := c.serviceExists(c.generatedClusterIPServiceName)
 	if err != nil {
 		return err
 	}
@@ -528,7 +532,12 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx conte
 	c.infoLog.Info("deleting cluster ip for impersonation proxy",
 		"service", klog.KRef(c.namespace, c.generatedClusterIPServiceName),
 	)
-	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedClusterIPServiceName, metav1.DeleteOptions{})
+	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedClusterIPServiceName, metav1.DeleteOptions{
+		Preconditions: &metav1.Preconditions{
+			UID:             &service.UID,
+			ResourceVersion: &service.ResourceVersion,
+		},
+	})
 	return utilerrors.FilterOut(err, k8serrors.IsNotFound)
 }
 
@@ -942,7 +951,6 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 	keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
 
 	if err := c.tlsServingCertDynamicCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
-		c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
 		return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err)
 	}
 
@@ -955,39 +963,33 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 }
 
 func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Context) error {
-	tlsSecretExists, _, err := c.tlsSecretExists()
+	tlsSecretExists, secret, err := c.tlsSecretExists()
 	if err != nil {
 		return err
 	}
 	if !tlsSecretExists {
 		return nil
 	}
-	c.infoLog.Info("deleting TLS certificates for impersonation proxy",
+	c.infoLog.Info("deleting TLS serving certificate for impersonation proxy",
 		"secret", klog.KRef(c.namespace, c.tlsSecretName),
 	)
-	err = c.k8sClient.CoreV1().Secrets(c.namespace).Delete(ctx, c.tlsSecretName, metav1.DeleteOptions{})
+	err = c.k8sClient.CoreV1().Secrets(c.namespace).Delete(ctx, c.tlsSecretName, metav1.DeleteOptions{
-	notFound := k8serrors.IsNotFound(err)
+		Preconditions: &metav1.Preconditions{
-	if notFound {
+			UID:             &secret.UID,
-		// its okay if we tried to delete and we got a not found error. This probably means
+			ResourceVersion: &secret.ResourceVersion,
+		},
+	})
+	// it is okay if we tried to delete and we got a not found error. This probably means
 	// another instance of the concierge got here first so there's nothing to delete.
-		return nil
+	return utilerrors.FilterOut(err, k8serrors.IsNotFound)
-	}
-	if err != nil {
-		return err
-	}
-
-	c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
-
-	return nil
 }
 
-func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStatus) error {
+func (c *impersonatorConfigController) clearTLSSecret() {
-	// Clear it when the impersonator is not completely ready.
+	c.debugLog.Info("clearing TLS serving certificate for impersonation proxy")
-	if status != v1alpha1.SuccessStrategyStatus {
+	c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
-		c.clearSignerCA()
+}
-		return nil
-	}
 
+func (c *impersonatorConfigController) loadSignerCA() error {
 	signingCertSecret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.impersonationSignerSecretName)
 	if err != nil {
 		return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err)
@@ -997,7 +999,7 @@ func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStat
 	keyPEM := signingCertSecret.Data[apicerts.CACertificatePrivateKeySecretKey]
 
 	if err := c.impersonationSigningCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
-		return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err)
+		return fmt.Errorf("could not set the impersonator's credential signing secret: %w", err)
 	}
 
 	c.infoLog.Info("loading credential signing certificate for impersonation proxy",

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -29,11 +29,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	kubeinformers "k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"
 
 	"go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
 	pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake"
@@ -266,6 +269,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 		var subject controllerlib.Controller
 		var kubeAPIClient *kubernetesfake.Clientset
+		var deleteOptions *[]metav1.DeleteOptions
+		var deleteOptionsRecorder kubernetes.Interface
 		var pinnipedAPIClient *pinnipedfake.Clientset
 		var pinnipedInformerClient *pinnipedfake.Clientset
 		var pinnipedInformers pinnipedinformers.SharedInformerFactory
@@ -275,6 +280,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 		var cancelContextCancelFunc context.CancelFunc
 		var syncContext *controllerlib.Context
 		var frozenNow time.Time
+		var tlsServingCertDynamicCertProvider dynamiccert.Private
 		var signingCertProvider dynamiccert.Provider
 		var signingCACertPEM, signingCAKeyPEM []byte
 		var signingCASecret *corev1.Secret
@@ -418,6 +424,20 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}
 		}
 
+		var requireTLSSecretProviderHasLoadedCerts = func() {
+			actualCert, actualKey := tlsServingCertDynamicCertProvider.CurrentCertKeyContent()
+			r.NotEmpty(actualCert)
+			r.NotEmpty(actualKey)
+			_, err := tls.X509KeyPair(actualCert, actualKey)
+			r.NoError(err)
+		}
+
+		var requireTLSSecretProviderIsEmpty = func() {
+			actualCert, actualKey := tlsServingCertDynamicCertProvider.CurrentCertKeyContent()
+			r.Nil(actualCert)
+			r.Nil(actualKey)
+		}
+
 		var requireTLSServerIsRunning = func(caCrt []byte, addr string, dnsOverrides map[string]string) {
 			r.Greater(impersonatorFuncWasCalled, 0)
 
@@ -469,6 +489,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.NoError(resp.Body.Close())
 			r.NoError(err)
 			r.Equal(fakeServerResponseBody, string(body))
+
+			requireTLSSecretProviderHasLoadedCerts()
 		}
 
 		var requireTLSServerIsRunningWithoutCerts = func() {
@@ -490,6 +512,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}, 20*time.Second, 50*time.Millisecond)
 			r.Error(err)
 			r.Regexp(expectedErrorRegex, err.Error())
+
+			requireTLSSecretProviderIsEmpty()
 		}
 
 		var requireTLSServerIsNoLongerRunning = func() {
@@ -508,10 +532,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}, 20*time.Second, 50*time.Millisecond)
 			r.Error(err)
 			r.Regexp(expectedErrorRegex, err.Error())
+
+			requireTLSSecretProviderIsEmpty()
 		}
 
 		var requireTLSServerWasNeverStarted = func() {
 			r.Equal(0, impersonatorFuncWasCalled)
+
+			requireTLSSecretProviderIsEmpty()
 		}
 
 		// Defer starting the informers until the last possible moment so that the
@@ -521,7 +549,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			subject = NewImpersonatorConfigController(
 				installedInNamespace,
 				credentialIssuerResourceName,
-				kubeAPIClient,
+				deleteOptionsRecorder,
 				pinnipedAPIClient,
 				pinnipedInformers.Config().V1alpha1().CredentialIssuers(),
 				kubeInformers.Core().V1().Services(),
@@ -538,6 +566,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				signingCertProvider,
 				testLog,
 			)
+			controllerlib.TestWrap(t, subject, func(syncer controllerlib.Syncer) controllerlib.Syncer {
+				tlsServingCertDynamicCertProvider = syncer.(*impersonatorConfigController).tlsServingCertDynamicCertProvider
+				return syncer
+			})
 
 			// Set this at the last second to support calling subject.Name().
 			syncContext = &controllerlib.Context{
@@ -566,6 +598,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name:            resourceName,
 					Namespace:       installedInNamespace,
+					UID:             "uid-1234", // simulate KAS filling out UID and RV
+					ResourceVersion: "rv-5678",
 				},
 				Data: data,
 			}
@@ -705,6 +739,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 		var addObjectFromCreateActionToInformerAndWait = func(action coretesting.Action, informer controllerlib.InformerGetter) {
 			createdObject, ok := action.(coretesting.CreateAction).GetObject().(kubeclient.Object)
 			r.True(ok, "should have been able to cast this action's object to kubeclient.Object: %v", action)
+
+			if secret, ok := createdObject.(*corev1.Secret); ok && len(secret.ResourceVersion) == 0 {
+				secret = secret.DeepCopy()
+				secret.UID = "uid-1234" // simulate KAS filling out UID and RV
+				secret.ResourceVersion = "rv-5678"
+				createdObject = secret
+			}
+
 			addObjectToKubeInformerAndWait(createdObject, informer)
 		}
 
@@ -986,6 +1028,18 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal("delete", deleteAction.GetVerb())
 			r.Equal(tlsSecretName, deleteAction.GetName())
 			r.Equal("secrets", deleteAction.GetResource().Resource)
+
+			// validate that we set delete preconditions correctly
+			r.NotEmpty(*deleteOptions)
+			for _, opt := range *deleteOptions {
+				uid := types.UID("uid-1234")
+				r.Equal(metav1.DeleteOptions{
+					Preconditions: &metav1.Preconditions{
+						UID:             &uid,
+						ResourceVersion: pointer.String("rv-5678"),
+					},
+				}, opt)
+			}
 		}
 
 		var requireCASecretWasCreated = func(action coretesting.Action) []byte {
@@ -1064,6 +1118,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				kubeinformers.WithNamespace(installedInNamespace),
 			)
 			kubeAPIClient = kubernetesfake.NewSimpleClientset()
+			deleteOptions = &[]metav1.DeleteOptions{}
+			deleteOptionsRecorder = testutil.NewDeleteOptionsRecorder(kubeAPIClient, deleteOptions)
 			pinnipedAPIClient = pinnipedfake.NewSimpleClientset()
 			frozenNow = time.Date(2021, time.March, 2, 7, 42, 0, 0, time.Local)
 			signingCertProvider = dynamiccert.NewCA(name)
@@ -1222,7 +1278,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 
@@ -1241,7 +1297,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireNodesListed(kubeAPIClient.Actions()[0])
 					requireCASecretWasCreated(kubeAPIClient.Actions()[1])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 
@@ -1260,7 +1316,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireNodesListed(kubeAPIClient.Actions()[0])
 					requireCASecretWasCreated(kubeAPIClient.Actions()[1])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 
@@ -1452,9 +1508,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					errString := "could not find valid IP addresses or hostnames from load balancer some-namespace/some-service-resource-name"
 					r.EqualError(runControllerSync(), errString)
 					r.Len(kubeAPIClient.Actions(), 1)                       // no new actions
-					requireTLSServerIsRunningWithoutCerts()
+					requireTLSServerIsRunning(caCrt, testServerAddr(), nil) // serving certificate is not unloaded in this case
 					requireCredentialIssuer(newErrorStrategy(errString))
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 		})
@@ -1510,7 +1566,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireTLSServerIsRunningWithoutCerts()
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 
 				it("returns an error when the impersonation TLS server fails to start", func() {
@@ -1545,7 +1601,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireCASecretWasCreated(kubeAPIClient.Actions()[1])
 					requireTLSServerIsRunningWithoutCerts()
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 
 				it("returns an error when the impersonation TLS server fails to start", func() {
@@ -1685,7 +1741,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireTLSServerIsRunningWithoutCerts()
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 
@@ -1780,7 +1836,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					// Check that the server is running without certs.
 					requireTLSServerIsRunningWithoutCerts()
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 
@@ -2129,7 +2185,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 						r.Len(kubeAPIClient.Actions(), 4)
 						requireTLSSecretWasDeleted(kubeAPIClient.Actions()[3]) // tried to delete cert but failed
 						requireCredentialIssuer(newErrorStrategy("error on tls secret delete"))
-						requireSigningCertProviderIsEmpty()
+						requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 					})
 				})
 			})
@@ -2160,7 +2216,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // load when enabled
 
 					// Simulate the informer cache's background update from its watch.
 					addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2178,7 +2234,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 4)
 					requireServiceWasDeleted(kubeAPIClient.Actions()[3], loadBalancerServiceName)
 					requireCredentialIssuer(newManuallyDisabledStrategy())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderIsEmpty() // only unload when disabled
 
 					deleteServiceFromTracker(loadBalancerServiceName, kubeInformerClient)
 					waitForObjectToBeDeletedFromInformer(loadBalancerServiceName, kubeInformers.Core().V1().Services())
@@ -2195,7 +2251,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 5)
 					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[4])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // load again when enabled
 				})
 			})
 
@@ -2226,7 +2282,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireClusterIPWasCreated(kubeAPIClient.Actions()[1])
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // load when enabled
 
 					// Simulate the informer cache's background update from its watch.
 					addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2244,7 +2300,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 4)
 					requireServiceWasDeleted(kubeAPIClient.Actions()[3], clusterIPServiceName)
 					requireCredentialIssuer(newManuallyDisabledStrategy())
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderIsEmpty() // only unload when disabled
 
 					deleteServiceFromTracker(clusterIPServiceName, kubeInformerClient)
 					waitForObjectToBeDeletedFromInformer(clusterIPServiceName, kubeInformers.Core().V1().Services())
@@ -2264,7 +2320,88 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 5)
 					requireClusterIPWasCreated(kubeAPIClient.Actions()[4])
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // load again when enabled
+				})
+			})
+
+			when("service type none with a hostname", func() {
+				const fakeHostname = "hello.com"
+				it.Before(func() {
+					addSecretToTrackers(signingCASecret, kubeInformerClient)
+					addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
+						ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
+						Spec: v1alpha1.CredentialIssuerSpec{
+							ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+								Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+								ExternalEndpoint: fakeHostname,
+								Service: v1alpha1.ImpersonationProxyServiceSpec{
+									Type: v1alpha1.ImpersonationProxyServiceTypeNone,
+								},
+							},
+						},
+					}, pinnipedInformerClient, pinnipedAPIClient)
+					addNodeWithRoleToTracker("worker", kubeAPIClient)
+				})
+
+				it("starts the impersonator, then shuts it down, then starts it again", func() {
+					startInformersAndController()
+
+					r.NoError(runControllerSync())
+					r.Len(kubeAPIClient.Actions(), 3)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					ca := requireCASecretWasCreated(kubeAPIClient.Actions()[1])
+					requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca)
+					requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()})
+					requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca))
+
+					// load when enabled
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+					requireTLSSecretProviderHasLoadedCerts()
+
+					// Simulate the informer cache's background update from its watch.
+					addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets())
+					addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets())
+
+					// Update the CredentialIssuer.
+					updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+						ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+							Mode: v1alpha1.ImpersonationProxyModeDisabled,
+						},
+					}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
+
+					r.NoError(runControllerSync())
+					requireTLSServerIsNoLongerRunning()
+					r.Len(kubeAPIClient.Actions(), 4)
+					requireTLSSecretWasDeleted(kubeAPIClient.Actions()[3])
+					requireCredentialIssuer(newManuallyDisabledStrategy())
+
+					// only unload when disabled
 					requireSigningCertProviderIsEmpty()
+					requireTLSSecretProviderIsEmpty()
+
+					deleteSecretFromTracker(tlsSecretName, kubeInformerClient)
+					waitForObjectToBeDeletedFromInformer(tlsSecretName, kubeInformers.Core().V1().Secrets())
+
+					// Update the CredentialIssuer again.
+					updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+						ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+							Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+							ExternalEndpoint: fakeHostname,
+							Service: v1alpha1.ImpersonationProxyServiceSpec{
+								Type: v1alpha1.ImpersonationProxyServiceTypeNone,
+							},
+						},
+					}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
+
+					r.NoError(runControllerSync())
+					requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()})
+					r.Len(kubeAPIClient.Actions(), 5)
+					requireTLSSecretWasCreated(kubeAPIClient.Actions()[4], ca)
+					requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca))
+
+					// load again when enabled
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+					requireTLSSecretProviderHasLoadedCerts()
 				})
 			})
 		})
@@ -2315,9 +2452,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 5)
 				requireLoadBalancerWasCreated(kubeAPIClient.Actions()[3])
 				requireTLSSecretWasDeleted(kubeAPIClient.Actions()[4]) // the Secret was deleted because it contained a cert with the wrong IP
-				requireTLSServerIsRunningWithoutCerts()
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)   // serving certificate is not unloaded in this case
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Services())
@@ -2327,9 +2464,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				// The controller should be waiting for the load balancer's ingress to become available.
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 5)                    // no new actions while it is waiting for the load balancer's ingress
-				requireTLSServerIsRunningWithoutCerts()
+				requireTLSServerIsRunning(ca, testServerAddr(), nil) // serving certificate is not unloaded in this case
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Update the ingress of the LB in the informer's client and run Sync again.
 				fakeIP := "127.0.0.123"
@@ -2767,7 +2904,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSServerIsRunningWithoutCerts()
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2777,7 +2914,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Equal(1, impersonatorFuncWasCalled)   // wasn't started a second time
 				requireTLSServerIsRunningWithoutCerts() // still running
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				r.Len(kubeAPIClient.Actions(), 3) // no new API calls
 			})
 
@@ -2790,7 +2927,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSServerIsRunningWithoutCerts()
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2827,7 +2964,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSServerIsRunningWithoutCerts()
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2918,6 +3055,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 		when("the impersonator start function returned by the impersonatorFunc returns an error immediately", func() {
 			it.Before(func() {
+				addSecretToTrackers(signingCASecret, kubeInformerClient)
 				addNodeWithRoleToTracker("worker", kubeAPIClient)
 				impersonatorFuncReturnedFuncError = errors.New("some immediate impersonator startup error")
 				addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
@@ -2948,7 +3086,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
 				requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
@@ -2966,7 +3104,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				// sync should be able to detect the error and return it.
 				r.EqualError(runControllerSync(), "some immediate impersonator startup error")
 				requireCredentialIssuer(newErrorStrategy("some immediate impersonator startup error"))
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Next time the controller starts the server, the server will start successfully.
 				impersonatorFuncReturnedFuncError = nil
@@ -2976,12 +3114,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				requireTLSServerIsRunningWithoutCerts()
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 			})
 		})
 
 		when("the impersonator server dies for no apparent reason after running for a while", func() {
 			it.Before(func() {
+				addSecretToTrackers(signingCASecret, kubeInformerClient)
 				addNodeWithRoleToTracker("worker", kubeAPIClient)
 				addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
 					ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
@@ -3004,7 +3143,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
 				requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				requireTLSServerIsRunningWithoutCerts()
 
 				// Simulate the informer cache's background update from its watch.
@@ -3026,7 +3165,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				// sync should be able to detect the error and return it.
 				r.EqualError(runControllerSync(), "unexpected shutdown of proxy server")
 				requireCredentialIssuer(newErrorStrategy("unexpected shutdown of proxy server"))
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
 				// Next time the controller starts the server, the server should behave as normal.
 				testHTTPServerInterruptCh = nil
@@ -3036,7 +3175,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				requireTLSServerIsRunningWithoutCerts()
 				requireCredentialIssuer(newPendingStrategyWaitingForLB())
-				requireSigningCertProviderIsEmpty()
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 			})
 		})
 
@@ -3471,16 +3610,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					},
 				}, pinnipedInformerClient, pinnipedAPIClient)
 				addNodeWithRoleToTracker("worker", kubeAPIClient)
-				tlsSecret := &corev1.Secret{
+				tlsSecret := newSecretWithData(tlsSecretName, map[string][]byte{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      tlsSecretName,
-						Namespace: installedInNamespace,
-					},
-					Data: map[string][]byte{
 					// "aGVsbG8gd29ybGQK" is "hello world" base64 encoded which is not a valid cert
 					corev1.TLSCertKey: []byte("-----BEGIN CERTIFICATE-----\naGVsbG8gd29ybGQK\n-----END CERTIFICATE-----\n"),
-					},
+				})
-				}
 				addSecretToTrackers(tlsSecret, kubeAPIClient, kubeInformerClient)
 			})
 
@@ -3705,7 +3838,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 				it("returns the error", func() {
 					startInformersAndController()
-					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
+					errString := `could not set the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
 					requireSigningCertProviderIsEmpty()
@@ -3720,7 +3853,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 				it("returns the error", func() {
 					startInformersAndController()
-					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
+					errString := `could not set the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
 					requireSigningCertProviderIsEmpty()
@@ -3756,10 +3889,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					addSecretToTrackers(updatedSigner, kubeInformerClient)
 					waitForObjectToAppearInInformer(updatedSigner, kubeInformers.Core().V1().Secrets())
 
-					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
+					errString := `could not set the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
-					requireSigningCertProviderIsEmpty()
+					requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 				})
 			})
 		})

internal/controller/kubecertagent/kubecertagent_test.go

@@ -961,7 +961,6 @@ func runControllerUntilQuiet(ctx context.Context, t *testing.T, controller contr
 
 	errorStream := make(chan error)
 	controllerlib.TestWrap(t, controller, func(syncer controllerlib.Syncer) controllerlib.Syncer {
-		controller.Name()
 		return controllerlib.SyncFunc(func(ctx controllerlib.Context) error {
 			err := syncer.Sync(ctx)
 			errorStream <- err

internal/testutil/delete.go

@@ -0,0 +1,47 @@
+// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package testutil
+
+import (
+	"context"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
+)
+
+func NewDeleteOptionsRecorder(client kubernetes.Interface, opts *[]metav1.DeleteOptions) kubernetes.Interface {
+	return &clientWrapper{
+		Interface: client,
+		opts:      opts,
+	}
+}
+
+type clientWrapper struct {
+	kubernetes.Interface
+	opts *[]metav1.DeleteOptions
+}
+
+func (c *clientWrapper) CoreV1() corev1client.CoreV1Interface {
+	return &coreWrapper{CoreV1Interface: c.Interface.CoreV1(), opts: c.opts}
+}
+
+type coreWrapper struct {
+	corev1client.CoreV1Interface
+	opts *[]metav1.DeleteOptions
+}
+
+func (c *coreWrapper) Secrets(namespace string) corev1client.SecretInterface {
+	return &secretsWrapper{SecretInterface: c.CoreV1Interface.Secrets(namespace), opts: c.opts}
+}
+
+type secretsWrapper struct {
+	corev1client.SecretInterface
+	opts *[]metav1.DeleteOptions
+}
+
+func (s *secretsWrapper) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error {
+	*s.opts = append(*s.opts, opts)
+	return s.SecretInterface.Delete(ctx, name, opts)
+}

test/integration/concierge_impersonation_proxy_test.go

@@ -197,7 +197,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		// We do this to ensure that future tests that use the impersonation proxy (e.g.,
 		// TestE2EFullIntegration) will start with a known-good state.
 		if clusterSupportsLoadBalancers {
-			performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)
+			performImpersonatorDiscovery(ctx, t, env, adminClient, adminConciergeClient, refreshCredential)
 		}
 	})
 
@@ -277,7 +277,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	// to discover the impersonator's URL and CA certificate. Until it has finished starting, it may not be included
 	// in the strategies array or it may be included in an error state. It can be in an error state for
 	// awhile when it is waiting for the load balancer to be assigned an ip/hostname.
-	impersonationProxyURL, impersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)
+	impersonationProxyURL, impersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminClient, adminConciergeClient, refreshCredential)
 	if !clusterSupportsLoadBalancers {
 		// In this case, we specified the endpoint in the configmap, so check that it was reported correctly in the CredentialIssuer.
 		require.Equal(t, "https://"+proxyServiceEndpoint, impersonationProxyURL)
@@ -1422,7 +1422,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 					require.Equal(t, &corev1.Pod{}, pod)
 				})
 
-				// - request to whoami (pinniped resource endpoing)
+				// - request to whoami (pinniped resource endpoint)
 				//   - through the impersonation proxy
 				//   - should succeed 200
 				//   - should respond "you are system:anonymous"
@@ -1733,10 +1733,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 		// wait until the credential issuer is updated with the new url
 		testlib.RequireEventuallyWithoutError(t, func() (bool, error) {
-			newImpersonationProxyURL, _ := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)
+			newImpersonationProxyURL, _ := performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)
 			return newImpersonationProxyURL == "https://"+clusterIPServiceURL, nil
 		}, 30*time.Second, 500*time.Millisecond)
-		newImpersonationProxyURL, newImpersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)
+		newImpersonationProxyURL, newImpersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminClient, adminConciergeClient, refreshCredential)
 
 		anonymousClient := newAnonymousImpersonationProxyClientWithProxy(t, newImpersonationProxyURL, newImpersonationProxyCACertPEM, nil).PinnipedConcierge
 		refreshedCredentials := refreshCredentialHelper(t, anonymousClient)
@@ -1941,7 +1941,64 @@ func expectedWhoAmIRequestResponse(username string, groups []string, extra map[s
 	}
 }
 
-func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *testlib.TestEnv, adminConciergeClient pinnipedconciergeclientset.Interface) (string, []byte) {
+func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *testlib.TestEnv,
+	adminClient kubernetes.Interface, adminConciergeClient pinnipedconciergeclientset.Interface,
+	refreshCredential func(t *testing.T, impersonationProxyURL string, impersonationProxyCACertPEM []byte) *loginv1alpha1.ClusterCredential) (string, []byte) {
+	t.Helper()
+
+	impersonationProxyURL, impersonationProxyCACertPEM := performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)
+
+	if len(env.Proxy) == 0 {
+		t.Log("no test proxy is available, skipping readiness checks for concierge impersonation proxy pods")
+		return impersonationProxyURL, impersonationProxyCACertPEM
+	}
+
+	impersonationProxyParsedURL, err := url.Parse(impersonationProxyURL)
+	require.NoError(t, err)
+
+	expectedGroups := make([]string, 0, len(env.TestUser.ExpectedGroups)+1) // make sure we do not mutate env.TestUser.ExpectedGroups
+	expectedGroups = append(expectedGroups, env.TestUser.ExpectedGroups...)
+	expectedGroups = append(expectedGroups, "system:authenticated")
+
+	// probe each pod directly for readiness since the concierge status is a lie - it just means a single pod is ready
+	testlib.RequireEventually(t, func(requireEventually *require.Assertions) {
+		pods, err := adminClient.CoreV1().Pods(env.ConciergeNamespace).List(ctx,
+			metav1.ListOptions{LabelSelector: "app=" + env.ConciergeAppName + ",!kube-cert-agent.pinniped.dev"}) // TODO replace with deployment.pinniped.dev=concierge
+		requireEventually.NoError(err)
+		requireEventually.Len(pods.Items, 2) // has to stay in sync with the defaults in our YAML
+
+		for _, pod := range pods.Items {
+			t.Logf("checking if concierge impersonation proxy pod %q is ready", pod.Name)
+
+			requireEventually.NotEmptyf(pod.Status.PodIP, "pod %q does not have an IP", pod.Name)
+
+			credentials := refreshCredential(t, impersonationProxyURL, impersonationProxyCACertPEM).DeepCopy()
+			credentials.Token = "not a valid token" // demonstrates that client certs take precedence over tokens by setting both on the requests
+
+			config := newImpersonationProxyConfigWithCredentials(t, credentials, impersonationProxyURL, impersonationProxyCACertPEM, nil)
+			config = rest.CopyConfig(config)
+			config.Proxy = kubeconfigProxyFunc(t, env.Proxy)                           // always use the proxy since we are talking directly to a pod IP
+			config.Host = "https://" + pod.Status.PodIP + ":8444"                      // hardcode the internal port - it should not change
+			config.TLSClientConfig.ServerName = impersonationProxyParsedURL.Hostname() // make SNI hostname TLS verification work even when using IP
+
+			whoAmI, err := testlib.NewKubeclient(t, config).PinnipedConcierge.IdentityV1alpha1().WhoAmIRequests().
+				Create(ctx, &identityv1alpha1.WhoAmIRequest{}, metav1.CreateOptions{})
+			requireEventually.NoError(err)
+			requireEventually.Equal(
+				expectedWhoAmIRequestResponse(
+					env.TestUser.ExpectedUsername,
+					expectedGroups,
+					nil,
+				),
+				whoAmI,
+			)
+		}
+	}, 10*time.Minute, 10*time.Second)
+
+	return impersonationProxyURL, impersonationProxyCACertPEM
+}
+
+func performImpersonatorDiscoveryURL(ctx context.Context, t *testing.T, env *testlib.TestEnv, adminConciergeClient pinnipedconciergeclientset.Interface) (string, []byte) {
 	t.Helper()
 
 	var impersonationProxyURL string