From 5e95c25d4fd3da2b4afd09c371aed61b9789f062 Mon Sep 17 00:00:00 2001
From: Matt Moyer <moyerm@vmware.com>
Date: Mon, 22 Mar 2021 11:33:02 -0500
Subject: [PATCH 01/13] Tweak some assertions in TestSupervisorOIDCDiscovery.

We've seen some test flakes caused by this test. Some small changes:

- Use a 30s timeout for each iteration of the test loop (so each iteration needs to check or fail more quickly).
- Log a bit more during the checks so we can diagnose what's going on.
- Increase the overall timeout from one minute to five minutes

Signed-off-by: Matt Moyer <moyerm@vmware.com>
---
 test/integration/supervisor_discovery_test.go | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go
index c164cb7b..d3b445e5 100644
--- a/test/integration/supervisor_discovery_test.go
+++ b/test/integration/supervisor_discovery_test.go
@@ -602,18 +602,25 @@ func requireDelete(t *testing.T, client pinnipedclientset.Interface, ns, name st
 
 func requireStatus(t *testing.T, client pinnipedclientset.Interface, ns, name string, status v1alpha1.FederationDomainStatusCondition) {
 	t.Helper()
-	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
-	defer cancel()
 
 	var federationDomain *v1alpha1.FederationDomain
 	var err error
 	assert.Eventually(t, func() bool {
+		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		defer cancel()
+
 		federationDomain, err = client.ConfigV1alpha1().FederationDomains(ns).Get(ctx, name, metav1.GetOptions{})
 		if err != nil {
-			t.Logf("error trying to get FederationDomain: %s", err.Error())
+			t.Logf("error trying to get FederationDomain %s/%s: %v", ns, name, err)
+			return false
 		}
-		return err == nil && federationDomain.Status.Status == status
-	}, time.Minute, 200*time.Millisecond)
+
+		if federationDomain.Status.Status != status {
+			t.Logf("found FederationDomain %s/%s with status %s", ns, name, federationDomain.Status.Status)
+			return false
+		}
+		return true
+	}, 5*time.Minute, 200*time.Millisecond)
 	require.NoError(t, err)
 	require.Equalf(t, status, federationDomain.Status.Status, "unexpected status (message = '%s')", federationDomain.Status.Message)
 }

From cd6e48bfa8476a8ded74eec53bca821c4b7d626c Mon Sep 17 00:00:00 2001
From: Margo Crawford <margaretc@vmware.com>
Date: Thu, 25 Mar 2021 15:12:17 -0700
Subject: [PATCH 02/13] Use a random password for the dex integration test user

Signed-off-by: Ryan Richard <richardry@vmware.com>
---
 hack/prepare-for-integration-tests.sh | 6 ++++--
 test/deploy/dex/dex.yaml              | 2 +-
 test/deploy/dex/values.yaml           | 3 +++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh
index e317c8f9..6c6d522d 100755
--- a/hack/prepare-for-integration-tests.sh
+++ b/hack/prepare-for-integration-tests.sh
@@ -210,12 +210,14 @@ if ! tilt_mode; then
   #
   # Deploy dex
   #
+  dex_test_password="$(openssl rand -hex 16)"
   pushd test/deploy/dex >/dev/null
 
   log_note "Deploying Dex to the cluster..."
   ytt --file . >"$manifest"
   ytt --file . \
     --data-value-yaml "supervisor_redirect_uris=[https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/callback]" \
+    --data-value "pinny_bcrypt_passwd_hash=$(htpasswd -nbBC 10 x "$dex_test_password" | sed -e "s/^x://")" \
     >"$manifest"
 
   kubectl apply --dry-run=client -f "$manifest" # Validate manifest schema.
@@ -328,7 +330,7 @@ export PINNIPED_TEST_CLI_OIDC_ISSUER_CA_BUNDLE="${test_ca_bundle_pem}"
 export PINNIPED_TEST_CLI_OIDC_CLIENT_ID=pinniped-cli
 export PINNIPED_TEST_CLI_OIDC_CALLBACK_URL=http://127.0.0.1:48095/callback
 export PINNIPED_TEST_CLI_OIDC_USERNAME=pinny@example.com
-export PINNIPED_TEST_CLI_OIDC_PASSWORD=password
+export PINNIPED_TEST_CLI_OIDC_PASSWORD=${dex_test_password}
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER=https://dex.dex.svc.cluster.local/dex
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER_CA_BUNDLE="${test_ca_bundle_pem}"
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ADDITIONAL_SCOPES=email
@@ -338,7 +340,7 @@ export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_ID=pinniped-supervisor
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_SECRET=pinniped-supervisor-secret
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CALLBACK_URL=https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/callback
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME=pinny@example.com
-export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_PASSWORD=password
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_PASSWORD=${dex_test_password}
 export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_EXPECTED_GROUPS= # Dex's local user store does not let us configure groups.
 export PINNIPED_TEST_API_GROUP_SUFFIX='${api_group_suffix}'
 
diff --git a/test/deploy/dex/dex.yaml b/test/deploy/dex/dex.yaml
index 624e14c9..5ca41cb7 100644
--- a/test/deploy/dex/dex.yaml
+++ b/test/deploy/dex/dex.yaml
@@ -32,7 +32,7 @@ enablePasswordDB: true
 staticPasswords:
 - username: "pinny"
   email: "pinny@example.com"
-  hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W" #! bcrypt("password")
+  hash: #@ data.values.pinny_bcrypt_passwd_hash
   userID: "061d23d1-fe1e-4777-9ae9-59cd12abeaaa"
 #@ end
 
diff --git a/test/deploy/dex/values.yaml b/test/deploy/dex/values.yaml
index 56e72c36..1f15c85e 100644
--- a/test/deploy/dex/values.yaml
+++ b/test/deploy/dex/values.yaml
@@ -19,3 +19,6 @@ ports:
 #! supervisor_redirect_uris is an array of redirect uris that dex will use for its pinniped-supervisor client.
 #! usage: --data-value-yaml "supervisor_redirect_uris=[some-redirect.com,some-other-redirect.com]"
 supervisor_redirect_uris: []
+
+#! The bcrypt-hashed password of the pinny test user account.
+pinny_bcrypt_passwd_hash:

From 6f2882b8318632d224333ece66a0047e6974edd5 Mon Sep 17 00:00:00 2001
From: Margo Crawford <margaretc@vmware.com>
Date: Thu, 25 Mar 2021 16:57:37 -0700
Subject: [PATCH 03/13] Explicitly set the correct authenticator for
 impersonator test

