New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate duplicate provider local names in required_providers
#31218
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
} | ||
Comment on lines
+150
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we also looking for resources that explicitly specify a provider local name that wasn't declared? provider = aws.foo I believe (though I haven't tested) that if we encounter the above without there being an
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, that is validated already which is why I didn't check it here, but not until core tries to connect the providers in the graph. This additional check was only intended to get the case of an implied local name not matching the |
||
|
||
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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the related situation of a provider requirement being implied by a
resource
ordata
block which doesn't set theprovider
argument and whose resource type name starts with a prefix that doesn't match any explicitly-declared identifier. Should we catch that one too, or does it end up not mattering because the resource ends up belonging to the correct provider configuration anyway?(Specifically: I don't recall if that backward-compatibility behavior causes us to implicitly declare a new provider configuration with the local name matching the prefix, or if it instead just hooks it up to whatever provider configuration it finds that has the assumed provider source address. If it's the latter then I guess it probably doesn't really matter, although it's still potentially confusing to have e.g.
aws_instance
automatically associated withprovider "notaws"
just becauserequired_providers
declarednotaws
as beinghashicorp/aws
. 🤔)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it does work out that the implied provider based on the resource prefix will just get hooked up to the correct provider, even if it has a different local name, perhaps it's best to warn about that case too? I didn't delve into that here because it doesn't cause any known issues, and it's not as straightforward as dealing with the provider configs alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it seems a little unclear what we should do here... I do like the consistency of treating a provider dependency implied by a resource the same way as a provider dependency implied by a provider configuration, since it seems like both of those things are a common source of confusion for folks anyway. But if it is particularly gnarly to do it then I would agree probably not worth it.
I have some memory of a function somewhere which takes something describing a provider and encapsulates the logic for deciding which provider configuration it ought to belong to, which if we could find it might allow all of these situations together as one rule (a
provider = aws.foo
can also imply a dependency onhashicorp/aws
if there isn't already anaws
in scope), but if we'd end up having to duplicate all of the logic for detecting implicit provider requirements in here then I'd say that's probably too high a cost for just a little consistency. (Unfortunately I don't remember where this function is, assuming I'm not misremembering it existing in the first place, but maybe you have seen it on your travels while working on this... 🤔)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking this final case wasn't that bad, we've already collected the
required_providers
local names, so we just need to iterate over the resources looking for implied providers which aren't in that set.The newest commit adds a diagnostic like:
This diagnostic output only uses the local names, because this situation can only happen with providers from the default namespace with a default provider name inferred from the resource name.