From 2515b2d710543b77e520734e50d60a622311539e Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 16 Mar 2021 14:59:59 -0500 Subject: [PATCH 1/2] Make TestAPIServingCertificateAutoCreationAndRotation more reliable. This test has occasionally flaked because it only waited for the APIService GET to finish, but did not wait for the controller to successfully update the target object. The new code should be more patient and allow the controller up to 10s to perform the expected action. Signed-off-by: Matt Moyer --- .../concierge_api_serving_certs_test.go | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index 78aefea6b..a6d3cd360 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -4,6 +4,7 @@ package integration import ( + "bytes" "context" "testing" "time" @@ -131,13 +132,19 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { require.Equal(t, env.ConciergeAppName, secret.Labels["app"]) // Expect that the APIService was also updated with the new CA. - aggregatedAPIUpdated := func() bool { - apiService, err = aggregatedClient.ApiregistrationV1().APIServices().Get(ctx, apiServiceName, metav1.GetOptions{}) - return err == nil - } - assert.Eventually(t, aggregatedAPIUpdated, 10*time.Second, 250*time.Millisecond) - require.NoError(t, err) // prints out the error and stops the test in case of failure - require.Equal(t, regeneratedCACert, apiService.Spec.CABundle) + require.Eventually(t, func() bool { + apiService, err := aggregatedClient.ApiregistrationV1().APIServices().Get(ctx, apiServiceName, metav1.GetOptions{}) + if err != nil { + t.Logf("get for APIService %q returned error %v", apiServiceName, err) + return false + } + if !bytes.Equal(regeneratedCACert, apiService.Spec.CABundle) { + t.Logf("CA bundle in APIService %q does not yet have the expected value", apiServiceName) + return false + } + t.Logf("found that APIService %q was updated to expected CA certificate", apiServiceName) + return true + }, 10*time.Second, 250*time.Millisecond, "never saw CA certificate rotate to expected value") // Check that we can still make requests to the aggregated API through the kube API server, // because the kube API server uses these certs when proxying requests to the aggregated API server, From 30a392b9009f0116155b2726f1812e5fb29f6c12 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 16 Mar 2021 17:22:28 -0500 Subject: [PATCH 2/2] Improve the reliability of TestGetPinnipedCategory. This test could flake in some rare scenarios. This change adds a bunch of retries, improves the debugging output if the tests fail, and puts all of the subtests in parallel which saves ~10s on my local machine. Signed-off-by: Matt Moyer --- test/integration/category_test.go | 158 ++++++++++++++---------------- 1 file changed, 75 insertions(+), 83 deletions(-) diff --git a/test/integration/category_test.go b/test/integration/category_test.go index 42d445823..014ccbf9f 100644 --- a/test/integration/category_test.go +++ b/test/integration/category_test.go @@ -6,118 +6,110 @@ package integration import ( "bytes" "os/exec" + "strings" + "sync" "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.pinniped.dev/test/library" ) +func runTestKubectlCommand(t *testing.T, args ...string) (string, string) { + t.Helper() + + var lock sync.Mutex + var stdOut, stdErr bytes.Buffer + var err error + start := time.Now() + attempts := 0 + if !assert.Eventually(t, func() bool { + lock.Lock() + defer lock.Unlock() + attempts++ + stdOut.Reset() + stdErr.Reset() + cmd := exec.Command("kubectl", args...) + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err = cmd.Run() + return err == nil + }, + 120*time.Second, + 200*time.Millisecond, + ) { + lock.Lock() + defer lock.Unlock() + t.Logf( + "never ran %q successfully even after %d attempts (%s)", + "kubectl "+strings.Join(args, " "), + attempts, + time.Since(start).Round(time.Second), + ) + t.Logf("last error: %v", err) + t.Logf("stdout:\n%s\n", stdOut.String()) + t.Logf("stderr:\n%s\n", stdErr.String()) + t.FailNow() + } + return stdOut.String(), stdErr.String() +} func TestGetPinnipedCategory(t *testing.T) { env := library.IntegrationEnv(t) dotSuffix := "." + env.APIGroupSuffix t.Run("category, no special params", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - var err error - require.Eventuallyf(t, func() bool { - cmd := exec.Command("kubectl", "get", "pinniped", "-A") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err = cmd.Run() - return err == nil - }, - 60*time.Second, - 1*time.Second, - "never ran 'kubectl get pinniped -A' successfully:\n%s\n\n%s", - stdErr.String(), - stdOut.String(), - ) - require.Empty(t, stdErr.String()) - require.NotContains(t, stdOut.String(), "MethodNotAllowed") - require.Contains(t, stdOut.String(), dotSuffix) + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "pinniped", "-A") + require.Empty(t, stderr) + require.NotContains(t, stdout, "MethodNotAllowed") + require.Contains(t, stdout, dotSuffix) }) t.Run("category, table params", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - cmd := exec.Command("kubectl", "get", "pinniped", "-A", "-o", "wide", "-v", "10") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err := cmd.Run() - require.NoError(t, err, stdErr.String(), stdOut.String()) - - require.NotContains(t, stdOut.String(), "MethodNotAllowed") - require.Contains(t, stdOut.String(), dotSuffix) - - require.Contains(t, stdErr.String(), `"kind":"Table"`) - require.Contains(t, stdErr.String(), `"resourceVersion":"0"`) - require.Contains(t, stdErr.String(), `/v1alpha1/tokencredentialrequests`) - require.Contains(t, stdErr.String(), `/v1alpha1/whoamirequests`) + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "pinniped", "-A", "-o", "wide", "-v", "10") + require.NotContains(t, stdout, "MethodNotAllowed") + require.Contains(t, stdout, dotSuffix) + require.Contains(t, stderr, `"kind":"Table"`) + require.Contains(t, stderr, `"resourceVersion":"0"`) + require.Contains(t, stderr, `/v1alpha1/tokencredentialrequests`) + require.Contains(t, stderr, `/v1alpha1/whoamirequests`) }) t.Run("list, no special params", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - //nolint: gosec // input is part of test env - cmd := exec.Command("kubectl", "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err := cmd.Run() - require.NoError(t, err, stdErr.String(), stdOut.String()) - require.Empty(t, stdOut.String()) - - require.NotContains(t, stdErr.String(), "MethodNotAllowed") - require.Contains(t, stdErr.String(), `No resources found`) + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A") + require.Empty(t, stdout) + require.NotContains(t, stderr, "MethodNotAllowed") + require.Contains(t, stderr, `No resources found`) }) t.Run("list, table params", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - //nolint: gosec // input is part of test env - cmd := exec.Command("kubectl", "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A", "-o", "wide", "-v", "10") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err := cmd.Run() - require.NoError(t, err, stdErr.String(), stdOut.String()) - require.Empty(t, stdOut.String()) - - require.NotContains(t, stdErr.String(), "MethodNotAllowed") - require.Contains(t, stdErr.String(), `"kind":"Table"`) - require.Contains(t, stdErr.String(), `"resourceVersion":"0"`) + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A", "-o", "wide", "-v", "10") + require.Empty(t, stdout) + require.NotContains(t, stderr, "MethodNotAllowed") + require.Contains(t, stderr, `"kind":"Table"`) + require.Contains(t, stderr, `"resourceVersion":"0"`) }) t.Run("raw request to see body, token cred", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - //nolint: gosec // input is part of test env - cmd := exec.Command("kubectl", "get", "--raw", "/apis/login.concierge"+dotSuffix+"/v1alpha1/tokencredentialrequests") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err := cmd.Run() - require.NoError(t, err, stdErr.String(), stdOut.String()) - require.Empty(t, stdErr.String()) - - require.NotContains(t, stdOut.String(), "MethodNotAllowed") - require.Contains(t, stdOut.String(), `{"kind":"TokenCredentialRequestList","apiVersion":"login.concierge`+ + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "--raw", "/apis/login.concierge"+dotSuffix+"/v1alpha1/tokencredentialrequests") + require.NotContains(t, stdout, "MethodNotAllowed") + require.Contains(t, stdout, `{"kind":"TokenCredentialRequestList","apiVersion":"login.concierge`+ dotSuffix+`/v1alpha1","metadata":{"resourceVersion":"0"},"items":[]}`) + require.Empty(t, stderr) }) t.Run("raw request to see body, whoami", func(t *testing.T) { - var stdOut, stdErr bytes.Buffer - - //nolint: gosec // input is part of test env - cmd := exec.Command("kubectl", "get", "--raw", "/apis/identity.concierge"+dotSuffix+"/v1alpha1/whoamirequests") - cmd.Stdout = &stdOut - cmd.Stderr = &stdErr - err := cmd.Run() - require.NoError(t, err, stdErr.String(), stdOut.String()) - require.Empty(t, stdErr.String()) - - require.NotContains(t, stdOut.String(), "MethodNotAllowed") - require.Contains(t, stdOut.String(), `{"kind":"WhoAmIRequestList","apiVersion":"identity.concierge`+ + t.Parallel() + stdout, stderr := runTestKubectlCommand(t, "get", "--raw", "/apis/identity.concierge"+dotSuffix+"/v1alpha1/whoamirequests") + require.NotContains(t, stdout, "MethodNotAllowed") + require.Contains(t, stdout, `{"kind":"WhoAmIRequestList","apiVersion":"identity.concierge`+ dotSuffix+`/v1alpha1","metadata":{"resourceVersion":"0"},"items":[]}`) + require.Empty(t, stderr) }) }