fix incorrect use of loop variable

[renatolabs]

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.

[hashicorp-cla]

All committers have signed the CLA.

[peteski22]

Hi @renatolabs, thanks for your submission! Please could I get you to add a change log entry (https://github.com/hashicorp/vault/blob/main/CONTRIBUTING.md#changelog-entries)?

[renatolabs]

Hi @renatolabs, thanks for your submission! Please could I get you to add a change log entry (https://github.com/hashicorp/vault/blob/main/CONTRIBUTING.md#changelog-entries)?

Done, thank you for the reminder!

[maxcoulombe]

Hey @renatolabs, thanks for this contribution!

False positives in tests can be really sneaky, I'm glad this can be fixed before a problem gets introduced unnoticed.

Would you mind triggering the CI so it has a chance to pass before I can approve the PR?
You can either apply the suggested change to the changelog or push an empty commit to trigger the validation steps:
git commit --allow-empty -m "trigger ci" && git push origin X

[maxcoulombe]

Since the changes eliminate a redundant check and fixes tests hiding possible problems, but do not fix a logic error in the business code itself, may I suggest updating this to improvement instead of bug?

[renatolabs]

Sure, changed to improvement 👍

[maxcoulombe]

Seems like the test failures are not flukes. I'm able to reproduce them when running the pull-request locally with make test. I'd be ok to approve the changes if we can get a green test run.

I tried the proposed changes on the tip of hashicorp/vault and it seems to pass. Would it be possible to rebase to a newer commit to see if it does the trick? Thanks again.

[renatolabs]

Seems like the test failures are not flukes. I'm able to reproduce them when running the pull-request locally with make test. I'd be ok to approve the changes if we can get a green test run.

I tried the proposed changes on the tip of hashicorp/vault and it seems to pass. Would it be possible to rebase to a newer commit to see if it does the trick? Thanks again.

Rebased 👍

[ncabatoff]

Looks like there's a panic, which I think is introduced by this change:

[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x43f9231]

goroutine 24844 [running]:
github.com/hashicorp/vault/command.(*ServerCommand).Run.func2(0xc000095400?)
	/home/circleci/go/src/github.com/hashicorp/vault/command/server.go:1333 +0x31
github.com/hashicorp/vault/command.(*ServerCommand).Run(0xc00120b860, {0xc002ab21e0, 0x2, 0x2})
	/home/circleci/go/src/github.com/hashicorp/vault/command/server.go:1773 +0x41e5
github.com/hashicorp/vault/command.TestServer_ReloadListener.func1()
	/home/circleci/go/src/github.com/hashicorp/vault/command/server_test.go:140 +0x5f
created by github.com/hashicorp/vault/command.TestServer_ReloadListener
	/home/circleci/go/src/github.com/hashicorp/vault/command/server_test.go:139 +0x56c

[renatolabs]

I suppose this is related to this section

// 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
}

which exists in command/operator_diagnose.go but not in command/server.go. So in that sense, this commit exposed (rather than introduced) the issue.

But I'll let folks with more knowledge of the codebase to weigh in; let me know if you want me to any changes here!

[ncabatoff]

But I'll let folks with more knowledge of the codebase to weigh in; let me know if you want me to any changes here!

Well, I agree with your analysis: you've exposed a bug, not introduced one. And I agree it makes sense to check for nil seals in that loop so we don't panic.

[renatolabs]

But I'll let folks with more knowledge of the codebase to weigh in; let me know if you want me to any changes here!

Well, I agree with your analysis: you've exposed a bug, not introduced one. And I agree it makes sense to check for nil seals in that loop so we don't panic.

Added a similar check to command/server.go 👍

[ncabatoff]

Thanks @renatolabs !