From d15a2bc024bb742eb9a5b2945bb97d630135d213 Mon Sep 17 00:00:00 2001 From: Theo Chupp Date: Tue, 15 Mar 2022 17:42:11 -0400 Subject: [PATCH] fix: local variables should not be overridden by remote variables during `terraform import` (#29972) * fix: local variables should not be overridden by remote variables during `terraform import` * chore: applied the same fix in the 'internal/cloud' package * backport changes from cloud package to remote package Co-authored-by: Alisdair McDiarmid Co-authored-by: uturunku1 --- internal/backend/remote/backend.go | 39 +-- internal/backend/remote/backend_apply_test.go | 16 +- internal/backend/remote/backend_context.go | 57 +++-- .../backend/remote/backend_context_test.go | 234 ++++++++++++++++++ internal/backend/remote/backend_plan_test.go | 2 +- internal/backend/remote/backend_test.go | 30 +-- .../backend/remote/testdata/variables/main.tf | 8 + internal/cloud/backend.go | 49 ++-- internal/cloud/backend_context.go | 40 ++- internal/cloud/backend_context_test.go | 220 ++++++++++++++++ internal/cloud/testdata/variables/main.tf | 8 + 11 files changed, 607 insertions(+), 96 deletions(-) create mode 100644 internal/backend/remote/testdata/variables/main.tf create mode 100644 internal/cloud/testdata/variables/main.tf diff --git a/internal/backend/remote/backend.go b/internal/backend/remote/backend.go index 4f51966c5..47c9f2ace 100644 --- a/internal/backend/remote/backend.go +++ b/internal/backend/remote/backend.go @@ -673,19 +673,14 @@ func (b *Remote) StateMgr(name string) (statemgr.Full, error) { return &remote.State{Client: client}, nil } -// Operation implements backend.Enhanced. -func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend.RunningOperation, error) { - // Get the remote workspace name. - name := op.Workspace - switch { - case op.Workspace == backend.DefaultStateName: - name = b.workspace - case b.prefix != "" && !strings.HasPrefix(op.Workspace, b.prefix): - name = b.prefix + op.Workspace - } +func isLocalExecutionMode(execMode string) bool { + return execMode == "local" +} +func (b *Remote) fetchWorkspace(ctx context.Context, organization string, name string) (*tfe.Workspace, error) { + remoteWorkspaceName := b.getRemoteWorkspaceName(name) // Retrieve the workspace for this operation. - w, err := b.client.Workspaces.Read(ctx, b.organization, name) + w, err := b.client.Workspaces.Read(ctx, b.organization, remoteWorkspaceName) if err != nil { switch err { case context.Canceled: @@ -695,17 +690,29 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend "workspace %s not found\n\n"+ "The configured \"remote\" backend returns '404 Not Found' errors for resources\n"+ "that do not exist, as well as for resources that a user doesn't have access\n"+ - "to. If the resource does exist, please check the rights for the used token.", + "to. If the resource does exist, please check the rights for the used token", name, ) default: - return nil, fmt.Errorf( - "The configured \"remote\" backend encountered an unexpected error:\n\n%s", + err := fmt.Errorf( + "the configured \"remote\" backend encountered an unexpected error:\n\n%s", err, ) + return nil, err } } + return w, nil +} + +// Operation implements backend.Enhanced. +func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend.RunningOperation, error) { + w, err := b.fetchWorkspace(ctx, b.organization, op.Workspace) + + if err != nil { + return nil, err + } + // Terraform remote version conflicts are not a concern for operations. We // are in one of three states: // @@ -718,7 +725,7 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend b.IgnoreVersionConflict() // Check if we need to use the local backend to run the operation. - if b.forceLocal || !w.Operations { + if b.forceLocal || isLocalExecutionMode(w.ExecutionMode) { // Record that we're forced to run operations locally to allow the // command package UI to operate correctly b.forceLocal = true @@ -902,7 +909,7 @@ func (b *Remote) VerifyWorkspaceTerraformVersion(workspaceName string) tfdiags.D // If the workspace has remote operations disabled, the remote Terraform // version is effectively meaningless, so we'll skip version verification. - if !workspace.Operations { + if isLocalExecutionMode(workspace.ExecutionMode) { return nil } diff --git a/internal/backend/remote/backend_apply_test.go b/internal/backend/remote/backend_apply_test.go index 1f0d319bf..96d5c5b97 100644 --- a/internal/backend/remote/backend_apply_test.go +++ b/internal/backend/remote/backend_apply_test.go @@ -1526,36 +1526,36 @@ func TestRemote_applyVersionCheck(t *testing.T) { localVersion string remoteVersion string forceLocal bool - hasOperations bool + executionMode string wantErr string }{ "versions can be different for remote apply": { localVersion: "0.14.0", remoteVersion: "0.13.5", - hasOperations: true, + executionMode: "remote", }, "versions can be different for local apply": { localVersion: "0.14.0", remoteVersion: "0.13.5", - hasOperations: false, + executionMode: "local", }, "force local with remote operations and different versions is acceptable": { localVersion: "0.14.0", remoteVersion: "0.14.0-acme-provider-bundle", forceLocal: true, - hasOperations: true, + executionMode: "remote", }, "no error if versions are identical": { localVersion: "0.14.0", remoteVersion: "0.14.0", forceLocal: true, - hasOperations: true, + executionMode: "remote", }, "no error if force local but workspace has remote operations disabled": { localVersion: "0.14.0", remoteVersion: "0.13.5", forceLocal: true, - hasOperations: false, + executionMode: "local", }, } @@ -1591,7 +1591,7 @@ func TestRemote_applyVersionCheck(t *testing.T) { b.organization, b.workspace, tfe.WorkspaceUpdateOptions{ - Operations: tfe.Bool(tc.hasOperations), + ExecutionMode: tfe.String(tc.executionMode), TerraformVersion: tfe.String(tc.remoteVersion), }, ) @@ -1644,7 +1644,7 @@ func TestRemote_applyVersionCheck(t *testing.T) { hasRemote := strings.Contains(output, "Running apply in the remote backend") hasSummary := strings.Contains(output, "1 added, 0 changed, 0 destroyed") hasResources := run.State.HasManagedResourceInstanceObjects() - if !tc.forceLocal && tc.hasOperations { + if !tc.forceLocal && !isLocalExecutionMode(tc.executionMode) { if !hasRemote { t.Errorf("missing remote backend header in output: %s", output) } diff --git a/internal/backend/remote/backend_context.go b/internal/backend/remote/backend_context.go index 5d50d11d4..ee990c4a0 100644 --- a/internal/backend/remote/backend_context.go +++ b/internal/backend/remote/backend_context.go @@ -82,22 +82,6 @@ func (b *Remote) LocalRun(op *backend.Operation) (*backend.LocalRun, statemgr.Fu } ret.Config = config - // The underlying API expects us to use the opaque workspace id to request - // variables, so we'll need to look that up using our organization name - // and workspace name. - remoteWorkspaceID, err := b.getRemoteWorkspaceID(context.Background(), op.Workspace) - if err != nil { - diags = diags.Append(fmt.Errorf("error finding remote workspace: %w", err)) - return nil, nil, diags - } - - log.Printf("[TRACE] backend/remote: retrieving variables from workspace %s/%s (%s)", remoteWorkspaceName, b.organization, remoteWorkspaceID) - tfeVariables, err := b.client.Variables.List(context.Background(), remoteWorkspaceID, tfe.VariableListOptions{}) - if err != nil && err != tfe.ErrResourceNotFound { - diags = diags.Append(fmt.Errorf("error loading variables: %w", err)) - return nil, nil, diags - } - if op.AllowUnsetVariables { // If we're not going to use the variables in an operation we'll be // more lax about them, stubbing out any unset ones as unknown. @@ -105,14 +89,41 @@ func (b *Remote) LocalRun(op *backend.Operation) (*backend.LocalRun, statemgr.Fu // but not enough information to run a real operation (plan, apply, etc) ret.PlanOpts.SetVariables = stubAllVariables(op.Variables, config.Module.Variables) } else { - if tfeVariables != nil { - if op.Variables == nil { - op.Variables = make(map[string]backend.UnparsedVariableValue) + // The underlying API expects us to use the opaque workspace id to request + // variables, so we'll need to look that up using our organization name + // and workspace name. + remoteWorkspaceID, err := b.getRemoteWorkspaceID(context.Background(), op.Workspace) + if err != nil { + diags = diags.Append(fmt.Errorf("error finding remote workspace: %w", err)) + return nil, nil, diags + } + + w, err := b.fetchWorkspace(context.Background(), b.organization, op.Workspace) + if err != nil { + diags = diags.Append(fmt.Errorf("error loading workspace: %w", err)) + return nil, nil, diags + } + + if isLocalExecutionMode(w.ExecutionMode) { + log.Printf("[TRACE] skipping retrieving variables from workspace %s/%s (%s), workspace is in Local Execution mode", remoteWorkspaceName, b.organization, remoteWorkspaceID) + } else { + log.Printf("[TRACE] backend/remote: retrieving variables from workspace %s/%s (%s)", remoteWorkspaceName, b.organization, remoteWorkspaceID) + tfeVariables, err := b.client.Variables.List(context.Background(), remoteWorkspaceID, tfe.VariableListOptions{}) + if err != nil && err != tfe.ErrResourceNotFound { + diags = diags.Append(fmt.Errorf("error loading variables: %w", err)) + return nil, nil, diags } - for _, v := range tfeVariables.Items { - if v.Category == tfe.CategoryTerraform { - op.Variables[v.Key] = &remoteStoredVariableValue{ - definition: v, + if tfeVariables != nil { + if op.Variables == nil { + op.Variables = make(map[string]backend.UnparsedVariableValue) + } + for _, v := range tfeVariables.Items { + if v.Category == tfe.CategoryTerraform { + if _, ok := op.Variables[v.Key]; !ok { + op.Variables[v.Key] = &remoteStoredVariableValue{ + definition: v, + } + } } } } diff --git a/internal/backend/remote/backend_context_test.go b/internal/backend/remote/backend_context_test.go index 819f583ec..f3a133421 100644 --- a/internal/backend/remote/backend_context_test.go +++ b/internal/backend/remote/backend_context_test.go @@ -2,6 +2,9 @@ package remote import ( "context" + "github.com/hashicorp/terraform/internal/terraform" + "github.com/hashicorp/terraform/internal/tfdiags" + "reflect" "testing" tfe "github.com/hashicorp/go-tfe" @@ -233,3 +236,234 @@ func TestRemoteContextWithVars(t *testing.T) { }) } } + +func TestRemoteVariablesDoNotOverride(t *testing.T) { + catTerraform := tfe.CategoryTerraform + + varName1 := "key1" + varName2 := "key2" + varName3 := "key3" + + varValue1 := "value1" + varValue2 := "value2" + varValue3 := "value3" + + tests := map[string]struct { + localVariables map[string]backend.UnparsedVariableValue + remoteVariables []*tfe.VariableCreateOptions + expectedVariables terraform.InputValues + }{ + "no local variables": { + map[string]backend.UnparsedVariableValue{}, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, + { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, + { + Key: &varName3, + Value: &varValue3, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + }, + }, + "single conflicting local variable": { + map[string]backend.UnparsedVariableValue{ + varName3: testUnparsedVariableValue(varValue3), + }, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, { + Key: &varName3, + Value: &varValue3, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, + }, + }, + "no conflicting local variable": { + map[string]backend.UnparsedVariableValue{ + varName3: testUnparsedVariableValue(varValue3), + }, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + configDir := "./testdata/variables" + + b, bCleanup := testBackendDefault(t) + defer bCleanup() + + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + defer configCleanup() + + workspaceID, err := b.getRemoteWorkspaceID(context.Background(), backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } + + streams, _ := terminal.StreamsForTesting(t) + view := views.NewStateLocker(arguments.ViewHuman, views.NewView(streams)) + + op := &backend.Operation{ + ConfigDir: configDir, + ConfigLoader: configLoader, + StateLocker: clistate.NewLocker(0, view), + Workspace: backend.DefaultStateName, + Variables: test.localVariables, + } + + for _, v := range test.remoteVariables { + b.client.Variables.Create(context.TODO(), workspaceID, *v) + } + + lr, _, diags := b.LocalRun(op) + + if diags.HasErrors() { + t.Fatalf("unexpected error\ngot: %s\nwant: ", diags.Err().Error()) + } + // When Context() succeeds, this should fail w/ "workspace already locked" + stateMgr, _ := b.StateMgr(backend.DefaultStateName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil { + t.Fatal("unexpected success locking state after Context") + } + + actual := lr.PlanOpts.SetVariables + expected := test.expectedVariables + + for expectedKey := range expected { + actualValue := actual[expectedKey] + expectedValue := expected[expectedKey] + + if !reflect.DeepEqual(*actualValue, *expectedValue) { + t.Fatalf("unexpected variable '%s'\ngot: %v\nwant: %v", expectedKey, actualValue, expectedValue) + } + } + }) + } +} + +type testUnparsedVariableValue string + +func (v testUnparsedVariableValue) ParseVariableValue(mode configs.VariableParsingMode) (*terraform.InputValue, tfdiags.Diagnostics) { + return &terraform.InputValue{ + Value: cty.StringVal(string(v)), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, nil +} diff --git a/internal/backend/remote/backend_plan_test.go b/internal/backend/remote/backend_plan_test.go index 95aacea97..3acf0796d 100644 --- a/internal/backend/remote/backend_plan_test.go +++ b/internal/backend/remote/backend_plan_test.go @@ -1241,7 +1241,7 @@ func TestRemote_planOtherError(t *testing.T) { } if !strings.Contains(err.Error(), - "The configured \"remote\" backend encountered an unexpected error:\n\nI'm a little teacup") { + "the configured \"remote\" backend encountered an unexpected error:\n\nI'm a little teacup") { t.Fatalf("expected error message, got: %s", err.Error()) } } diff --git a/internal/backend/remote/backend_test.go b/internal/backend/remote/backend_test.go index d00b61ae2..a98069de7 100644 --- a/internal/backend/remote/backend_test.go +++ b/internal/backend/remote/backend_test.go @@ -556,21 +556,21 @@ func TestRemote_StateMgr_versionCheckLatest(t *testing.T) { func TestRemote_VerifyWorkspaceTerraformVersion(t *testing.T) { testCases := []struct { - local string - remote string - operations bool - wantErr bool + local string + remote string + executionMode string + wantErr bool }{ - {"0.13.5", "0.13.5", true, false}, - {"0.14.0", "0.13.5", true, true}, - {"0.14.0", "0.13.5", false, false}, - {"0.14.0", "0.14.1", true, false}, - {"0.14.0", "1.0.99", true, false}, - {"0.14.0", "1.1.0", true, false}, - {"0.14.0", "1.2.0", true, true}, - {"1.2.0", "1.2.99", true, false}, - {"1.2.0", "1.3.0", true, true}, - {"0.15.0", "latest", true, false}, + {"0.13.5", "0.13.5", "remote", false}, + {"0.14.0", "0.13.5", "remote", true}, + {"0.14.0", "0.13.5", "local", false}, + {"0.14.0", "0.14.1", "remote", false}, + {"0.14.0", "1.0.99", "remote", false}, + {"0.14.0", "1.1.0", "remote", false}, + {"0.14.0", "1.2.0", "remote", true}, + {"1.2.0", "1.2.99", "remote", false}, + {"1.2.0", "1.3.0", "remote", true}, + {"0.15.0", "latest", "remote", false}, } for _, tc := range testCases { t.Run(fmt.Sprintf("local %s, remote %s", tc.local, tc.remote), func(t *testing.T) { @@ -601,7 +601,7 @@ func TestRemote_VerifyWorkspaceTerraformVersion(t *testing.T) { b.organization, b.workspace, tfe.WorkspaceUpdateOptions{ - Operations: tfe.Bool(tc.operations), + ExecutionMode: &tc.executionMode, TerraformVersion: tfe.String(tc.remote), }, ); err != nil { diff --git a/internal/backend/remote/testdata/variables/main.tf b/internal/backend/remote/testdata/variables/main.tf new file mode 100644 index 000000000..9e1a0a40f --- /dev/null +++ b/internal/backend/remote/testdata/variables/main.tf @@ -0,0 +1,8 @@ +variable "key1" { +} + +variable "key2" { +} + +variable "key3" { +} diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index 45ec969ea..0d414fcf3 100644 --- a/internal/cloud/backend.go +++ b/internal/cloud/backend.go @@ -601,28 +601,10 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) { // Operation implements backend.Enhanced. func (b *Cloud) Operation(ctx context.Context, op *backend.Operation) (*backend.RunningOperation, error) { - name := op.Workspace - // Retrieve the workspace for this operation. - w, err := b.client.Workspaces.Read(ctx, b.organization, name) + w, err := b.fetchWorkspace(ctx, b.organization, op.Workspace) if err != nil { - switch err { - case context.Canceled: - return nil, err - case tfe.ErrResourceNotFound: - return nil, fmt.Errorf( - "workspace %s not found\n\n"+ - "For security, Terraform Cloud returns '404 Not Found' responses for resources\n"+ - "for resources that a user doesn't have access to, in addition to resources that\n"+ - "do not exist. If the resource does exist, please check the permissions of the provided token.", - name, - ) - default: - return nil, fmt.Errorf( - "Terraform Cloud returned an unexpected error:\n\n%s", - err, - ) - } + return nil, err } // Terraform remote version conflicts are not a concern for operations. We @@ -969,6 +951,33 @@ func isLocalExecutionMode(execMode string) bool { return execMode == "local" } +func (b *Cloud) fetchWorkspace(ctx context.Context, organization string, workspace string) (*tfe.Workspace, error) { + // Retrieve the workspace for this operation. + w, err := b.client.Workspaces.Read(ctx, organization, workspace) + if err != nil { + switch err { + case context.Canceled: + return nil, err + case tfe.ErrResourceNotFound: + return nil, fmt.Errorf( + "workspace %s not found\n\n"+ + "For security, Terraform Cloud returns '404 Not Found' responses for resources\n"+ + "for resources that a user doesn't have access to, in addition to resources that\n"+ + "do not exist. If the resource does exist, please check the permissions of the provided token.", + workspace, + ) + default: + err := fmt.Errorf( + "Terraform Cloud returned an unexpected error:\n\n%s", + err, + ) + return nil, err + } + } + + return w, nil +} + func (wm WorkspaceMapping) tfeTags() []*tfe.Tag { var tags []*tfe.Tag diff --git a/internal/cloud/backend_context.go b/internal/cloud/backend_context.go index 27afadd2f..e6e846143 100644 --- a/internal/cloud/backend_context.go +++ b/internal/cloud/backend_context.go @@ -5,8 +5,9 @@ import ( "fmt" "log" - tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/hcl/v2" + + tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs" @@ -97,21 +98,34 @@ func (b *Cloud) LocalRun(op *backend.Operation) (*backend.LocalRun, statemgr.Ful return nil, nil, diags } - log.Printf("[TRACE] cloud: retrieving variables from workspace %s/%s (%s)", remoteWorkspaceName, b.organization, remoteWorkspaceID) - tfeVariables, err := b.client.Variables.List(context.Background(), remoteWorkspaceID, tfe.VariableListOptions{}) - if err != nil && err != tfe.ErrResourceNotFound { - diags = diags.Append(fmt.Errorf("error loading variables: %w", err)) + // Retrieve the workspace for this operation. + w, err := b.fetchWorkspace(context.Background(), b.organization, op.Workspace) + if err != nil { + diags = diags.Append(fmt.Errorf("error loading workspace: %w", err)) return nil, nil, diags } - - if tfeVariables != nil { - if op.Variables == nil { - op.Variables = make(map[string]backend.UnparsedVariableValue) + if isLocalExecutionMode(w.ExecutionMode) { + log.Printf("[TRACE] skipping retrieving variables from workspace %s/%s (%s), workspace is in Local Execution mode", remoteWorkspaceName, b.organization, remoteWorkspaceID) + } else { + log.Printf("[TRACE] cloud: retrieving variables from workspace %s/%s (%s)", remoteWorkspaceName, b.organization, remoteWorkspaceID) + tfeVariables, err := b.client.Variables.List(context.Background(), remoteWorkspaceID, tfe.VariableListOptions{}) + if err != nil && err != tfe.ErrResourceNotFound { + diags = diags.Append(fmt.Errorf("error loading variables: %w", err)) + return nil, nil, diags } - for _, v := range tfeVariables.Items { - if v.Category == tfe.CategoryTerraform { - op.Variables[v.Key] = &remoteStoredVariableValue{ - definition: v, + + if tfeVariables != nil { + if op.Variables == nil { + op.Variables = make(map[string]backend.UnparsedVariableValue) + } + + for _, v := range tfeVariables.Items { + if v.Category == tfe.CategoryTerraform { + if _, ok := op.Variables[v.Key]; !ok { + op.Variables[v.Key] = &remoteStoredVariableValue{ + definition: v, + } + } } } } diff --git a/internal/cloud/backend_context_test.go b/internal/cloud/backend_context_test.go index e1001ee52..635efc88b 100644 --- a/internal/cloud/backend_context_test.go +++ b/internal/cloud/backend_context_test.go @@ -2,6 +2,7 @@ package cloud import ( "context" + "reflect" "testing" tfe "github.com/hashicorp/go-tfe" @@ -13,6 +14,8 @@ import ( "github.com/hashicorp/terraform/internal/initwd" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terminal" + "github.com/hashicorp/terraform/internal/terraform" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -233,3 +236,220 @@ func TestRemoteContextWithVars(t *testing.T) { }) } } + +func TestRemoteVariablesDoNotOverride(t *testing.T) { + catTerraform := tfe.CategoryTerraform + + varName1 := "key1" + varName2 := "key2" + varName3 := "key3" + + varValue1 := "value1" + varValue2 := "value2" + varValue3 := "value3" + + tests := map[string]struct { + localVariables map[string]backend.UnparsedVariableValue + remoteVariables []*tfe.VariableCreateOptions + expectedVariables terraform.InputValues + }{ + "no local variables": { + map[string]backend.UnparsedVariableValue{}, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, + { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, + { + Key: &varName3, + Value: &varValue3, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + }, + }, + "single conflicting local variable": { + map[string]backend.UnparsedVariableValue{ + varName3: testUnparsedVariableValue{source: terraform.ValueFromNamedFile, value: cty.StringVal(varValue3)}, + }, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, { + Key: &varName3, + Value: &varValue3, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, + }, + }, + "no conflicting local variable": { + map[string]backend.UnparsedVariableValue{ + varName3: testUnparsedVariableValue{source: terraform.ValueFromNamedFile, value: cty.StringVal(varValue3)}, + }, + []*tfe.VariableCreateOptions{ + { + Key: &varName1, + Value: &varValue1, + Category: &catTerraform, + }, { + Key: &varName2, + Value: &varValue2, + Category: &catTerraform, + }, + }, + terraform.InputValues{ + varName1: &terraform.InputValue{ + Value: cty.StringVal(varValue1), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName2: &terraform.InputValue{ + Value: cty.StringVal(varValue2), + SourceType: terraform.ValueFromInput, + SourceRange: tfdiags.SourceRange{ + Filename: "", + Start: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + End: tfdiags.SourcePos{Line: 0, Column: 0, Byte: 0}, + }, + }, + varName3: &terraform.InputValue{ + Value: cty.StringVal(varValue3), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + configDir := "./testdata/variables" + + b, bCleanup := testBackendWithName(t) + defer bCleanup() + + _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) + defer configCleanup() + + workspaceID, err := b.getRemoteWorkspaceID(context.Background(), testBackendSingleWorkspaceName) + if err != nil { + t.Fatal(err) + } + + streams, _ := terminal.StreamsForTesting(t) + view := views.NewStateLocker(arguments.ViewHuman, views.NewView(streams)) + + op := &backend.Operation{ + ConfigDir: configDir, + ConfigLoader: configLoader, + StateLocker: clistate.NewLocker(0, view), + Workspace: testBackendSingleWorkspaceName, + Variables: test.localVariables, + } + + for _, v := range test.remoteVariables { + b.client.Variables.Create(context.TODO(), workspaceID, *v) + } + + lr, _, diags := b.LocalRun(op) + + if diags.HasErrors() { + t.Fatalf("unexpected error\ngot: %s\nwant: ", diags.Err().Error()) + } + // When Context() succeeds, this should fail w/ "workspace already locked" + stateMgr, _ := b.StateMgr(testBackendSingleWorkspaceName) + if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil { + t.Fatal("unexpected success locking state after Context") + } + + actual := lr.PlanOpts.SetVariables + expected := test.expectedVariables + + for expectedKey := range expected { + actualValue := actual[expectedKey] + expectedValue := expected[expectedKey] + + if !reflect.DeepEqual(*actualValue, *expectedValue) { + t.Fatalf("unexpected variable '%s'\ngot: %v\nwant: %v", expectedKey, actualValue, expectedValue) + } + } + }) + } +} diff --git a/internal/cloud/testdata/variables/main.tf b/internal/cloud/testdata/variables/main.tf new file mode 100644 index 000000000..9e1a0a40f --- /dev/null +++ b/internal/cloud/testdata/variables/main.tf @@ -0,0 +1,8 @@ +variable "key1" { +} + +variable "key2" { +} + +variable "key3" { +}