Provider configuration_aliases and module validation #27739

jbardin · 2021-02-10T21:52:50Z

Here we have the initial implementation of configuration_aliases for providers within modules. The idea here is to replace the need for empty configuration blocks within modules as a "proxy" to receive provider configuration passed in from a parent module. So instead of writing

provider "foo" {
  alias = "bar"
}

the foo.bar name is declared along with the foo provider local name within required_providers.

terraform {
  required_providers {
    foo = {
      source = "hashicorp/foo"
      configuration_aliases = [ foo.bar ]
    }
  }
}

Adding the new field to the required_providers attributes required rewriting the parser for required_providers in order to allow the aliases to be bare references, matching their usage in other locations within configuration. The overall behavior of the parser should be the same, with the only noticeable changes in tests being some more precise diagnostic locations. The hcl and terraform-config-instpect packages were updated to work with the new config loader. We can update the hcl package to a tagged version before releasing 0.15.

All validation was moved out of the configload package, since that package should only be concerned with the loading of configuration, not the interpretation thereof. The output is slightly different from the previous validation, as instead of validating whether the modules are allowed to have provider configurations, we validate all the various combinations of provider structures themselves. Configurations which were working correctly previously should generate no more than diagnostic warnings in the new validation scheme. Configurations which were incorrect, or could produce unpredictable behavior now generate errors.

The most notable of these new warnings will be advice to remove empty configuration blocks from the module config. Each diagnostic produces a contextual message, with the intent of directing users to a supported configuration. For example, an empty configuration with an alias would present the warning:

