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/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/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/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 diff --git a/configs/provider_requirements.go b/configs/provider_requirements.go index ef746f3faa..f870e1cc96 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,202 @@ 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") { + 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(), + }) + continue + } + + rp.Requirement = vc + rp.Type = addrs.ImpliedProviderForUnqualifiedType(pType) + ret.RequiredProviders[name] = rp + + continue + } + + // 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 := expr.GetAttr("version") - if !constraint.Type().Equals(cty.String) || constraint.IsNull() { + + 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: attr.Expr.Range().Ptr(), + Subject: kv.Value.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 - } + continue } - } - if expr.Type().HasAttribute("source") { - source := expr.GetAttr("source") - if !source.Type().Equals(cty.String) || source.IsNull() { + + 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: attr.Expr.Range().Ptr(), + Subject: kv.Value.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 } + + 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" and "source" attributes. To configure a provider, use a "provider" block.`, - Subject: attr.Expr.Range().Ptr(), + 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 } - 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 + 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{ 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/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/configload/testdata/expand-modules/valid/child/main.tf b/configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf similarity index 80% rename from configs/configload/testdata/expand-modules/valid/child/main.tf rename to configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf index a5c3c47b17..f2695a6611 100644 --- a/configs/configload/testdata/expand-modules/valid/child/main.tf +++ b/configs/testdata/config-diagnostics/nested-provider/child/child2/main.tf @@ -1,7 +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/configload/testdata/expand-modules/more-nested-provider/root.tf b/configs/testdata/config-diagnostics/nested-provider/root.tf similarity index 100% rename from configs/configload/testdata/expand-modules/more-nested-provider/root.tf rename to configs/testdata/config-diagnostics/nested-provider/root.tf 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 +} 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..dd9fb084d4 --- /dev/null +++ b/configs/testdata/valid-modules/provider-aliases/main.tf @@ -0,0 +1,17 @@ +terraform { + required_providers { + foo-test = { + source = "foo/test" + configuration_aliases = [foo-test.a, foo-test.b] + } + } +} + +resource "test_instance" "explicit" { + provider = foo-test.a +} + +data "test_resource" "explicit" { + provider = foo-test.b +} + 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= 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/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.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. 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"]