From dcb8b45e0f9a929900dfe6b691997c1bd2b836c2 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Mon, 27 Apr 2020 14:58:36 -0400 Subject: [PATCH] configs: Fix for resources with implied providers Previously, resources without explicit provider configuration (i.e. a `provider =` attribute) would be assigned a default provider based upon the resource type. For example, a resource `foo_bar` would be assigned provider `hashicorp/foo`. This behaviour did not work well with community or partner providers, with sources configured in `terraform.required_providers` blocks. With the following configuration: terraform { required_providers { foo = { source = "acme/foo" } } } resource foo_bar "a" { } the resource would be configured with the `hashicorp/foo` provider. This commit fixes this implied provider behaviour. First we look for a provider with local name matching the resource type in the module's required providers map. If one is found, this provider is assigned to the resource. Otherwise, we still fall back to a default provider. --- configs/module.go | 38 ++++--- configs/module_test.go | 106 ++++++++++++++++++ .../implied-providers/providers.tf | 21 ++++ .../implied-providers/resources.tf | 12 ++ 4 files changed, 160 insertions(+), 17 deletions(-) create mode 100644 configs/testdata/valid-modules/implied-providers/providers.tf create mode 100644 configs/testdata/valid-modules/implied-providers/resources.tf diff --git a/configs/module.go b/configs/module.go index 8f7dc10d9..bc517078d 100644 --- a/configs/module.go +++ b/configs/module.go @@ -287,14 +287,10 @@ func (m *Module) appendFile(file *File) hcl.Diagnostics { // set the provider FQN for the resource if r.ProviderConfigRef != nil { - if existing, exists := m.ProviderRequirements.RequiredProviders[r.ProviderConfigAddr().LocalName]; exists { - r.Provider = existing.Type - } else { - r.Provider = addrs.ImpliedProviderForUnqualifiedType(r.ProviderConfigAddr().LocalName) - } - continue + r.Provider = m.ProviderForLocalConfig(r.ProviderConfigAddr()) + } else { + r.Provider = m.ImpliedProviderForUnqualifiedType(r.Addr().ImpliedProvider()) } - r.Provider = addrs.ImpliedProviderForUnqualifiedType(r.Addr().ImpliedProvider()) } for _, r := range file.DataResources { @@ -312,14 +308,10 @@ func (m *Module) appendFile(file *File) hcl.Diagnostics { // set the provider FQN for the resource if r.ProviderConfigRef != nil { - if existing, exists := m.ProviderRequirements.RequiredProviders[r.ProviderConfigAddr().LocalName]; exists { - r.Provider = existing.Type - } else { - r.Provider = addrs.ImpliedProviderForUnqualifiedType(r.ProviderConfigAddr().LocalName) - } - continue + r.Provider = m.ProviderForLocalConfig(r.ProviderConfigAddr()) + } else { + r.Provider = m.ImpliedProviderForUnqualifiedType(r.Addr().ImpliedProvider()) } - r.Provider = addrs.ImpliedProviderForUnqualifiedType(r.Addr().ImpliedProvider()) } return diags @@ -505,10 +497,22 @@ func (m *Module) LocalNameForProvider(p addrs.Provider) string { } } -// ProviderForLocalConfig returns the provider FQN for a given LocalProviderConfig +// ProviderForLocalConfig returns the provider FQN for a given +// LocalProviderConfig, based on its local name. func (m *Module) ProviderForLocalConfig(pc addrs.LocalProviderConfig) addrs.Provider { - if provider, exists := m.ProviderRequirements.RequiredProviders[pc.LocalName]; exists { + return m.ImpliedProviderForUnqualifiedType(pc.LocalName) +} + +// ImpliedProviderForUnqualifiedType returns the provider FQN for a given type, +// first by looking up the type in the provider requirements map, and falling +// back to an implied default provider. +// +// The intended behaviour is that configuring a provider with local name "foo" +// in a required_providers block will result in resources with type "foo" using +// that provider. +func (m *Module) ImpliedProviderForUnqualifiedType(pType string) addrs.Provider { + if provider, exists := m.ProviderRequirements.RequiredProviders[pType]; exists { return provider.Type } - return addrs.ImpliedProviderForUnqualifiedType(pc.LocalName) + return addrs.ImpliedProviderForUnqualifiedType(pType) } diff --git a/configs/module_test.go b/configs/module_test.go index d5e3659ad..526f7b3ee 100644 --- a/configs/module_test.go +++ b/configs/module_test.go @@ -203,3 +203,109 @@ func TestModule_required_provider_overrides(t *testing.T) { ) } } + +// Resources without explicit provider configuration are assigned a provider +// implied based on the resource type. For example, this resource: +// +// resource foo_instance "test" { } +// +// is assigned a provider with type "foo". +// +// To find the correct provider, we first look in the module's provider +// requirements map for a local name matching the resource type, and fall back +// to a default provider if none is found. This applies to both managed and +// data resources. +func TestModule_implied_provider(t *testing.T) { + mod, diags := testModuleFromDir("testdata/valid-modules/implied-providers") + if diags.HasErrors() { + t.Fatal(diags.Error()) + } + + // The three providers used in the config resources + foo := addrs.NewProvider("registry.acme.corp", "acme", "foo") + whatever := addrs.NewProvider(addrs.DefaultRegistryHost, "acme", "something") + bar := addrs.NewDefaultProvider("bar") + + // Verify that the registry.acme.corp/acme/foo provider is defined in the + // module provider requirements with local name "foo" + req, exists := mod.ProviderRequirements.RequiredProviders["foo"] + if !exists { + t.Fatal("no provider requirements found for \"foo\"") + } + if req.Type != foo { + t.Errorf("wrong provider addr for \"foo\"\ngot: %s\nwant: %s", + req.Type, foo, + ) + } + + // Verify that the acme/something provider is defined in the + // module provider requirements with local name "whatever" + req, exists = mod.ProviderRequirements.RequiredProviders["whatever"] + if !exists { + t.Fatal("no provider requirements found for \"foo\"") + } + if req.Type != whatever { + t.Errorf("wrong provider addr for \"whatever\"\ngot: %s\nwant: %s", + req.Type, whatever, + ) + } + + // Check that resources are assigned the correct providers: foo_* resources + // should have the custom foo provider, bar_* resources the default bar + // provider. + tests := []struct { + Address string + Provider addrs.Provider + }{ + {"foo_resource.a", foo}, + {"data.foo_resource.b", foo}, + {"bar_resource.c", bar}, + {"data.bar_resource.d", bar}, + {"whatever_resource.e", whatever}, + {"data.whatever_resource.f", whatever}, + } + for _, test := range tests { + resources := mod.ManagedResources + if strings.HasPrefix(test.Address, "data.") { + resources = mod.DataResources + } + resource, exists := resources[test.Address] + if !exists { + t.Errorf("could not find resource %q in %#v", test.Address, resources) + continue + } + if got := resource.Provider; !got.Equals(test.Provider) { + t.Errorf("wrong provider addr for %q\ngot: %s\nwant: %s", + test.Address, got, test.Provider, + ) + } + } +} + +func TestImpliedProviderForUnqualifiedType(t *testing.T) { + mod, diags := testModuleFromDir("testdata/valid-modules/implied-providers") + if diags.HasErrors() { + t.Fatal(diags.Error()) + } + + foo := addrs.NewProvider("registry.acme.corp", "acme", "foo") + whatever := addrs.NewProvider(addrs.DefaultRegistryHost, "acme", "something") + bar := addrs.NewDefaultProvider("bar") + tf := addrs.NewBuiltInProvider("terraform") + + tests := []struct { + Type string + Provider addrs.Provider + }{ + {"foo", foo}, + {"whatever", whatever}, + {"bar", bar}, + {"terraform", tf}, + } + for _, test := range tests { + got := mod.ImpliedProviderForUnqualifiedType(test.Type) + if !got.Equals(test.Provider) { + t.Errorf("wrong result for %q: got %#v, want %#v\n", test.Type, got, test.Provider) + } + } +} diff --git a/configs/testdata/valid-modules/implied-providers/providers.tf b/configs/testdata/valid-modules/implied-providers/providers.tf new file mode 100644 index 000000000..63b5b14df --- /dev/null +++ b/configs/testdata/valid-modules/implied-providers/providers.tf @@ -0,0 +1,21 @@ +terraform { + required_providers { + // This is an expected "real world" example of a community provider, which + // has resources named "foo_*" and will likely be used in configurations + // with the local name of "foo". + foo = { + source = "registry.acme.corp/acme/foo" + } + + // However, implied provider lookups are based on local name, not provider + // type, and this example clarifies that. Only resources with addresses + // starting "whatever_" will be assigned this provider implicitly. + // + // This is _not_ a recommended usage pattern. The best practice is for + // local name and type to be the same, and only use a different local name + // if there are provider type collisions. + whatever = { + source = "acme/something" + } + } +} diff --git a/configs/testdata/valid-modules/implied-providers/resources.tf b/configs/testdata/valid-modules/implied-providers/resources.tf new file mode 100644 index 000000000..6b4a8af87 --- /dev/null +++ b/configs/testdata/valid-modules/implied-providers/resources.tf @@ -0,0 +1,12 @@ +// These resources map to the configured "foo" provider" +resource foo_resource "a" {} +data foo_resource "b" {} + +// These resources map to a default "hashicorp/bar" provider +resource bar_resource "c" {} +data bar_resource "d" {} + +// These resources map to the configured "whatever" provider, which has FQN +// "acme/something". +resource whatever_resource "e" {} +data whatever_resource "f" {}