From 041f4dd8caf4b4e1123e0dc2ab010b8b3291e8cc Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Thu, 14 May 2020 09:00:58 -0400 Subject: [PATCH] configs: require normalized provider local names (#24945) * addrs: replace NewLegacyProvider with NewDefaultProvider in ParseProviderSourceString ParseProviderSourceString was still defaulting to NewLegacyProvider when encountering single-part strings. This has been fixed. This commit also adds a new function, IsProviderPartNormalized, which returns a bool indicating if the string given is the same as a normalized version (as normalized by ParseProviderPart) or an error. This is intended for use by the configs package when decoding provider configurations. * terraform: fix provider local names in tests * configs: validate that all provider names are normalized The addrs package normalizes all source strings, but not the local names. This caused very odd behavior if for e.g. a provider local name was capitalized in one place and not another. We considered enabling case-sensitivity for provider local names, but decided that since this was not something that worked in previous versions of terraform (and we have yet to encounter any use cases for this feature) we could generate an error if the provider local name is not normalized. This error also provides instructions on how to fix it. * configs: refactor decodeProviderRequirements to consistently not set an FQN when there are errors --- addrs/provider.go | 15 ++++- addrs/provider_test.go | 11 +--- configs/provider.go | 37 ++++++++++- configs/provider_meta.go | 5 +- configs/provider_requirements.go | 7 +- configs/provider_requirements_test.go | 66 +++++++++++++------ configs/resource.go | 7 +- .../provider-localname-normalization.tf | 20 ++++++ .../child/main.tf | 4 +- .../transform-provider-fqns-module/main.tf | 4 +- .../testdata/transform-provider-fqns/main.tf | 4 +- 11 files changed, 140 insertions(+), 40 deletions(-) create mode 100644 configs/testdata/invalid-files/provider-localname-normalization.tf diff --git a/addrs/provider.go b/addrs/provider.go index c13ed8046..bc264d613 100644 --- a/addrs/provider.go +++ b/addrs/provider.go @@ -272,8 +272,7 @@ func ParseProviderSourceString(str string) (Provider, tfdiags.Diagnostics) { ret.Hostname = DefaultRegistryHost if len(parts) == 1 { - // FIXME: update this to NewDefaultProvider in the provider source release - return NewLegacyProvider(parts[0]), diags + return NewDefaultProvider(parts[0]), diags } if len(parts) >= 2 { @@ -406,3 +405,15 @@ func MustParseProviderPart(given string) string { } return result } + +// IsProviderPartNormalized compares a given string to the result of ParseProviderPart(string) +func IsProviderPartNormalized(str string) (bool, error) { + normalized, err := ParseProviderPart(str) + if err != nil { + return false, err + } + if str == normalized { + return true, nil + } + return false, nil +} diff --git a/addrs/provider_test.go b/addrs/provider_test.go index e0c996c0e..6dafae58d 100644 --- a/addrs/provider_test.go +++ b/addrs/provider_test.go @@ -282,20 +282,15 @@ func TestParseProviderSourceStr(t *testing.T) { "aws": { Provider{ Type: "aws", - Namespace: "-", + Namespace: "hashicorp", Hostname: DefaultRegistryHost, }, false, }, "AWS": { Provider{ - // No case folding here because we're currently handling this - // as a legacy one. When this changes to be a _default_ - // address in future (registry.terraform.io/hashicorp/aws) - // then we should start applying case folding to it, making - // Type appear as "aws" here instead. - Type: "AWS", - Namespace: "-", + Type: "aws", + Namespace: "hashicorp", Hostname: DefaultRegistryHost, }, false, diff --git a/configs/provider.go b/configs/provider.go index 0dd187045..3213e6858 100644 --- a/configs/provider.go +++ b/configs/provider.go @@ -40,8 +40,15 @@ func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { content, config, moreDiags := block.Body.PartialContent(providerBlockSchema) diags = append(diags, moreDiags...) + // Provider names must be localized. Produce an error with a message + // indicating the action the user can take to fix this message if the local + // name is not localized. + name := block.Labels[0] + nameDiags := checkProviderNameNormalized(name, block.DefRange) + diags = append(diags, nameDiags...) + provider := &Provider{ - Name: block.Labels[0], + Name: name, NameRange: block.LabelRanges[0], Config: config, DeclRange: block.DefRange, @@ -207,3 +214,31 @@ var providerBlockSchema = &hcl.BodySchema{ {Type: "locals"}, }, } + +// checkProviderNameNormalized verifies that the given string is already +// normalized and returns an error if not. +func checkProviderNameNormalized(name string, declrange hcl.Range) hcl.Diagnostics { + var diags hcl.Diagnostics + // verify that the provider local name is normalized + normalized, err := addrs.IsProviderPartNormalized(name) + if err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provider local name", + Detail: fmt.Sprintf("%s is an invalid provider local name: %s", name, err), + Subject: &declrange, + }) + return diags + } + if !normalized { + // we would have returned this error already + normalizedProvider, _ := addrs.ParseProviderPart(name) + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provider local name", + Detail: fmt.Sprintf("Provider names must be normalized. Replace %q with %q to fix this error.", name, normalizedProvider), + Subject: &declrange, + }) + } + return diags +} diff --git a/configs/provider_meta.go b/configs/provider_meta.go index 5db321373..e49614f64 100644 --- a/configs/provider_meta.go +++ b/configs/provider_meta.go @@ -13,10 +13,13 @@ type ProviderMeta struct { } func decodeProviderMetaBlock(block *hcl.Block) (*ProviderMeta, hcl.Diagnostics) { + // verify that the local name is already localized or produce an error. + diags := checkProviderNameNormalized(block.Labels[0], block.DefRange) + return &ProviderMeta{ Provider: block.Labels[0], ProviderRange: block.LabelRanges[0], Config: block.Body, DeclRange: block.DefRange, - }, nil + }, diags } diff --git a/configs/provider_requirements.go b/configs/provider_requirements.go index 531ba4f7f..361fec5eb 100644 --- a/configs/provider_requirements.go +++ b/configs/provider_requirements.go @@ -35,6 +35,10 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia diags = append(diags, err...) } + // verify that the local name is already localized or produce an error. + nameDiags := checkProviderNameNormalized(name, attr.Expr.Range()) + diags = append(diags, nameDiags...) + rp := &RequiredProvider{ Name: name, DeclRange: attr.Expr.Range(), @@ -69,6 +73,7 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia } if expr.Type().HasAttribute("source") { rp.Source = expr.GetAttr("source").AsString() + fqn, sourceDiags := addrs.ParseProviderSourceString(rp.Source) if sourceDiags.HasErrors() { @@ -98,7 +103,7 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia }) } - if rp.Type.IsZero() { + if rp.Type.IsZero() && !diags.HasErrors() { // Don't try to generate an FQN if we've encountered errors pType, err := addrs.ParseProviderPart(rp.Name) if err != nil { diags = append(diags, &hcl.Diagnostic{ diff --git a/configs/provider_requirements_test.go b/configs/provider_requirements_test.go index d204223b9..3d1da1a23 100644 --- a/configs/provider_requirements_test.go +++ b/configs/provider_requirements_test.go @@ -78,8 +78,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { Type: "required_providers", Body: hcltest.MockBody(&hcl.BodyContent{ Attributes: hcl.Attributes{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Expr: hcltest.MockExprLiteral(cty.ObjectVal(map[string]cty.Value{ "source": cty.StringVal("mycloud/test"), "version": cty.StringVal("2.0.0"), @@ -91,8 +91,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { }, Want: &RequiredProviders{ RequiredProviders: map[string]*RequiredProvider{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Source: "mycloud/test", Type: addrs.NewProvider(addrs.DefaultRegistryHost, "mycloud", "test"), Requirement: testVC("2.0.0"), @@ -111,8 +111,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { Name: "legacy", Expr: hcltest.MockExprLiteral(cty.StringVal("1.0.0")), }, - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Expr: hcltest.MockExprLiteral(cty.ObjectVal(map[string]cty.Value{ "source": cty.StringVal("mycloud/test"), "version": cty.StringVal("2.0.0"), @@ -130,8 +130,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { Requirement: testVC("1.0.0"), DeclRange: mockRange, }, - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Source: "mycloud/test", Type: addrs.NewProvider(addrs.DefaultRegistryHost, "mycloud", "test"), Requirement: testVC("2.0.0"), @@ -173,8 +173,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { Type: "required_providers", Body: hcltest.MockBody(&hcl.BodyContent{ Attributes: hcl.Attributes{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Expr: hcltest.MockExprLiteral(cty.ObjectVal(map[string]cty.Value{ "source": cty.StringVal("some/invalid/provider/source/test"), "version": cty.StringVal("~>2.0.0"), @@ -186,10 +186,9 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { }, Want: &RequiredProviders{ RequiredProviders: map[string]*RequiredProvider{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Source: "some/invalid/provider/source/test", - Type: addrs.Provider{}, Requirement: testVC("~>2.0.0"), DeclRange: mockRange, }, @@ -198,7 +197,7 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { }, Error: "Invalid provider source string", }, - "localname is invalid provider name": { + "invalid localname": { Block: &hcl.Block{ Type: "required_providers", Body: hcltest.MockBody(&hcl.BodyContent{ @@ -224,15 +223,43 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { }, DeclRange: blockRange, }, - Error: "Invalid provider name", + Error: "Invalid provider local name", + }, + "invalid localname caps": { + Block: &hcl.Block{ + Type: "required_providers", + Body: hcltest.MockBody(&hcl.BodyContent{ + Attributes: hcl.Attributes{ + "MYTEST": { + Name: "MYTEST", + Expr: hcltest.MockExprLiteral(cty.ObjectVal(map[string]cty.Value{ + "version": cty.StringVal("~>2.0.0"), + })), + }, + }, + }), + DefRange: blockRange, + }, + Want: &RequiredProviders{ + RequiredProviders: map[string]*RequiredProvider{ + "MYTEST": { + Name: "MYTEST", + Type: addrs.Provider{}, + Requirement: testVC("~>2.0.0"), + DeclRange: mockRange, + }, + }, + DeclRange: blockRange, + }, + Error: "Invalid provider local name", }, "version constraint error": { Block: &hcl.Block{ Type: "required_providers", Body: hcltest.MockBody(&hcl.BodyContent{ Attributes: hcl.Attributes{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Expr: hcltest.MockExprLiteral(cty.ObjectVal(map[string]cty.Value{ "source": cty.StringVal("mycloud/test"), "version": cty.StringVal("invalid"), @@ -244,8 +271,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { }, Want: &RequiredProviders{ RequiredProviders: map[string]*RequiredProvider{ - "my_test": { - Name: "my_test", + "my-test": { + Name: "my-test", Source: "mycloud/test", Type: addrs.NewProvider(addrs.DefaultRegistryHost, "mycloud", "test"), DeclRange: mockRange, @@ -272,7 +299,6 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { RequiredProviders: map[string]*RequiredProvider{ "test": { Name: "test", - Type: addrs.NewDefaultProvider("test"), DeclRange: mockRange, }, }, diff --git a/configs/resource.go b/configs/resource.go index 4643b98b6..72adf66f0 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -418,8 +418,13 @@ func decodeProviderConfigRef(expr hcl.Expression, argName string) (*ProviderConf return nil, diags } + // verify that the provider local name is normalized + name := traversal.RootName() + nameDiags := checkProviderNameNormalized(name, traversal[0].SourceRange()) + diags = append(diags, nameDiags...) + ret := &ProviderConfigRef{ - Name: traversal.RootName(), + Name: name, NameRange: traversal[0].SourceRange(), } diff --git a/configs/testdata/invalid-files/provider-localname-normalization.tf b/configs/testdata/invalid-files/provider-localname-normalization.tf new file mode 100644 index 000000000..34c6e62ba --- /dev/null +++ b/configs/testdata/invalid-files/provider-localname-normalization.tf @@ -0,0 +1,20 @@ +terraform { + required_providers { + test = { + source = "mycorp/test" + } + } +} + +provider "TEST" { + +} + +resource test_resource "test" { + // this resource is (implicitly) provided by "mycorp/test" +} + +resource test_resource "TEST" { + // this resource is (explicitly) provided by "hashicorp/test" + provider = TEST +} diff --git a/terraform/testdata/transform-provider-fqns-module/child/main.tf b/terraform/testdata/transform-provider-fqns-module/child/main.tf index 5c83ed0c1..5c56b7693 100644 --- a/terraform/testdata/transform-provider-fqns-module/child/main.tf +++ b/terraform/testdata/transform-provider-fqns-module/child/main.tf @@ -1,11 +1,11 @@ terraform { required_providers { - your_aws = { + your-aws = { source = "hashicorp/aws" } } } resource "aws_instance" "web" { - provider = "your_aws" + provider = "your-aws" } diff --git a/terraform/testdata/transform-provider-fqns-module/main.tf b/terraform/testdata/transform-provider-fqns-module/main.tf index 54d67e9ea..dd582c063 100644 --- a/terraform/testdata/transform-provider-fqns-module/main.tf +++ b/terraform/testdata/transform-provider-fqns-module/main.tf @@ -1,11 +1,11 @@ terraform { required_providers { - my_aws = { + my-aws = { source = "hashicorp/aws" } } } resource "aws_instance" "web" { - provider = "my_aws" + provider = "my-aws" } diff --git a/terraform/testdata/transform-provider-fqns/main.tf b/terraform/testdata/transform-provider-fqns/main.tf index 54d67e9ea..dd582c063 100644 --- a/terraform/testdata/transform-provider-fqns/main.tf +++ b/terraform/testdata/transform-provider-fqns/main.tf @@ -1,11 +1,11 @@ terraform { required_providers { - my_aws = { + my-aws = { source = "hashicorp/aws" } } } resource "aws_instance" "web" { - provider = "my_aws" + provider = "my-aws" }