diff --git a/internal/configs/config_test.go b/internal/configs/config_test.go index 21c400a85a54..b5360278df7b 100644 --- a/internal/configs/config_test.go +++ b/internal/configs/config_test.go @@ -158,6 +158,12 @@ func TestConfigProviderRequirements(t *testing.T) { } } +func TestConfigProviderRequirementsDuplicate(t *testing.T) { + _, diags := testNestedModuleConfigFromDir(t, "testdata/duplicate-local-name") + assertDiagnosticCount(t, diags, 3) + assertDiagnosticSummary(t, diags, "Duplicate required provider") +} + func TestConfigProviderRequirementsShallow(t *testing.T) { cfg, diags := testNestedModuleConfigFromDir(t, "testdata/provider-reqs") // TODO: Version Constraint Deprecation. diff --git a/internal/configs/provider_validation.go b/internal/configs/provider_validation.go index 56262bb2aab3..a6feb4279eaf 100644 --- a/internal/configs/provider_validation.go +++ b/internal/configs/provider_validation.go @@ -82,7 +82,54 @@ func validateProviderConfigs(parentCall *ModuleCall, cfg *Config, noProviderConf } if mod.ProviderRequirements != nil { + // Track all known local types too to ensure we don't have duplicated + // with different local names. + localTypes := map[string]bool{} + + // check for duplicate requirements of the same type for _, req := range mod.ProviderRequirements.RequiredProviders { + if localTypes[req.Type.String()] { + // find the last declaration to give a better error + prevDecl := "" + for localName, typ := range localNames { + if typ.Equals(req.Type) { + prevDecl = localName + } + } + + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Duplicate required provider", + Detail: fmt.Sprintf( + "Provider %s with the local name %q was previously required as %q. A provider can only be required once within required_providers.", + req.Type.ForDisplay(), req.Name, prevDecl, + ), + Subject: &req.DeclRange, + }) + } else if req.Type.IsDefault() { + // Now check for possible implied duplicates, where a provider + // block uses a default namespaced provider, but that provider + // was required via a different name. + impliedLocalName := req.Type.Type + // We have to search through the configs for a match, since the keys contains any aliases. + for _, pc := range mod.ProviderConfigs { + if pc.Name == impliedLocalName && req.Name != impliedLocalName { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Duplicate required provider", + Detail: fmt.Sprintf( + "Provider %s with the local name %q was implicitly required via a configuration block as %q. The provider configuration block name must match the name used in required_providers.", + req.Type.ForDisplay(), req.Name, req.Type.Type, + ), + Subject: &req.DeclRange, + }) + break + } + } + } + + localTypes[req.Type.String()] = true + localNames[req.Name] = req.Type for _, alias := range req.Aliases { addr := addrs.AbsProviderConfig{ @@ -95,6 +142,44 @@ func validateProviderConfigs(parentCall *ModuleCall, cfg *Config, noProviderConf } } + checkImpliedProviderNames := func(resourceConfigs map[string]*Resource) { + // Now that we have all the provider configs and requirements validated, + // check for any resources which use an implied localname which doesn't + // match that of required_providers + for _, r := range resourceConfigs { + // We're looking for resources with no specific provider reference + if r.ProviderConfigRef != nil { + continue + } + + localName := r.Addr().ImpliedProvider() + if _, ok := localNames[localName]; ok { + // OK, this was listed directly in the required_providers + continue + } + + defAddr := addrs.ImpliedProviderForUnqualifiedType(localName) + + // Now make sure we don't have the same provider required under a + // different name. + for prevLocalName, addr := range localNames { + if addr.Equals(defAddr) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Duplicate required provider", + Detail: fmt.Sprintf( + "Provider %q was implicitly required via resource %q, but listed in required_providers as %q. Either the local name in required_providers must match the resource name, or the %q provider must be assigned within the resource block.", + defAddr, r.Addr(), prevLocalName, prevLocalName, + ), + Subject: &r.DeclRange, + }) + } + } + } + } + checkImpliedProviderNames(mod.ManagedResources) + checkImpliedProviderNames(mod.DataResources) + // collect providers passed from the parent if parentCall != nil { for _, passed := range parentCall.Providers { diff --git a/internal/configs/testdata/duplicate-local-name/main.tf b/internal/configs/testdata/duplicate-local-name/main.tf new file mode 100644 index 000000000000..9bac5451cdf8 --- /dev/null +++ b/internal/configs/testdata/duplicate-local-name/main.tf @@ -0,0 +1,23 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + } + dupe = { + source = "hashicorp/test" + } + other = { + source = "hashicorp/default" + } + + wrong-name = { + source = "hashicorp/foo" + } + } +} + +provider "default" { +} + +resource "foo_resource" { +} diff --git a/internal/terraform/transform_provider.go b/internal/terraform/transform_provider.go index 3e459e7a7b11..2e1f8bd5f3f8 100644 --- a/internal/terraform/transform_provider.go +++ b/internal/terraform/transform_provider.go @@ -543,6 +543,13 @@ func (t *ProviderConfigTransformer) transformSingle(g *Graph, c *configs.Config) Module: path, } + if _, ok := t.providers[addr.String()]; ok { + // The config validation warns about this too, but we can't + // completely prevent it in v1. + log.Printf("[WARN] ProviderConfigTransformer: duplicate required_providers entry for %s", addr) + continue + } + abstract := &NodeAbstractProvider{ Addr: addr, } @@ -568,6 +575,13 @@ func (t *ProviderConfigTransformer) transformSingle(g *Graph, c *configs.Config) Module: path, } + if _, ok := t.providers[addr.String()]; ok { + // The abstract provider node may already have been added from the + // provider requirements. + log.Printf("[WARN] ProviderConfigTransformer: provider node %s already added", addr) + continue + } + abstract := &NodeAbstractProvider{ Addr: addr, } diff --git a/internal/terraform/transform_provider_test.go b/internal/terraform/transform_provider_test.go index 0436fc03248f..3580a548b300 100644 --- a/internal/terraform/transform_provider_test.go +++ b/internal/terraform/transform_provider_test.go @@ -446,6 +446,42 @@ provider["registry.terraform.io/hashicorp/test"].z` } } +func TestProviderConfigTransformer_duplicateLocalName(t *testing.T) { + mod := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + required_providers { + # We have to allow this since it wasn't previously prevented. If the + # default config is equivalent to the provider config, the user may never + # see an error. + dupe = { + source = "registry.terraform.io/hashicorp/test" + } + } +} + +provider "test" { +} +`}) + concrete := func(a *NodeAbstractProvider) dag.Vertex { return a } + + g := testProviderTransformerGraph(t, mod) + tf := ProviderConfigTransformer{ + Config: mod, + Concrete: concrete, + } + if err := tf.Transform(g); err != nil { + t.Fatalf("err: %s", err) + } + + expected := `provider["registry.terraform.io/hashicorp/test"]` + + actual := strings.TrimSpace(g.String()) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + const testTransformProviderBasicStr = ` aws_instance.web provider["registry.terraform.io/hashicorp/aws"]