From 9042643d63716bb2dc9faf5b1ade02ca0053bfa2 Mon Sep 17 00:00:00 2001
From: Alexander Scheel <alex.scheel@hashicorp.com>
Date: Tue, 31 Jan 2023 08:18:21 -0500
Subject: [PATCH] Fix command.RunCustom(...) correctly (#18904)

* Revert "Remove t.Parallel() due to initialization race (#18751)"

This reverts commit ebcd65310221aff1dfcb94a571d70e38944006df.

We're going to fix this properly, running initCommands exactly once.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Prevent parallel testing racing in initCommands(...)

When running initCommands(...) from multiple tests, they can potentially
race, causing a panic. Test callers needing to set formatting
information must use RunCustom(...) instead of directly invoking the
test backend directly. When using t.Parallel(...) in these top-level
tests, we thus could race.

This removes the Commands global variable, making it a local variable
instead as nothing else appears to use it. We'll update Enterprise to
add in the Enterprise-specific commands to the existing list.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
---
 command/commands.go              | 12 +++++-------
 command/main.go                  |  4 ++--
 command/pki_health_check_test.go | 12 ++++++++++++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/command/commands.go b/command/commands.go
index 8e8cc44bd9..4f214fe265 100644
--- a/command/commands.go
+++ b/command/commands.go
@@ -219,13 +219,10 @@ var (
 		"kubernetes": ksr.NewServiceRegistration,
 	}
 
-	initCommandsEnt = func(ui, serverCmdUi cli.Ui, runOpts *RunOptions) {}
+	initCommandsEnt = func(ui, serverCmdUi cli.Ui, runOpts *RunOptions, commands map[string]cli.CommandFactory) {}
 )
 
-// Commands is the mapping of all the available commands.
-var Commands map[string]cli.CommandFactory
-
-func initCommands(ui, serverCmdUi cli.Ui, runOpts *RunOptions) {
+func initCommands(ui, serverCmdUi cli.Ui, runOpts *RunOptions) map[string]cli.CommandFactory {
 	loginHandlers := map[string]LoginHandler{
 		"alicloud": &credAliCloud.CLIHandler{},
 		"aws":      &credAws.CLIHandler{},
@@ -258,7 +255,7 @@ func initCommands(ui, serverCmdUi cli.Ui, runOpts *RunOptions) {
 		}
 	}
 
-	Commands = map[string]cli.CommandFactory{
+	commands := map[string]cli.CommandFactory{
 		"agent": func() (cli.Command, error) {
 			return &AgentCommand{
 				BaseCommand: &BaseCommand{
@@ -841,7 +838,8 @@ func initCommands(ui, serverCmdUi cli.Ui, runOpts *RunOptions) {
 		},
 	}
 
-	initCommandsEnt(ui, serverCmdUi, runOpts)
+	initCommandsEnt(ui, serverCmdUi, runOpts, commands)
+	return commands
 }
 
 // MakeShutdownCh returns a channel that can be used for shutdown
diff --git a/command/main.go b/command/main.go
index 1ac7287506..3c6597decb 100644
--- a/command/main.go
+++ b/command/main.go
@@ -217,14 +217,14 @@ func RunCustom(args []string, runOpts *RunOptions) int {
 		return 1
 	}
 
-	initCommands(ui, serverCmdUi, runOpts)
+	commands := initCommands(ui, serverCmdUi, runOpts)
 
 	hiddenCommands := []string{"version"}
 
 	cli := &cli.CLI{
 		Name:     "vault",
 		Args:     args,
-		Commands: Commands,
+		Commands: commands,
 		HelpFunc: groupedHelpFunc(
 			cli.BasicHelpFunc("vault"),
 		),
diff --git a/command/pki_health_check_test.go b/command/pki_health_check_test.go
index a34dbdf5b8..bdd491a049 100644
--- a/command/pki_health_check_test.go
+++ b/command/pki_health_check_test.go
@@ -16,6 +16,8 @@ import (
 )
 
 func TestPKIHC_AllGood(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()
 
@@ -70,6 +72,8 @@ func TestPKIHC_AllGood(t *testing.T) {
 }
 
 func TestPKIHC_AllBad(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()
 
@@ -130,6 +134,8 @@ func TestPKIHC_AllBad(t *testing.T) {
 }
 
 func TestPKIHC_OnlyIssuer(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()
 
@@ -152,6 +158,8 @@ func TestPKIHC_OnlyIssuer(t *testing.T) {
 }
 
 func TestPKIHC_NoMount(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()
 
@@ -166,6 +174,8 @@ func TestPKIHC_NoMount(t *testing.T) {
 }
 
 func TestPKIHC_ExpectedEmptyMount(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()
 
@@ -186,6 +196,8 @@ func TestPKIHC_ExpectedEmptyMount(t *testing.T) {
 }
 
 func TestPKIHC_NoPerm(t *testing.T) {
+	t.Parallel()
+
 	client, closer := testVaultServer(t)
 	defer closer()