Merge pull request #7485 from hashicorp/dnephin/do-not-skip-tests-on-ci

ci: Make it harder to accidentally skip tests on CI, and doc why some are skipped

.circleci/config.yml

@@ -605,8 +605,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:
@@ -622,7 +622,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
 
@@ -670,6 +670,8 @@ workflows:
             - go-test
             - go-test-api
             - go-test-sdk
+      - test-connect-ca-providers: *go-test
+
   build-distros:
     jobs:
       - check-vendor:
@@ -746,9 +748,7 @@ workflows:
       - envoy-integration-test-1.13.0:
           requires:
             - dev-build
-      - vault-ca-provider:
-          requires:
-            - dev-build
+
   website:
     jobs:
       - build-website

GNUmakefile

@@ -356,14 +356,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:

agent/connect/ca/provider_aws_test.go

@@ -10,22 +10,25 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func skipIfAWSNotConfigured(t *testing.T) bool {
+// 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)
 	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 +86,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 +180,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 +214,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 +243,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 {

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) {
@@ -395,10 +385,11 @@ func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[strin
 	return provider, testVault
 }
 
-var printedVaultVersion sync.Once
-
-// skipIfVaultNotPresent skips the test and returns true if vault is not found
-func skipIfVaultNotPresent(t *testing.T) bool {
+// 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 == "" {
 		vaultBinaryName = "vault"
@@ -407,9 +398,7 @@ func skipIfVaultNotPresent(t *testing.T) bool {
 	path, err := exec.LookPath(vaultBinaryName)
 	if err != nil || path == "" {
 		t.Skipf("%q not found on $PATH - download and install to run this test", vaultBinaryName)
-		return true
 	}
-	return false
 }
 
 func runTestVault() (*testVaultServer, error) {
@@ -482,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) {

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 {

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)