From a03359822422a38f516bb64948264062097f6974 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 9 Feb 2021 08:37:59 -0500 Subject: [PATCH 1/7] update hcl and terraform-config-inspect --- go.mod | 4 ++-- go.sum | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 22bd84b6b1..180cdf2124 100644 --- a/go.mod +++ b/go.mod @@ -64,10 +64,10 @@ require ( github.com/hashicorp/go-uuid v1.0.1 github.com/hashicorp/go-version v1.2.0 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f - github.com/hashicorp/hcl/v2 v2.8.2 + github.com/hashicorp/hcl/v2 v2.8.3-0.20210208211639-2520246c49a7 github.com/hashicorp/memberlist v0.1.0 // indirect github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb // indirect - github.com/hashicorp/terraform-config-inspect v0.0.0-20191212124732-c6ae6269b9d7 + github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2 github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734 github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d // indirect github.com/jmespath/go-jmespath v0.4.0 diff --git a/go.sum b/go.sum index 70bbf69c4c..dbb03bf7ed 100644 --- a/go.sum +++ b/go.sum @@ -349,12 +349,16 @@ github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f/go.mod h1:oZtUIOe8dh github.com/hashicorp/hcl/v2 v2.0.0/go.mod h1:oVVDG71tEinNGYCxinCYadcmKU9bglqW9pV3txagJ90= github.com/hashicorp/hcl/v2 v2.8.2 h1:wmFle3D1vu0okesm8BTLVDyJ6/OL9DCLUwn0b2OptiY= github.com/hashicorp/hcl/v2 v2.8.2/go.mod h1:bQTN5mpo+jewjJgh8jr0JUguIi7qPHUF6yIfAEN3jqY= +github.com/hashicorp/hcl/v2 v2.8.3-0.20210208211639-2520246c49a7 h1:9YW4rFk/VhcwWW2Mu0SuMO4/ygk6dg7EIRqPd2avDXQ= +github.com/hashicorp/hcl/v2 v2.8.3-0.20210208211639-2520246c49a7/go.mod h1:bQTN5mpo+jewjJgh8jr0JUguIi7qPHUF6yIfAEN3jqY= github.com/hashicorp/memberlist v0.1.0 h1:qSsCiC0WYD39lbSitKNt40e30uorm2Ss/d4JGU1hzH8= github.com/hashicorp/memberlist v0.1.0/go.mod h1:ncdBp14cuox2iFOq3kDiquKU6fqsTBc3W6JvZwjxxsE= github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb h1:ZbgmOQt8DOg796figP87/EFCVx2v2h9yRvwHF/zceX4= github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb/go.mod h1:h/Ru6tmZazX7WO/GDmwdpS975F019L4t5ng5IgwbNrE= github.com/hashicorp/terraform-config-inspect v0.0.0-20191212124732-c6ae6269b9d7 h1:Pc5TCv9mbxFN6UVX0LH6CpQrdTM5YjbVI2w15237Pjk= github.com/hashicorp/terraform-config-inspect v0.0.0-20191212124732-c6ae6269b9d7/go.mod h1:p+ivJws3dpqbp1iP84+npOyAmTTOLMgCzrXd3GSdn/A= +github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2 h1:l+bLFvHjqtgNQwWxwrFX9PemGAAO2P1AGZM7zlMNvCs= +github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2/go.mod h1:Z0Nnk4+3Cy89smEbrq+sl1bxc9198gIP4I7wcQF6Kqs= github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734 h1:HKLsbzeOsfXmKNpr3GiT18XAblV0BjCbzL8KQAMZGa0= github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734/go.mod h1:kNDNcF7sN4DocDLBkQYz73HGKwN1ANB1blq4lIYLYvg= github.com/hashicorp/yamux v0.0.0-20180604194846-3520598351bb h1:b5rjCoWHc7eqmAS4/qyk21ZsHyb6Mxv/jykxvNTkU4M= From ac585be079ac51e6bb091ba7e05be961e7e46800 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 9 Feb 2021 08:38:30 -0500 Subject: [PATCH 2/7] initial support for parsing configuration_aliases Add support for parsing configuration_aliases in required_providers entries. The decoder needed to be re-written here in order to support the bare reference style usage of provider names so that they match the usage in other location within configuration. The only change to existing handling of the required_providers block is more precise error locations in a couple cases. --- configs/provider_requirements.go | 289 +++++++++++------- configs/provider_requirements_test.go | 78 +---- .../error-files/provider-source-prefix.tf | 8 +- .../valid-modules/provider-aliases/main.tf | 19 ++ 4 files changed, 226 insertions(+), 168 deletions(-) create mode 100644 configs/testdata/valid-modules/provider-aliases/main.tf diff --git a/configs/provider_requirements.go b/configs/provider_requirements.go index ef746f3faa..2c376d3daf 100644 --- a/configs/provider_requirements.go +++ b/configs/provider_requirements.go @@ -1,6 +1,8 @@ package configs import ( + "fmt" + version "github.com/hashicorp/go-version" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" @@ -17,6 +19,7 @@ type RequiredProvider struct { Type addrs.Provider Requirement VersionConstraint DeclRange hcl.Range + Aliases []addrs.LocalProviderConfig } type RequiredProviders struct { @@ -26,118 +29,27 @@ type RequiredProviders struct { func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Diagnostics) { attrs, diags := block.Body.JustAttributes() + if diags.HasErrors() { + return nil, diags + } + ret := &RequiredProviders{ RequiredProviders: make(map[string]*RequiredProvider), DeclRange: block.DefRange, } + for name, attr := range attrs { - expr, err := attr.Expr.Value(nil) - if err != nil { - 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(), } - switch { - case expr.Type().IsPrimitiveType(): + // Look for a single static string, in case we have the legacy version-only + // format in the configuration. + if expr, err := attr.Expr.Value(nil); err == nil && expr.Type().IsPrimitiveType() { vc, reqDiags := decodeVersionConstraint(attr) diags = append(diags, reqDiags...) - rp.Requirement = vc - case expr.Type().IsObjectType(): - if expr.Type().HasAttribute("version") { - vc := VersionConstraint{ - DeclRange: attr.Range, - } - constraint := expr.GetAttr("version") - if !constraint.Type().Equals(cty.String) || constraint.IsNull() { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid version constraint", - Detail: "Version must be specified as a string.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - constraintStr := constraint.AsString() - constraints, err := version.NewConstraint(constraintStr) - if err != nil { - // NewConstraint doesn't return user-friendly errors, so we'll just - // ignore the provided error and produce our own generic one. - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid version constraint", - Detail: "This string does not use correct version constraint syntax.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - vc.Required = constraints - rp.Requirement = vc - } - } - } - if expr.Type().HasAttribute("source") { - source := expr.GetAttr("source") - if !source.Type().Equals(cty.String) || source.IsNull() { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid source", - Detail: "Source must be specified as a string.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - rp.Source = source.AsString() - - fqn, sourceDiags := addrs.ParseProviderSourceString(rp.Source) - - if sourceDiags.HasErrors() { - hclDiags := sourceDiags.ToHCL() - // The diagnostics from ParseProviderSourceString don't contain - // source location information because it has no context to compute - // them from, and so we'll add those in quickly here before we - // return. - for _, diag := range hclDiags { - if diag.Subject == nil { - diag.Subject = attr.Expr.Range().Ptr() - } - } - diags = append(diags, hclDiags...) - } else { - rp.Type = fqn - } - } - } - attrTypes := expr.Type().AttributeTypes() - for name := range attrTypes { - if name == "version" || name == "source" { - continue - } - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid required_providers object", - Detail: `required_providers objects can only contain "version" and "source" attributes. To configure a provider, use a "provider" block.`, - Subject: attr.Expr.Range().Ptr(), - }) - break - } - - default: - // should not happen - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid required_providers syntax", - Detail: "required_providers entries must be strings or objects.", - Subject: attr.Expr.Range().Ptr(), - }) - } - - 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{ @@ -146,12 +58,185 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia Detail: err.Error(), Subject: attr.Expr.Range().Ptr(), }) - } else { - rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) + continue } + + rp.Requirement = vc + rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) + ret.RequiredProviders[name] = rp + + continue } - ret.RequiredProviders[rp.Name] = rp + // verify that the local name is already localized or produce an error. + nameDiags := checkProviderNameNormalized(name, attr.Expr.Range()) + if nameDiags.HasErrors() { + diags = append(diags, nameDiags...) + continue + } + + kvs, mapDiags := hcl.ExprMap(attr.Expr) + if mapDiags.HasErrors() { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid required_providers object", + Detail: "required_providers entries must be strings or objects.", + Subject: attr.Expr.Range().Ptr(), + }) + continue + } + + for _, kv := range kvs { + key, keyDiags := kv.Key.Value(nil) + if keyDiags.HasErrors() { + diags = append(diags, keyDiags...) + continue + } + + if key.Type() != cty.String { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid Attribute", + Detail: fmt.Sprintf("Invalid attribute value for provider requirement: %#v", key), + Subject: kv.Key.Range().Ptr(), + }) + continue + } + + switch key.AsString() { + case "version": + vc := VersionConstraint{ + DeclRange: attr.Range, + } + + constraint, valDiags := kv.Value.Value(nil) + if valDiags.HasErrors() || !constraint.Type().Equals(cty.String) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid version constraint", + Detail: "Version must be specified as a string.", + Subject: kv.Value.Range().Ptr(), + }) + continue + } + + constraintStr := constraint.AsString() + constraints, err := version.NewConstraint(constraintStr) + if err != nil { + // NewConstraint doesn't return user-friendly errors, so we'll just + // ignore the provided error and produce our own generic one. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid version constraint", + Detail: "This string does not use correct version constraint syntax.", + Subject: kv.Value.Range().Ptr(), + }) + continue + } + + vc.Required = constraints + rp.Requirement = vc + + case "source": + source, err := kv.Value.Value(nil) + if err != nil || !source.Type().Equals(cty.String) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid source", + Detail: "Source must be specified as a string.", + Subject: kv.Value.Range().Ptr(), + }) + continue + } + + fqn, sourceDiags := addrs.ParseProviderSourceString(source.AsString()) + if sourceDiags.HasErrors() { + hclDiags := sourceDiags.ToHCL() + // The diagnostics from ParseProviderSourceString don't contain + // source location information because it has no context to compute + // them from, and so we'll add those in quickly here before we + // return. + for _, diag := range hclDiags { + if diag.Subject == nil { + diag.Subject = kv.Value.Range().Ptr() + } + } + diags = append(diags, hclDiags...) + continue + } + + rp.Source = source.AsString() + rp.Type = fqn + + case "configuration_aliases": + exprs, listDiags := hcl.ExprList(kv.Value) + if listDiags.HasErrors() { + diags = append(diags, listDiags...) + continue + } + + for _, expr := range exprs { + traversal, travDiags := hcl.AbsTraversalForExpr(expr) + if travDiags.HasErrors() { + diags = append(diags, travDiags...) + continue + } + + addr, cfgDiags := ParseProviderConfigCompact(traversal) + if cfgDiags.HasErrors() { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid configuration_aliases value", + Detail: `Configuration aliases can only contain references to local provider configuration names in the format of provider.alias`, + Subject: kv.Value.Range().Ptr(), + }) + continue + } + + if addr.LocalName != name { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid configuration_aliases value", + Detail: fmt.Sprintf(`Configuration aliases must be prefixed with the provider name. Expected %q, but found %q.`, name, addr.LocalName), + Subject: kv.Value.Range().Ptr(), + }) + continue + } + + rp.Aliases = append(rp.Aliases, addr) + } + + default: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid required_providers object", + Detail: `required_providers objects can only contain "version", "source" and "configuration_aliases" attributes. To configure a provider, use a "provider" block.`, + Subject: kv.Key.Range().Ptr(), + }) + break + } + + } + + // finally add the required provider as long as there were no errors + if !diags.HasErrors() { + // if a source was not given, create an implied type + if rp.Type.IsZero() { + pType, err := addrs.ParseProviderPart(rp.Name) + if err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provider name", + Detail: err.Error(), + Subject: attr.Expr.Range().Ptr(), + }) + } else { + rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) + } + } + + ret.RequiredProviders[rp.Name] = rp + } } return ret, diags diff --git a/configs/provider_requirements_test.go b/configs/provider_requirements_test.go index bf4d998821..69cac1b5b3 100644 --- a/configs/provider_requirements_test.go +++ b/configs/provider_requirements_test.go @@ -185,15 +185,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "my-test": { - Name: "my-test", - Source: "some/invalid/provider/source/test", - Requirement: testVC("~>2.0.0"), - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid provider source string", }, @@ -213,15 +206,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "my_test": { - Name: "my_test", - Type: addrs.Provider{}, - Requirement: testVC("~>2.0.0"), - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid provider local name", }, @@ -241,15 +227,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "MYTEST": { - Name: "MYTEST", - Type: addrs.Provider{}, - Requirement: testVC("~>2.0.0"), - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid provider local name", }, @@ -270,15 +249,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "my-test": { - Name: "my-test", - Source: "mycloud/test", - Type: addrs.NewProvider(addrs.DefaultRegistryHost, "mycloud", "test"), - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid version constraint", }, @@ -296,15 +268,10 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "test": { - Name: "test", - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, - Error: "Invalid required_providers syntax", + Error: "Invalid required_providers object", }, "invalid source attribute type": { Block: &hcl.Block{ @@ -322,13 +289,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "my-test": { - Name: "my-test", - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid source", }, @@ -350,16 +312,8 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { DefRange: blockRange, }, Want: &RequiredProviders{ - RequiredProviders: map[string]*RequiredProvider{ - "my-test": { - Name: "my-test", - Source: "mycloud/test", - Type: addrs.NewProvider(addrs.DefaultRegistryHost, "mycloud", "test"), - Requirement: testVC("2.0.0"), - DeclRange: mockRange, - }, - }, - DeclRange: blockRange, + RequiredProviders: map[string]*RequiredProvider{}, + DeclRange: blockRange, }, Error: "Invalid required_providers object", }, @@ -370,7 +324,7 @@ func TestDecodeRequiredProvidersBlock(t *testing.T) { got, diags := decodeRequiredProvidersBlock(test.Block) if diags.HasErrors() { if test.Error == "" { - t.Fatalf("unexpected error") + t.Fatalf("unexpected error: %v", diags) } if gotErr := diags[0].Summary; gotErr != test.Error { t.Errorf("wrong error, got %q, want %q", gotErr, test.Error) diff --git a/configs/testdata/error-files/provider-source-prefix.tf b/configs/testdata/error-files/provider-source-prefix.tf index 99cd76df70..96811699fc 100644 --- a/configs/testdata/error-files/provider-source-prefix.tf +++ b/configs/testdata/error-files/provider-source-prefix.tf @@ -1,10 +1,10 @@ terraform { required_providers { - usererror = { # ERROR: Invalid provider type - source = "foo/terraform-provider-foo" + usererror = { + source = "foo/terraform-provider-foo" # ERROR: Invalid provider type } - badname = { # ERROR: Invalid provider type - source = "foo/terraform-foo" + badname = { + source = "foo/terraform-foo" # ERROR: Invalid provider type } } } diff --git a/configs/testdata/valid-modules/provider-aliases/main.tf b/configs/testdata/valid-modules/provider-aliases/main.tf new file mode 100644 index 0000000000..b9795b30d2 --- /dev/null +++ b/configs/testdata/valid-modules/provider-aliases/main.tf @@ -0,0 +1,19 @@ +terraform { + required_providers { + foo-test = { + source = "foo/test" + // TODO: these are strings until the parsing code is refactored to allow + // raw references + configuration_aliases = [foo-test.a, foo-test.b] + } + } +} + +resource "test_instance" "explicit" { + provider = foo-test.a +} + +data "test_resource" "explicit" { + provider = foo-test.b +} + From 7aaffac223c62bfb262907e2d0d2a01eea1bf227 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Feb 2021 10:20:40 -0500 Subject: [PATCH 3/7] configload should not be doing validation The configload package should only be responsible for locating and loading the configuration, and not be further inspecting the config source itself. Moving the validating into the configs package. --- configs/configload/loader_load.go | 78 ------------------- configs/configload/loader_load_test.go | 63 --------------- .../.terraform/modules/modules.json | 34 -------- .../more-nested-provider/child/main.tf | 4 - .../more-nested-provider/child2/main.tf | 4 - .../more-nested-provider/child3/main.tf | 7 -- .../more-nested-provider/root.tf | 4 - .../.terraform/modules/modules.json | 24 ------ .../nested-provider/child/main.tf | 4 - .../nested-provider/child2/main.tf | 7 -- .../expand-modules/nested-provider/root.tf | 4 - .../.terraform/modules/modules.json | 14 ---- .../no-provider-passed/child/main.tf | 7 -- .../expand-modules/no-provider-passed/root.tf | 9 --- .../.terraform/modules/modules.json | 14 ---- .../provider-configured/child/main.tf | 7 -- .../provider-configured/root.tf | 11 --- .../valid/.terraform/modules/modules.json | 19 ----- .../valid/child-with-alias/main.tf | 8 -- .../expand-modules/valid/child/main.tf | 7 -- .../testdata/expand-modules/valid/root.tf | 20 ----- 21 files changed, 349 deletions(-) delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf delete mode 100644 configs/configload/testdata/expand-modules/more-nested-provider/root.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/child2/main.tf delete mode 100644 configs/configload/testdata/expand-modules/nested-provider/root.tf delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/no-provider-passed/root.tf delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/provider-configured/root.tf delete mode 100644 configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json delete mode 100644 configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf delete mode 100644 configs/configload/testdata/expand-modules/valid/child/main.tf delete mode 100644 configs/configload/testdata/expand-modules/valid/root.tf diff --git a/configs/configload/loader_load.go b/configs/configload/loader_load.go index eab38495c2..e3c9bdca6c 100644 --- a/configs/configload/loader_load.go +++ b/configs/configload/loader_load.go @@ -100,83 +100,5 @@ func (l *Loader) moduleWalkerLoad(req *configs.ModuleRequest) (*configs.Module, } } - // The providers associated with expanding modules must be present in the proxy/passed providers - // block. Guarding here for accessing the module call just in case. - if mc, exists := req.Parent.Module.ModuleCalls[req.Name]; exists { - var validateDiags hcl.Diagnostics - validateDiags = validateProviderConfigs(mc, mod, req.Parent, validateDiags) - diags = append(diags, validateDiags...) - } return mod, record.Version, diags } - -func validateProviderConfigs(mc *configs.ModuleCall, mod *configs.Module, parent *configs.Config, diags hcl.Diagnostics) hcl.Diagnostics { - if mc.Count != nil || mc.ForEach != nil || mc.DependsOn != nil { - for key, pc := range mod.ProviderConfigs { - // Use these to track if a provider is configured (not allowed), - // or if we've found its matching proxy - var isConfigured bool - var foundMatchingProxy bool - - // Validate the config against an empty schema to see if it's empty. - _, pcConfigDiags := pc.Config.Content(&hcl.BodySchema{}) - if pcConfigDiags.HasErrors() || pc.Version.Required != nil { - isConfigured = true - } - - // If it is empty or only has an alias, - // does this provider exist in our proxy configs? - for _, r := range mc.Providers { - // Must match on name and Alias - if pc.Name == r.InChild.Name && pc.Alias == r.InChild.Alias { - foundMatchingProxy = true - break - } - } - if isConfigured || !foundMatchingProxy { - if mc.Count != nil { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Module does not support count", - Detail: fmt.Sprintf(moduleProviderError, mc.Name, "count", key, pc.NameRange), - Subject: mc.Count.Range().Ptr(), - }) - } - if mc.ForEach != nil { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Module does not support for_each", - Detail: fmt.Sprintf(moduleProviderError, mc.Name, "for_each", key, pc.NameRange), - Subject: mc.ForEach.Range().Ptr(), - }) - } - if mc.DependsOn != nil { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Module does not support depends_on", - Detail: fmt.Sprintf(moduleProviderError, mc.Name, "depends_on", key, pc.NameRange), - Subject: mc.SourceAddrRange.Ptr(), - }) - } - } - } - } - // If this module has further parents, go through them recursively - if !parent.Path.IsRoot() { - // Use the path to get the name so we can look it up in the parent module calls - path := parent.Path - name := path[len(path)-1] - // This parent's module call, so we can check for count/for_each here, - // guarding with exists just in case. We pass the diags through to the recursive - // call so they will accumulate if needed. - if mc, exists := parent.Parent.Module.ModuleCalls[name]; exists { - return validateProviderConfigs(mc, mod, parent.Parent, diags) - } - } - - return diags -} - -var moduleProviderError = `Module "%s" cannot be used with %s because it contains a nested provider configuration for "%s", at %s. - -This module can be made compatible with %[2]s by changing it to receive all of its provider configurations from the calling module, by using the "providers" argument in the calling module block.` diff --git a/configs/configload/loader_load_test.go b/configs/configload/loader_load_test.go index b7f396cf22..845b227a38 100644 --- a/configs/configload/loader_load_test.go +++ b/configs/configload/loader_load_test.go @@ -1,7 +1,6 @@ package configload import ( - "fmt" "path/filepath" "reflect" "sort" @@ -81,65 +80,3 @@ func TestLoaderLoadConfig_addVersion(t *testing.T) { t.Fatalf("wrong error\ngot:\n%s\n\nwant: containing %q", got, want) } } - -func TestLoaderLoadConfig_moduleExpand(t *testing.T) { - // We do not allow providers to be configured in expanding modules - // In addition, if a provider is present but an empty block, it is allowed, - // but IFF a provider is passed through the module call - paths := []string{"provider-configured", "no-provider-passed", "nested-provider", "more-nested-provider"} - for _, p := range paths { - fixtureDir := filepath.Clean(fmt.Sprintf("testdata/expand-modules/%s", p)) - loader, err := NewLoader(&Config{ - ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"), - }) - if err != nil { - t.Fatalf("unexpected error from NewLoader at path %s: %s", p, err) - } - - _, diags := loader.LoadConfig(fixtureDir) - if !diags.HasErrors() { - t.Fatalf("success; want error at path %s", p) - } - got := diags.Error() - want := "Module does not support count" - if !strings.Contains(got, want) { - t.Fatalf("wrong error at path %s \ngot:\n%s\n\nwant: containing %q", p, got, want) - } - } -} - -func TestLoaderLoadConfig_moduleExpandValid(t *testing.T) { - // This tests for when valid configs are passing a provider through as a proxy, - // either with or without an alias present. - fixtureDir := filepath.Clean("testdata/expand-modules/valid") - loader, err := NewLoader(&Config{ - ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"), - }) - if err != nil { - t.Fatalf("unexpected error from NewLoader: %s", err) - } - - _, diags := loader.LoadConfig(fixtureDir) - assertNoDiagnostics(t, diags) -} - -func TestLoaderLoadConfig_moduleDependsOnProviders(t *testing.T) { - // We do not allow providers to be configured in module using depends_on. - fixtureDir := filepath.Clean("testdata/module-depends-on") - loader, err := NewLoader(&Config{ - ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"), - }) - if err != nil { - t.Fatalf("unexpected error from NewLoader: %s", err) - } - - _, diags := loader.LoadConfig(fixtureDir) - if !diags.HasErrors() { - t.Fatal("success; want error") - } - got := diags.Error() - want := "Module does not support depends_on" - if !strings.Contains(got, want) { - t.Fatalf("wrong error\ngot:\n%s\n\nwant: containing %q", got, want) - } -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json deleted file mode 100644 index 203e0876d5..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/.terraform/modules/modules.json +++ /dev/null @@ -1,34 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/nested-provider" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/nested-provider/child" - }, - { - "Key": "child2", - "Source": "./child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child3", - "Source": "./child3", - "Dir": "testdata/expand-modules/nested-provider/child3" - }, - { - "Key": "child.child2", - "Source": "../child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child.child2.child3", - "Source": "../child3", - "Dir": "testdata/expand-modules/nested-provider/child3" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf deleted file mode 100644 index b4bbb38c13..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child2" { - source = "../child2" - -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf deleted file mode 100644 index d107faad87..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child2/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child3" { - source = "../child3" - -} diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf b/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf deleted file mode 100644 index 01cd854232..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/child3/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - -} - -output "my_output" { - value = "my output" -} \ No newline at end of file diff --git a/configs/configload/testdata/expand-modules/more-nested-provider/root.tf b/configs/configload/testdata/expand-modules/more-nested-provider/root.tf deleted file mode 100644 index 71b90f6d67..0000000000 --- a/configs/configload/testdata/expand-modules/more-nested-provider/root.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - count = 1 - source = "./child" -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json deleted file mode 100644 index 28f813039e..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/.terraform/modules/modules.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/nested-provider" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/nested-provider/child" - }, - { - "Key": "child2", - "Source": "./child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - }, - { - "Key": "child.child2", - "Source": "../child2", - "Dir": "testdata/expand-modules/nested-provider/child2" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/child/main.tf b/configs/configload/testdata/expand-modules/nested-provider/child/main.tf deleted file mode 100644 index b4bbb38c13..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/child/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child2" { - source = "../child2" - -} diff --git a/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf b/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf deleted file mode 100644 index 01cd854232..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/child2/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - -} - -output "my_output" { - value = "my output" -} \ No newline at end of file diff --git a/configs/configload/testdata/expand-modules/nested-provider/root.tf b/configs/configload/testdata/expand-modules/nested-provider/root.tf deleted file mode 100644 index 71b90f6d67..0000000000 --- a/configs/configload/testdata/expand-modules/nested-provider/root.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - count = 1 - source = "./child" -} diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json deleted file mode 100644 index 8c0d923676..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/.terraform/modules/modules.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/no-provider-passed" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/no-provider-passed/child" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf b/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf deleted file mode 100644 index a5c3c47b17..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { -} - -output "my_output" { - value = "my output" -} - diff --git a/configs/configload/testdata/expand-modules/no-provider-passed/root.tf b/configs/configload/testdata/expand-modules/no-provider-passed/root.tf deleted file mode 100644 index 195cfeb5d4..0000000000 --- a/configs/configload/testdata/expand-modules/no-provider-passed/root.tf +++ /dev/null @@ -1,9 +0,0 @@ -provider "aws" { - alias = "usw2" - region = "us-west-2" -} -module "child" { - count = 1 - source = "./child" - # To make this test fail, add a valid providers {} block passing "aws" to the child -} diff --git a/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json deleted file mode 100644 index b7a474ea32..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/.terraform/modules/modules.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/provider-configured" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/provider-configured/child" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/provider-configured/child/main.tf b/configs/configload/testdata/expand-modules/provider-configured/child/main.tf deleted file mode 100644 index 61ff4b572a..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { - region = "us-west-2" -} - -output "my_output" { - value = "my output" -} diff --git a/configs/configload/testdata/expand-modules/provider-configured/root.tf b/configs/configload/testdata/expand-modules/provider-configured/root.tf deleted file mode 100644 index 953d4ab554..0000000000 --- a/configs/configload/testdata/expand-modules/provider-configured/root.tf +++ /dev/null @@ -1,11 +0,0 @@ -provider "aws" { - region = "us-west-2" -} - -module "child" { - count = 1 - source = "./child" - providers = { - aws = aws.w2 - } -} diff --git a/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json b/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json deleted file mode 100644 index 0bdb37d5bd..0000000000 --- a/configs/configload/testdata/expand-modules/valid/.terraform/modules/modules.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "Modules": [ - { - "Key": "", - "Source": "", - "Dir": "testdata/expand-modules/valid" - }, - { - "Key": "child", - "Source": "./child", - "Dir": "testdata/expand-modules/valid/child" - }, - { - "Key": "child_with_alias", - "Source": "./child-with-alias", - "Dir": "testdata/expand-modules/valid/child-with-alias" - } - ] -} diff --git a/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf b/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf deleted file mode 100644 index 3a59131cb9..0000000000 --- a/configs/configload/testdata/expand-modules/valid/child-with-alias/main.tf +++ /dev/null @@ -1,8 +0,0 @@ -provider "aws" { - alias = "east" -} - -output "my_output" { - value = "my output" -} - diff --git a/configs/configload/testdata/expand-modules/valid/child/main.tf b/configs/configload/testdata/expand-modules/valid/child/main.tf deleted file mode 100644 index a5c3c47b17..0000000000 --- a/configs/configload/testdata/expand-modules/valid/child/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -provider "aws" { -} - -output "my_output" { - value = "my output" -} - diff --git a/configs/configload/testdata/expand-modules/valid/root.tf b/configs/configload/testdata/expand-modules/valid/root.tf deleted file mode 100644 index 27205a508a..0000000000 --- a/configs/configload/testdata/expand-modules/valid/root.tf +++ /dev/null @@ -1,20 +0,0 @@ -provider "aws" { - region = "us-east-1" - alias = "east" -} - -module "child" { - count = 1 - source = "./child" - providers = { - aws = aws.east - } -} - -module "child_with_alias" { - for_each = toset(["a", "b"]) - source = "./child-with-alias" - providers = { - aws.east = aws.east - } -} \ No newline at end of file From da252de1a040bbf10e3df8eaa40da2a96a5f731f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Feb 2021 11:09:35 -0500 Subject: [PATCH 4/7] add provider validation Add validation which was removed from the configload package, along with additional validation checks. The output is slightly different, as instead of validating whether the modules are allowed to have provider configurations, we validate the various combinations of provider structures themselves. --- configs/config_build.go | 3 + configs/config_build_test.go | 125 +++++++++ configs/provider_requirements.go | 38 +-- configs/provider_validation.go | 243 ++++++++++++++++++ .../config-diagnostics/empty-configs/main.tf | 20 ++ .../empty-configs/mod/main.tf | 22 ++ .../config-diagnostics/empty-configs/warnings | 4 + .../config-diagnostics/incorrect-type/errors | 1 + .../config-diagnostics/incorrect-type/main.tf | 18 ++ .../incorrect-type/mod/main.tf | 14 + .../incorrect-type/warnings | 1 + .../nested-provider/child/child2/main.tf | 7 + .../nested-provider/child/main.tf | 4 + .../config-diagnostics/nested-provider/errors | 3 + .../nested-provider/root.tf | 4 + .../override-provider/errors | 1 + .../override-provider/main.tf | 19 ++ .../override-provider/mod/main.tf | 12 + .../config-diagnostics/required-alias/errors | 1 + .../config-diagnostics/required-alias/main.tf | 4 + .../required-alias/mod/main.tf | 13 + .../unexpected-provider/main.tf | 15 ++ .../unexpected-provider/mod/main.tf | 2 + .../unexpected-provider/warnings | 2 + .../with-depends-on/main.tf | 14 + .../with-depends-on/mod1/main.tf | 19 ++ .../with-depends-on/mod1/mod2/main.tf | 15 ++ .../with-depends-on/mod1/mod2/mod3/main.tf | 12 + 28 files changed, 618 insertions(+), 18 deletions(-) create mode 100644 configs/provider_validation.go create mode 100644 configs/testdata/config-diagnostics/empty-configs/main.tf create mode 100644 configs/testdata/config-diagnostics/empty-configs/mod/main.tf create mode 100644 configs/testdata/config-diagnostics/empty-configs/warnings create mode 100644 configs/testdata/config-diagnostics/incorrect-type/errors create mode 100644 configs/testdata/config-diagnostics/incorrect-type/main.tf create mode 100644 configs/testdata/config-diagnostics/incorrect-type/mod/main.tf create mode 100644 configs/testdata/config-diagnostics/incorrect-type/warnings create mode 100644 configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf create mode 100644 configs/testdata/config-diagnostics/nested-provider/child/main.tf create mode 100644 configs/testdata/config-diagnostics/nested-provider/errors create mode 100644 configs/testdata/config-diagnostics/nested-provider/root.tf create mode 100644 configs/testdata/config-diagnostics/override-provider/errors create mode 100644 configs/testdata/config-diagnostics/override-provider/main.tf create mode 100644 configs/testdata/config-diagnostics/override-provider/mod/main.tf create mode 100644 configs/testdata/config-diagnostics/required-alias/errors create mode 100644 configs/testdata/config-diagnostics/required-alias/main.tf create mode 100644 configs/testdata/config-diagnostics/required-alias/mod/main.tf create mode 100644 configs/testdata/config-diagnostics/unexpected-provider/main.tf create mode 100644 configs/testdata/config-diagnostics/unexpected-provider/mod/main.tf create mode 100644 configs/testdata/config-diagnostics/unexpected-provider/warnings create mode 100644 configs/testdata/config-diagnostics/with-depends-on/main.tf create mode 100644 configs/testdata/config-diagnostics/with-depends-on/mod1/main.tf create mode 100644 configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/main.tf create mode 100644 configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/mod3/main.tf diff --git a/configs/config_build.go b/configs/config_build.go index 653a85b0f8..345c678144 100644 --- a/configs/config_build.go +++ b/configs/config_build.go @@ -22,6 +22,9 @@ func BuildConfig(root *Module, walker ModuleWalker) (*Config, hcl.Diagnostics) { } cfg.Root = cfg // Root module is self-referential. cfg.Children, diags = buildChildModules(cfg, walker) + + diags = append(diags, validateProviderConfigs(nil, cfg, false)...) + return cfg, diags } diff --git a/configs/config_build_test.go b/configs/config_build_test.go index 59f9228c14..a977e432be 100644 --- a/configs/config_build_test.go +++ b/configs/config_build_test.go @@ -2,6 +2,7 @@ package configs import ( "fmt" + "io/ioutil" "path/filepath" "reflect" "sort" @@ -154,3 +155,127 @@ func TestBuildConfigChildModuleBackend(t *testing.T) { t.Fatalf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want)) } } + +func TestBuildConfigInvalidModules(t *testing.T) { + testDir := "testdata/config-diagnostics" + dirs, err := ioutil.ReadDir(testDir) + if err != nil { + t.Fatal(err) + } + + for _, info := range dirs { + name := info.Name() + t.Run(name, func(t *testing.T) { + parser := NewParser(nil) + path := filepath.Join(testDir, name) + + mod, diags := parser.LoadConfigDir(path) + if diags.HasErrors() { + // these tests should only trigger errors that are caught in + // the config loader. + t.Errorf("error loading config dir") + for _, diag := range diags { + t.Logf("- %s", diag) + } + } + + readDiags := func(data []byte, _ error) []string { + var expected []string + for _, s := range strings.Split(string(data), "\n") { + msg := strings.TrimSpace(s) + msg = strings.ReplaceAll(msg, `\n`, "\n") + if msg != "" { + expected = append(expected, msg) + } + } + return expected + } + + // Load expected errors and warnings. + // Each line in the file is matched as a substring against the + // diagnostic outputs. + // Capturing part of the path and source range in the message lets + // us also ensure the diagnostic is being attributed to the + // expected location in the source, but is not required. + // The literal characters `\n` are replaced with newlines, but + // otherwise the string is unchanged. + expectedErrs := readDiags(ioutil.ReadFile(filepath.Join(testDir, name, "errors"))) + expectedWarnings := readDiags(ioutil.ReadFile(filepath.Join(testDir, name, "warnings"))) + + _, buildDiags := BuildConfig(mod, ModuleWalkerFunc( + func(req *ModuleRequest) (*Module, *version.Version, hcl.Diagnostics) { + // for simplicity, these tests will treat all source + // addresses as relative to the root module + sourcePath := filepath.Join(path, req.SourceAddr) + mod, diags := parser.LoadConfigDir(sourcePath) + version, _ := version.NewVersion("1.0.0") + return mod, version, diags + }, + )) + + // we can make this less repetitive later if we want + for _, msg := range expectedErrs { + found := false + for _, diag := range buildDiags { + if diag.Severity == hcl.DiagError && strings.Contains(diag.Error(), msg) { + found = true + break + } + } + + if !found { + t.Errorf("Expected error diagnostic containing %q", msg) + } + } + + for _, diag := range buildDiags { + if diag.Severity != hcl.DiagError { + continue + } + found := false + for _, msg := range expectedErrs { + if strings.Contains(diag.Error(), msg) { + found = true + break + } + } + + if !found { + t.Errorf("Unexpected error: %q", diag) + } + } + + for _, msg := range expectedWarnings { + found := false + for _, diag := range buildDiags { + if diag.Severity == hcl.DiagWarning && strings.Contains(diag.Error(), msg) { + found = true + break + } + } + + if !found { + t.Errorf("Expected warning diagnostic containing %q", msg) + } + } + + for _, diag := range buildDiags { + if diag.Severity != hcl.DiagWarning { + continue + } + found := false + for _, msg := range expectedWarnings { + if strings.Contains(diag.Error(), msg) { + found = true + break + } + } + + if !found { + t.Errorf("Unexpected warning: %q", diag) + } + } + + }) + } +} diff --git a/configs/provider_requirements.go b/configs/provider_requirements.go index 2c376d3daf..f870e1cc96 100644 --- a/configs/provider_requirements.go +++ b/configs/provider_requirements.go @@ -218,25 +218,27 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Dia } - // finally add the required provider as long as there were no errors - if !diags.HasErrors() { - // if a source was not given, create an implied type - if rp.Type.IsZero() { - pType, err := addrs.ParseProviderPart(rp.Name) - if err != nil { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid provider name", - Detail: err.Error(), - Subject: attr.Expr.Range().Ptr(), - }) - } else { - rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) - } - } - - ret.RequiredProviders[rp.Name] = rp + if diags.HasErrors() { + continue } + + // We can add the required provider when there are no errors. + // If a source was not given, create an implied type. + if rp.Type.IsZero() { + pType, err := addrs.ParseProviderPart(rp.Name) + if err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid provider name", + Detail: err.Error(), + Subject: attr.Expr.Range().Ptr(), + }) + } else { + rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) + } + } + + ret.RequiredProviders[rp.Name] = rp } return ret, diags diff --git a/configs/provider_validation.go b/configs/provider_validation.go new file mode 100644 index 0000000000..57ad0fcdf8 --- /dev/null +++ b/configs/provider_validation.go @@ -0,0 +1,243 @@ +package configs + +import ( + "fmt" + "strings" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/addrs" +) + +// validateProviderConfigs walks the full configuration tree from the root +// module outward, static validation rules to the various combinations of +// provider configuration, required_providers values, and module call providers +// mappings. +// +// To retain compatibility with previous terraform versions, empty "proxy +// provider blocks" are still allowed within modules, though they will +// generate warnings when the configuration is loaded. The new validation +// however will generate an error if a suitable provider configuration is not +// passed in through the module call. +// +// The call argument is the ModuleCall for the provided Config cfg. The +// noProviderConfig argument is passed down the call stack, indicating that the +// module call, or a parent module call, has used a feature that precludes +// providers from being configured at all within the module. +func validateProviderConfigs(call *ModuleCall, cfg *Config, noProviderConfig bool) (diags hcl.Diagnostics) { + for name, child := range cfg.Children { + mc := cfg.Module.ModuleCalls[name] + + // if the module call has any of count, for_each or depends_on, + // providers are prohibited from being configured in this module, or + // any module beneath this module. + nope := noProviderConfig || mc.Count != nil || mc.ForEach != nil || mc.DependsOn != nil + diags = append(diags, validateProviderConfigs(mc, child, nope)...) + } + + // nothing else to do in the root module + if call == nil { + return diags + } + + // the set of provider configuration names passed into the module, with the + // source range of the provider assignment in the module call. + passedIn := map[string]PassedProviderConfig{} + + // the set of empty configurations that could be proxy configurations, with + // the source range of the empty configuration block. + emptyConfigs := map[string]*hcl.Range{} + + // the set of provider with a defined configuration, with the source range + // of the configuration block declaration. + configured := map[string]*hcl.Range{} + + // the set of configuration_aliases defined in the required_providers + // block, with the fully qualified provider type. + configAliases := map[string]addrs.AbsProviderConfig{} + + // the set of provider names defined in the required_providers block, and + // their provider types. + localNames := map[string]addrs.AbsProviderConfig{} + + for _, passed := range call.Providers { + name := providerName(passed.InChild.Name, passed.InChild.Alias) + passedIn[name] = passed + } + + mod := cfg.Module + + for _, pc := range mod.ProviderConfigs { + name := providerName(pc.Name, pc.Alias) + // Validate the config against an empty schema to see if it's empty. + _, pcConfigDiags := pc.Config.Content(&hcl.BodySchema{}) + if pcConfigDiags.HasErrors() || pc.Version.Required != nil { + configured[name] = &pc.DeclRange + } else { + emptyConfigs[name] = &pc.DeclRange + } + } + + if mod.ProviderRequirements != nil { + for _, req := range mod.ProviderRequirements.RequiredProviders { + addr := addrs.AbsProviderConfig{ + Module: cfg.Path, + Provider: req.Type, + } + localNames[req.Name] = addr + for _, alias := range req.Aliases { + addr := addrs.AbsProviderConfig{ + Module: cfg.Path, + Provider: req.Type, + Alias: alias.Alias, + } + configAliases[providerName(alias.LocalName, alias.Alias)] = addr + } + } + } + + // there cannot be any configurations if no provider config is allowed + if len(configured) > 0 && noProviderConfig { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Module %s contains provider configuration", cfg.Path), + Detail: "Providers cannot be configured within modules using count, for_each or depends_on.", + }) + } + + // now check that the user is not attempting to override a config + for name := range configured { + if passed, ok := passedIn[name]; ok { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot override provider configuration", + Detail: fmt.Sprintf("Provider %s is configured within the module %s and cannot be overridden.", name, cfg.Path), + Subject: &passed.InChild.NameRange, + }) + } + } + + // A declared alias requires either a matching configuration within the + // module, or one must be passed in. + for name, providerAddr := range configAliases { + _, confOk := configured[name] + _, passedOk := passedIn[name] + + if confOk || passedOk { + continue + } + + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("No configuration for provider %s", name), + Detail: fmt.Sprintf("Configuration required for %s.", providerAddr), + Subject: &call.DeclRange, + }) + } + + // You cannot pass in a provider that cannot be used + for name, passed := range passedIn { + providerAddr := addrs.AbsProviderConfig{ + Module: cfg.Path, + Provider: addrs.NewDefaultProvider(passed.InChild.Name), + Alias: passed.InChild.Alias, + } + + localAddr, localName := localNames[name] + if localName { + providerAddr = localAddr + } + + aliasAddr, configAlias := configAliases[name] + if configAlias { + providerAddr = aliasAddr + } + + _, emptyConfig := emptyConfigs[name] + + if !(localName || configAlias || emptyConfig) { + severity := hcl.DiagError + + // we still allow default configs, so switch to a warning if the incoming provider is a default + if providerAddr.Provider.IsDefault() { + severity = hcl.DiagWarning + } + + diags = append(diags, &hcl.Diagnostic{ + Severity: severity, + Summary: fmt.Sprintf("Provider %s is undefined", name), + Detail: fmt.Sprintf("Module %s does not declare a provider named %s.\n", cfg.Path, name) + + fmt.Sprintf("If you wish to specify a provider configuration for the module, add an entry for %s in the required_providers block within the module.", name), + Subject: &passed.InChild.NameRange, + }) + } + + // The provider being passed in must also be of the correct type. + // While we would like to ensure required_providers exists here, + // implied default configuration is still allowed. + pTy := addrs.NewDefaultProvider(passed.InParent.Name) + + // use the full address for a nice diagnostic output + parentAddr := addrs.AbsProviderConfig{ + Module: cfg.Parent.Path, + Provider: pTy, + Alias: passed.InParent.Alias, + } + + if cfg.Parent.Module.ProviderRequirements != nil { + req, defined := cfg.Parent.Module.ProviderRequirements.RequiredProviders[name] + if defined { + parentAddr.Provider = req.Type + } + } + + if !providerAddr.Provider.Equals(parentAddr.Provider) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Invalid type for provider %s", providerAddr), + Detail: fmt.Sprintf("Cannot use configuration from %s for %s. ", parentAddr, providerAddr) + + "The given provider configuration is for a different provider type.", + Subject: &passed.InChild.NameRange, + }) + } + } + + // Empty configurations are no longer needed + for name, src := range emptyConfigs { + detail := fmt.Sprintf("Remove the %s provider block from %s.", name, cfg.Path) + + isAlias := strings.Contains(name, ".") + _, isConfigAlias := configAliases[name] + _, isLocalName := localNames[name] + + if isAlias && !isConfigAlias { + localName := strings.Split(name, ".")[0] + detail = fmt.Sprintf("Remove the %s provider block from %s. Add %s to the list of configuration_aliases for %s in required_providers to define the provider configuration name.", name, cfg.Path, name, localName) + } + + if !isAlias && !isLocalName { + // if there is no local name, add a note to include it in the + // required_provider block + detail += fmt.Sprintf("\nTo ensure the correct provider configuration is used, add %s to the required_providers configuration", name) + } + + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Empty provider configuration blocks are not required", + Detail: detail, + Subject: src, + }) + } + + if diags.HasErrors() { + return diags + } + + return diags +} + +func providerName(name, alias string) string { + if alias != "" { + name = name + "." + alias + } + return name +} diff --git a/configs/testdata/config-diagnostics/empty-configs/main.tf b/configs/testdata/config-diagnostics/empty-configs/main.tf new file mode 100644 index 0000000000..c0edba2753 --- /dev/null +++ b/configs/testdata/config-diagnostics/empty-configs/main.tf @@ -0,0 +1,20 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + } + baz = { + source = "hashicorp/baz" + } + } +} + +module "mod" { + source = "./mod" + providers = { + foo = foo + foo.bar = foo + baz = baz + baz.bing = baz + } +} diff --git a/configs/testdata/config-diagnostics/empty-configs/mod/main.tf b/configs/testdata/config-diagnostics/empty-configs/mod/main.tf new file mode 100644 index 0000000000..50995ca0bd --- /dev/null +++ b/configs/testdata/config-diagnostics/empty-configs/mod/main.tf @@ -0,0 +1,22 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + configuration_aliases = [ foo.bar ] + } + } +} + +provider "foo" { +} + +provider "foo" { + alias = "bar" +} + +provider "baz" { +} + +provider "baz" { + alias = "bing" +} diff --git a/configs/testdata/config-diagnostics/empty-configs/warnings b/configs/testdata/config-diagnostics/empty-configs/warnings new file mode 100644 index 0000000000..dcf6736e93 --- /dev/null +++ b/configs/testdata/config-diagnostics/empty-configs/warnings @@ -0,0 +1,4 @@ +empty-configs/mod/main.tf:10,1-15: Empty provider configuration blocks are not required; Remove the foo provider block from module.mod +empty-configs/mod/main.tf:13,1-15: Empty provider configuration blocks are not required; Remove the foo.bar provider block from module.mod +empty-configs/mod/main.tf:17,1-15: Empty provider configuration blocks are not required; Remove the baz provider block from module.mod.\nTo ensure the correct provider configuration is used, add baz to the required_providers configuration +empty-configs/mod/main.tf:20,1-15: Empty provider configuration blocks are not required; Remove the baz.bing provider block from module.mod. Add baz.bing to the list of configuration_aliases for baz in required_providers to define the provider configuration name diff --git a/configs/testdata/config-diagnostics/incorrect-type/errors b/configs/testdata/config-diagnostics/incorrect-type/errors new file mode 100644 index 0000000000..28b2108506 --- /dev/null +++ b/configs/testdata/config-diagnostics/incorrect-type/errors @@ -0,0 +1 @@ +incorrect-type/main.tf:15,5-8: Invalid type for provider module.mod.provider["example.com/vendor/foo"]; Cannot use configuration from provider["registry.terraform.io/hashicorp/foo"] for module.mod.provider["example.com/vendor/foo"] diff --git a/configs/testdata/config-diagnostics/incorrect-type/main.tf b/configs/testdata/config-diagnostics/incorrect-type/main.tf new file mode 100644 index 0000000000..074cc8422d --- /dev/null +++ b/configs/testdata/config-diagnostics/incorrect-type/main.tf @@ -0,0 +1,18 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + } + baz = { + source = "hashicorp/baz" + } + } +} + +module "mod" { + source = "./mod" + providers = { + foo = foo + baz = baz + } +} diff --git a/configs/testdata/config-diagnostics/incorrect-type/mod/main.tf b/configs/testdata/config-diagnostics/incorrect-type/mod/main.tf new file mode 100644 index 0000000000..14c3239e92 --- /dev/null +++ b/configs/testdata/config-diagnostics/incorrect-type/mod/main.tf @@ -0,0 +1,14 @@ +terraform { + required_providers { + foo = { + source = "example.com/vendor/foo" + } + } +} + +resource "foo_resource" "a" { +} + +// implied default provider baz +resource "baz_resource" "a" { +} diff --git a/configs/testdata/config-diagnostics/incorrect-type/warnings b/configs/testdata/config-diagnostics/incorrect-type/warnings new file mode 100644 index 0000000000..a87f1f7421 --- /dev/null +++ b/configs/testdata/config-diagnostics/incorrect-type/warnings @@ -0,0 +1 @@ +incorrect-type/main.tf:16,5-8: Provider baz is undefined; Module module.mod does not declare a provider named baz.\nIf you wish to specify a provider configuration for the module diff --git a/configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf b/configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf new file mode 100644 index 0000000000..f2695a6611 --- /dev/null +++ b/configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf @@ -0,0 +1,7 @@ +provider "aws" { + value = "foo" +} + +output "my_output" { + value = "my output" +} diff --git a/configs/testdata/config-diagnostics/nested-provider/child/main.tf b/configs/testdata/config-diagnostics/nested-provider/child/main.tf new file mode 100644 index 0000000000..9a725a5209 --- /dev/null +++ b/configs/testdata/config-diagnostics/nested-provider/child/main.tf @@ -0,0 +1,4 @@ +module "child2" { + // the test fixture treats these sources as relative to the root + source = "./child/child2" +} diff --git a/configs/testdata/config-diagnostics/nested-provider/errors b/configs/testdata/config-diagnostics/nested-provider/errors new file mode 100644 index 0000000000..8f44cac78f --- /dev/null +++ b/configs/testdata/config-diagnostics/nested-provider/errors @@ -0,0 +1,3 @@ +Module module.child.module.child2 contains provider configuration; Providers cannot be configured within modules using count, for_each or depends_on + + diff --git a/configs/testdata/config-diagnostics/nested-provider/root.tf b/configs/testdata/config-diagnostics/nested-provider/root.tf new file mode 100644 index 0000000000..71b90f6d67 --- /dev/null +++ b/configs/testdata/config-diagnostics/nested-provider/root.tf @@ -0,0 +1,4 @@ +module "child" { + count = 1 + source = "./child" +} diff --git a/configs/testdata/config-diagnostics/override-provider/errors b/configs/testdata/config-diagnostics/override-provider/errors new file mode 100644 index 0000000000..a8d59d6e56 --- /dev/null +++ b/configs/testdata/config-diagnostics/override-provider/errors @@ -0,0 +1 @@ +override-provider/main.tf:17,5-8: Cannot override provider configuration; Provider bar is configured within the module module.mod and cannot be overridden. diff --git a/configs/testdata/config-diagnostics/override-provider/main.tf b/configs/testdata/config-diagnostics/override-provider/main.tf new file mode 100644 index 0000000000..30feec1c97 --- /dev/null +++ b/configs/testdata/config-diagnostics/override-provider/main.tf @@ -0,0 +1,19 @@ +terraform { + required_providers { + bar = { + version = "~>1.0.0" + } + } +} + +provider "bar" { + value = "not ok" +} + +// this module configures its own provider, which cannot be overridden +module "mod" { + source = "./mod" + providers = { + bar = bar + } +} diff --git a/configs/testdata/config-diagnostics/override-provider/mod/main.tf b/configs/testdata/config-diagnostics/override-provider/mod/main.tf new file mode 100644 index 0000000000..c0b6169710 --- /dev/null +++ b/configs/testdata/config-diagnostics/override-provider/mod/main.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + bar = { + version = "~>1.0.0" + } + } +} + +// this configuration cannot be overridden from an outside module +provider "bar" { + value = "ok" +} diff --git a/configs/testdata/config-diagnostics/required-alias/errors b/configs/testdata/config-diagnostics/required-alias/errors new file mode 100644 index 0000000000..a1b944a43b --- /dev/null +++ b/configs/testdata/config-diagnostics/required-alias/errors @@ -0,0 +1 @@ +required-alias/main.tf:1,1-13: No configuration for provider foo.bar; Configuration required for module.mod.provider["registry.terraform.io/hashicorp/foo"].bar diff --git a/configs/testdata/config-diagnostics/required-alias/main.tf b/configs/testdata/config-diagnostics/required-alias/main.tf new file mode 100644 index 0000000000..c2cfe60b87 --- /dev/null +++ b/configs/testdata/config-diagnostics/required-alias/main.tf @@ -0,0 +1,4 @@ +module "mod" { + source = "./mod" + // missing providers with foo.bar provider config +} diff --git a/configs/testdata/config-diagnostics/required-alias/mod/main.tf b/configs/testdata/config-diagnostics/required-alias/mod/main.tf new file mode 100644 index 0000000000..0f2a52168c --- /dev/null +++ b/configs/testdata/config-diagnostics/required-alias/mod/main.tf @@ -0,0 +1,13 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + version = "1.0.0" + configuration_aliases = [ foo.bar ] + } + } +} + +resource "foo_resource" "a" { + provider = foo.bar +} diff --git a/configs/testdata/config-diagnostics/unexpected-provider/main.tf b/configs/testdata/config-diagnostics/unexpected-provider/main.tf new file mode 100644 index 0000000000..cd859a7268 --- /dev/null +++ b/configs/testdata/config-diagnostics/unexpected-provider/main.tf @@ -0,0 +1,15 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + version = "1.0.0" + } + } +} + +module "mod" { + source = "./mod" + providers = { + foo = foo + } +} diff --git a/configs/testdata/config-diagnostics/unexpected-provider/mod/main.tf b/configs/testdata/config-diagnostics/unexpected-provider/mod/main.tf new file mode 100644 index 0000000000..f69bfa813b --- /dev/null +++ b/configs/testdata/config-diagnostics/unexpected-provider/mod/main.tf @@ -0,0 +1,2 @@ +resource "foo_resource" "a" { +} diff --git a/configs/testdata/config-diagnostics/unexpected-provider/warnings b/configs/testdata/config-diagnostics/unexpected-provider/warnings new file mode 100644 index 0000000000..0e41b39a94 --- /dev/null +++ b/configs/testdata/config-diagnostics/unexpected-provider/warnings @@ -0,0 +1,2 @@ +unexpected-provider/main.tf:13,5-8: Provider foo is undefined; Module module.mod does not declare a provider named foo. + diff --git a/configs/testdata/config-diagnostics/with-depends-on/main.tf b/configs/testdata/config-diagnostics/with-depends-on/main.tf new file mode 100644 index 0000000000..49c2dcd6e7 --- /dev/null +++ b/configs/testdata/config-diagnostics/with-depends-on/main.tf @@ -0,0 +1,14 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + } + } +} + +module "mod2" { + source = "./mod1" + providers = { + foo = foo + } +} diff --git a/configs/testdata/config-diagnostics/with-depends-on/mod1/main.tf b/configs/testdata/config-diagnostics/with-depends-on/mod1/main.tf new file mode 100644 index 0000000000..c318484b5b --- /dev/null +++ b/configs/testdata/config-diagnostics/with-depends-on/mod1/main.tf @@ -0,0 +1,19 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + } + } +} + +resource "foo_resource" "a" { +} + +module "mod2" { + depends_on = [foo_resource.a] + // test fixture source is from root + source = "./mod1/mod2" + providers = { + foo = foo + } +} diff --git a/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/main.tf b/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/main.tf new file mode 100644 index 0000000000..eaa3550bd7 --- /dev/null +++ b/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/main.tf @@ -0,0 +1,15 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + } + } +} + +module "mod3" { + // test fixture source is from root + source = "./mod1/mod2/mod3" + providers = { + foo.bar = foo + } +} diff --git a/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/mod3/main.tf b/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/mod3/main.tf new file mode 100644 index 0000000000..b1827126d5 --- /dev/null +++ b/configs/testdata/config-diagnostics/with-depends-on/mod1/mod2/mod3/main.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + foo = { + source = "hashicorp/foo" + configuration_aliases = [ foo.bar ] + } + } +} + +resource "foo_resource" "a" { + providers = foo.bar +} From 9b11ff03587053b70875e0d4944949ebfaeb6006 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Feb 2021 13:47:42 -0500 Subject: [PATCH 5/7] remove outdated tests These cases are now caught early in the configuration loading process, and do not make it to the point of graph transformation. --- .../main.tf | 10 --- .../mod/main.tf | 2 - .../transform-provider-invalid/main.tf | 11 ---- .../transform-provider-invalid/mod/main.tf | 2 - terraform/transform_provider_test.go | 63 ------------------- 5 files changed, 88 deletions(-) delete mode 100644 terraform/testdata/transform-provider-implicit-module/main.tf delete mode 100644 terraform/testdata/transform-provider-implicit-module/mod/main.tf delete mode 100644 terraform/testdata/transform-provider-invalid/main.tf delete mode 100644 terraform/testdata/transform-provider-invalid/mod/main.tf diff --git a/terraform/testdata/transform-provider-implicit-module/main.tf b/terraform/testdata/transform-provider-implicit-module/main.tf deleted file mode 100644 index 141a04d3f9..0000000000 --- a/terraform/testdata/transform-provider-implicit-module/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -provider "aws" { - alias = "foo" -} - -module "mod" { - source = "./mod" - providers = { - aws = aws.foo - } -} diff --git a/terraform/testdata/transform-provider-implicit-module/mod/main.tf b/terraform/testdata/transform-provider-implicit-module/mod/main.tf deleted file mode 100644 index 01cf0803c0..0000000000 --- a/terraform/testdata/transform-provider-implicit-module/mod/main.tf +++ /dev/null @@ -1,2 +0,0 @@ -resource "aws_instance" "bar" { -} diff --git a/terraform/testdata/transform-provider-invalid/main.tf b/terraform/testdata/transform-provider-invalid/main.tf deleted file mode 100644 index ec23232ae6..0000000000 --- a/terraform/testdata/transform-provider-invalid/main.tf +++ /dev/null @@ -1,11 +0,0 @@ -provider "aws" { -} - -module "mod" { - source = "./mod" - - # aws.foo doesn't exist, and should report an error - providers = { - aws = aws.foo - } -} diff --git a/terraform/testdata/transform-provider-invalid/mod/main.tf b/terraform/testdata/transform-provider-invalid/mod/main.tf deleted file mode 100644 index 03641197f0..0000000000 --- a/terraform/testdata/transform-provider-invalid/mod/main.tf +++ /dev/null @@ -1,2 +0,0 @@ -resource "aws_resource" "foo" { -} diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 5b05c7a72e..fbc9a41872 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -434,69 +434,6 @@ func TestProviderConfigTransformer_grandparentProviders(t *testing.T) { } } -// pass a specific provider into a module using it implicitly -func TestProviderConfigTransformer_implicitModule(t *testing.T) { - mod := testModule(t, "transform-provider-implicit-module") - concrete := func(a *NodeAbstractProvider) dag.Vertex { return a } - - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - { - tf := &AttachResourceConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - { - tf := TransformProviders([]string{"aws"}, concrete, mod) - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(`module.mod.aws_instance.bar - provider["registry.terraform.io/hashicorp/aws"].foo -provider["registry.terraform.io/hashicorp/aws"].foo`) - if actual != expected { - t.Fatalf("wrong result\n\nexpected:\n%s\n\ngot:\n%s", expected, actual) - } -} - -// error out when a non-existent provider is named in a module providers map -func TestProviderConfigTransformer_invalidProvider(t *testing.T) { - mod := testModule(t, "transform-provider-invalid") - concrete := func(a *NodeAbstractProvider) dag.Vertex { return a } - - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - { - tf := &AttachResourceConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - tf := TransformProviders([]string{"aws"}, concrete, mod) - err := tf.Transform(&g) - if err == nil { - t.Fatal("expected missing provider error") - } - if !strings.Contains(err.Error(), `provider["registry.terraform.io/hashicorp/aws"].foo`) { - t.Fatalf("error should reference missing provider, got: %s", err) - } -} - const testTransformProviderBasicStr = ` aws_instance.web provider["registry.terraform.io/hashicorp/aws"] From 00730aed0b2a16743660d8481e9ab64e3b2c36b3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Feb 2021 14:05:29 -0500 Subject: [PATCH 6/7] basic configuration_aliases support in core --- terraform/context_plan2_test.go | 58 +++++++++++++++++++++++++++++++++ terraform/transform_provider.go | 37 +++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go index 9ea5d66c8a..1e93e35fad 100644 --- a/terraform/context_plan2_test.go +++ b/terraform/context_plan2_test.go @@ -1,6 +1,7 @@ package terraform import ( + "errors" "testing" "github.com/hashicorp/terraform/addrs" @@ -191,3 +192,60 @@ output "out" { t.Fatalf("expected %#v, got %#v\n", expected, change.After) } } + +func TestContext2Plan_basicConfigurationAliases(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +provider "test" { + alias = "z" + test_string = "config" +} + +module "mod" { + source = "./mod" + providers = { + test.x = test.z + } +} +`, + + "mod/main.tf": ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + configuration_aliases = [ test.x ] + } + } +} + +resource "test_object" "a" { + provider = test.x +} + +`, + }) + + p := simpleMockProvider() + + // The resource within the module should be using the provider configured + // from the root module. We should never see an empty configuration. + p.ConfigureFn = func(req providers.ConfigureRequest) (resp providers.ConfigureResponse) { + if req.Config.GetAttr("test_string").IsNull() { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing test_string value")) + } + return resp + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } +} diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index fec371cfa4..f8f390b0ab 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -605,6 +605,43 @@ func (t *ProviderConfigTransformer) transformSingle(g *Graph, c *configs.Config) t.proxiable[key] = !diags.HasErrors() } + if mod.ProviderRequirements != nil { + // Add implied provider configs from the required_providers + // Since we're still treating empty configs as proxies, we can just add + // these as empty configs too. We'll ensure that these are given a + // configuration during validation to prevent them from becoming + // fully-fledged config instances. + for _, p := range mod.ProviderRequirements.RequiredProviders { + for _, aliasAddr := range p.Aliases { + addr := addrs.AbsProviderConfig{ + Provider: mod.ProviderForLocalConfig(aliasAddr), + Module: path, + Alias: aliasAddr.Alias, + } + + key := addr.String() + if _, ok := t.providers[key]; ok { + continue + } + + abstract := &NodeAbstractProvider{ + Addr: addr, + } + var v dag.Vertex + if t.Concrete != nil { + v = t.Concrete(abstract) + } else { + v = abstract + } + + // Add it to the graph + g.Add(v) + t.providers[key] = v.(GraphNodeProvider) + t.proxiable[key] = true + } + } + } + // Now replace the provider nodes with proxy nodes if a provider was being // passed in, and create implicit proxies if there was no config. Any extra // proxies will be removed in the prune step. From 41955319258b6f82c02dc0fea030c1aed168d9ae Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 11 Feb 2021 17:39:51 -0500 Subject: [PATCH 7/7] old comment --- configs/testdata/valid-modules/provider-aliases/main.tf | 2 -- 1 file changed, 2 deletions(-) diff --git a/configs/testdata/valid-modules/provider-aliases/main.tf b/configs/testdata/valid-modules/provider-aliases/main.tf index b9795b30d2..dd9fb084d4 100644 --- a/configs/testdata/valid-modules/provider-aliases/main.tf +++ b/configs/testdata/valid-modules/provider-aliases/main.tf @@ -2,8 +2,6 @@ terraform { required_providers { foo-test = { source = "foo/test" - // TODO: these are strings until the parsing code is refactored to allow - // raw references configuration_aliases = [foo-test.a, foo-test.b] } }