Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validate duplicate provider local names in required_providers #31218

Merged
merged 3 commits into from Jun 15, 2022
Merged

Validate duplicate provider local names in required_providers #31218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2581bc9
Check for duplicate types in required_providers
jbardin Jun 9, 2022
d7238d5
skip already added provider nodes
jbardin Jun 9, 2022
57c0deb
check for duplicate defaults from resource names
jbardin Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/configs/config_test.go
View file
Open in desktop
Expand Up @@ -158,6 +158,12 @@ func TestConfigProviderRequirements(t *testing.T) {
}
}

func TestConfigProviderRequirementsDuplicate(t *testing.T) {
_, diags := testNestedModuleConfigFromDir(t, "testdata/duplicate-local-name")
assertDiagnosticCount(t, diags, 2)
assertDiagnosticSummary(t, diags, "Duplicate required provider")
}

func TestConfigProviderRequirementsShallow(t *testing.T) {
cfg, diags := testNestedModuleConfigFromDir(t, "testdata/provider-reqs")
// TODO: Version Constraint Deprecation.
Expand Down
47 changes: 47 additions & 0 deletions internal/configs/provider_validation.go
View file
Open in desktop
Expand Up @@ -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 {
Copy link
Member

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 or data block which doesn't set the provider 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 with provider "notaws" just because required_providers declared notaws as being hashicorp/aws. 🤔)

Copy link
Member Author

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.

Copy link
Member

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 on hashicorp/aws if there isn't already an aws 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... 🤔)

Copy link
Member Author

@jbardin jbardin Jun 15, 2022 •
edited

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:

│ Warning: Duplicate required provider
...
│ Provider "null" was implicitly required via resource "null_resource.a", but listed in
│ required_providers as "nope". Either the local name in required_providers must match the resource
│ name, or the "nope" provider must be assigned within the resource block.

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.

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. Make sure the provider configuration block name matches 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{
Expand Down
16 changes: 16 additions & 0 deletions internal/configs/testdata/duplicate-local-name/main.tf
View file
Open in desktop
@@ -0,0 +1,16 @@
terraform {
required_providers {
test = {
source = "hashicorp/test"
}
dupe = {
source = "hashicorp/test"
}
other = {
source = "hashicorp/default"
}
}
}

provider "default" {
}
14 changes: 14 additions & 0 deletions internal/terraform/transform_provider.go
View file
Open in desktop
Expand Up @@ -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,
}
Expand All @@ -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,
}
Expand Down
36 changes: 36 additions & 0 deletions internal/terraform/transform_provider_test.go
View file
Open in desktop
Expand Up @@ -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"]
Expand Down