Merge pull request #33432 from hashicorp/TF-7327-using-multiple-terraform-remote-state-data-sources-can-cause-race-conditions-when-the-backend-configuring-localterraform-com

Relocate localterraform.com aliasing to backend configurators

internal/backend/backend.go

@@ -14,6 +14,7 @@ import (
 	"log"
 	"os"
 
+	svchost "github.com/hashicorp/terraform-svchost"
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/command/clistate"
 	"github.com/hashicorp/terraform/internal/command/views"
@@ -119,6 +120,13 @@ type Backend interface {
 	Workspaces() ([]string, error)
 }
 
+// HostAlias describes a list of aliases that should be used when initializing an
+// Enhanced Backend
+type HostAlias struct {
+	From svchost.Hostname
+	To   svchost.Hostname
+}
+
 // Enhanced implements additional behavior on top of a normal backend.
 //
 // 'Enhanced' backends are an implementation detail only, and are no longer reflected as an external
@@ -136,6 +144,10 @@ type Enhanced interface {
 	// responsibility of the Backend to lock the state for the duration of the
 	// running operation.
 	Operation(context.Context, *Operation) (*RunningOperation, error)
+
+	// ServiceDiscoveryAliases returns a mapping of Alias -> Target hosts to
+	// configure.
+	ServiceDiscoveryAliases() ([]HostAlias, error)
 }
 
 // Local implements additional behavior on a Backend that allows local

internal/backend/local/backend.go

@@ -183,6 +183,10 @@ func (b *Local) Configure(obj cty.Value) tfdiags.Diagnostics {
 	return diags
 }
 
+func (b *Local) ServiceDiscoveryAliases() ([]backend.HostAlias, error) {
+	return []backend.HostAlias{}, nil
+}
+
 func (b *Local) Workspaces() ([]string, error) {
 	// If we have a backend handling state, defer to that.
 	if b.Backend != nil {

internal/backend/remote/backend.go

@@ -198,21 +198,26 @@ func (b *Remote) PrepareConfig(obj cty.Value) (cty.Value, tfdiags.Diagnostics) {
 	return obj, diags
 }
 
-// configureGenericHostname aliases the remote backend hostname configuration
-// as a generic "localterraform.com" hostname. This was originally added as a
-// Terraform Enterprise feature and is useful for re-using whatever the
-// Cloud/Enterprise backend host is in nested module sources in order
-// to prevent code churn when re-using config between multiple
-// Terraform Enterprise environments.
-func (b *Remote) configureGenericHostname() {
-	// This won't be an error for the given constant value
-	genericHost, _ := svchost.ForComparison(genericHostname)
+func (b *Remote) ServiceDiscoveryAliases() ([]backend.HostAlias, error) {
+	aliasHostname, err := svchost.ForComparison(genericHostname)
+	if err != nil {
+		// This should never happen because the hostname is statically defined.
+		return nil, fmt.Errorf("failed to create backend alias from alias %q. The hostname is not in the correct format. This is a bug in the backend", genericHostname)
+	}
 
-	// This won't be an error because, by this time, the hostname has been parsed and
-	// service discovery requests made against it.
-	targetHost, _ := svchost.ForComparison(b.hostname)
+	targetHostname, err := svchost.ForComparison(b.hostname)
+	if err != nil {
+		// This should never happen because the 'to' alias is the backend host, which has likely
+		// already been evaluated as a svchost.Hostname by now
+		return nil, fmt.Errorf("failed to create backend alias to target %q. The hostname is not in the correct format", b.hostname)
+	}
 
-	b.services.Alias(genericHost, targetHost)
+	return []backend.HostAlias{
+		{
+			From: aliasHostname,
+			To:   targetHostname,
+		},
+	}, nil
 }
 
 // Configure implements backend.Enhanced.
@@ -317,8 +322,6 @@ func (b *Remote) Configure(obj cty.Value) tfdiags.Diagnostics {
 		return diags
 	}
 
-	b.configureGenericHostname()
-
 	cfg := &tfe.Config{
 		Address:      service.String(),
 		BasePath:     service.Path,

internal/backend/remote/backend_test.go

@@ -725,3 +725,29 @@ func TestRemote_VerifyWorkspaceTerraformVersion_ignoreFlagSet(t *testing.T) {
 		t.Errorf("wrong summary: got %s, want %s", got, wantDetail)
 	}
 }
+
+func TestRemote_ServiceDiscoveryAliases(t *testing.T) {
+	s := testServer(t)
+	b := New(testDisco(s))
+
+	diag := b.Configure(cty.ObjectVal(map[string]cty.Value{
+		"hostname":     cty.NullVal(cty.String), // Forces aliasing to test server
+		"organization": cty.StringVal("hashicorp"),
+		"token":        cty.NullVal(cty.String),
+		"workspaces": cty.ObjectVal(map[string]cty.Value{
+			"name":   cty.StringVal("prod"),
+			"prefix": cty.NullVal(cty.String),
+		}),
+	}))
+	if diag.HasErrors() {
+		t.Fatalf("expected no diagnostic errors, got %s", diag.Err())
+	}
+
+	aliases, err := b.ServiceDiscoveryAliases()
+	if err != nil {
+		t.Fatalf("expected no errors, got %s", err)
+	}
+	if len(aliases) != 1 {
+		t.Fatalf("expected 1 alias but got %d", len(aliases))
+	}
+}

internal/cloud/backend.go

@@ -204,21 +204,26 @@ func (b *Cloud) PrepareConfig(obj cty.Value) (cty.Value, tfdiags.Diagnostics) {
 	return obj, diags
 }
 
-// configureGenericHostname aliases the cloud backend hostname configuration
-// as a generic "localterraform.com" hostname. This was originally added as a
-// Terraform Enterprise feature and is useful for re-using whatever the
-// Cloud/Enterprise backend host is in nested module sources in order
-// to prevent code churn when re-using config between multiple
-// Terraform Enterprise environments.
-func (b *Cloud) configureGenericHostname() {
-	// This won't be an error for the given constant value
-	genericHost, _ := svchost.ForComparison(genericHostname)
+func (b *Cloud) ServiceDiscoveryAliases() ([]backend.HostAlias, error) {
+	aliasHostname, err := svchost.ForComparison(genericHostname)
+	if err != nil {
+		// This should never happen because the hostname is statically defined.
+		return nil, fmt.Errorf("failed to create backend alias from alias %q. The hostname is not in the correct format. This is a bug in the backend", genericHostname)
+	}
 
-	// This won't be an error because, by this time, the hostname has been parsed and
-	// service discovery requests made against it.
-	targetHost, _ := svchost.ForComparison(b.hostname)
+	targetHostname, err := svchost.ForComparison(b.hostname)
+	if err != nil {
+		// This should never happen because the 'to' alias is the backend host, which has
+		// already been ev
+		return nil, fmt.Errorf("failed to create backend alias to target %q. The hostname is not in the correct format.", b.hostname)
+	}
 
-	b.services.Alias(genericHost, targetHost)
+	return []backend.HostAlias{
+		{
+			From: aliasHostname,
+			To:   targetHostname,
+		},
+	}, nil
 }
 
 // Configure implements backend.Enhanced.
@@ -287,7 +292,6 @@ func (b *Cloud) Configure(obj cty.Value) tfdiags.Diagnostics {
 	}
 
 	b.token = token
-	b.configureGenericHostname()
 
 	if b.client == nil {
 		cfg := &tfe.Config{

internal/cloud/backend_test.go

@@ -1145,7 +1145,7 @@ func TestCloud_VerifyWorkspaceTerraformVersion_ignoreFlagSet(t *testing.T) {
 	}
 }
 
-func TestClodBackend_DeleteWorkspace_SafeAndForce(t *testing.T) {
+func TestCloudBackend_DeleteWorkspace_SafeAndForce(t *testing.T) {
 	b, bCleanup := testBackendWithTags(t)
 	defer bCleanup()
 	safeDeleteWorkspaceName := "safe-delete-workspace"
@@ -1211,7 +1211,7 @@ func TestClodBackend_DeleteWorkspace_SafeAndForce(t *testing.T) {
 	}
 }
 
-func TestClodBackend_DeleteWorkspace_DoesNotExist(t *testing.T) {
+func TestCloudBackend_DeleteWorkspace_DoesNotExist(t *testing.T) {
 	b, bCleanup := testBackendWithTags(t)
 	defer bCleanup()
 
@@ -1220,3 +1220,29 @@ func TestClodBackend_DeleteWorkspace_DoesNotExist(t *testing.T) {
 		t.Fatalf("expected deleting a workspace which does not exist to succeed")
 	}
 }
+
+func TestCloud_ServiceDiscoveryAliases(t *testing.T) {
+	s := testServer(t)
+	b := New(testDisco(s))
+
+	diag := b.Configure(cty.ObjectVal(map[string]cty.Value{
+		"hostname":     cty.NullVal(cty.String), // Forces aliasing to test server
+		"organization": cty.StringVal("hashicorp"),
+		"token":        cty.NullVal(cty.String),
+		"workspaces": cty.ObjectVal(map[string]cty.Value{
+			"name": cty.StringVal("prod"),
+			"tags": cty.NullVal(cty.Set(cty.String)),
+		}),
+	}))
+	if diag.HasErrors() {
+		t.Fatalf("expected no diagnostic errors, got %s", diag.Err())
+	}
+
+	aliases, err := b.ServiceDiscoveryAliases()
+	if err != nil {
+		t.Fatalf("expected no errors, got %s", err)
+	}
+	if len(aliases) != 1 {
+		t.Fatalf("expected 1 alias but got %d", len(aliases))
+	}
+}

internal/command/meta_backend.go

@@ -291,7 +291,7 @@ func (m *Meta) selectWorkspace(b backend.Backend) error {
 	}
 
 	workspace = workspaces[idx-1]
-	log.Printf("[TRACE] Meta.selectWorkspace: setting the current workpace according to user selection (%s)", workspace)
+	log.Printf("[TRACE] Meta.selectWorkspace: setting the current workspace according to user selection (%s)", workspace)
 	return m.SetWorkspace(workspace)
 }
 
@@ -352,6 +352,10 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags
 	// then return that as-is. This works even if b == nil (it will be !ok).
 	if enhanced, ok := b.(backend.Enhanced); ok {
 		log.Printf("[TRACE] Meta.BackendForPlan: backend %T supports operations", b)
+		if err := m.setupEnhancedBackendAliases(enhanced); err != nil {
+			diags = diags.Append(err)
+			return nil, diags
+		}
 		return enhanced, nil
 	}
 
@@ -831,6 +835,17 @@ func (m *Meta) backendFromState(ctx context.Context) (backend.Backend, tfdiags.D
 		return nil, diags
 	}
 
+	// If the result of loading the backend is an enhanced backend,
+	// then set up enhanced backend service aliases.
+	if enhanced, ok := b.(backend.Enhanced); ok {
+		log.Printf("[TRACE] Meta.BackendForPlan: backend %T supports operations", b)
+
+		if err := m.setupEnhancedBackendAliases(enhanced); err != nil {
+			diags = diags.Append(err)
+			return nil, diags
+		}
+	}
+
 	return b, diags
 }
 
@@ -1277,6 +1292,17 @@ func (m *Meta) savedBackend(sMgr *clistate.LocalState) (backend.Backend, tfdiags
 		return nil, diags
 	}
 
+	// If the result of loading the backend is an enhanced backend,
+	// then set up enhanced backend service aliases.
+	if enhanced, ok := b.(backend.Enhanced); ok {
+		log.Printf("[TRACE] Meta.BackendForPlan: backend %T supports operations", b)
+
+		if err := m.setupEnhancedBackendAliases(enhanced); err != nil {
+			diags = diags.Append(err)
+			return nil, diags
+		}
+	}
+
 	return b, diags
 }
 
@@ -1397,9 +1423,36 @@ func (m *Meta) backendInitFromConfig(c *configs.Backend) (backend.Backend, cty.V
 	configureDiags := b.Configure(newVal)
 	diags = diags.Append(configureDiags.InConfigBody(c.Config, ""))
 
+	// If the result of loading the backend is an enhanced backend,
+	// then set up enhanced backend service aliases.
+	if enhanced, ok := b.(backend.Enhanced); ok {
+		log.Printf("[TRACE] Meta.BackendForPlan: backend %T supports operations", b)
+		if err := m.setupEnhancedBackendAliases(enhanced); err != nil {
+			diags = diags.Append(err)
+			return nil, cty.NilVal, diags
+		}
+	}
+
 	return b, configVal, diags
 }
 
+// Helper method to get aliases from the enhanced backend and alias them
+// in the Meta service discovery. It's unfortunate that the Meta backend
+// is modifying the service discovery at this level, but the owner
+// of the service discovery pointer does not have easy access to the backend.
+func (m *Meta) setupEnhancedBackendAliases(b backend.Enhanced) error {
+	// Set up the service discovery aliases specified by the enhanced backend.
+	serviceAliases, err := b.ServiceDiscoveryAliases()
+	if err != nil {
+		return err
+	}
+
+	for _, alias := range serviceAliases {
+		m.Services.Alias(alias.From, alias.To)
+	}
+	return nil
+}
+
 // Helper method to ignore remote/cloud backend version conflicts. Only call this
 // for commands which cannot accidentally upgrade remote state files.
 func (m *Meta) ignoreRemoteVersionConflict(b backend.Backend) {