impersonatorconfig: only unload dynamiccert when proxy is disabled

In the upstream dynamiccertificates package, we rely on two pieces
of code:

1. DynamicServingCertificateController.newTLSContent which calls
   - clientCA.CurrentCABundleContent
   - servingCert.CurrentCertKeyContent
2. unionCAContent.VerifyOptions which calls
   - unionCAContent.CurrentCABundleContent

This results in calls to our tlsServingCertDynamicCertProvider and
impersonationSigningCertProvider.  If we Unset these providers, we
subtly break these consumers.  At best this results in test slowness
and flakes while we wait for reconcile loops to converge.  At worst,
it results in actual errors during runtime.  For example, we
previously would Unset the impersonationSigningCertProvider on any
sync loop error (even a transient one caused by a network blip or
a conflict between writes from different replicas of the concierge).
This would cause us to transiently fail to issue new certificates
from the token credential require API.  It would also cause us to
transiently fail to authenticate previously issued client certs
(which results in occasional Unauthorized errors in CI).

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

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,27 +279,32 @@ 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 {
-		return nil, err
+		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) {
-		return nil, err
+		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,
-		// another instance of the concierge got here first so there's nothing to delete.
+		},
-		return nil
+	})
-	}
+	// it is okay if we tried to delete and we got a not found error. This probably means
-	if err != nil {
+	// another instance of the concierge got here first so there's nothing to delete.
-		return err
+	return utilerrors.FilterOut(err, k8serrors.IsNotFound)
-	}
-
-	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{
@@ -564,8 +596,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 		var newSecretWithData = func(resourceName string, data map[string][]byte) *corev1.Secret {
 			return &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      resourceName,
+					Name:            resourceName,
-					Namespace: installedInNamespace,
+					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)
 				})
 			})
 
@@ -1451,10 +1507,10 @@ 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
+					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())
@@ -2326,10 +2463,10 @@ 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
+				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{
+					// "aGVsbG8gd29ybGQK" is "hello world" base64 encoded which is not a valid cert
-						Name:      tlsSecretName,
+					corev1.TLSCertKey: []byte("-----BEGIN CERTIFICATE-----\naGVsbG8gd29ybGQK\n-----END CERTIFICATE-----\n"),
-						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