Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion command/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,12 @@ func (c *InitCommand) Run(args []string) int {
// on a previous run) we'll use the current state as a potential source
// of provider dependencies.
if back != nil {
sMgr, err := back.StateMgr(c.Workspace())
workspace, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
sMgr, err := back.StateMgr(workspace)
if err != nil {
c.Ui.Error(fmt.Sprintf("Error loading state: %s", err))
return 1
Expand Down
22 changes: 16 additions & 6 deletions command/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,12 @@ const (

// contextOpts returns the options to use to initialize a Terraform
// context with the settings from this Meta.
func (m *Meta) contextOpts() *terraform.ContextOpts {
func (m *Meta) contextOpts() (*terraform.ContextOpts, error) {
workspace, err := m.Workspace()
if err != nil {
return nil, err
}

var opts terraform.ContextOpts
opts.Hooks = []terraform.Hook{m.uiHook()}
opts.Hooks = append(opts.Hooks, m.ExtraHooks...)
Expand Down Expand Up @@ -379,10 +384,10 @@ func (m *Meta) contextOpts() *terraform.ContextOpts {
}

opts.Meta = &terraform.ContextMeta{
Env: m.Workspace(),
Env: workspace,
}

return &opts
return &opts, nil
}

// defaultFlagSet creates a default flag set for commands.
Expand Down Expand Up @@ -599,11 +604,16 @@ func (m *Meta) outputShadowError(err error, output bool) bool {
// and `terraform workspace delete`.
const WorkspaceNameEnvVar = "TF_WORKSPACE"

var invalidWorkspaceNameEnvVar = fmt.Errorf("Invalid workspace name set using %s", WorkspaceNameEnvVar)

// Workspace returns the name of the currently configured workspace, corresponding
// to the desired named state.
func (m *Meta) Workspace() string {
current, _ := m.WorkspaceOverridden()
return current
func (m *Meta) Workspace() (string, error) {
current, overridden := m.WorkspaceOverridden()
if overridden && !validWorkspaceName(current) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you're only checking if the workspace name is valid when it's the result of an override? (I presume it's checked elsewhere/before this runs, but if so that might be a helpful comment!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extremely good question, and it highlights that I haven't thought this all the way through yet! 🙏

My concern here was that because of this bug, users may have found themselves with an invalid workspace name. I wanted to make sure that they had a way out of this situation, so if the invalid workspace is selected via the .terraform/environment file, I didn't want to throw an error here.

That said, this should at the minimum have a test. And (assuming this path is even the right one) I think it should probably also result in changes to terraform workspace select and delete so that users can deal with any invalid workspaces they've already created.

I'll think this through further and come up with some next steps. But if all this sounds like a bad idea please do let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I arrived at:

  • 7f51be8 adds a test for this branch, which hopefully explains the intent in code. Having an invalid workspace name currently selected (using the .terraform/environment file) shouldn't break everything by returning an error from Meta.Workspace.
  • 512878a ensures that users can delete workspaces which have an invalid name. This really should only have happened as a result of TF_WORKSPACE allows workspace names 'terraform workspace' does not #24564, but it seems to me that we needn't validate workspace name on delete anyway—it's closing the barn door after the horse has been badly named.

Does this all make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's closing the barn door after the horse has been badly named.
😂 😂 😂 👍

return "", invalidWorkspaceNameEnvVar
}
return current, nil
}

// WorkspaceOverridden returns the name of the currently configured workspace,
Expand Down
41 changes: 33 additions & 8 deletions command/meta_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ func (m *Meta) Backend(opts *BackendOpts) (backend.Enhanced, tfdiags.Diagnostics
}

// Setup the CLI opts we pass into backends that support it.
cliOpts := m.backendCLIOpts()
cliOpts, err := m.backendCLIOpts()
if err != nil {
diags = diags.Append(err)
return nil, diags
}
cliOpts.Validation = true

// If the backend supports CLI initialization, do it.
Expand Down Expand Up @@ -180,7 +184,10 @@ func (m *Meta) selectWorkspace(b backend.Backend) error {
}

// Get the currently selected workspace.
workspace := m.Workspace()
workspace, err := m.Workspace()
if err != nil {
return err
}

// Check if any of the existing workspaces matches the selected
// workspace and create a numbered list of existing workspaces.
Expand Down Expand Up @@ -249,7 +256,11 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags

// If the backend supports CLI initialization, do it.
if cli, ok := b.(backend.CLI); ok {
cliOpts := m.backendCLIOpts()
cliOpts, err := m.backendCLIOpts()
if err != nil {
diags = diags.Append(err)
return nil, diags
}
if err := cli.CLIInit(cliOpts); err != nil {
diags = diags.Append(fmt.Errorf(
"Error initializing backend %T: %s\n\n"+
Expand All @@ -270,7 +281,11 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags
// Otherwise, we'll wrap our state-only remote backend in the local backend
// to cause any operations to be run locally.
log.Printf("[TRACE] Meta.Backend: backend %T does not support operations, so wrapping it in a local backend", b)
cliOpts := m.backendCLIOpts()
cliOpts, err := m.backendCLIOpts()
if err != nil {
diags = diags.Append(err)
return nil, diags
}
cliOpts.Validation = false // don't validate here in case config contains file(...) calls where the file doesn't exist
local := backendLocal.NewWithBackend(b)
if err := local.CLIInit(cliOpts); err != nil {
Expand All @@ -283,18 +298,22 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags

// backendCLIOpts returns a backend.CLIOpts object that should be passed to
// a backend that supports local CLI operations.
func (m *Meta) backendCLIOpts() *backend.CLIOpts {
func (m *Meta) backendCLIOpts() (*backend.CLIOpts, error) {
contextOpts, err := m.contextOpts()
if err != nil {
return nil, err
}
return &backend.CLIOpts{
CLI: m.Ui,
CLIColor: m.Colorize(),
ShowDiagnostics: m.showDiagnostics,
StatePath: m.statePath,
StateOutPath: m.stateOutPath,
StateBackupPath: m.backupPath,
ContextOpts: m.contextOpts(),
ContextOpts: contextOpts,
Input: m.Input(),
RunningInAutomation: m.RunningInAutomation,
}
}, nil
}

// IsLocalBackend returns true if the backend is a local backend. We use this
Expand All @@ -318,7 +337,13 @@ func (m *Meta) IsLocalBackend(b backend.Backend) bool {
// be called.
func (m *Meta) Operation(b backend.Backend) *backend.Operation {
schema := b.ConfigSchema()
workspace := m.Workspace()
workspace, err := m.Workspace()
if err != nil {
// An invalid workspace error would have been raised when creating the
// backend, and the caller should have already exited. Seeing the error
// here first is a bug, so panic.
panic(fmt.Sprintf("invalid workspace: %s", err))
}
planOutBackend, err := m.backendState.ForPlan(schema, workspace)
if err != nil {
// Always indicates an implementation error in practice, because
Expand Down
10 changes: 8 additions & 2 deletions command/meta_backend_migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ func (m *Meta) backendMigrateState_S_S(opts *backendMigrateOpts) error {
func (m *Meta) backendMigrateState_S_s(opts *backendMigrateOpts) error {
log.Printf("[TRACE] backendMigrateState: target backend type %q does not support named workspaces", opts.TwoType)

currentEnv := m.Workspace()
currentEnv, err := m.Workspace()
if err != nil {
return err
}

migrate := opts.force
if !migrate {
Expand Down Expand Up @@ -261,9 +264,12 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
return nil, err
}

// Ignore invalid workspace name as it is irrelevant in this context.
workspace, _ := m.Workspace()

// If the currently selected workspace is the default workspace, then set
// the named workspace as the new selected workspace.
if m.Workspace() == backend.DefaultStateName {
if workspace == backend.DefaultStateName {
if err := m.SetWorkspace(opts.twoEnv); err != nil {
return nil, fmt.Errorf("Failed to set new workspace: %s", err)
}
Expand Down
6 changes: 5 additions & 1 deletion command/meta_backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,11 @@ func TestMetaBackend_configuredChangeCopy_multiToSingle(t *testing.T) {
}

// Verify we are now in the default env, or we may not be able to access the new backend
if env := m.Workspace(); env != backend.DefaultStateName {
env, err := m.Workspace()
if err != nil {
t.Fatal(err)
}
if env != backend.DefaultStateName {
t.Fatal("using non-default env with single-env backend")
}
}
Expand Down
82 changes: 79 additions & 3 deletions command/meta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/google/go-cmp/cmp"

"github.com/hashicorp/terraform/backend"
"github.com/hashicorp/terraform/backend/local"
"github.com/hashicorp/terraform/terraform"
)

Expand Down Expand Up @@ -170,7 +171,10 @@ func TestMeta_Env(t *testing.T) {

m := new(Meta)

env := m.Workspace()
env, err := m.Workspace()
if err != nil {
t.Fatal(err)
}

if env != backend.DefaultStateName {
t.Fatalf("expected env %q, got env %q", backend.DefaultStateName, env)
Expand All @@ -181,7 +185,7 @@ func TestMeta_Env(t *testing.T) {
t.Fatal("error setting env:", err)
}

env = m.Workspace()
env, _ = m.Workspace()
if env != testEnv {
t.Fatalf("expected env %q, got env %q", testEnv, env)
}
Expand All @@ -190,12 +194,84 @@ func TestMeta_Env(t *testing.T) {
t.Fatal("error setting env:", err)
}

env = m.Workspace()
env, _ = m.Workspace()
if env != backend.DefaultStateName {
t.Fatalf("expected env %q, got env %q", backend.DefaultStateName, env)
}
}

func TestMeta_Workspace_override(t *testing.T) {
defer func(value string) {
os.Setenv(WorkspaceNameEnvVar, value)
}(os.Getenv(WorkspaceNameEnvVar))

m := new(Meta)

testCases := map[string]struct {
workspace string
err error
}{
"": {
"default",
nil,
},
"development": {
"development",
nil,
},
"invalid name": {
"",
invalidWorkspaceNameEnvVar,
},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
os.Setenv(WorkspaceNameEnvVar, name)
workspace, err := m.Workspace()
if workspace != tc.workspace {
t.Errorf("Unexpected workspace\n got: %s\nwant: %s\n", workspace, tc.workspace)
}
if err != tc.err {
t.Errorf("Unexpected error\n got: %s\nwant: %s\n", err, tc.err)
}
})
}
}

func TestMeta_Workspace_invalidSelected(t *testing.T) {
td := tempDir(t)
os.MkdirAll(td, 0755)
defer os.RemoveAll(td)
defer testChdir(t, td)()

// this is an invalid workspace name
workspace := "test workspace"

// create the workspace directories
if err := os.MkdirAll(filepath.Join(local.DefaultWorkspaceDir, workspace), 0755); err != nil {
t.Fatal(err)
}

// create the workspace file to select it
if err := os.MkdirAll(DefaultDataDir, 0755); err != nil {
t.Fatal(err)
}
if err := ioutil.WriteFile(filepath.Join(DefaultDataDir, local.DefaultWorkspaceFile), []byte(workspace), 0644); err != nil {
t.Fatal(err)
}

m := new(Meta)

ws, err := m.Workspace()
if ws != workspace {
t.Errorf("Unexpected workspace\n got: %s\nwant: %s\n", ws, workspace)
}
if err != nil {
t.Errorf("Unexpected error: %s", err)
}
}

func TestMeta_process(t *testing.T) {
test = false
defer func() { test = true }()
Expand Down
6 changes: 5 additions & 1 deletion command/output.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,11 @@ func (c *OutputCommand) Run(args []string) int {
return 1
}

env := c.Workspace()
env, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}

// Get the state
stateStore, err := b.StateMgr(env)
Expand Down
7 changes: 6 additions & 1 deletion command/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,12 @@ func (c *PlanCommand) Run(args []string) int {
}
var backendForPlan plans.Backend
backendForPlan.Type = backendPseudoState.Type
backendForPlan.Workspace = c.Workspace()
workspace, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
backendForPlan.Workspace = workspace

// Configuration is a little more awkward to handle here because it's
// stored in state as raw JSON but we need it as a plans.DynamicValue
Expand Down
6 changes: 5 additions & 1 deletion command/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ func (c *ProvidersCommand) Run(args []string) int {
}

// Get the state
env := c.Workspace()
env, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
s, err := b.StateMgr(env)
if err != nil {
c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
Expand Down
6 changes: 5 additions & 1 deletion command/show.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,11 @@ func (c *ShowCommand) Run(args []string) int {
}
}
} else {
env := c.Workspace()
env, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
stateFile, stateErr = getStateFromEnv(b, env)
if stateErr != nil {
c.Ui.Error(stateErr.Error())
Expand Down
6 changes: 5 additions & 1 deletion command/state_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ func (c *StateListCommand) Run(args []string) int {
}

// Get the state
env := c.Workspace()
env, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
stateMgr, err := b.StateMgr(env)
if err != nil {
c.Ui.Error(fmt.Sprintf(errStateLoadingState, err))
Expand Down
5 changes: 4 additions & 1 deletion command/state_meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ func (c *StateMeta) State() (statemgr.Full, error) {
return nil, backendDiags.Err()
}

workspace := c.Workspace()
workspace, err := c.Workspace()
if err != nil {
return nil, err
}
// Get the state
s, err := b.StateMgr(workspace)
if err != nil {
Expand Down
6 changes: 5 additions & 1 deletion command/state_pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ func (c *StatePullCommand) Run(args []string) int {
}

// Get the state manager for the current workspace
env := c.Workspace()
env, err := c.Workspace()
if err != nil {
c.Ui.Error(fmt.Sprintf("Error selecting workspace: %s", err))
return 1
}
stateMgr, err := b.StateMgr(env)
if err != nil {
c.Ui.Error(fmt.Sprintf(errStateLoadingState, err))
Expand Down
Loading