Skip to content

Commit

Permalink
fix incorrect use of loop variable (#16872)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
renatolabs authored Oct 4, 2022
1 parent 61707d5 commit eb338de
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 28 deletions.
3 changes: 3 additions & 0 deletions changelog/16872.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
agent: fix incorrectly used loop variables in parallel tests and when finalizing seals
```
1 change: 1 addition & 0 deletions command/agent/approle_end_to_end_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions command/kv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
37 changes: 18 additions & 19 deletions command/operator_diagnose.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,26 +447,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()
}
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()
}(&seal)
}
}
finalizeSealSpan.End()
}(&seal)
}

if barrierSeal == nil {
Expand Down
22 changes: 13 additions & 9 deletions command/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1337,16 +1337,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 {
Expand Down

0 comments on commit eb338de

Please sign in to comment.