From e4832653b31efde21427bdb87ff6e28c9bedf712 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 14 Feb 2020 08:41:01 -0800 Subject: [PATCH 1/5] test: only test latest Z of each X.Y.Z release --- e2e/vault/vault_test.go | 63 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/e2e/vault/vault_test.go b/e2e/vault/vault_test.go index f5102af52..59350a043 100644 --- a/e2e/vault/vault_test.go +++ b/e2e/vault/vault_test.go @@ -13,19 +13,18 @@ import ( "os" "path/filepath" "runtime" + "sort" "testing" "time" "github.com/hashicorp/go-version" - "github.com/hashicorp/nomad/command/agent" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs/config" "github.com/hashicorp/nomad/testutil" + vapi "github.com/hashicorp/vault/api" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" - - vapi "github.com/hashicorp/vault/api" ) var ( @@ -39,7 +38,10 @@ func syncVault(t *testing.T) map[string]string { binDir := filepath.Join(os.TempDir(), "vault-bins/") - versions := vaultVersions(t) + urls := vaultVersions(t) + + _, versions, err := pruneVersions(urls) + require.NoError(t, err) // Get the binaries we need to download missing, err := missingVault(binDir, versions) @@ -153,6 +155,59 @@ func vaultVersions(t *testing.T) map[string]string { return versions } +// pruneVersions only takes the latest Z for each X.Y.Z release. Returns a +// sorted list and map of kept versions. +func pruneVersions(all map[string]string) ([]*version.Version, map[string]string, error) { + if len(all) == 0 { + return nil, nil, fmt.Errorf("0 Vault versions") + } + + sorted := make([]*version.Version, 0, len(all)) + + for k := range all { + sorted = append(sorted, version.Must(version.NewVersion(k))) + } + + sort.Sort(version.Collection(sorted)) + + keep := make([]*version.Version, 0, len(all)) + + for _, v := range sorted { + segments := v.Segments() + if len(segments) < 3 { + // Drop malformed versions + continue + } + + if len(keep) == 0 { + keep = append(keep, v) + continue + } + + last := keep[len(keep)-1].Segments() + + if segments[0] == last[0] && segments[1] == last[1] { + // current X.Y == last X.Y, replace last with current + keep[len(keep)-1] = v + } else { + // current X.Y != last X.Y, append + keep = append(keep, v) + } + } + + // Create a new map of canonicalized versions to urls + urls := make(map[string]string, len(keep)) + for _, v := range keep { + origURL := all[v.Original()] + if origURL == "" { + return nil, nil, fmt.Errorf("missing version %s", v.Original()) + } + urls[v.String()] = origURL + } + + return keep, urls, nil +} + // createBinDir creates the binary directory func createBinDir(binDir string) error { // Check if the directory exists, otherwise create it From 6362e32161295fa959ebe46b93cea0ea1a9bdd72 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 14 Feb 2020 10:32:58 -0800 Subject: [PATCH 2/5] test: capture url to fix flaky test --- e2e/vault/vault_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e/vault/vault_test.go b/e2e/vault/vault_test.go index 59350a043..8c6cbe035 100644 --- a/e2e/vault/vault_test.go +++ b/e2e/vault/vault_test.go @@ -61,6 +61,7 @@ func syncVault(t *testing.T) map[string]string { g, _ := errgroup.WithContext(ctx) for ver, url := range missing { dst := filepath.Join(binDir, ver) + url := url g.Go(func() error { sema <- 1 defer func() { From 63791d645c07db517af9ccceb52fcdafb02bc822 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 14 Feb 2020 10:33:17 -0800 Subject: [PATCH 3/5] test: sort vault tests by version --- e2e/vault/vault_test.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/e2e/vault/vault_test.go b/e2e/vault/vault_test.go index 8c6cbe035..5add49a57 100644 --- a/e2e/vault/vault_test.go +++ b/e2e/vault/vault_test.go @@ -33,14 +33,15 @@ var ( ) // syncVault discovers available versions of Vault, downloads the binaries, -// returns a map of version to binary path. -func syncVault(t *testing.T) map[string]string { +// returns a map of version to binary path as well as a sorted list of +// versions. +func syncVault(t *testing.T) ([]*version.Version, map[string]string) { binDir := filepath.Join(os.TempDir(), "vault-bins/") urls := vaultVersions(t) - _, versions, err := pruneVersions(urls) + sorted, versions, err := pruneVersions(urls) require.NoError(t, err) // Get the binaries we need to download @@ -79,7 +80,7 @@ func syncVault(t *testing.T) map[string]string { for ver, _ := range versions { binaries[ver] = filepath.Join(binDir, ver) } - return binaries + return sorted, binaries } // vaultVersions discovers available Vault versions from releases.hashicorp.com @@ -310,12 +311,13 @@ func TestVaultCompatibility(t *testing.T) { t.Skip("skipping test in non-integration mode: add -integration flag to run") } - vaultBinaries := syncVault(t) + sorted, vaultBinaries := syncVault(t) - for version, vaultBin := range vaultBinaries { - ver := version - bin := vaultBin - t.Run(version, func(t *testing.T) { + for _, v := range sorted { + ver := v.String() + bin := vaultBinaries[ver] + require.NotZerof(t, bin, "missing version: %s", ver) + t.Run(ver, func(t *testing.T) { testVaultCompatibility(t, bin, ver) }) } @@ -389,11 +391,8 @@ func setupVault(t *testing.T, client *vapi.Client, vaultVersion string) string { // pre-0.9.0 vault servers do not work with our new vault client for the policy endpoint // perform this using a raw HTTP request - newApi, _ := version.NewVersion("0.9.0") - testVersion, err := version.NewVersion(vaultVersion) - if err != nil { - t.Fatalf("failed to parse test version from '%v': %v", t.Name(), err) - } + newApi := version.Must(version.NewVersion("0.9.0")) + testVersion := version.Must(version.NewVersion(vaultVersion)) if testVersion.LessThan(newApi) { body := map[string]string{ "rules": policy, From 69bee220cc3d921761e65b246664276cd1cf180b Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 14 Feb 2020 10:53:54 -0800 Subject: [PATCH 4/5] test: remove errgroup to take advantage of vet go vet would have prevented the bug fixed in 6362e32161295fa959ebe46b93cea0ea1a9bdd72 but our use of errgroup prevented that. Rip out errgroup to take advantage of vet, and remove download limiting now that we're downloading far fewer binaries overall. --- e2e/vault/vault_test.go | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/e2e/vault/vault_test.go b/e2e/vault/vault_test.go index 5add49a57..5011a3253 100644 --- a/e2e/vault/vault_test.go +++ b/e2e/vault/vault_test.go @@ -3,7 +3,6 @@ package vault import ( "archive/zip" "bytes" - "context" "encoding/json" "flag" "fmt" @@ -24,7 +23,6 @@ import ( "github.com/hashicorp/nomad/testutil" vapi "github.com/hashicorp/vault/api" "github.com/stretchr/testify/require" - "golang.org/x/sync/errgroup" ) var ( @@ -51,27 +49,24 @@ func syncVault(t *testing.T) ([]*version.Version, map[string]string) { // Create the directory for the binaries require.NoError(t, createBinDir(binDir)) - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) - defer cancel() - - // Limit to N concurrent downloads - sema := make(chan int, 5) - // Download in parallel start := time.Now() - g, _ := errgroup.WithContext(ctx) + errCh := make(chan error, len(missing)) for ver, url := range missing { dst := filepath.Join(binDir, ver) url := url - g.Go(func() error { - sema <- 1 - defer func() { - <-sema - }() - return getVault(dst, url) - }) + go func() { + errCh <- getVault(dst, url) + }() + } + for i := 0; i < len(missing); i++ { + select { + case err := <-errCh: + require.NoError(t, err) + case <-time.After(5 * time.Minute): + t.Fatalf("timed out downloading Vault binaries") + } } - require.NoError(t, g.Wait()) if n := len(missing); n > 0 { t.Logf("Downloaded %d versions of Vault in %s", n, time.Now().Sub(start)) } From eecc8600b2dd3ae10fef7000ec612f43e4c20013 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 14 Feb 2020 11:10:33 -0800 Subject: [PATCH 5/5] test: explicitly pass vars vs enclosing them --- e2e/vault/vault_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/e2e/vault/vault_test.go b/e2e/vault/vault_test.go index 5011a3253..491c50edf 100644 --- a/e2e/vault/vault_test.go +++ b/e2e/vault/vault_test.go @@ -53,11 +53,9 @@ func syncVault(t *testing.T) ([]*version.Version, map[string]string) { start := time.Now() errCh := make(chan error, len(missing)) for ver, url := range missing { - dst := filepath.Join(binDir, ver) - url := url - go func() { + go func(dst, url string) { errCh <- getVault(dst, url) - }() + }(filepath.Join(binDir, ver), url) } for i := 0; i < len(missing); i++ { select {