Merge pull request #31218 from hashicorp/jbardin/validate-provider-local-names

Validate duplicate provider local names in `required_providers`

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.

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 {

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" {
+}

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,
 		}

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"]