From 19a1d569c9598babf4b9f83a0d2368c779a53a9c Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 14 Oct 2020 12:28:08 -0500 Subject: [PATCH] Restructure this test to avoid data races. Signed-off-by: Matt Moyer --- go.mod | 1 + test/integration/cli_test.go | 118 ++++++++++++++++++++++------------- 2 files changed, 77 insertions(+), 42 deletions(-) diff --git a/go.mod b/go.mod index 0b476b08..6adfc17b 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( go.pinniped.dev/generated/1.19/client v0.0.0-00010101000000-000000000000 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 golang.org/x/oauth2 v0.0.0-20191202225959-858c2ad4c8b6 + golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 gopkg.in/square/go-jose.v2 v2.2.2 k8s.io/api v0.19.2 k8s.io/apimachinery v0.19.2 diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 45ed4476..4ba66899 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -4,8 +4,11 @@ package integration import ( "bufio" + "bytes" "context" "encoding/json" + "errors" + "fmt" "io" "io/ioutil" "os" @@ -14,12 +17,12 @@ import ( "regexp" "strconv" "strings" - "sync" "testing" "time" "github.com/sclevine/agouti" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" "gopkg.in/square/go-jose.v2" clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" @@ -153,60 +156,70 @@ func TestCLILoginOIDC(t *testing.T) { t.Logf("building CLI binary") pinnipedExe := buildPinnipedCLI(t) + // Start the CLI running the "alpha login oidc [...]" command with stdout/stderr connected to pipes. + t.Logf("starting CLI subprocess") cmd := exec.CommandContext(ctx, pinnipedExe, "alpha", "login", "oidc", "--issuer", env.OIDCUpstream.Issuer, "--client-id", env.OIDCUpstream.ClientID, "--listen-port", strconv.Itoa(env.OIDCUpstream.LocalhostPort), "--skip-browser", ) - - // Create a WaitGroup that will wait for all child goroutines to finish, so they can assert errors. - var wg sync.WaitGroup - t.Cleanup(wg.Wait) - - // Start a background goroutine to read stderr from the CLI and parse out the login URL. - loginURLChan := make(chan string) stderr, err := cmd.StderrPipe() require.NoError(t, err) - wg.Add(1) - go func() { - defer wg.Done() - r := bufio.NewReader(stderr) - line, err := r.ReadString('\n') - require.NoError(t, err) - const prompt = "Please log in: " - require.Truef(t, strings.HasPrefix(line, prompt), "expected %q to have prefix %q", line, prompt) - loginURLChan <- strings.TrimPrefix(line, prompt) - _, err = io.Copy(ioutil.Discard, r) - - t.Logf("stderr stream closed") - require.NoError(t, err) - }() - - // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. - credOutputChan := make(chan clientauthenticationv1beta1.ExecCredential) stdout, err := cmd.StdoutPipe() require.NoError(t, err) - wg.Add(1) - go func() { - defer wg.Done() - r := bufio.NewReader(stdout) - - var out clientauthenticationv1beta1.ExecCredential - require.NoError(t, json.NewDecoder(r).Decode(&out)) - credOutputChan <- out - - _, err = io.Copy(ioutil.Discard, r) - t.Logf("stdout stream closed") - require.NoError(t, err) - }() - - t.Logf("starting CLI subprocess") require.NoError(t, cmd.Start()) t.Cleanup(func() { err := cmd.Wait() - t.Logf("CLI subprocess exited") - require.NoError(t, err) + t.Logf("CLI subprocess exited with code %d", cmd.ProcessState.ExitCode()) + require.NoErrorf(t, err, "CLI process did not exit cleanly") + }) + + // Start a background goroutine to read stderr from the CLI and parse out the login URL. + loginURLChan := make(chan string) + spawnTestGoroutine(t, func() (err error) { + defer func() { + closeErr := stderr.Close() + if closeErr == nil || errors.Is(closeErr, os.ErrClosed) { + return + } + if err == nil { + err = fmt.Errorf("stderr stream closed with error: %w", closeErr) + } + }() + + reader := bufio.NewReader(stderr) + line, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("could not read login URL line from stderr: %w", err) + } + const prompt = "Please log in: " + if !strings.HasPrefix(line, prompt) { + return fmt.Errorf("expected %q to have prefix %q", line, prompt) + } + loginURLChan <- strings.TrimPrefix(line, prompt) + return readAndExpectEmpty(reader) + }) + + // Start a background goroutine to read stdout from the CLI and parse out an ExecCredential. + credOutputChan := make(chan clientauthenticationv1beta1.ExecCredential) + spawnTestGoroutine(t, func() (err error) { + defer func() { + closeErr := stderr.Close() + if closeErr == nil || errors.Is(closeErr, os.ErrClosed) { + return + } + if err == nil { + err = fmt.Errorf("stdout stream closed with error: %w", closeErr) + } + }() + reader := bufio.NewReader(stdout) + var out clientauthenticationv1beta1.ExecCredential + if err := json.NewDecoder(reader).Decode(&out); err != nil { + return fmt.Errorf("could not read ExecCredential from stdout: %w", err) + } + credOutputChan <- out + return readAndExpectEmpty(reader) }) // Start the browser driver. @@ -312,3 +325,24 @@ func waitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) { return err == nil && pat.MatchString(url) }, 10*time.Second, 100*time.Millisecond) } + +func readAndExpectEmpty(r io.Reader) (err error) { + var remainder bytes.Buffer + _, err = io.Copy(&remainder, r) + if err != nil { + return err + } + if r := remainder.String(); r != "" { + return fmt.Errorf("expected remainder to be empty, but got %q", r) + } + return nil +} + +func spawnTestGoroutine(t *testing.T, f func() error) { + t.Helper() + var eg errgroup.Group + t.Cleanup(func() { + require.NoError(t, eg.Wait(), "background goroutine failed") + }) + eg.Go(f) +}