From c1a730314da49e83066089b1614ec6581e4a843c Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 26 Jun 2023 11:02:55 -0600 Subject: [PATCH] Relocate localterraform.com aliasing to backend configurators Previously, remote and cloud backends would automatically alias localterraform.com as the configured hostname during configuration. This turned out to be an issue with how backends could potentially be used within the builtin terraform_remote_state data source. Those data sources each configure the same service discovery with different targets for localterraform.com, and do so simultaneously, creating an occasional concurrent map read & write panic when multiple data sources are defined. localterraform.com is obviously not useful for every backend configuration. Therefore, I relocated the alias configuration to the callers, so they may specify when to use it. The modified design adds a new method to backend.Enhanced to allow configurators to ask which aliases should be defined. --- internal/backend/backend.go | 12 ++++++ internal/backend/local/backend.go | 4 ++ internal/backend/remote/backend.go | 33 ++++++++------- internal/backend/remote/backend_test.go | 26 ++++++++++++ internal/cloud/backend.go | 32 +++++++------- internal/cloud/backend_test.go | 30 +++++++++++++- internal/command/meta_backend.go | 55 ++++++++++++++++++++++++- 7 files changed, 160 insertions(+), 32 deletions(-) diff --git a/internal/backend/backend.go b/internal/backend/backend.go index eb67acfddc..cee0f5015d 100644 --- a/internal/backend/backend.go +++ b/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 diff --git a/internal/backend/local/backend.go b/internal/backend/local/backend.go index 7ea4a7ebbd..592ffe07fc 100644 --- a/internal/backend/local/backend.go +++ b/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 { diff --git a/internal/backend/remote/backend.go b/internal/backend/remote/backend.go index e440b2cbab..a6bbe0978a 100644 --- a/internal/backend/remote/backend.go +++ b/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, diff --git a/internal/backend/remote/backend_test.go b/internal/backend/remote/backend_test.go index 009762d621..743a3472aa 100644 --- a/internal/backend/remote/backend_test.go +++ b/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)) + } +} diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index b851fafb03..4fa2fc1ef0 100644 --- a/internal/cloud/backend.go +++ b/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{ diff --git a/internal/cloud/backend_test.go b/internal/cloud/backend_test.go index 7d19deab99..8591390033 100644 --- a/internal/cloud/backend_test.go +++ b/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)) + } +} diff --git a/internal/command/meta_backend.go b/internal/command/meta_backend.go index 80cdeaee93..2d59a30c83 100644 --- a/internal/command/meta_backend.go +++ b/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) {