Retry lint fixes (#19151)

* Add a make target to run lint-consul-retry on all the modules
* Cleanup sdk/testutil/retry
* Fix a bunch of retry.Run* usage to not use the outer testing.T
* Fix some more recent retry lint issues and pin to v1.4.0 of lint-consul-retry
* Fix codegen copywrite lint issues
* Don’t perform cleanup after each retry attempt by default.
* Use the common testutil.TestingTB interface in test-integ/tenancy
* Fix retry tests
* Update otel access logging extension test to perform requests within the retry block

.github/workflows/go-tests.yml

@@ -156,7 +156,7 @@ jobs:
     - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
       with:
         go-version-file: 'go.mod'
-    - run: go install github.com/hashicorp/lint-consul-retry@v1.3.0 && lint-consul-retry
+    - run: make lint-consul-retry
 
   lint:
     needs:

Makefile

@@ -20,7 +20,7 @@ PROTOC_GO_INJECT_TAG_VERSION='v1.3.0'
 PROTOC_GEN_GO_BINARY_VERSION='v0.1.0'
 DEEP_COPY_VERSION='bc3f5aa5735d8a54961580a3a24422c308c831c2'
 COPYWRITE_TOOL_VERSION='v0.16.4'
-LINT_CONSUL_RETRY_VERSION='v1.3.0'
+LINT_CONSUL_RETRY_VERSION='v1.4.0'
 # Go imports formatter
 GCI_VERSION='v0.11.2'
 
@@ -258,6 +258,15 @@ lint/%:
 	@echo "--> Running enumcover ($*)"
 	@cd $* && GOWORK=off enumcover ./...
 
+.PHONY: lint-consul-retry
+lint-consul-retry: $(foreach mod,$(GO_MODULES),lint-consul-retry/$(mod))
+
+.PHONY: lint-consul-retry/%
+lint-consul-retry/%: lint-tools
+	@echo "--> Running lint-consul-retry ($*)"
+	@cd $* && GOWORK=off lint-consul-retry
+
+
 # check that the test-container module only imports allowlisted packages
 # from the root consul module. Generally we don't want to allow these imports.
 # In a few specific instances though it is okay to import test definitions and

agent/agent_endpoint_test.go

@@ -1877,7 +1877,7 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) {
 	require.NoError(t, a.updateTTLCheck(checkID, api.HealthPassing, "testing-agent-reload-001"))
 
 	checkStr := func(r *retry.R, evaluator func(string) error) {
-		t.Helper()
+		r.Helper()
 		contentsStr := ""
 		// Wait for watch to be populated
 		for i := 1; i < 7; i++ {
@@ -1890,14 +1890,14 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) {
 				break
 			}
 			time.Sleep(time.Duration(i) * time.Second)
-			testutil.Logger(t).Info("Watch not yet populated, retrying")
+			testutil.Logger(r).Info("Watch not yet populated, retrying")
 		}
 		if err := evaluator(contentsStr); err != nil {
 			r.Errorf("ERROR: Test failing: %s", err)
 		}
 	}
 	ensureNothingCritical := func(r *retry.R, mustContain string) {
-		t.Helper()
+		r.Helper()
 		eval := func(contentsStr string) error {
 			if strings.Contains(contentsStr, "critical") {
 				return fmt.Errorf("MUST NOT contain critical:= %s", contentsStr)
@@ -1915,7 +1915,7 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) {
 	}
 
 	retry.RunWith(retriesWithDelay(), t, func(r *retry.R) {
-		testutil.Logger(t).Info("Consul is now ready")
+		testutil.Logger(r).Info("Consul is now ready")
 		// it should contain the output
 		checkStr(r, func(contentStr string) error {
 			if contentStr == "[]" {
@@ -4340,7 +4340,7 @@ func testDefaultSidecar(svc string, port int, fns ...func(*structs.NodeService))
 }
 
 // testCreateToken creates a Policy for the provided rules and a Token linked to that Policy.
-func testCreateToken(t *testing.T, a *TestAgent, rules string) string {
+func testCreateToken(t testutil.TestingTB, a *TestAgent, rules string) string {
 	policyName, err := uuid.GenerateUUID() // we just need a unique name for the test and UUIDs are definitely unique
 	require.NoError(t, err)
 
@@ -4369,7 +4369,7 @@ func testCreateToken(t *testing.T, a *TestAgent, rules string) string {
 	return aclResp.SecretID
 }
 
-func testCreatePolicy(t *testing.T, a *TestAgent, name, rules string) string {
+func testCreatePolicy(t testutil.TestingTB, a *TestAgent, name, rules string) string {
 	args := map[string]interface{}{
 		"Name":  name,
 		"Rules": rules,

agent/agent_test.go

@@ -90,7 +90,7 @@ func requireServiceMissing(t *testing.T, a *TestAgent, id string) {
 	require.Nil(t, getService(a, id), "have service %q (expected missing)", id)
 }
 
-func requireCheckExists(t *testing.T, a *TestAgent, id types.CheckID) *structs.HealthCheck {
+func requireCheckExists(t testutil.TestingTB, a *TestAgent, id types.CheckID) *structs.HealthCheck {
 	t.Helper()
 	chk := getCheck(a, id)
 	require.NotNil(t, chk, "missing check %q", id)
@@ -853,7 +853,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) {
 	assert.NoError(t, err)
 
 	retry.Run(t, func(r *retry.R) {
-		t.Helper()
+		r.Helper()
 		var args structs.NodeSpecificRequest
 		args.Datacenter = "dc1"
 		args.Node = "node1"
@@ -1888,7 +1888,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 
 	// We do this so that the agent logs and the informational messages from
 	// the test itself are interwoven properly.
-	logf := func(t *testing.T, a *TestAgent, format string, args ...interface{}) {
+	logf := func(a *TestAgent, format string, args ...interface{}) {
 		a.logger.Info("testharness: " + fmt.Sprintf(format, args...))
 	}
 
@@ -1947,12 +1947,12 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 	retryUntilCheckState := func(t *testing.T, a *TestAgent, checkID string, expectedStatus string) {
 		t.Helper()
 		retry.Run(t, func(r *retry.R) {
-			chk := requireCheckExists(t, a, types.CheckID(checkID))
+			chk := requireCheckExists(r, a, types.CheckID(checkID))
 			if chk.Status != expectedStatus {
-				logf(t, a, "check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status)
+				logf(a, "check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status)
 				r.Fatalf("check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status)
 			}
-			logf(t, a, "check %q has reached desired status %q", checkID, expectedStatus)
+			logf(a, "check %q has reached desired status %q", checkID, expectedStatus)
 		})
 	}
 
@@ -1963,7 +1963,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 	retryUntilCheckState(t, a, "service:ping", api.HealthPassing)
 	retryUntilCheckState(t, a, "service:ping-sidecar-proxy", api.HealthPassing)
 
-	logf(t, a, "==== POWERING DOWN ORIGINAL ====")
+	logf(a, "==== POWERING DOWN ORIGINAL ====")
 
 	require.NoError(t, a.Shutdown())
 
@@ -1985,7 +1985,7 @@ node_name = "` + a.Config.NodeName + `"
 		// reregister during standup; we use an adjustable timing to try and force a race
 		sleepDur := time.Duration(idx+1) * 500 * time.Millisecond
 		time.Sleep(sleepDur)
-		logf(t, a2, "re-registering checks and services after a delay of %v", sleepDur)
+		logf(a2, "re-registering checks and services after a delay of %v", sleepDur)
 		for i := 0; i < 20; i++ { // RACE RACE RACE!
 			registerServicesAndChecks(t, a2)
 			time.Sleep(50 * time.Millisecond)
@@ -1995,7 +1995,7 @@ node_name = "` + a.Config.NodeName + `"
 
 		retryUntilCheckState(t, a2, "service:ping", api.HealthPassing)
 
-		logf(t, a2, "giving the alias check a chance to notice...")
+		logf(a2, "giving the alias check a chance to notice...")
 		time.Sleep(5 * time.Second)
 
 		retryUntilCheckState(t, a2, "service:ping-sidecar-proxy", api.HealthPassing)

agent/catalog_endpoint_test.go

@@ -1167,7 +1167,7 @@ func TestCatalogServiceNodes_DistanceSort(t *testing.T) {
 			r.Fatalf("err: %v", err)
 		}
 
-		assertIndex(t, resp)
+		assertIndex(r, resp)
 		nodes = obj.(structs.ServiceNodes)
 		if len(nodes) != 2 {
 			r.Fatalf("bad: %v", obj)

agent/connect/ca/provider_vault_test.go

@@ -295,7 +295,7 @@ func TestVaultCAProvider_ConfigureFailureGoroutineLeakCheck(t *testing.T) {
 			profile := pprof.Lookup("goroutine")
 			sb := strings.Builder{}
 			require.NoError(r, profile.WriteTo(&sb, 2))
-			t.Log(sb.String())
+			r.Log(sb.String())
 			require.Contains(r, sb.String(),
 				"created by github.com/hashicorp/consul/agent/connect/ca.(*VaultProvider).Configure",
 				"expected renewal goroutine, got none")

agent/connect/ca/testing.go

@@ -126,7 +126,7 @@ func SkipIfVaultNotPresent(t testing.T, reqs ...vaultRequirements) {
 	}
 }
 
-func NewTestVaultServer(t testing.T) *TestVaultServer {
+func NewTestVaultServer(t retry.TestingTB) *TestVaultServer {
 	vaultBinaryName := os.Getenv("VAULT_BINARY_NAME")
 	if vaultBinaryName == "" {
 		vaultBinaryName = "vault"
@@ -204,7 +204,7 @@ func (v *TestVaultServer) Client() *vaultapi.Client {
 	return v.client
 }
 
-func (v *TestVaultServer) WaitUntilReady(t testing.T) {
+func (v *TestVaultServer) WaitUntilReady(t retry.TestingTB) {
 	var version string
 	retry.Run(t, func(r *retry.R) {
 		resp, err := v.client.Sys().Health()

agent/connect/testing_spiffe.go

@@ -3,24 +3,22 @@
 
 package connect
 
-import (
-	"github.com/mitchellh/go-testing-interface"
-)
+import "github.com/hashicorp/consul/sdk/testutil"
 
 // TestSpiffeIDService returns a SPIFFE ID representing a service.
-func TestSpiffeIDService(t testing.T, service string) *SpiffeIDService {
+func TestSpiffeIDService(t testutil.TestingTB, service string) *SpiffeIDService {
 	return TestSpiffeIDServiceWithHost(t, service, TestClusterID+".consul")
 }
 
 // TestSpiffeIDServiceWithHost returns a SPIFFE ID representing a service with
 // the specified trust domain.
-func TestSpiffeIDServiceWithHost(t testing.T, service, host string) *SpiffeIDService {
+func TestSpiffeIDServiceWithHost(t testutil.TestingTB, service, host string) *SpiffeIDService {
 	return TestSpiffeIDServiceWithHostDC(t, service, host, "dc1")
 }
 
 // TestSpiffeIDServiceWithHostDC returns a SPIFFE ID representing a service with
 // the specified trust domain for the given datacenter.
-func TestSpiffeIDServiceWithHostDC(t testing.T, service, host, datacenter string) *SpiffeIDService {
+func TestSpiffeIDServiceWithHostDC(t testutil.TestingTB, service, host, datacenter string) *SpiffeIDService {
 	return &SpiffeIDService{
 		Host:       host,
 		Namespace:  "default",

agent/consul/client_test.go

@@ -509,7 +509,7 @@ func newClient(t *testing.T, config *Config) *Client {
 	return client
 }
 
-func newTestResolverConfig(t *testing.T, suffix string, dc, agentType string) resolver.Config {
+func newTestResolverConfig(t testutil.TestingTB, suffix string, dc, agentType string) resolver.Config {
 	n := t.Name()
 	s := strings.Replace(n, "/", "", -1)
 	s = strings.Replace(s, "_", "", -1)
@@ -520,7 +520,7 @@ func newTestResolverConfig(t *testing.T, suffix string, dc, agentType string) re
 	}
 }
 
-func newDefaultDeps(t *testing.T, c *Config) Deps {
+func newDefaultDeps(t testutil.TestingTB, c *Config) Deps {
 	t.Helper()
 
 	logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{

agent/consul/enterprise_server_ce_test.go

@@ -6,12 +6,11 @@
 package consul
 
 import (
-	"testing"
-
+	"github.com/hashicorp/consul/sdk/testutil"
 	hclog "github.com/hashicorp/go-hclog"
 )
 
-func newDefaultDepsEnterprise(t *testing.T, _ hclog.Logger, _ *Config) EnterpriseDeps {
+func newDefaultDepsEnterprise(t testutil.TestingTB, _ hclog.Logger, _ *Config) EnterpriseDeps {
 	t.Helper()
 	return EnterpriseDeps{}
 }

agent/consul/server_test.go

@@ -121,7 +121,7 @@ func waitForLeaderEstablishment(t *testing.T, servers ...*Server) {
 	})
 }
 
-func testServerConfig(t *testing.T) (string, *Config) {
+func testServerConfig(t testutil.TestingTB) (string, *Config) {
 	dir := testutil.TempDir(t, "consul")
 	config := DefaultConfig()
 
@@ -237,7 +237,7 @@ func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *S
 	var deps Deps
 	// Retry added to avoid cases where bind addr is already in use
 	retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) {
-		dir, config = testServerConfig(t)
+		dir, config = testServerConfig(r)
 		for _, fn := range configOpts {
 			fn(config)
 		}
@@ -250,8 +250,8 @@ func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *S
 		config.ACLResolverSettings.EnterpriseMeta = *config.AgentEnterpriseMeta()
 
 		var err error
-		deps = newDefaultDeps(t, config)
-		srv, err = newServerWithDeps(t, config, deps)
+		deps = newDefaultDeps(r, config)
+		srv, err = newServerWithDeps(r, config, deps)
 		if err != nil {
 			r.Fatalf("err: %v", err)
 		}
@@ -331,7 +331,7 @@ func newServer(t *testing.T, c *Config) (*Server, error) {
 	return newServerWithDeps(t, c, newDefaultDeps(t, c))
 }
 
-func newServerWithDeps(t *testing.T, c *Config, deps Deps) (*Server, error) {
+func newServerWithDeps(t testutil.TestingTB, c *Config, deps Deps) (*Server, error) {
 	// chain server up notification
 	oldNotify := c.NotifyListen
 	up := make(chan struct{})

agent/event_endpoint_test.go

@@ -234,7 +234,7 @@ func TestEventList_ACLFilter(t *testing.T) {
 
 	t.Run("token with access to one event type", func(t *testing.T) {
 		retry.Run(t, func(r *retry.R) {
-			token := testCreateToken(t, a, `
+			token := testCreateToken(r, a, `
 				event "foo" {
 					policy = "read"
 				}

agent/grpc-external/services/peerstream/stream_test.go

@@ -690,7 +690,7 @@ func TestStreamResources_Server_StreamTracker(t *testing.T) {
 			req := msg.GetRequest()
 			require.NotNil(r, req)
 			require.Equal(r, pbpeerstream.TypeURLExportedService, req.ResourceURL)
-			prototest.AssertDeepEqual(t, expectAck, msg)
+			prototest.AssertDeepEqual(r, expectAck, msg)
 		})
 
 		expect := Status{

agent/health_endpoint_test.go

@@ -258,7 +258,7 @@ func TestHealthChecksInState_DistanceSort(t *testing.T) {
 		if err != nil {
 			r.Fatalf("err: %v", err)
 		}
-		assertIndex(t, resp)
+		assertIndex(r, resp)
 		nodes = obj.(structs.HealthChecks)
 		if len(nodes) != 2 {
 			r.Fatalf("bad: %v", nodes)
@@ -613,7 +613,7 @@ func TestHealthServiceChecks_DistanceSort(t *testing.T) {
 		if err != nil {
 			r.Fatalf("err: %v", err)
 		}
-		assertIndex(t, resp)
+		assertIndex(r, resp)
 		nodes = obj.(structs.HealthChecks)
 		if len(nodes) != 2 {
 			r.Fatalf("bad: %v", obj)
@@ -1371,7 +1371,7 @@ func TestHealthServiceNodes_DistanceSort(t *testing.T) {
 		if err != nil {
 			r.Fatalf("err: %v", err)
 		}
-		assertIndex(t, resp)
+		assertIndex(r, resp)
 		nodes = obj.(structs.CheckServiceNodes)
 		if len(nodes) != 2 {
 			r.Fatalf("bad: %v", obj)

agent/http_test.go

@@ -1628,10 +1628,8 @@ func TestAllowedNets(t *testing.T) {
 }
 
 // assertIndex tests that X-Consul-Index is set and non-zero
-func assertIndex(t require.TestingT, resp *httptest.ResponseRecorder) {
-	if tt, ok := t.(*testing.T); ok {
-		tt.Helper()
-	}
+func assertIndex(t testutil.TestingTB, resp *httptest.ResponseRecorder) {
+	t.Helper()
 	require.NoError(t, checkIndex(resp))
 }
 

agent/remote_exec_test.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/go-uuid"
@@ -358,9 +359,9 @@ func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string
 	retry.Run(t, func(r *retry.R) {
 		event := &remoteExecEvent{
 			Prefix:  "_rexec",
-			Session: makeRexecSession(t, a.Agent, ""),
+			Session: makeRexecSession(r, a.Agent, ""),
 		}
-		defer destroySession(t, a.Agent, event.Session, "")
+		defer destroySession(r, a.Agent, event.Session, "")
 
 		spec := &remoteExecSpec{
 			Command: command,
@@ -429,7 +430,7 @@ func TestHandleRemoteExecFailed(t *testing.T) {
 	testHandleRemoteExec(t, "echo failing;exit 2", "failing", "2")
 }
 
-func makeRexecSession(t *testing.T, a *Agent, token string) string {
+func makeRexecSession(t testutil.TestingTB, a *Agent, token string) string {
 	args := structs.SessionRequest{
 		Datacenter: a.config.Datacenter,
 		Op:         structs.SessionCreate,
@@ -448,7 +449,7 @@ func makeRexecSession(t *testing.T, a *Agent, token string) string {
 	return out
 }
 
-func destroySession(t *testing.T, a *Agent, session string, token string) {
+func destroySession(t testutil.TestingTB, a *Agent, session string, token string) {
 	args := structs.SessionRequest{
 		Datacenter: a.config.Datacenter,
 		Op:         structs.SessionDestroy,