│ Warning: Empty provider configuration blocks are not required
│
│   on mod/main.tf line 9:
│    9: provider "foo" {
│
│ Remove the foo.bar provider block from module.mod. Add foo.bar
| to the list of configuration_aliases for foo in required_providers to 
| define the provider configuration name.

Additional checks, like verifying the provider type have also been added. This prevents hard to decipher evaluation time errors like "unknown provider provider["example.com/vendor/name"].foo" when a correct provider config has not been supplied in a child module. Instead the user will receive a message during init like:

│ Error: Invalid type for provider module.mod.provider["registry.terraform.io/hashicorp/foo"].bar
│
│   on main.tf line 7, in module "mod":
│    7:     foo.bar = aws
│
│ Cannot use configuration from provider["registry.terraform.io/hashicorp/aws"] for
| module.mod.provider["registry.terraform.io/hashicorp/foo"].bar. The given
| provider configuration is for a different provider type.

Closes #27539
Closes #27409
Closes #26617
Closes #26448
Closes #26354

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.

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.

codecov · 2021-02-10T21:52:56Z

Codecov Report

Merging #27739 (e6d394a) into master (8057f19) will increase coverage by 0.03%.
The diff coverage is 76.73%.

Impacted Files	Coverage Δ
configs/configload/loader_load.go	`34.00% <ø> (-22.05%)`	⬇️
configs/provider_requirements.go	`58.74% <53.50%> (-21.48%)`	⬇️
terraform/transform_provider.go	`77.65% <77.77%> (-3.78%)`	⬇️
configs/config_build.go	`82.14% <100.00%> (+0.32%)`	⬆️
configs/provider_validation.go	`100.00% <100.00%> (ø)`
internal/providercache/dir.go	`67.34% <0.00%> (-6.13%)`	⬇️
dag/marshal.go	`61.90% <0.00%> (-1.59%)`	⬇️
dag/walk.go	`92.25% <0.00%> (+0.70%)`	⬆️
... and 1 more

mildwonkey

This all looks good, and I'm so glad to see the improved validation and errors/warnings!

I left a couple of comments; most of them aren't blocking so I'll approve this now and you can decide what to do (or not) about the notes I left.

mildwonkey · 2021-02-11T14:00:07Z

configs/provider_validation.go

+		// 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


lulz nope

mildwonkey · 2021-02-11T14:06:00Z

configs/provider_validation.go

+// 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) {


Excellent comments and warning/error messages in this entire function, thank you!

configs/provider_validation.go

configs/config_build_test.go

mildwonkey · 2021-02-11T14:16:55Z

configs/provider_requirements.go

@@ -26,132 +29,214 @@ type RequiredProviders struct {

 func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Diagnostics) {
 	attrs, diags := block.Body.JustAttributes()
+	if diags.HasErrors() {


i bet whoever forgot to check that is feeling pretty silly right about now 🤦

configs/provider_requirements.go

configs/provider_validation.go

mildwonkey · 2021-02-11T14:32:07Z

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) {


I presume these have been removed because it's caught during the earlier configload validate now?

Yes, these fail while loading the config, so there's no transformation to test.

mildwonkey · 2021-02-11T14:33:48Z

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 {


Is this covered in a test, and if not, can it be? (I know the answer to that isn't always straightforward).

Yes, there's a context test added which covers this, and I'll be adding more as I re-validate the overall behavior within the terraform package itself. This PR was getting big, and I wanted to get the parser and validation in ASAP.

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.

These cases are now caught early in the configuration loading process, and do not make it to the point of graph transformation.

alisdair

This seems great to me!

alisdair · 2021-02-11T17:32:47Z

configs/testdata/valid-modules/provider-aliases/main.tf

+      // TODO: these are strings until the parsing code is refactored to allow
+      // raw references
+      configuration_aliases = [foo-test.a, foo-test.b]


alisdair · 2021-02-11T17:39:15Z

configs/provider_validation.go

+
+		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)


I worry a bit that users might not find this easy to understand, given that we're introducing configuration_aliases in the same release we're adding these warnings.

Assuming that we're not going to provide an auto-upgrade command, could it make sense to make this warning even more explicit about what to do? Maybe with sample configuration?

Yes, I was thinking the same thing! I'm going to see how this gets formatted with config in the detail section. My hesitation was that I can't always generate the complete required_providers attribute, and it could cause more confusion than if they just look up the docs 🤔

ok, adding config examples is turning into a much larger task. I'll fix the dandling comment, and get this merged. Other improvements can come later.

pselle · 2021-02-11T22:52:09Z

A post-merge 🐘 nudge for user-facing documentation for this feature!

jbardin · 2021-02-11T22:56:31Z

@pselle, sorry, I should have mentioned it in the PR body, docs will be coming next in a separate PR!

ghost · 2021-03-14T01:50:07Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jbardin added 3 commits February 9, 2021 08:37

update hcl and terraform-config-inspect

a033598

configload should not be doing validation

7aaffac

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.

jbardin requested a review from a team February 10, 2021 21:52

mildwonkey approved these changes Feb 11, 2021

View reviewed changes

jbardin added 3 commits February 11, 2021 10:22

remove outdated tests

9b11ff0

These cases are now caught early in the configuration loading process, and do not make it to the point of graph transformation.

basic configuration_aliases support in core

00730ae

jbardin force-pushed the jbardin/provider-aliases branch from e6d394a to 00730ae Compare February 11, 2021 15:22

alisdair approved these changes Feb 11, 2021

View reviewed changes

old comment

4195531

jbardin merged commit 4e12ba3 into master Feb 11, 2021

jbardin deleted the jbardin/provider-aliases branch February 11, 2021 22:49

jbardin mentioned this pull request Feb 12, 2021

Log warning for unmatched configuration providers #27757

Closed

This was referenced Mar 10, 2021

Confirm/ensure support of Terraform 0.15 hashicorp/terraform-ls#425

Closed

Add schema for Terraform 0.15 hashicorp/terraform-schema#24

Merged

jbardin mentioned this pull request Mar 12, 2021

Terraform does not pass provider options (subscription_id at least) to a child module #28066

Closed

ghost locked as resolved and limited conversation to collaborators Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provider configuration_aliases and module validation #27739

Provider configuration_aliases and module validation #27739

jbardin commented Feb 10, 2021 •

edited

Loading

codecov bot commented Feb 10, 2021 •

edited

Loading

mildwonkey left a comment

mildwonkey Feb 11, 2021

mildwonkey Feb 11, 2021

mildwonkey Feb 11, 2021

mildwonkey Feb 11, 2021

jbardin Feb 11, 2021

mildwonkey Feb 11, 2021

jbardin Feb 11, 2021

alisdair left a comment

alisdair Feb 11, 2021

alisdair Feb 11, 2021

jbardin Feb 11, 2021

jbardin Feb 11, 2021

pselle commented Feb 11, 2021 •

edited

Loading

jbardin commented Feb 11, 2021

ghost commented Mar 14, 2021

Provider configuration_aliases and module validation #27739

Provider configuration_aliases and module validation #27739

Conversation

jbardin commented Feb 10, 2021 • edited Loading

codecov bot commented Feb 10, 2021 • edited Loading

Codecov Report

mildwonkey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisdair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pselle commented Feb 11, 2021 • edited Loading

jbardin commented Feb 11, 2021

ghost commented Mar 14, 2021

jbardin commented Feb 10, 2021 •

edited

Loading

codecov bot commented Feb 10, 2021 •

edited

Loading

pselle commented Feb 11, 2021 •

edited

Loading