From 52357707aedcde2753c44776481b02e679cf165a Mon Sep 17 00:00:00 2001
From: Renato Costa <renato@cockroachlabs.com>
Date: Wed, 24 Aug 2022 12:45:10 -0400
Subject: [PATCH] fix incorrect use of loop variable

This fixes a couple of references to loop variables in parallel tests
and deferred functions. When running a parallel test (calling
`t.Parallel()`) combined with the table-driven pattern, it's necessary
to copy the test case loop variable, otherwise only the last test case
is exercised. This is documented in the `testing` package:

https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks

`defer` statements that invoke a closure should also not reference a
loop variable directly as the referenced value will change in each
iteration of the loop.

Issues were automatically found with the `loopvarcapture` linter.
---
 changelog/16872.txt                      |  3 ++
 command/agent/approle_end_to_end_test.go |  1 +
 command/kv_test.go                       |  1 +
 command/operator_diagnose.go             | 37 ++++++++++++------------
 command/server.go                        | 22 ++++++++------
 5 files changed, 36 insertions(+), 28 deletions(-)
 create mode 100644 changelog/16872.txt

diff --git a/changelog/16872.txt b/changelog/16872.txt
new file mode 100644
index 000000000000..5bbad14067bd
--- /dev/null
+++ b/changelog/16872.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+agent: fix incorrectly used loop variables in parallel tests and when finalizing seals
+```
diff --git a/command/agent/approle_end_to_end_test.go b/command/agent/approle_end_to_end_test.go
index 35186cd8e606..2c3e52dfb3a4 100644
--- a/command/agent/approle_end_to_end_test.go
+++ b/command/agent/approle_end_to_end_test.go
@@ -54,6 +54,7 @@ func TestAppRoleEndToEnd(t *testing.T) {
 		if tc.removeSecretIDFile {
 			secretFileAction = "remove"
 		}
+		tc := tc // capture range variable
 		t.Run(fmt.Sprintf("%s_secret_id_file bindSecretID=%v secretIDLess=%v expectToken=%v", secretFileAction, tc.bindSecretID, tc.secretIDLess, tc.expectToken), func(t *testing.T) {
 			t.Parallel()
 			testAppRoleEndToEnd(t, tc.removeSecretIDFile, tc.bindSecretID, tc.secretIDLess, tc.expectToken)
diff --git a/command/kv_test.go b/command/kv_test.go
index d3b131d4371a..eba5910435db 100644
--- a/command/kv_test.go
+++ b/command/kv_test.go
@@ -716,6 +716,7 @@ func TestKVPatchCommand_ArgValidation(t *testing.T) {
 	}
 
 	for _, tc := range cases {
+		tc := tc // capture range variable
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
diff --git a/command/operator_diagnose.go b/command/operator_diagnose.go
index cdabf139011c..3973ba7716d4 100644
--- a/command/operator_diagnose.go
+++ b/command/operator_diagnose.go
@@ -446,26 +446,25 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error
 		goto SEALFAIL
 	}
 
-	if seals != nil {
-		for _, seal := range seals {
-			// There is always one nil seal. We need to skip it so we don't start an empty Finalize-Seal-Shamir
-			// section.
-			if seal == nil {
-				continue
-			}
-			// Ensure that the seal finalizer is called, even if using verify-only
-			defer func(seal *vault.Seal) {
-				sealType := diagnose.CapitalizeFirstLetter((*seal).BarrierType().String())
-				finalizeSealContext, finalizeSealSpan := diagnose.StartSpan(ctx, "Finalize "+sealType+" Seal")
-				err = (*seal).Finalize(finalizeSealContext)
-				if err != nil {
-					diagnose.Fail(finalizeSealContext, "Error finalizing seal.")
-					diagnose.Advise(finalizeSealContext, "This likely means that the barrier is still in use; therefore, finalizing the seal timed out.")
-					finalizeSealSpan.End()
-				}
-				finalizeSealSpan.End()
-			}(&seal)
+	for _, seal := range seals {
+		// There is always one nil seal. We need to skip it so we don't start an empty Finalize-Seal-Shamir
+		// section.
+		if seal == nil {
+			continue
 		}
+		seal := seal // capture range variable
+		// Ensure that the seal finalizer is called, even if using verify-only
+		defer func(seal *vault.Seal) {
+			sealType := diagnose.CapitalizeFirstLetter((*seal).BarrierType().String())
+			finalizeSealContext, finalizeSealSpan := diagnose.StartSpan(ctx, "Finalize "+sealType+" Seal")
+			err = (*seal).Finalize(finalizeSealContext)
+			if err != nil {
+				diagnose.Fail(finalizeSealContext, "Error finalizing seal.")
+				diagnose.Advise(finalizeSealContext, "This likely means that the barrier is still in use; therefore, finalizing the seal timed out.")
+				finalizeSealSpan.End()
+			}
+			finalizeSealSpan.End()
+		}(&seal)
 	}
 
 	if barrierSeal == nil {
diff --git a/command/server.go b/command/server.go
index 8e6fdf37d0d1..d8e8c3d3b49d 100644
--- a/command/server.go
+++ b/command/server.go
@@ -1325,16 +1325,20 @@ func (c *ServerCommand) Run(args []string) int {
 		return 1
 	}
 
-	if seals != nil {
-		for _, seal := range seals {
-			// Ensure that the seal finalizer is called, even if using verify-only
-			defer func(seal *vault.Seal) {
-				err = (*seal).Finalize(context.Background())
-				if err != nil {
-					c.UI.Error(fmt.Sprintf("Error finalizing seals: %v", err))
-				}
-			}(&seal)
+	for _, seal := range seals {
+		// There is always one nil seal. We need to skip it so we don't start an empty Finalize-Seal-Shamir
+		// section.
+		if seal == nil {
+			continue
 		}
+		seal := seal // capture range variable
+		// Ensure that the seal finalizer is called, even if using verify-only
+		defer func(seal *vault.Seal) {
+			err = (*seal).Finalize(context.Background())
+			if err != nil {
+				c.UI.Error(fmt.Sprintf("Error finalizing seals: %v", err))
+			}
+		}(&seal)
 	}
 
 	if barrierSeal == nil {