Signed-off-by: Ryan Richard <richardry@vmware.com>
---
 test/integration/concierge_impersonation_proxy_test.go | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index 7f196484..712d5e34 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -275,7 +275,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		}
 
 		t.Run("kubectl port-forward and keeping the connection open for over a minute", func(t *testing.T) {
-			kubeconfigPath, envVarsWithProxy, _ := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM)
+			kubeconfigPath, envVarsWithProxy, _ := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM, credentialRequestSpecWithWorkingCredentials.Authenticator)
 
 			// Run the kubectl port-forward command.
 			timeout, cancelFunc := context.WithTimeout(ctx, 2*time.Minute)
@@ -546,7 +546,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		})
 
 		t.Run("kubectl as a client", func(t *testing.T) {
-			kubeconfigPath, envVarsWithProxy, tempDir := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM)
+			kubeconfigPath, envVarsWithProxy, tempDir := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM, credentialRequestSpecWithWorkingCredentials.Authenticator)
 
 			// Try "kubectl exec" through the impersonation proxy.
 			echoString := "hello world"
@@ -1084,7 +1084,7 @@ func credentialIssuerName(env *library.TestEnv) string {
 	return env.ConciergeAppName + "-config"
 }
 
-func getImpersonationKubeconfig(t *testing.T, env *library.TestEnv, impersonationProxyURL string, impersonationProxyCACertPEM []byte) (string, []string, string) {
+func getImpersonationKubeconfig(t *testing.T, env *library.TestEnv, impersonationProxyURL string, impersonationProxyCACertPEM []byte, authenticator corev1.TypedLocalObjectReference) (string, []string, string) {
 	t.Helper()
 
 	pinnipedExe := library.PinnipedCLIPath(t)
@@ -1101,6 +1101,8 @@ func getImpersonationKubeconfig(t *testing.T, env *library.TestEnv, impersonatio
 		"--concierge-api-group-suffix", env.APIGroupSuffix,
 		"--oidc-skip-browser",
 		"--static-token", env.TestUser.Token,
+		"--concierge-authenticator-name", authenticator.Name,
+		"--concierge-authenticator-type", authenticator.Kind,
 		// Force the use of impersonation proxy strategy, but let it auto-discover the endpoint and CA.
 		"--concierge-mode", "ImpersonationProxy"}
 	t.Log("Running:", pinnipedExe, getKubeConfigCmd)

From b6e217e13ad582ea894e9052a60e788048dae8d6 Mon Sep 17 00:00:00 2001
From: Margo Crawford <margaretc@vmware.com>
Date: Thu, 25 Mar 2021 17:19:47 -0700
Subject: [PATCH 04/13] Hardcode type "webhook" in
 concierge_impersonation_proxy_test.go

Signed-off-by: Ryan Richard <richardry@vmware.com>
---
 test/integration/concierge_impersonation_proxy_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index 712d5e34..d95431ab 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -1102,7 +1102,7 @@ func getImpersonationKubeconfig(t *testing.T, env *library.TestEnv, impersonatio
 		"--oidc-skip-browser",
 		"--static-token", env.TestUser.Token,
 		"--concierge-authenticator-name", authenticator.Name,
-		"--concierge-authenticator-type", authenticator.Kind,
+		"--concierge-authenticator-type", "webhook",
 		// Force the use of impersonation proxy strategy, but let it auto-discover the endpoint and CA.
 		"--concierge-mode", "ImpersonationProxy"}
 	t.Log("Running:", pinnipedExe, getKubeConfigCmd)

From 2179c2879ab2ddf8a28eda2c8a7bdec89e04c564 Mon Sep 17 00:00:00 2001
From: Monis Khan <i@monis.app>
Date: Thu, 25 Mar 2021 17:09:29 -0400
Subject: [PATCH 05/13] impersonation proxy: add RBAC to impersonate user extra
 and SAs

Signed-off-by: Monis Khan <mok@vmware.com>
---
 deploy/concierge/rbac.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/deploy/concierge/rbac.yaml b/deploy/concierge/rbac.yaml
index 3f881731..6370d380 100644
--- a/deploy/concierge/rbac.yaml
+++ b/deploy/concierge/rbac.yaml
@@ -32,7 +32,10 @@ rules:
     verbs: [ use ]
     resourceNames: [ nonroot ]
   - apiGroups: [ "" ]
-    resources: [ "users", "groups" ]
+    resources: [ "users", "groups", "serviceaccounts" ]
+    verbs: [ "impersonate" ]
+  - apiGroups: [ "authentication.k8s.io" ]
+    resources: [ "*" ]  #! What we really want is userextras/* but the RBAC authorizer only supports */subresource, not resource/*
     verbs: [ "impersonate" ]
   - apiGroups: [ "" ]
     resources: [ nodes ]

From a084544f08aa36d9c3209801a6fc4770fd5e4849 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Fri, 26 Mar 2021 08:02:45 -0700
Subject: [PATCH 06/13] Add hasExternalLoadBalancerProvider to AKS/EKS
 capabilities files

---
 test/cluster_capabilities/aks.yaml | 3 +++
 test/cluster_capabilities/eks.yaml | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/test/cluster_capabilities/aks.yaml b/test/cluster_capabilities/aks.yaml
index cf8a1186..cc3f68ed 100644
--- a/test/cluster_capabilities/aks.yaml
+++ b/test/cluster_capabilities/aks.yaml
@@ -7,6 +7,9 @@ capabilities:
   # Is it possible to borrow the cluster's signing key from the kube API server?
   clusterSigningKeyIsAvailable: false
 
+  # Will the cluster successfully provision a load balancer if requested?
+  hasExternalLoadBalancerProvider: true
+
   # Does the cluster allow requests without authentication?
   # https://kubernetes.io/docs/reference/access-authn-authz/authentication/#anonymous-requests
   anonymousAuthenticationSupported: false
diff --git a/test/cluster_capabilities/eks.yaml b/test/cluster_capabilities/eks.yaml
index 67fa6bbc..6d545f2f 100644
--- a/test/cluster_capabilities/eks.yaml
+++ b/test/cluster_capabilities/eks.yaml
@@ -7,6 +7,9 @@ capabilities:
   # Is it possible to borrow the cluster's signing key from the kube API server?
   clusterSigningKeyIsAvailable: false
 
+  # Will the cluster successfully provision a load balancer if requested?
+  hasExternalLoadBalancerProvider: true
+
   # Does the cluster allow requests without authentication?
   # https://kubernetes.io/docs/reference/access-authn-authz/authentication/#anonymous-requests
   anonymousAuthenticationSupported: true

From 7e166191468483ef7392451479769f560da87218 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Fri, 26 Mar 2021 09:28:42 -0700
Subject: [PATCH 07/13] concierge_impersonation_proxy_test.go: handle TKGS test
 clusters

Handle any test cluster which supports load balancers but should
not automatically start the impersonator, e.g. TKGS clusters.
---
 .../concierge_impersonation_proxy_test.go     | 71 ++++++++++++++-----
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index d95431ab..e2143f64 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -74,12 +74,18 @@ func (sb *syncBuffer) Write(b []byte) (int, error) {
 }
 
 // Note that this test supports being run on all of our integration test cluster types:
-//   - load balancers not supported, has squid proxy (e.g. kind)
-//   - load balancers supported, has squid proxy (e.g. EKS)
-//   - load balancers supported, no squid proxy (e.g. GKE)
+// - TKGS acceptance (long-running) cluster: auto mode will choose disabled, supports LBs, does not have squid.
+// - GKE acceptance (long-running) cluster: auto will choose enabled, support LBs, does not have squid.
+// - kind: auto mode will choose disabled, does not support LBs, has squid.
+// - GKE ephemeral clusters: auto mode will choose enabled, supports LBs, has squid.
+// - AKS ephemeral clusters: auto mode will choose enabled, supports LBs, has squid.
+// - EKS ephemeral clusters: auto mode will choose enabled, supports LBs, has squid.
 func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's complex.
 	env := library.IntegrationEnv(t)
 
+	impersonatorShouldHaveStartedAutomaticallyByDefault := !env.HasCapability(library.HasExternalLoadBalancerProvider)
+	clusterSupportsLoadBalancers := env.HasCapability(library.HasExternalLoadBalancerProvider)
+
 	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Minute)
 	defer cancel()
 
@@ -130,8 +136,9 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 	newImpersonationProxyClientWithCredentials := func(credentials *loginv1alpha1.ClusterCredential, impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
 		kubeconfig := impersonationProxyRestConfig(credentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
-		if !env.HasCapability(library.HasExternalLoadBalancerProvider) {
-			// Send traffic through the Squid proxy
+		if !clusterSupportsLoadBalancers {
+			// Only if there is no possibility to send traffic through a load balancer, then send the traffic through the Squid proxy.
+			// Prefer to go through a load balancer because that's how the impersonator is intended to be used in the real world.
 			kubeconfig.Proxy = kubeconfigProxyFunc(t, env.Proxy)
 		}
 		return library.NewKubeclient(t, kubeconfig)
@@ -180,17 +187,45 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		// CredentialIssuer will be updated eventually with a successful impersonation proxy frontend.
 		// We do this to ensure that future tests that use the impersonation proxy (e.g.,
 		// TestE2EFullIntegration) will start with a known-good state.
-		if env.HasCapability(library.HasExternalLoadBalancerProvider) {
+		if clusterSupportsLoadBalancers {
 			performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)
 		}
 	})
 
-	if env.HasCapability(library.HasExternalLoadBalancerProvider) { //nolint:nestif // come on... it's just a test
+	// Done with set-up and ready to get started with the test. There are several states that we could be in at
+	// this point depending on the capabilities of the cluster under test. We handle each possible case here.
+	switch {
+	case impersonatorShouldHaveStartedAutomaticallyByDefault && clusterSupportsLoadBalancers:
+		// Auto mode should have decided that the impersonator will run and should have started a load balancer,
+		// and we will be able to use the load balancer to access the impersonator. (e.g. GKE, AKS, EKS)
 		// Check that load balancer has been automatically created by the impersonator's "auto" mode.
 		library.RequireEventuallyWithoutError(t, func() (bool, error) {
 			return hasImpersonationProxyLoadBalancerService(ctx, env, adminClient)
 		}, 30*time.Second, 500*time.Millisecond)
-	} else {
+
+	case impersonatorShouldHaveStartedAutomaticallyByDefault && !clusterSupportsLoadBalancers:
+		t.Fatal("None of the clusters types that we currently test against should automatically" +
+			"enable the impersonation proxy without also supporting load balancers. If we add such a" +
+			"cluster type in the future then we should enhance this test.")
+
+	case !impersonatorShouldHaveStartedAutomaticallyByDefault && clusterSupportsLoadBalancers:
+		// Auto mode should have decided that the impersonator will be disabled. We need to manually enable it.
+		// The cluster supports load balancers so we should enable it and let the impersonator create a load balancer
+		// automatically. (e.g. TKGS)
+		// The CredentialIssuer's strategies array should have been updated to include an unsuccessful impersonation
+		// strategy saying that it was automatically disabled.
+		requireDisabledStrategy(ctx, t, env, adminConciergeClient)
+
+		// Create configuration to make the impersonation proxy turn on with no endpoint (i.e. automatically create a load balancer).
+		configMap := impersonationProxyConfigMapForConfig(t, env, impersonator.Config{Mode: impersonator.ModeEnabled})
+		t.Logf("creating configmap %s", configMap.Name)
+		_, err = adminClient.CoreV1().ConfigMaps(env.ConciergeNamespace).Create(ctx, &configMap, metav1.CreateOptions{})
+		require.NoError(t, err)
+
+	default:
+		// Auto mode should have decided that the impersonator will be disabled. We need to manually enable it.
+		// However, the cluster does not support load balancers so we should enable it without a load balancer
+		// and use squid to make requests. (e.g. kind)
 		require.NotEmpty(t, env.Proxy,
 			"test cluster does not support load balancers but also doesn't have a squid proxy... "+
 				"this is not a supported configuration for test clusters")
@@ -206,7 +241,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		require.Truef(t, isErr, "wanted error %q to be service unavailable via squid error, but: %s", err, message)
 
 		// Create configuration to make the impersonation proxy turn on with a hard coded endpoint (without a load balancer).
-		configMap := configMapForConfig(t, env, impersonator.Config{
+		configMap := impersonationProxyConfigMapForConfig(t, env, impersonator.Config{
 			Mode:     impersonator.ModeEnabled,
 			Endpoint: proxyServiceEndpoint,
 		})
@@ -221,7 +256,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	// 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)
-	if !env.HasCapability(library.HasExternalLoadBalancerProvider) {
+	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)
 	}
@@ -634,7 +669,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			dialer := websocket.Dialer{
 				TLSClientConfig: tlsConfig,
 			}
-			if !env.HasCapability(library.HasExternalLoadBalancerProvider) {
+			if !clusterSupportsLoadBalancers {
 				dialer.Proxy = func(req *http.Request) (*url.URL, error) {
 					proxyURL, err := url.Parse(env.Proxy)
 					require.NoError(t, err)
@@ -709,7 +744,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			httpTransport := http.Transport{
 				TLSClientConfig: tlsConfig,
 			}
-			if !env.HasCapability(library.HasExternalLoadBalancerProvider) {
+			if !clusterSupportsLoadBalancers {
 				httpTransport.Proxy = func(req *http.Request) (*url.URL, error) {
 					proxyURL, err := url.Parse(env.Proxy)
 					require.NoError(t, err)
@@ -777,8 +812,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 	t.Run("manually disabling the impersonation proxy feature", func(t *testing.T) {
 		// Update configuration to force the proxy to disabled mode
-		configMap := configMapForConfig(t, env, impersonator.Config{Mode: impersonator.ModeDisabled})
-		if env.HasCapability(library.HasExternalLoadBalancerProvider) {
+		configMap := impersonationProxyConfigMapForConfig(t, env, impersonator.Config{Mode: impersonator.ModeDisabled})
+		if clusterSupportsLoadBalancers {
 			t.Logf("creating configmap %s", configMap.Name)
 			_, err := adminClient.CoreV1().ConfigMaps(env.ConciergeNamespace).Create(ctx, &configMap, metav1.CreateOptions{})
 			require.NoError(t, err)
@@ -788,7 +823,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			require.NoError(t, err)
 		}
 
-		if env.HasCapability(library.HasExternalLoadBalancerProvider) {
+		if clusterSupportsLoadBalancers {
 			// The load balancer should have been deleted when we disabled the impersonation proxy.
 			// Note that this can take kind of a long time on real cloud providers (e.g. ~22 seconds on EKS).
 			library.RequireEventuallyWithoutError(t, func() (bool, error) {
@@ -827,7 +862,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 		// At this point the impersonator should be stopped. The CredentialIssuer's strategies array should be updated to
 		// include an unsuccessful impersonation strategy saying that it was manually configured to be disabled.
-		requireDisabledByConfigurationStrategy(ctx, t, env, adminConciergeClient)
+		requireDisabledStrategy(ctx, t, env, adminConciergeClient)
 
 		if !env.HasCapability(library.ClusterSigningKeyIsAvailable) {
 			// This cluster does not support the cluster signing key strategy, so now that we've manually disabled the
@@ -962,7 +997,7 @@ func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *librar
 	return impersonationProxyURL, impersonationProxyCACertPEM
 }
 
-func requireDisabledByConfigurationStrategy(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient versioned.Interface) {
+func requireDisabledStrategy(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient versioned.Interface) {
 	t.Helper()
 
 	library.RequireEventuallyWithoutError(t, func() (bool, error) {
@@ -1039,7 +1074,7 @@ func kubeconfigProxyFunc(t *testing.T, squidProxyURL string) func(req *http.Requ
 	}
 }
 
-func configMapForConfig(t *testing.T, env *library.TestEnv, config impersonator.Config) corev1.ConfigMap {
+func impersonationProxyConfigMapForConfig(t *testing.T, env *library.TestEnv, config impersonator.Config) corev1.ConfigMap {
 	t.Helper()
 	configString, err := yaml.Marshal(config)
 	require.NoError(t, err)

From 3359311228d023f37b78247931af0ff8bcb5e9c4 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Fri, 26 Mar 2021 09:49:49 -0700
Subject: [PATCH 08/13] concierge_impersonation_proxy_test.go: fix typo in
 previous commit

---
 test/integration/concierge_impersonation_proxy_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index e2143f64..73738600 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -83,7 +83,7 @@ func (sb *syncBuffer) Write(b []byte) (int, error) {
 func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's complex.
 	env := library.IntegrationEnv(t)
 
-	impersonatorShouldHaveStartedAutomaticallyByDefault := !env.HasCapability(library.HasExternalLoadBalancerProvider)
+	impersonatorShouldHaveStartedAutomaticallyByDefault := !env.HasCapability(library.ClusterSigningKeyIsAvailable)
 	clusterSupportsLoadBalancers := env.HasCapability(library.HasExternalLoadBalancerProvider)
 
 	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Minute)

From c6d7724b679554fe71b9044e40addc486940f697 Mon Sep 17 00:00:00 2001
From: Matt Moyer <moyerm@vmware.com>
Date: Fri, 26 Mar 2021 16:28:33 -0500
Subject: [PATCH 09/13] In TestImpersonationProxy, instead of failing in this
 case just skip the test.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
---
 test/integration/concierge_impersonation_proxy_test.go | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index 73738600..fb31fe30 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -226,9 +226,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		// Auto mode should have decided that the impersonator will be disabled. We need to manually enable it.
 		// However, the cluster does not support load balancers so we should enable it without a load balancer
 		// and use squid to make requests. (e.g. kind)
-		require.NotEmpty(t, env.Proxy,
-			"test cluster does not support load balancers but also doesn't have a squid proxy... "+
+		if env.Proxy == "" {
+			t.Skip("test cluster does not support load balancers but also doesn't have a squid proxy... " +
 				"this is not a supported configuration for test clusters")
+		}
 
 		// Check that no load balancer has been created by the impersonator's "auto" mode.
 		library.RequireNeverWithoutError(t, func() (bool, error) {

From defad3cdd7bec41373c7fd0fe037ce83dc1a92b7 Mon Sep 17 00:00:00 2001
From: Matt Moyer <moyerm@vmware.com>
Date: Fri, 26 Mar 2021 16:43:02 -0500
Subject: [PATCH 10/13] Remove library.DumpLogs test helper.

We had this code that printed out pod logs when certain tests failed, but it is a bit cumbersome. We're removing it because we added a CI task that exports all pod logs after every CI run, which accomplishes the same thing and provides us a bunch more data.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
---
 test/integration/e2e_test.go              |  4 --
 test/integration/supervisor_login_test.go |  4 --
 test/library/assertions.go                |  7 +--
 test/library/dumplogs.go                  | 64 -----------------------
 4 files changed, 2 insertions(+), 77 deletions(-)
 delete mode 100644 test/library/dumplogs.go

diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go
index c0137cab..2cb207e7 100644
--- a/test/integration/e2e_test.go
+++ b/test/integration/e2e_test.go
@@ -42,10 +42,6 @@ import (
 func TestE2EFullIntegration(t *testing.T) {
 	env := library.IntegrationEnv(t)
 
-	// If anything in this test crashes, dump out the supervisor and proxy pod logs.
-	defer library.DumpLogs(t, env.SupervisorNamespace, "")
-	defer library.DumpLogs(t, "dex", "app=proxy")
-
 	ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancelFunc()
 
diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go
index efc0134a..962895da 100644
--- a/test/integration/supervisor_login_test.go
+++ b/test/integration/supervisor_login_test.go
@@ -39,10 +39,6 @@ import (
 func TestSupervisorLogin(t *testing.T) {
 	env := library.IntegrationEnv(t)
 
-	// If anything in this test crashes, dump out the supervisor and proxy pod logs.
-	defer library.DumpLogs(t, env.SupervisorNamespace, "")
-	defer library.DumpLogs(t, "dex", "app=proxy")
-
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancel()
 
diff --git a/test/library/assertions.go b/test/library/assertions.go
index 35c2278b..5fccbca3 100644
--- a/test/library/assertions.go
+++ b/test/library/assertions.go
@@ -82,7 +82,7 @@ func assertNoRestartsDuringTest(t *testing.T, namespace, labelSelector string) {
 			}
 
 			// Expect the restart count to be the same as it was before the test.
-			if !assert.Equal(
+			assert.Equal(
 				t,
 				previousRestartCount,
 				currentRestartCount,
@@ -90,10 +90,7 @@ func assertNoRestartsDuringTest(t *testing.T, namespace, labelSelector string) {
 				key.String(),
 				currentRestartCount,
 				previousRestartCount,
-			) {
-				// Attempt to dump the logs from the previous container that crashed.
-				dumpContainerLogs(ctx, t, kubeClient, key.namespace, key.pod, key.container, true)
-			}
+			)
 		}
 	})
 }
diff --git a/test/library/dumplogs.go b/test/library/dumplogs.go
deleted file mode 100644
index dc0bf89c..00000000
--- a/test/library/dumplogs.go
+++ /dev/null
@@ -1,64 +0,0 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
-// SPDX-License-Identifier: Apache-2.0
-
-package library
-
-import (
-	"bufio"
-	"context"
-	"fmt"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
-)
-
-// DumpLogs is meant to be called in a `defer` to dump the logs of components in the cluster on a test failure.
-func DumpLogs(t *testing.T, namespace string, labelSelector string) {
-	// Only trigger on failed tests.
-	if !t.Failed() {
-		return
-	}
-
-	kubeClient := NewKubernetesClientset(t)
-	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
-	defer cancel()
-
-	pods, err := kubeClient.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: labelSelector})
-	require.NoError(t, err)
-
-	for _, pod := range pods.Items {
-		for _, container := range pod.Status.ContainerStatuses {
-			if container.RestartCount > 0 {
-				dumpContainerLogs(ctx, t, kubeClient, pod.Namespace, pod.Name, container.Name, true)
-			}
-			dumpContainerLogs(ctx, t, kubeClient, pod.Namespace, pod.Name, container.Name, false)
-		}
-	}
-}
-
-func dumpContainerLogs(ctx context.Context, t *testing.T, kubeClient kubernetes.Interface, namespace, pod, container string, prev bool) {
-	logTailLines := int64(40)
-	shortName := fmt.Sprintf("%s/%s/%s", namespace, pod, container)
-	logReader, err := kubeClient.CoreV1().Pods(namespace).GetLogs(pod, &corev1.PodLogOptions{
-		Container: container,
-		TailLines: &logTailLines,
-		Previous:  prev,
-	}).Stream(ctx)
-	if !assert.NoErrorf(t, err, "failed to stream logs for container %s", shortName) {
-		return
-	}
-	scanner := bufio.NewScanner(logReader)
-	for scanner.Scan() {
-		prefix := shortName
-		if prev {
-			prefix += " (previous)"
-		}
-		t.Logf("%s > %s", prefix, scanner.Text())
-	}
-	assert.NoError(t, scanner.Err(), "failed to read logs from container %s", shortName)
-}

From 32422f18f100e7becd89e826fb2335ad9004813b Mon Sep 17 00:00:00 2001
From: Mo Khan <i@monis.app>
Date: Fri, 26 Mar 2021 22:11:16 -0400
Subject: [PATCH 11/13] Update Google Group Link

Remove the user account prefix.
---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 196569d0..bc09ff98 100644
--- a/README.md
+++ b/README.md
@@ -47,7 +47,7 @@ Care to kick the tires? It's easy to [install and try Pinniped](https://pinniped
 
 ## Community meetings
 
-Pinniped is better because of our contributors and maintainers. It is because of you that we can bring great software to the community. Please join us during our online community meetings, occurring every first and third Thursday of the month at 9 AM PT / 12 PM PT. Use [this Zoom Link](https://vmware.zoom.us/j/93798188973?pwd=T3pIMWxReEQvcWljNm1admRoZTFSZz09) to attend and add any agenda items you wish to discuss to [the notes document](https://hackmd.io/rd_kVJhjQfOvfAWzK8A3tQ?view). Join our [Google Group](https://groups.google.com/u/1/g/project-pinniped) to receive invites to this meeting.
+Pinniped is better because of our contributors and maintainers. It is because of you that we can bring great software to the community. Please join us during our online community meetings, occurring every first and third Thursday of the month at 9 AM PT / 12 PM PT. Use [this Zoom Link](https://vmware.zoom.us/j/93798188973?pwd=T3pIMWxReEQvcWljNm1admRoZTFSZz09) to attend and add any agenda items you wish to discuss to [the notes document](https://hackmd.io/rd_kVJhjQfOvfAWzK8A3tQ?view). Join our [Google Group](https://groups.google.com/g/project-pinniped) to receive invites to this meeting.
 
 If the meeting day falls on a US holiday, please consider that occurrence of the meeting to be canceled.
 

From 95bb4c4be5d17b6ad50776e12741c75cec96cd59 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Fri, 26 Mar 2021 19:32:33 -0700
Subject: [PATCH 12/13] Fix concierge_impersonation_proxy_test.go on AKS

Also send the correct instance of `t` into a helper function which
makes assertions.
---
 .../concierge_impersonation_proxy_test.go     | 113 +++++++++++-------
 1 file changed, 69 insertions(+), 44 deletions(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index fb31fe30..2938c08f 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -42,7 +42,7 @@ import (
 	conciergev1alpha "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
 	identityv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/identity/v1alpha1"
 	loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1"
-	"go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
+	pinnipedconciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
 	"go.pinniped.dev/internal/concierge/impersonator"
 	"go.pinniped.dev/internal/kubeclient"
 	"go.pinniped.dev/internal/testutil"
@@ -102,8 +102,23 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	// The address of the ClusterIP service that points at the impersonation proxy's port (used when there is no load balancer).
 	proxyServiceEndpoint := fmt.Sprintf("%s-proxy.%s.svc.cluster.local", env.ConciergeAppName, env.ConciergeNamespace)
 
+	newImpersonationProxyClientWithCredentials := func(credentials *loginv1alpha1.ClusterCredential, impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
+		kubeconfig := impersonationProxyRestConfig(credentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
+		if !clusterSupportsLoadBalancers {
+			// Only if there is no possibility to send traffic through a load balancer, then send the traffic through the Squid proxy.
+			// Prefer to go through a load balancer because that's how the impersonator is intended to be used in the real world.
+			kubeconfig.Proxy = kubeconfigProxyFunc(t, env.Proxy)
+		}
+		return library.NewKubeclient(t, kubeconfig)
+	}
+
+	newAnonymousImpersonationProxyClient := func(impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
+		emptyCredentials := &loginv1alpha1.ClusterCredential{}
+		return newImpersonationProxyClientWithCredentials(emptyCredentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
+	}
+
 	var mostRecentTokenCredentialRequestResponse *loginv1alpha1.TokenCredentialRequest
-	refreshCredential := func() *loginv1alpha1.ClusterCredential {
+	refreshCredential := func(t *testing.T, impersonationProxyURL string, impersonationProxyCACertPEM []byte) *loginv1alpha1.ClusterCredential {
 		if mostRecentTokenCredentialRequestResponse == nil || credentialAlmostExpired(t, mostRecentTokenCredentialRequestResponse) {
 			var err error
 			// Make a TokenCredentialRequest. This can either return a cert signed by the Kube API server's CA (e.g. on kind)
@@ -112,8 +127,14 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			//
 			// However, we issue short-lived certs, so this cert will only be valid for a few minutes.
 			// Cache it until it is almost expired and then refresh it whenever it is close to expired.
-			mostRecentTokenCredentialRequestResponse, err = library.CreateTokenCredentialRequest(ctx, t, credentialRequestSpecWithWorkingCredentials)
-			require.NoError(t, err)
+			//
+			// Also, use an anonymous client which goes through the impersonation proxy to make the request because that's
+			// what would normally happen when a user is using a kubeconfig where the server is the impersonation proxy,
+			// so it more closely simulates the normal use case, and also because we want this to work on AKS clusters
+			// which do not allow anonymous requests.
+			client := newAnonymousImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "").PinnipedConcierge
+			mostRecentTokenCredentialRequestResponse, err = createTokenCredentialRequest(credentialRequestSpecWithWorkingCredentials, client)
+			require.NoError(t, err, library.Sdump(err))
 
 			require.Nil(t, mostRecentTokenCredentialRequestResponse.Status.Message,
 				"expected no error message but got: %s", library.Sdump(mostRecentTokenCredentialRequestResponse.Status.Message))
@@ -134,27 +155,12 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		return library.NewKubeclient(t, kubeconfig).Kubernetes
 	}
 
-	newImpersonationProxyClientWithCredentials := func(credentials *loginv1alpha1.ClusterCredential, impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
-		kubeconfig := impersonationProxyRestConfig(credentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
-		if !clusterSupportsLoadBalancers {
-			// Only if there is no possibility to send traffic through a load balancer, then send the traffic through the Squid proxy.
-			// Prefer to go through a load balancer because that's how the impersonator is intended to be used in the real world.
-			kubeconfig.Proxy = kubeconfigProxyFunc(t, env.Proxy)
-		}
-		return library.NewKubeclient(t, kubeconfig)
-	}
-
-	newImpersonationProxyClient := func(impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
-		refreshedCredentials := refreshCredential().DeepCopy()
+	newImpersonationProxyClient := func(t *testing.T, impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
+		refreshedCredentials := refreshCredential(t, impersonationProxyURL, impersonationProxyCACertPEM).DeepCopy()
 		refreshedCredentials.Token = "not a valid token" // demonstrates that client certs take precedence over tokens by setting both on the requests
 		return newImpersonationProxyClientWithCredentials(refreshedCredentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
 	}
 
-	newAnonymousImpersonationProxyClient := func(impersonationProxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) *kubeclient.Client {
-		emptyCredentials := &loginv1alpha1.ClusterCredential{}
-		return newImpersonationProxyClientWithCredentials(emptyCredentials, impersonationProxyURL, impersonationProxyCACertPEM, doubleImpersonateUser)
-	}
-
 	oldConfigMap, err := adminClient.CoreV1().ConfigMaps(env.ConciergeNamespace).Get(ctx, impersonationProxyConfigMapName(env), metav1.GetOptions{})
 	if !k8serrors.IsNotFound(err) {
 		require.NoError(t, err) // other errors aside from NotFound are unexpected
@@ -266,8 +272,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	// credentials before they expire. Create a closure to capture the arguments to newImpersonationProxyClient
 	// so we don't have to keep repeating them.
 	// This client performs TLS checks, so it also provides test coverage that the impersonation proxy server is generating TLS certs correctly.
-	impersonationProxyKubeClient := func() kubernetes.Interface {
-		return newImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "").Kubernetes
+	impersonationProxyKubeClient := func(t *testing.T) kubernetes.Interface {
+		return newImpersonationProxyClient(t, impersonationProxyURL, impersonationProxyCACertPEM, "").Kubernetes
 	}
 
 	t.Run("positive tests", func(t *testing.T) {
@@ -300,13 +306,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		// influencing RBAC checks correctly.
 		t.Run(
 			"access as user",
-			library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, impersonationProxyKubeClient()),
+			library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, impersonationProxyKubeClient(t)),
 		)
 		for _, group := range env.TestUser.ExpectedGroups {
 			group := group
 			t.Run(
 				"access as group "+group,
-				library.AccessAsGroupTest(ctx, group, impersonationProxyKubeClient()),
+				library.AccessAsGroupTest(ctx, group, impersonationProxyKubeClient(t)),
 			)
 		}
 
@@ -354,7 +360,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 			// Create and start informer to exercise the "watch" verb for us.
 			informerFactory := k8sinformers.NewSharedInformerFactoryWithOptions(
-				impersonationProxyKubeClient(),
+				impersonationProxyKubeClient(t),
 				0,
 				k8sinformers.WithNamespace(namespaceName))
 			informer := informerFactory.Core().V1().ConfigMaps()
@@ -374,17 +380,17 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}
 
 			// Test "create" verb through the impersonation proxy.
-			_, err := impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Create(ctx,
+			_, err := impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Create(ctx,
 				&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-1", Labels: configMapLabels}},
 				metav1.CreateOptions{},
 			)
 			require.NoError(t, err)
-			_, err = impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Create(ctx,
+			_, err = impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Create(ctx,
 				&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-2", Labels: configMapLabels}},
 				metav1.CreateOptions{},
 			)
 			require.NoError(t, err)
-			_, err = impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Create(ctx,
+			_, err = impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Create(ctx,
 				&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-3", Labels: configMapLabels}},
 				metav1.CreateOptions{},
 			)
@@ -400,11 +406,11 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 10*time.Second, 50*time.Millisecond)
 
 			// Test "get" verb through the impersonation proxy.
-			configMap3, err := impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Get(ctx, "configmap-3", metav1.GetOptions{})
+			configMap3, err := impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Get(ctx, "configmap-3", metav1.GetOptions{})
 			require.NoError(t, err)
 
 			// Test "list" verb through the impersonation proxy.
-			listResult, err := impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).List(ctx, metav1.ListOptions{
+			listResult, err := impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).List(ctx, metav1.ListOptions{
 				LabelSelector: configMapLabels.String(),
 			})
 			require.NoError(t, err)
@@ -412,7 +418,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 			// Test "update" verb through the impersonation proxy.
 			configMap3.Data = map[string]string{"foo": "bar"}
-			updateResult, err := impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Update(ctx, configMap3, metav1.UpdateOptions{})
+			updateResult, err := impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Update(ctx, configMap3, metav1.UpdateOptions{})
 			require.NoError(t, err)
 			require.Equal(t, "bar", updateResult.Data["foo"])
 
@@ -423,7 +429,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 10*time.Second, 50*time.Millisecond)
 
 			// Test "patch" verb through the impersonation proxy.
-			patchResult, err := impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Patch(ctx,
+			patchResult, err := impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Patch(ctx,
 				"configmap-3",
 				types.MergePatchType,
 				[]byte(`{"data":{"baz":"42"}}`),
@@ -440,7 +446,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 10*time.Second, 50*time.Millisecond)
 
 			// Test "delete" verb through the impersonation proxy.
-			err = impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).Delete(ctx, "configmap-3", metav1.DeleteOptions{})
+			err = impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).Delete(ctx, "configmap-3", metav1.DeleteOptions{})
 			require.NoError(t, err)
 
 			// Make sure that the deleted ConfigMap shows up in the informer's cache.
@@ -451,7 +457,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 10*time.Second, 50*time.Millisecond)
 
 			// Test "deletecollection" verb through the impersonation proxy.
-			err = impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{})
+			err = impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{})
 			require.NoError(t, err)
 
 			// Make sure that the deleted ConfigMaps shows up in the informer's cache.
@@ -461,7 +467,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 10*time.Second, 50*time.Millisecond)
 
 			// There should be no ConfigMaps left.
-			listResult, err = impersonationProxyKubeClient().CoreV1().ConfigMaps(namespaceName).List(ctx, metav1.ListOptions{
+			listResult, err = impersonationProxyKubeClient(t).CoreV1().ConfigMaps(namespaceName).List(ctx, metav1.ListOptions{
 				LabelSelector: configMapLabels.String(),
 			})
 			require.NoError(t, err)
@@ -471,11 +477,11 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		t.Run("double impersonation as a regular user is blocked", func(t *testing.T) {
 			// Make a client which will send requests through the impersonation proxy and will also add
 			// impersonate headers to the request.
-			doubleImpersonationKubeClient := newImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate").Kubernetes
+			doubleImpersonationKubeClient := newImpersonationProxyClient(t, impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate").Kubernetes
 
 			// Check that we can get some resource through the impersonation proxy without any impersonation headers on the request.
 			// We could use any resource for this, but we happen to know that this one should exist.
-			_, err := impersonationProxyKubeClient().CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{})
+			_, err := impersonationProxyKubeClient(t).CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{})
 			require.NoError(t, err)
 
 			// Now we'll see what happens when we add an impersonation header to the request. This should generate a
@@ -525,7 +531,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 		t.Run("WhoAmIRequests and different kinds of authentication through the impersonation proxy", func(t *testing.T) {
 			// Test using the TokenCredentialRequest for authentication.
-			impersonationProxyPinnipedConciergeClient := newImpersonationProxyClient(
+			impersonationProxyPinnipedConciergeClient := newImpersonationProxyClient(t,
 				impersonationProxyURL, impersonationProxyCACertPEM, "",
 			).PinnipedConcierge
 			whoAmI, err := impersonationProxyPinnipedConciergeClient.IdentityV1alpha1().WhoAmIRequests().
@@ -654,7 +660,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		t.Run("websocket client", func(t *testing.T) {
 			namespaceName := createTestNamespace(t, adminClient)
 
-			impersonationRestConfig := impersonationProxyRestConfig(refreshCredential(), impersonationProxyURL, impersonationProxyCACertPEM, "")
+			impersonationRestConfig := impersonationProxyRestConfig(
+				refreshCredential(t, impersonationProxyURL, impersonationProxyCACertPEM),
+				impersonationProxyURL, impersonationProxyCACertPEM, "",
+			)
 			tlsConfig, err := rest.TLSConfigFor(impersonationRestConfig)
 			require.NoError(t, err)
 
@@ -738,7 +747,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			})
 
 			// create rest client
-			restConfig := impersonationProxyRestConfig(refreshCredential(), impersonationProxyURL, impersonationProxyCACertPEM, "")
+			restConfig := impersonationProxyRestConfig(
+				refreshCredential(t, impersonationProxyURL, impersonationProxyCACertPEM),
+				impersonationProxyURL, impersonationProxyCACertPEM, "",
+			)
 
 			tlsConfig, err := rest.TLSConfigFor(restConfig)
 			require.NoError(t, err)
@@ -865,11 +877,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		// include an unsuccessful impersonation strategy saying that it was manually configured to be disabled.
 		requireDisabledStrategy(ctx, t, env, adminConciergeClient)
 
-		if !env.HasCapability(library.ClusterSigningKeyIsAvailable) {
+		if !env.HasCapability(library.ClusterSigningKeyIsAvailable) && env.HasCapability(library.AnonymousAuthenticationSupported) {
 			// This cluster does not support the cluster signing key strategy, so now that we've manually disabled the
 			// impersonation strategy, we should be left with no working strategies.
 			// Given that there are no working strategies, a TokenCredentialRequest which would otherwise work should now
 			// fail, because there is no point handing out credentials that are not going to work for any strategy.
+			// Note that library.CreateTokenCredentialRequest makes an unauthenticated request, so we can't meaningfully
+			// perform this part of the test on a cluster which does not allow anonymous authentication.
 			tokenCredentialRequestResponse, err := library.CreateTokenCredentialRequest(ctx, t, credentialRequestSpecWithWorkingCredentials)
 			require.NoError(t, err)
 
@@ -953,7 +967,7 @@ func expectedWhoAmIRequestResponse(username string, groups []string) *identityv1
 	}
 }
 
-func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient versioned.Interface) (string, []byte) {
+func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient pinnipedconciergeclientset.Interface) (string, []byte) {
 	t.Helper()
 	var impersonationProxyURL string
 	var impersonationProxyCACertPEM []byte
@@ -998,7 +1012,7 @@ func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *librar
 	return impersonationProxyURL, impersonationProxyCACertPEM
 }
 
-func requireDisabledStrategy(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient versioned.Interface) {
+func requireDisabledStrategy(ctx context.Context, t *testing.T, env *library.TestEnv, adminConciergeClient pinnipedconciergeclientset.Interface) {
 	t.Helper()
 
 	library.RequireEventuallyWithoutError(t, func() (bool, error) {
@@ -1230,3 +1244,14 @@ func requireClose(t *testing.T, c chan struct{}, timeout time.Duration) {
 		require.FailNow(t, "failed to receive from channel within "+timeout.String())
 	}
 }
+
+func createTokenCredentialRequest(
+	spec loginv1alpha1.TokenCredentialRequestSpec,
+	client pinnipedconciergeclientset.Interface,
+) (*loginv1alpha1.TokenCredentialRequest, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+	return client.LoginV1alpha1().TokenCredentialRequests().Create(ctx,
+		&loginv1alpha1.TokenCredentialRequest{Spec: spec}, metav1.CreateOptions{},
+	)
+}

From d8baa4390380f649d809f6f673c378a9d942e38b Mon Sep 17 00:00:00 2001
From: Margo Crawford <margaretc@vmware.com>
Date: Mon, 29 Mar 2021 09:30:20 -0700
Subject: [PATCH 13/13] Add new non-idle timeout integration test for
 impersonation proxy

Signed-off-by: Ryan Richard <richardry@vmware.com>
---
 .../concierge_impersonation_proxy_test.go     | 90 +++++++++++++++----
 1 file changed, 73 insertions(+), 17 deletions(-)

diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go
index 2938c08f..fee7e4df 100644
--- a/test/integration/concierge_impersonation_proxy_test.go
+++ b/test/integration/concierge_impersonation_proxy_test.go
@@ -316,7 +316,64 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			)
 		}
 
-		t.Run("kubectl port-forward and keeping the connection open for over a minute", func(t *testing.T) {
+		t.Run("kubectl port-forward and keeping the connection open for over a minute (non-idle)", func(t *testing.T) {
+			kubeconfigPath, envVarsWithProxy, _ := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM, credentialRequestSpecWithWorkingCredentials.Authenticator)
+
+			// Run the kubectl port-forward command.
+			timeout, cancelFunc := context.WithTimeout(ctx, 2*time.Minute)
+			defer cancelFunc()
+			portForwardCmd, _, portForwardStderr := kubectlCommand(timeout, t, kubeconfigPath, envVarsWithProxy, "port-forward", "--namespace", env.ConciergeNamespace, conciergePod.Name, "8443:8443")
+			portForwardCmd.Env = envVarsWithProxy
+
+			// Start, but don't wait for the command to finish.
+			err := portForwardCmd.Start()
+			require.NoError(t, err, `"kubectl port-forward" failed`)
+			go func() {
+				assert.EqualErrorf(t, portForwardCmd.Wait(), "signal: killed", `wanted "kubectl port-forward" to get signaled because context was cancelled (stderr: %q)`, portForwardStderr.String())
+			}()
+
+			// The server should recognize this this
+			// is going to be a long-running command and keep the connection open as long as the client stays connected.
+
+			// curl the endpoint as many times as we can within 70 seconds.
+			// this will ensure that we don't run into idle timeouts.
+			var curlStdOut, curlStdErr bytes.Buffer
+			timeout, cancelFunc = context.WithTimeout(ctx, 75*time.Second)
+			defer cancelFunc()
+			startTime := time.Now()
+			for time.Now().Before(startTime.Add(70 * time.Second)) {
+				curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:8443") // -sS turns off the progressbar but still prints errors
+				curlCmd.Stdout = &curlStdOut
+				curlCmd.Stderr = &curlStdErr
+				curlErr := curlCmd.Run()
+				if curlErr != nil {
+					t.Log("curl error: " + curlErr.Error())
+					t.Log("curlStdErr: " + curlStdErr.String())
+					t.Log("stdout: " + curlStdOut.String())
+				}
+				t.Log("time: ", time.Now())
+				time.Sleep(1 * time.Second)
+			}
+
+			// curl the endpoint once more, once 70 seconds has elapsed, to make sure the connection is still open.
+			timeout, cancelFunc = context.WithTimeout(ctx, 30*time.Second)
+			defer cancelFunc()
+			curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:8443") // -sS turns off the progressbar but still prints errors
+			curlCmd.Stdout = &curlStdOut
+			curlCmd.Stderr = &curlStdErr
+			curlErr := curlCmd.Run()
+
+			if curlErr != nil {
+				t.Log("curl error: " + curlErr.Error())
+				t.Log("curlStdErr: " + curlStdErr.String())
+				t.Log("stdout: " + curlStdOut.String())
+			}
+			// We expect this to 403, but all we care is that it gets through.
+			require.NoError(t, curlErr)
+			require.Contains(t, curlStdOut.String(), "\"forbidden: User \\\"system:anonymous\\\" cannot get path \\\"/\\\"\"")
+		})
+
+		t.Run("kubectl port-forward and keeping the connection open for over a minute (idle)", func(t *testing.T) {
 			kubeconfigPath, envVarsWithProxy, _ := getImpersonationKubeconfig(t, env, impersonationProxyURL, impersonationProxyCACertPEM, credentialRequestSpecWithWorkingCredentials.Authenticator)
 
 			// Run the kubectl port-forward command.
@@ -336,22 +393,21 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			// is going to be a long-running command and keep the connection open as long as the client stays connected.
 			time.Sleep(70 * time.Second)
 
-			require.Eventually(t, func() bool {
-				timeout, cancelFunc = context.WithTimeout(ctx, 2*time.Minute)
-				defer cancelFunc()
-				curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:8443") // -sS turns off the progressbar but still prints errors
-				var curlStdOut, curlStdErr bytes.Buffer
-				curlCmd.Stdout = &curlStdOut
-				curlCmd.Stderr = &curlStdErr
-				err = curlCmd.Run()
-				if err != nil {
-					t.Log("curl error: " + err.Error())
-					t.Log("curlStdErr: " + curlStdErr.String())
-					t.Log("stdout: " + curlStdOut.String())
-				}
-				// We expect this to 403, but all we care is that it gets through.
-				return err == nil && strings.Contains(curlStdOut.String(), "\"forbidden: User \\\"system:anonymous\\\" cannot get path \\\"/\\\"\"")
-			}, 1*time.Minute, 500*time.Millisecond)
+			timeout, cancelFunc = context.WithTimeout(ctx, 2*time.Minute)
+			defer cancelFunc()
+			curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:8443") // -sS turns off the progressbar but still prints errors
+			var curlStdOut, curlStdErr bytes.Buffer
+			curlCmd.Stdout = &curlStdOut
+			curlCmd.Stderr = &curlStdErr
+			err = curlCmd.Run()
+			if err != nil {
+				t.Log("curl error: " + err.Error())
+				t.Log("curlStdErr: " + curlStdErr.String())
+				t.Log("stdout: " + curlStdOut.String())
+			}
+			// We expect this to 403, but all we care is that it gets through.
+			require.NoError(t, err)
+			require.Contains(t, curlStdOut.String(), "\"forbidden: User \\\"system:anonymous\\\" cannot get path \\\"/\\\"\"")
 		})
 
 		t.Run("using and watching all the basic verbs", func(t *testing.T) {