Merge pull request #503 from mattmoyer/rework-restart-assertions-helper

Rework integration test assertions for pod restarts.

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()

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

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()
 

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()
 

test/integration/concierge_credentialrequest_test.go

@@ -23,9 +23,7 @@ import (
 )
 
 func TestUnsuccessfulCredentialRequest(t *testing.T) {
-	env := library.IntegrationEnv(t).WithCapability(library.AnonymousAuthenticationSupported)
+	_ = library.IntegrationEnv(t).WithCapability(library.AnonymousAuthenticationSupported)
-
-	library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "")
 
 	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.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable)
-
-	library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "")
 
 	// 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.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable)
-
-	library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "")
 
 	// 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.IntegrationEnv(t).WithoutCapability(library.ClusterSigningKeyIsAvailable).WithCapability(library.AnonymousAuthenticationSupported)
-
-	library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "")
 
 	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
 	defer cancel()

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()
 

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()
 

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()

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()
 

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()
 

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()
 

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{

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
@@ -29,52 +30,77 @@ 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()
+	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",
+				"container %s existed at beginning of the test, but not the end",
-				key,
+				key.String(),
 			) {
-				assert.Equal(
+				continue
-					t,
+			}
-					previousRestartCount,
+
-					currentRestartCount,
+			// Expect the restart count to be the same as it was before the test.
-					"pod namespace/name/container %s has restarted %d times (original count was %d)",
+			if !assert.Equal(
-					key,
+				t,
-					currentRestartCount,
+				previousRestartCount,
-					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 {
+type containerRestartKey struct {
-	t.Helper()
+	namespace string
+	pod       string
+	container string
+}
 
-	kubeClient := NewKubernetesClientset(t)
+func (k containerRestartKey) String() string {
-	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
+	return fmt.Sprintf("%s/%s/%s", k.namespace, k.pod, k.container)
-	defer cancel()
+}
+
+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
 		}
 	}

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)
+			if container.RestartCount > 0 {
-			req := kubeClient.CoreV1().Pods(namespace).GetLogs(pod.Name, &corev1.PodLogOptions{
+				dumpContainerLogs(ctx, t, kubeClient, pod.Namespace, pod.Name, container.Name, true)
-				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())
 			}
-			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)
+}

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,8 +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, "")
 	return &result
 }