From 5a43a5d53a57260792dfbe7f9e90daa9769cbbb4 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 17 Mar 2021 11:08:01 -0500 Subject: [PATCH 1/4] Remove library.AssertNoRestartsDuringTest and make that assertion implicit in library.IntegrationEnv. This means we (hopefully) can't forget to include these assertions in any integration test. Signed-off-by: Matt Moyer --- test/integration/cli_test.go | 2 -- .../concierge_api_serving_certs_test.go | 2 -- test/integration/concierge_client_test.go | 2 -- .../concierge_credentialissuer_test.go | 2 -- .../concierge_credentialrequest_test.go | 18 ++++-------------- .../concierge_kubecertagent_test.go | 2 -- test/integration/e2e_test.go | 3 --- test/integration/supervisor_discovery_test.go | 6 ------ test/integration/supervisor_healthz_test.go | 2 -- test/integration/supervisor_login_test.go | 2 -- test/integration/supervisor_secrets_test.go | 2 -- test/integration/supervisor_upstream_test.go | 2 -- test/library/assertions.go | 4 ++-- test/library/env.go | 4 ++++ 14 files changed, 10 insertions(+), 43 deletions(-) diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 910d1ac4..ca994ac0 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -35,8 +35,6 @@ import ( func TestCLIGetKubeconfigStaticToken(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - // Create a test webhook configuration to use with the CLI. ctx, cancelFunc := context.WithTimeout(context.Background(), 4*time.Minute) defer cancelFunc() diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index a6d3cd36..af6666d5 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -23,8 +23,6 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { env := library.IntegrationEnv(t) defaultServingCertResourceName := env.ConciergeAppName + "-api-tls-serving-certificate" - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - tests := []struct { name string forceRotation func(context.Context, kubernetes.Interface, string) error diff --git a/test/integration/concierge_client_test.go b/test/integration/concierge_client_test.go index 1cad0de5..1bf1524e 100644 --- a/test/integration/concierge_client_test.go +++ b/test/integration/concierge_client_test.go @@ -57,8 +57,6 @@ var maskKey = func(s string) string { return strings.ReplaceAll(s, "TESTING KEY" func TestClient(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() diff --git a/test/integration/concierge_credentialissuer_test.go b/test/integration/concierge_credentialissuer_test.go index c7ed6e66..172022c6 100644 --- a/test/integration/concierge_credentialissuer_test.go +++ b/test/integration/concierge_credentialissuer_test.go @@ -23,8 +23,6 @@ func TestCredentialIssuer(t *testing.T) { client := library.NewConciergeClientset(t) aggregatedClientset := library.NewAggregatedClientset(t) - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 00fe5d0f..81984095 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -23,9 +23,7 @@ import ( ) func TestUnsuccessfulCredentialRequest(t *testing.T) { - env := library.IntegrationEnv(t).WithCapability(library.AnonymousAuthenticationSupported) - - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") + _ = library.IntegrationEnv(t).WithCapability(library.AnonymousAuthenticationSupported) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -44,8 +42,6 @@ func TestUnsuccessfulCredentialRequest(t *testing.T) { func TestSuccessfulCredentialRequest(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 6*time.Minute) defer cancel() @@ -131,9 +127,7 @@ func TestSuccessfulCredentialRequest(t *testing.T) { } func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTheUser(t *testing.T) { - env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") + _ = library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) // Create a testWebhook so we have a legitimate authenticator to pass to the // TokenCredentialRequest API. @@ -154,9 +148,7 @@ func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthentic } func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) { - env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") + _ = library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) // Create a testWebhook so we have a legitimate authenticator to pass to the // TokenCredentialRequest API. @@ -184,9 +176,7 @@ func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T } func TestCredentialRequest_OtherwiseValidRequestWithRealTokenShouldFailWhenTheClusterIsNotCapable(t *testing.T) { - env := library.IntegrationEnv(t).WithoutCapability(library.ClusterSigningKeyIsAvailable).WithCapability(library.AnonymousAuthenticationSupported) - - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") + _ = library.IntegrationEnv(t).WithoutCapability(library.ClusterSigningKeyIsAvailable).WithCapability(library.AnonymousAuthenticationSupported) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() diff --git a/test/integration/concierge_kubecertagent_test.go b/test/integration/concierge_kubecertagent_test.go index 37392013..5ce1cff4 100644 --- a/test/integration/concierge_kubecertagent_test.go +++ b/test/integration/concierge_kubecertagent_test.go @@ -28,8 +28,6 @@ const ( func TestKubeCertAgent(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) defer cancel() diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 6ffdf0b9..275d0b8a 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -46,9 +46,6 @@ func TestE2EFullIntegration(t *testing.T) { defer library.DumpLogs(t, env.SupervisorNamespace, "") defer library.DumpLogs(t, "dex", "app=proxy") - library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Minute) defer cancelFunc() diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 8712a316..c164cb7b 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -45,8 +45,6 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { env := library.IntegrationEnv(t) client := library.NewSupervisorClientset(t) - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) @@ -152,8 +150,6 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { pinnipedClient := library.NewSupervisorClientset(t) kubeClient := library.NewKubernetesClientset(t) - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() @@ -225,8 +221,6 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { pinnipedClient := library.NewSupervisorClientset(t) kubeClient := library.NewKubernetesClientset(t) - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() diff --git a/test/integration/supervisor_healthz_test.go b/test/integration/supervisor_healthz_test.go index 20323aca..dafe1d08 100644 --- a/test/integration/supervisor_healthz_test.go +++ b/test/integration/supervisor_healthz_test.go @@ -29,8 +29,6 @@ func TestSupervisorHealthz(t *testing.T) { t.Skip("PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS not defined") } - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 84fa8efa..b3e4760c 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -44,8 +44,6 @@ func TestSupervisorLogin(t *testing.T) { defer library.DumpLogs(t, env.SupervisorNamespace, "") defer library.DumpLogs(t, "dex", "app=proxy") - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go index d8498955..2cf44128 100644 --- a/test/integration/supervisor_secrets_test.go +++ b/test/integration/supervisor_secrets_test.go @@ -24,8 +24,6 @@ func TestSupervisorSecrets(t *testing.T) { kubeClient := library.NewKubernetesClientset(t) supervisorClient := library.NewSupervisorClientset(t) - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() diff --git a/test/integration/supervisor_upstream_test.go b/test/integration/supervisor_upstream_test.go index 22a92ff1..b43735a6 100644 --- a/test/integration/supervisor_upstream_test.go +++ b/test/integration/supervisor_upstream_test.go @@ -17,8 +17,6 @@ import ( func TestSupervisorUpstreamOIDCDiscovery(t *testing.T) { env := library.IntegrationEnv(t) - library.AssertNoRestartsDuringTest(t, env.SupervisorNamespace, "") - t.Run("invalid missing secret and bad issuer", func(t *testing.T) { t.Parallel() spec := v1alpha1.OIDCIdentityProviderSpec{ diff --git a/test/library/assertions.go b/test/library/assertions.go index 4f42f5a7..71dd6bc5 100644 --- a/test/library/assertions.go +++ b/test/library/assertions.go @@ -29,9 +29,9 @@ func RequireEventuallyWithoutError( require.NoError(t, wait.PollImmediate(tick, waitFor, f), msgAndArgs...) } -// NewRestartAssertion allows a caller to assert that there were no restarts for a Pod in the +// assertNoRestartsDuringTest allows a caller to assert that there were no restarts for a Pod in the // provided namespace with the provided labelSelector during the lifetime of a test. -func AssertNoRestartsDuringTest(t *testing.T, namespace, labelSelector string) { +func assertNoRestartsDuringTest(t *testing.T, namespace, labelSelector string) { t.Helper() previousRestartCounts := getRestartCounts(t, namespace, labelSelector) diff --git a/test/library/env.go b/test/library/env.go index 3f96b9a0..28caf52f 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -97,6 +97,10 @@ func IntegrationEnv(t *testing.T) *TestEnv { loadEnvVars(t, &result) + // In every integration test, assert that no pods in our namespaces restart during the test. + assertNoRestartsDuringTest(t, result.ConciergeNamespace, "") + assertNoRestartsDuringTest(t, result.SupervisorNamespace, "") + result.t = t return &result } From 6520c5a3a1852808b5f99c054f03f9fdef24da68 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 17 Mar 2021 11:24:59 -0500 Subject: [PATCH 2/4] Extend library.DumpLogs() to dump logs from the previous container, if one exists. This is important in case the container has crashed and has been restarted. Signed-off-by: Matt Moyer --- test/library/dumplogs.go | 41 +++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/test/library/dumplogs.go b/test/library/dumplogs.go index dca0d276..86dfd252 100644 --- a/test/library/dumplogs.go +++ b/test/library/dumplogs.go @@ -6,12 +6,15 @@ 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. @@ -25,25 +28,37 @@ func DumpLogs(t *testing.T, namespace string, labelSelector string) { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - logTailLines := int64(40) 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 { - t.Logf("pod %s/%s container %s restarted %d times:", pod.Namespace, pod.Name, container.Name, container.RestartCount) - req := kubeClient.CoreV1().Pods(namespace).GetLogs(pod.Name, &corev1.PodLogOptions{ - Container: container.Name, - TailLines: &logTailLines, - }) - logReader, err := req.Stream(ctx) - require.NoError(t, err) - - scanner := bufio.NewScanner(logReader) - for scanner.Scan() { - t.Logf("%s/%s/%s > %s", pod.Namespace, pod.Name, container.Name, scanner.Text()) + if container.RestartCount > 0 { + dumpContainerLogs(ctx, t, kubeClient, pod.Namespace, pod.Name, container.Name, true) } - require.NoError(t, scanner.Err()) + 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 0dd2b358fbb01d28d4a06aa69f08946c62244052 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 17 Mar 2021 11:46:55 -0500 Subject: [PATCH 3/4] Extend assertNoRestartsDuringTest to dump logs from containers that restarted. Signed-off-by: Matt Moyer --- test/library/assertions.go | 68 ++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/test/library/assertions.go b/test/library/assertions.go index 71dd6bc5..c74cdd95 100644 --- a/test/library/assertions.go +++ b/test/library/assertions.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" ) // RequireEventuallyWithoutError is a wrapper around require.Eventually() that allows the caller to @@ -33,48 +34,73 @@ func RequireEventuallyWithoutError( // provided namespace with the provided labelSelector during the lifetime of a test. func assertNoRestartsDuringTest(t *testing.T, namespace, labelSelector string) { t.Helper() + kubeClient := NewKubernetesClientset(t) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() - previousRestartCounts := getRestartCounts(t, namespace, labelSelector) + previousRestartCounts := getRestartCounts(ctx, t, kubeClient, namespace, labelSelector) t.Cleanup(func() { - currentRestartCounts := getRestartCounts(t, namespace, labelSelector) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + currentRestartCounts := getRestartCounts(ctx, t, kubeClient, namespace, labelSelector) for key, previousRestartCount := range previousRestartCounts { currentRestartCount, ok := currentRestartCounts[key] - if assert.Truef( + + // If the container no longer exists, that's a test failure. + if !assert.Truef( t, ok, - "pod namespace/name/container %s existed at beginning of the test, but not the end", - key, + "container %s existed at beginning of the test, but not the end", + key.String(), ) { - assert.Equal( - t, - previousRestartCount, - currentRestartCount, - "pod namespace/name/container %s has restarted %d times (original count was %d)", - key, - currentRestartCount, - previousRestartCount, - ) + continue + } + + // Expect the restart count to be the same as it was before the test. + if !assert.Equal( + t, + previousRestartCount, + currentRestartCount, + "container %s has restarted %d times (original count was %d)", + 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) } } }) } -func getRestartCounts(t *testing.T, namespace, labelSelector string) map[string]int32 { - t.Helper() +type containerRestartKey struct { + namespace string + pod string + container string +} - kubeClient := NewKubernetesClientset(t) - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - defer cancel() +func (k containerRestartKey) String() string { + return fmt.Sprintf("%s/%s/%s", k.namespace, k.pod, k.container) +} + +type containerRestartMap map[containerRestartKey]int32 + +func getRestartCounts(ctx context.Context, t *testing.T, kubeClient kubernetes.Interface, namespace, labelSelector string) containerRestartMap { + t.Helper() pods, err := kubeClient.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: labelSelector}) require.NoError(t, err) - restartCounts := make(map[string]int32) + restartCounts := make(containerRestartMap) for _, pod := range pods.Items { for _, container := range pod.Status.ContainerStatuses { - key := fmt.Sprintf("%s/%s/%s", pod.Namespace, pod.Name, container.Name) + key := containerRestartKey{ + namespace: pod.Namespace, + pod: pod.Name, + container: container.Name, + } restartCounts[key] = container.RestartCount } } From 74df6d138b87fa53218dc0fea7a12f0a51bda7ba Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 17 Mar 2021 12:47:38 -0500 Subject: [PATCH 4/4] Memoize library.IntegrationEnv so it's only constructed once per test. This is probably a good idea regardless, but it also avoids an infinite recursion from IntegrationEnv() -> assertNoRestartsDuringTest() -> NewKubeclient() -> IntegrationEnv() -> ... Signed-off-by: Matt Moyer --- test/library/env.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/library/env.go b/test/library/env.go index 28caf52f..97b24c22 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "os" "strings" + "sync" "testing" "github.com/stretchr/testify/require" @@ -73,9 +74,17 @@ func (e *TestEnv) ProxyEnv() []string { return []string{"http_proxy=" + e.Proxy, "https_proxy=" + e.Proxy, "no_proxy=127.0.0.1"} } +// memoizedTestEnvsByTest maps *testing.T pointers to *TestEnv. It exists so that we don't do all the +// environment parsing N times per test and so that any implicit assertions happen only once. +var memoizedTestEnvsByTest sync.Map //nolint: gochecknoglobals + // IntegrationEnv gets the integration test environment from OS environment variables. This // method also implies SkipUnlessIntegration(). func IntegrationEnv(t *testing.T) *TestEnv { + if existing, exists := memoizedTestEnvsByTest.Load(t); exists { + return existing.(*TestEnv) + } + t.Helper() SkipUnlessIntegration(t) @@ -96,12 +105,12 @@ func IntegrationEnv(t *testing.T) *TestEnv { require.NoErrorf(t, err, "capabilities specification was invalid YAML") loadEnvVars(t, &result) + result.t = t + memoizedTestEnvsByTest.Store(t, &result) // In every integration test, assert that no pods in our namespaces restart during the test. assertNoRestartsDuringTest(t, result.ConciergeNamespace, "") assertNoRestartsDuringTest(t, result.SupervisorNamespace, "") - - result.t = t return &result }