From f4a35dfd84921f241575a8a12c0671471ad37f1e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 23 Mar 2020 17:02:13 -0400 Subject: [PATCH 1/2] ci: Do not skip tests because of missing binaries on CI If the CI environment is not correct for running tests the tests should fail, so that we don't accidentally stop running some tests because of a change to our CI environment. Also removed a duplicate delcaration from init. I believe one was overriding the other as they are both in the same package. --- agent/connect/ca/provider_aws_test.go | 29 ++++--------------- agent/connect/ca/provider_vault_test.go | 30 +++++++------------ agent/connect/generate_test.go | 38 +++++++++++-------------- agent/connect/testing_ca_test.go | 32 +++++++-------------- 4 files changed, 44 insertions(+), 85 deletions(-) diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go index 21bedc9721..beab9402a1 100644 --- a/agent/connect/ca/provider_aws_test.go +++ b/agent/connect/ca/provider_aws_test.go @@ -10,22 +10,18 @@ import ( "github.com/stretchr/testify/require" ) -func skipIfAWSNotConfigured(t *testing.T) bool { +func skipIfAWSNotConfigured(t *testing.T) { enabled := os.Getenv("ENABLE_AWS_PCA_TESTS") ok, err := strconv.ParseBool(enabled) if err != nil || !ok { t.Skip("Skipping because AWS tests are not enabled") - return true } - return false } func TestAWSBootstrapAndSignPrimary(t *testing.T) { // Note not parallel since we could easily hit AWS limits of too many CAs if // all of these tests run at once. - if skipIfAWSNotConfigured(t) { - return - } + skipIfAWSNotConfigured(t) for _, tc := range KeyTestCases { tc := tc @@ -83,9 +79,7 @@ func testSignAndValidate(t *testing.T, p Provider, rootPEM string, intermediateP func TestAWSBootstrapAndSignSecondary(t *testing.T) { // Note not parallel since we could easily hit AWS limits of too many CAs if // all of these tests run at once. - if skipIfAWSNotConfigured(t) { - return - } + skipIfAWSNotConfigured(t) p1 := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer p1.Cleanup() @@ -179,9 +173,7 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { func TestAWSBootstrapAndSignSecondaryConsul(t *testing.T) { // Note not parallel since we could easily hit AWS limits of too many CAs if // all of these tests run at once. - if skipIfAWSNotConfigured(t) { - return - } + skipIfAWSNotConfigured(t) t.Run("pri=consul,sec=aws", func(t *testing.T) { conf := testConsulCAConfig() @@ -215,9 +207,7 @@ func TestAWSBootstrapAndSignSecondaryConsul(t *testing.T) { } func TestAWSNoCrossSigning(t *testing.T) { - if skipIfAWSNotConfigured(t) { - return - } + skipIfAWSNotConfigured(t) p1 := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer p1.Cleanup() @@ -246,15 +236,6 @@ func testAWSProvider(t *testing.T, cfg ProviderConfig) *AWSProvider { return p } -type testLogger struct { - t *testing.T -} - -func (l *testLogger) Write(b []byte) (int, error) { - l.t.Log(string(b)) - return len(b), nil -} - func testProviderConfigPrimary(t *testing.T, cfg map[string]interface{}) ProviderConfig { rawCfg := make(map[string]interface{}) for k, v := range cfg { diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index c5707f1e3d..098439aee1 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -40,9 +40,7 @@ func TestVaultCAProvider_VaultTLSConfig(t *testing.T) { func TestVaultCAProvider_Bootstrap(t *testing.T) { t.Parallel() - if skipIfVaultNotPresent(t) { - return - } + skipIfVaultNotPresent(t) provider, testVault := testVaultProvider(t) defer testVault.Stop() @@ -103,9 +101,7 @@ func assertCorrectKeyType(t *testing.T, want, certPEM string) { func TestVaultCAProvider_SignLeaf(t *testing.T) { t.Parallel() - if skipIfVaultNotPresent(t) { - return - } + skipIfVaultNotPresent(t) for _, tc := range KeyTestCases { tc := tc @@ -189,9 +185,7 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) { func TestVaultCAProvider_CrossSignCA(t *testing.T) { t.Parallel() - if skipIfVaultNotPresent(t) { - return - } + skipIfVaultNotPresent(t) tests := CASigningKeyTypeCases() @@ -246,9 +240,7 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) { func TestVaultProvider_SignIntermediate(t *testing.T) { t.Parallel() - if skipIfVaultNotPresent(t) { - return - } + skipIfVaultNotPresent(t) tests := CASigningKeyTypeCases() @@ -277,9 +269,7 @@ func TestVaultProvider_SignIntermediate(t *testing.T) { func TestVaultProvider_SignIntermediateConsul(t *testing.T) { t.Parallel() - if skipIfVaultNotPresent(t) { - return - } + skipIfVaultNotPresent(t) // primary = Vault, secondary = Consul t.Run("pri=vault,sec=consul", func(t *testing.T) { @@ -397,8 +387,9 @@ func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[strin var printedVaultVersion sync.Once -// skipIfVaultNotPresent skips the test and returns true if vault is not found -func skipIfVaultNotPresent(t *testing.T) bool { +var mustAlwaysRun = os.Getenv("CI") == "true" + +func skipIfVaultNotPresent(t *testing.T) { vaultBinaryName := os.Getenv("VAULT_BINARY_NAME") if vaultBinaryName == "" { vaultBinaryName = "vault" @@ -406,10 +397,11 @@ func skipIfVaultNotPresent(t *testing.T) bool { path, err := exec.LookPath(vaultBinaryName) if err != nil || path == "" { + if mustAlwaysRun { + t.Fatalf("%q not found on $PATH", vaultBinaryName) + } t.Skipf("%q not found on $PATH - download and install to run this test", vaultBinaryName) - return true } - return false } func runTestVault() (*testVaultServer, error) { diff --git a/agent/connect/generate_test.go b/agent/connect/generate_test.go index 226ffd853d..6d79cd4af7 100644 --- a/agent/connect/generate_test.go +++ b/agent/connect/generate_test.go @@ -17,27 +17,23 @@ type KeyConfig struct { keyBits int } -var goodParams, badParams []KeyConfig - -func init() { - goodParams = []KeyConfig{ - {keyType: "rsa", keyBits: 2048}, - {keyType: "rsa", keyBits: 4096}, - {keyType: "ec", keyBits: 224}, - {keyType: "ec", keyBits: 256}, - {keyType: "ec", keyBits: 384}, - {keyType: "ec", keyBits: 521}, - } - badParams = []KeyConfig{ - {keyType: "rsa", keyBits: 0}, - {keyType: "rsa", keyBits: 1024}, - {keyType: "rsa", keyBits: 24601}, - {keyType: "ec", keyBits: 0}, - {keyType: "ec", keyBits: 512}, - {keyType: "ec", keyBits: 321}, - {keyType: "ecdsa", keyBits: 256}, // test for "ecdsa" instead of "ec" - {keyType: "aes", keyBits: 128}, - } +var goodParams = []KeyConfig{ + {keyType: "rsa", keyBits: 2048}, + {keyType: "rsa", keyBits: 4096}, + {keyType: "ec", keyBits: 224}, + {keyType: "ec", keyBits: 256}, + {keyType: "ec", keyBits: 384}, + {keyType: "ec", keyBits: 521}, +} +var badParams = []KeyConfig{ + {keyType: "rsa", keyBits: 0}, + {keyType: "rsa", keyBits: 1024}, + {keyType: "rsa", keyBits: 24601}, + {keyType: "ec", keyBits: 0}, + {keyType: "ec", keyBits: 512}, + {keyType: "ec", keyBits: 321}, + {keyType: "ecdsa", keyBits: 256}, // test for "ecdsa" instead of "ec" + {keyType: "aes", keyBits: 128}, } func makeConfig(kc KeyConfig) structs.CommonCAProviderConfig { diff --git a/agent/connect/testing_ca_test.go b/agent/connect/testing_ca_test.go index 4fdf7ccc1a..29c0413318 100644 --- a/agent/connect/testing_ca_test.go +++ b/agent/connect/testing_ca_test.go @@ -12,29 +12,22 @@ import ( "github.com/stretchr/testify/require" ) -// hasOpenSSL is used to determine if the openssl CLI exists for unit tests. -var hasOpenSSL bool +var mustAlwaysRun = os.Getenv("CI") == "true" -func init() { - goodParams = []KeyConfig{ - {keyType: "rsa", keyBits: 2048}, - {keyType: "rsa", keyBits: 4096}, - {keyType: "ec", keyBits: 224}, - {keyType: "ec", keyBits: 256}, - {keyType: "ec", keyBits: 384}, - {keyType: "ec", keyBits: 521}, +func skipIfMissingOpenSSL(t *testing.T) { + openSSLBinaryName := "openssl" + _, err := exec.LookPath(openSSLBinaryName) + if err != nil { + if mustAlwaysRun { + t.Fatalf("%q not found on $PATH", openSSLBinaryName) + } + t.Skipf("%q not found on $PATH", openSSLBinaryName) } - - _, err := exec.LookPath("openssl") - hasOpenSSL = err == nil } // Test that the TestCA and TestLeaf functions generate valid certificates. func testCAAndLeaf(t *testing.T, keyType string, keyBits int) { - if !hasOpenSSL { - t.Skip("openssl not found") - return - } + skipIfMissingOpenSSL(t) require := require.New(t) @@ -66,10 +59,7 @@ func testCAAndLeaf(t *testing.T, keyType string, keyBits int) { // Test cross-signing. func testCAAndLeaf_xc(t *testing.T, keyType string, keyBits int) { - if !hasOpenSSL { - t.Skip("openssl not found") - return - } + skipIfMissingOpenSSL(t) assert := assert.New(t) From 61ec7aa5c9190c576a90eab2b8ca357b1cfed17e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 24 Mar 2020 15:16:13 -0400 Subject: [PATCH 2/2] ci: Run all connect/ca tests from the integration suite To reduce the chance of some tests not being run because it does not match the regex passed to '-run'. Also document why some tests are allowed to be skipped on CI. --- .circleci/config.yml | 12 ++++++------ GNUmakefile | 8 ++++---- agent/connect/ca/provider_aws_test.go | 7 +++++++ agent/connect/ca/provider_vault_test.go | 13 ++++++------- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 411363598b..8cf6b23544 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -569,8 +569,8 @@ jobs: ENVOY_VERSIONS: "1.13.0" steps: *ENVOY_INTEGRATION_TEST_STEPS - # run tests on vault ca provider integration tests - vault-ca-provider: + # run integration tests for the connect ca providers + test-connect-ca-providers: docker: - image: *GOLANG_IMAGE environment: @@ -586,7 +586,7 @@ jobs: # Gather deps to run go tests - checkout # Run go tests - - run: make test-vault-ca-provider + - run: make test-connect-ca-providers - store_test_results: path: *TEST_RESULTS_DIR @@ -635,6 +635,8 @@ workflows: - go-test - go-test-api - go-test-sdk + - test-connect-ca-providers: *go-test + build-distros: jobs: - check-vendor: @@ -686,9 +688,7 @@ workflows: - envoy-integration-test-1.13.0: requires: - dev-build - - vault-ca-provider: - requires: - - dev-build + website: jobs: - build-website diff --git a/GNUmakefile b/GNUmakefile index def8b73e5d..e611b7f531 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -381,14 +381,14 @@ ui-docker: ui-build-image test-envoy-integ: $(ENVOY_INTEG_DEPS) @$(SHELL) $(CURDIR)/test/integration/connect/envoy/run-tests.sh -test-vault-ca-provider: +test-connect-ca-providers: ifeq ("$(CIRCLECI)","true") # Run in CI - gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- $(CURDIR)/agent/connect/ca/* -run 'TestVault(CA)?Provider' + gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- ./agent/connect/ca else # Run locally - @echo "Running /agent/connect/ca TestVault(CA)?Provider tests in verbose mode" - @go test $(CURDIR)/agent/connect/ca/* -run 'TestVault(CA)?Provider' -v + @echo "Running /agent/connect/ca tests in verbose mode" + @go test -v ./agent/connect/ca endif proto-delete: diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go index beab9402a1..12cf1e3ab8 100644 --- a/agent/connect/ca/provider_aws_test.go +++ b/agent/connect/ca/provider_aws_test.go @@ -10,6 +10,13 @@ import ( "github.com/stretchr/testify/require" ) +// skipIfAWSNotConfigured skips the test unless ENABLE_AWS_PCA_TESTS=true. +// +// These tests are not run in CI. If you are making changes to the AWS provider +// you probably want to run these tests locally. The tests will run using any +// credentials available to the AWS SDK. See +// https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials +// for a list of options. func skipIfAWSNotConfigured(t *testing.T) { enabled := os.Getenv("ENABLE_AWS_PCA_TESTS") ok, err := strconv.ParseBool(enabled) diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 098439aee1..b1935365f7 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -385,10 +385,10 @@ func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[strin return provider, testVault } -var printedVaultVersion sync.Once - -var mustAlwaysRun = os.Getenv("CI") == "true" - +// skipIfVaultNotPresent skips the test if the vault binary is not in PATH. +// +// These tests may be skipped in CI. They are run as part of a separate +// integration test suite. func skipIfVaultNotPresent(t *testing.T) { vaultBinaryName := os.Getenv("VAULT_BINARY_NAME") if vaultBinaryName == "" { @@ -397,9 +397,6 @@ func skipIfVaultNotPresent(t *testing.T) { path, err := exec.LookPath(vaultBinaryName) if err != nil || path == "" { - if mustAlwaysRun { - t.Fatalf("%q not found on $PATH", vaultBinaryName) - } t.Skipf("%q not found on $PATH - download and install to run this test", vaultBinaryName) } } @@ -474,6 +471,8 @@ type testVaultServer struct { returnPortsFn func() } +var printedVaultVersion sync.Once + func (v *testVaultServer) WaitUntilReady(t *testing.T) { var version string retry.Run(t, func(r *retry.R) {