validate: ignore providers with no configuration #24896

bendrucker · 2020-05-08T05:23:23Z

This PR changes the behavior of terraform validate so that provider schema validation is bypassed if the user does not supply provider configuration. The strategy is pretty simple: if the provider config validates against any empty schema, consider it valid. In other words, if the configuration is completely empty, skip validation against the provider schema. Otherwise, proceed to fetch the provider schema, build the configuration against it, and report errors as usual.

I could particularly use guidance on any side effects this might have, particularly if validation is used ahead of other steps that might depend on current behavior.

What follows is a bit of background on the different variations on provider configuration I'm expecting.

The below provider configurations, many of which @apparentlymart described in #21408 (comment), can become valid at runtime but will currently raise errors during a terraform validate.

Child Module

When authoring a child module, it's useful to terraform validate, particularly because it interacts with provider plugins. It's Terraform's "compile" step—we might still have runtime errors, but we're using types correctly.

Inherited providers

No provider declared, the default instance of this provider will be inherited from the parent

resource "aws_instance" "foo" {
  instance_type = "t3.nano"
  ami = "abc-123"
}

Proxy provider blocks

provider "aws" {
  alias = "foo"
}

resource "aws_instance" "foo" {
  provider = aws.foo
  instance_type = "t3.nano"
  ami = "abc-123"
}

foo is declared but not configured, and we expect the caller to pass providers = { aws.foo = ... }.

Fully configured provider blocks

resource "aws_organizations_account" "account" {
  name = "my_new_account"
  email = "john@doe.org"
  role_name = "OrganizationAccountAccessRole"
}

provider "aws" {
  alias = "account"

  assume_role {
    role_arn = "arn:aws:iam:${aws_organizations_account.account.id}:role/${aws_organizations_account.account.role_name}"
  }
}

resource "aws_instance" "foo" {
  provider = aws.foo
  instance_type = "t3.nano"
  ami = "abc-123"
}

The module inherits a provider in order to dynamically configure another provider (here in a new AWS account). Or perhaps the module just accepts variables and then configures the provider, though this should be uncommon.

Unnecessarily declared provider blocks

I included this as an edit, because my change will end up handling them.

provider "aws" {
  version = "~> 2"
}

resource "aws_instance" "foo" {
  instance_type = "t3.nano"
  ami = "abc-123"
}

In a child module, this is unnecessary and will lead to weird bugs when the user inevitably doesn't set providers = { aws = aws } once and the child provider has none of the configuration the module is supposed to inherit.

Terraform could warn on version-only provider blocks in child modules, probably even in validate, I'd be up for working on that in a separate PR.

Root Module

All the same uses of terraform validate apply to root modules. The provider configuration is more likely to be populated, but isn't necessarily so.

Implicit provider

resource "aws_instance" "foo" {
  instance_type = "t3.nano"
  ami = "abc-123"
}

The configuration doesn't declare a provider.

Declared empty provider

provider "aws" {}

resource "aws_instance" "foo" {
  instance_type = "t3.nano"
  ami = "abc-123"
}

The provider is declared but has no configuration. This configuration is valid at runtime if environment variables satisfy the required provider configuration. And I believe it is also required to enable provider passing to a module, e.g. providers = { aws.foo = aws } as in the case 2 for child modules.

My experience as a Terraform user has been that writing provider configuration is a rarity and I'm able to rely on environment variables almost exclusively. This has translated nicely to Terraform Cloud (Enterprise in my case) where we've wired up the tfe_workspace and tfe_variable resources in a module that distributes credentials for ~10 providers to the workspaces that need them such that the Terraform configuration repos/directories have zero provider configuration required.

Configured providers

provider "aws" {
  region = "us-west-2"
}

resource "aws_instance" "foo" {
  instance_type = "t3.nano"
  ami = "abc-123"
}

The provider supplies at least partial configuration. At this point I think we have to validate what we're given. If provider authors want to have required non-credential fields, they'll need to make the credentials fields optional, and validate in the ConfigureFunc. And even then, there are good reasons to make credentials optional (https://github.com/terraform-providers/terraform-provider-datadog/pull/474).

Even the somewhat odd required features {} block in the Azure provider would seem to be compatible with this stance:

https://www.terraform.io/docs/providers/azurerm/index.html#example-usage

This may not entirely satisfy users who'd like all required fields to be optional, but I don't see how we can accomplish that without throwing away legitimate errors. Azure is a good example there, features {} is required, it would be wrong for terraform validate to preempt or discard an error. Required fields are often credentials which probably use schema.EnvDefaultFunc, but there's no simple/safe heuristic Terraform could use to ignore those errors.

Closes #21408

codecov · 2020-05-08T05:40:57Z

Codecov Report

Merging #24896 (a39273c) into master (9ac8e3c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files	Coverage Δ
terraform/node_provider.go	`88.63% <100.00%> (-0.62%)`	⬇️
dag/marshal.go	`61.90% <0.00%> (-1.59%)`	⬇️

jsf9k · 2020-05-13T20:07:39Z

I had to turn take the terraform validate guardrails off my CI configuration because of this very bug. Very glad to see that it is getting fixed! Thanks @bendrucker!

bendrucker · 2020-05-20T23:52:38Z

Willing and eager to help this land as part of 0.13 if I can!

pselle · 2020-11-05T16:22:42Z

@bendrucker Thank you for this PR, and apologies for the wait! If you could update this PR on top of the latest main branch, I'll review!

We are currently in the 0.14 release cycle, so this would be going into 0.15, for transparency (the main branch is targeting 0.15)

bendrucker · 2020-11-05T19:04:11Z

Hi! Happy to update, thrilled to hear this might make 0.15.

…ty-provider # Conflicts: # terraform/eval_validate.go

bendrucker · 2020-11-10T00:40:03Z

Hey @pselle, merged with master! Relocated this logic to its new location, but the change itself remains the same.

To my note above:

I could particularly use guidance on any side effects this might have, particularly if validation is used ahead of other steps that might depend on current behavior.

I noticed that ConfigureProvider makes no assumptions that the configuration has already been validated, since it implicitly re-validates as part of evaluating the config:

https://github.com/hashicorp/terraform/blob/master/terraform/node_provider.go#L95-L100

So this wouldn't have a side effect of causing a ConfigureFunc to panic because a required attribute is missing. Or in other words, it should only affect the validate command.

backend/local/backend_refresh_test.go

pselle · 2020-11-17T19:09:43Z

terraform/context_validate_test.go

@@ -595,7 +595,7 @@ func TestContext2Validate_providerConfig_bad(t *testing.T) {
 	}
 }

-func TestContext2Validate_providerConfig_badEmpty(t *testing.T) {
+func TestContext2Validate_providerConfig_skippedEmpty(t *testing.T) {
 	m := testModule(t, "validate-bad-pc-empty")


To match things up, it could be nice to rename this file to match "skipped" versus "bad"

pselle · 2020-11-17T19:12:35Z

terraform/node_provider.go

@@ -48,6 +48,13 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi

 	configBody := buildProviderConfig(ctx, n.Addr, n.ProviderConfig())

+	// if provider config is empty, return early


More explanation here in the comment would be nice -- why are we returning early? (explain this)

Suggestion, mashed up from the suggestions that led to this PR:

// if a provider config is empty (only an alias), return early and don't continue // validation. validate doesn't need to fully configure the provider itself, so // skipping a provider with an implied configuration won't prevent other validation from completing.

pselle · 2020-11-17T19:15:16Z

terraform/node_provider.go

@@ -48,6 +48,13 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi

 	configBody := buildProviderConfig(ctx, n.Addr, n.ProviderConfig())

+	// if provider config is empty, return early
+	emptySchema := &configschema.Block{}
+	_, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey)


Silly Terraform tricks: another way to do this (the approach here is totally valid!)

_, noConfigDiags := configBody.Content(&hcl.BodySchema{}) if !noConfigDiags.HasErrors() { return nil }

This is more to share, and offer if you like it, but your way works absolutely fine.

pselle · 2020-11-17T19:17:49Z

I noticed that ConfigureProvider makes no assumptions that the configuration has already been validated, since it implicitly re-validates as part of evaluating the config:

The section you've highlighted shows that there's no check to ensure that resp.Provider.Block isn't nil. The validation request still happens here: https://github.com/hashicorp/terraform/blob/master/terraform/node_provider.go#L122-L128 within the ConfigureProvider method.

To say, I've read through to make sure the validation is still happening -- the thing that is not happening is the nil check, which is in the ValidateProvider function with a note that "it should never happen in real code" so I'm inclined, if we didn't run into any issues in the tests, to add that nil check when it's warranted by a real use case.

With that, this is looking merge-able to me, provided you rebase the PR (sorry about that 😅 ).

bendrucker · 2020-11-17T19:20:10Z

Appreciate all the notes! Will address everything and fix the conflict this week.

…ty-provider

bendrucker · 2020-12-06T18:06:07Z

Ok, took me a bit longer than expected to get back to this, but should be ready! All comments addressed, and the conflict is resolved.

…ty-provider

bendrucker · 2020-12-14T22:49:34Z

There was a meaningful conflict with #27261, which added improved error handling when a provider block is not defined. I've run into that before, particularly in Terraform Cloud, so I was glad to see that improvement.

In order to resolve the conflict, I made the following changes here:

Removed the conditional sourceless diagnostic from ValidateProvider, since the earlier check against an empty schema added here makes that an unreachable branch
Modified the validate tests so that rather than a missing required attribute there is a type mismatch
Added test with a missing required attribute which should not result in errors

ConfigureProvider still retains all the new behavior.

pselle

Thanks for the updates and rebases (and patience!!) here @bendrucker! Happy to get this merged now!

bendrucker · 2021-01-07T23:12:55Z

Awesome, thrilled to see it land!

drdamour · 2021-01-08T04:00:39Z

@bendrucker so what does the azurerm provider need to do so that it can take advantage of this? i didn't understand your early explanation unfortunately

bendrucker · 2021-01-08T15:16:58Z

Providers don't necessarily need to do anything. Sorry but I'm not going to provide support for your setup, please don't snipe pull requests like this.

drdamour · 2021-01-08T16:24:31Z

@bendrucker not sure what you mean by snipe, this pr closed an issue the azure rm folks had pointed us to a few times regarding validate requiring the functions block. Sorry my ignorance offended you some way

drdamour · 2021-01-08T16:27:49Z

Cf hashicorp/terraform-provider-azurerm#7359

pselle · 2021-01-08T17:12:21Z

@drdamour To clarify "when can I use this" from the Core side, you'll see this behavior in the 0.15 version of Terraform. This code will go out in the next alpha release, and it would be very helpful for you to test this with the AzureRM intersection when that's available (about over two and a half weeks from now is when we expect to ship the next alpha). You won't need to upgrade the provider/change the provider to get access to this behavior.

drdamour · 2021-01-08T18:04:27Z

@pselle my question was more around your original comments which seemed to indicate further steps are needed

“ If provider authors want to have required non-credential fields, they'll need to make the credentials fields optional, and validate in the ConfigureFunc. And even then, there are good reasons to make credentials optional (https://github.com/terraform-providers/terraform-provider-datadog/pull/474).

Even the somewhat odd required features {} block in the Azure provider would seem to be compatible with this stance”

That seemed like a call to action for the azurerm provider

pselle · 2021-01-08T19:43:30Z

@drdamour Yes, I indeed misinterpreted. Not being familiar with the AzureRM provider, I'd recommend raising this issue with them, but I'd really recommend only doing so after you've tested with the next alpha to see if it's relevant.

ghost · 2021-02-07T01:50:10Z

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.

bendrucker added 5 commits May 7, 2020 20:26

validate: ignore providers with no configuration

16a08c6

update context tests to expect provider validation to be skipped

07dbe7b

rename test

d794247

remove assertion that PrepareProviderConfig was called

77f082b

reuse emptySchema, set directly to Block

1fda88e

bendrucker mentioned this pull request May 8, 2020

"terraform validate" should not fail on implicit provider config when provider arguments are required #21408

Closed

mcdonnnj mentioned this pull request May 13, 2020

Update pre-commit Hook Versions cisagov/skeleton-generic#44

Merged

9 tasks

tombuildsstuff mentioned this pull request Jun 24, 2020

Mandatory "features" block is breaking best-practice usage of "terraform validate" hashicorp/terraform-provider-azurerm#7359

Closed

pselle self-assigned this Nov 5, 2020

bendrucker added 3 commits November 9, 2020 16:24

Merge remote-tracking branch 'origin/master' into validate-ignore-emp…

afe8b67

…ty-provider # Conflicts: # terraform/eval_validate.go

fix error output

2244053

move empty provider logic to node_provider

cd7d789

pselle self-requested a review November 12, 2020 22:08

pselle reviewed Nov 17, 2020

View reviewed changes

bendrucker added 5 commits December 6, 2020 09:35

rename empty provider config test

eacf8b5

expand early return comment

186eff9

call configBody.Content

a2c3108

Merge remote-tracking branch 'origin/master' into validate-ignore-emp…

7e11b97

…ty-provider

add backend refresh test with provider config

2549e53

Merge remote-tracking branch 'origin/master' into validate-ignore-emp…

a39273c

…ty-provider

bendrucker requested a review from pselle December 14, 2020 22:49

pselle approved these changes Jan 7, 2021

View reviewed changes

pselle merged commit c9f372a into hashicorp:master Jan 7, 2021

bendrucker deleted the validate-ignore-empty-provider branch January 7, 2021 23:18

ghost locked as resolved and limited conversation to collaborators Feb 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate: ignore providers with no configuration #24896

validate: ignore providers with no configuration #24896

bendrucker commented May 8, 2020 •

edited

Loading

codecov bot commented May 8, 2020 •

edited

Loading

jsf9k commented May 13, 2020

bendrucker commented May 20, 2020

pselle commented Nov 5, 2020

bendrucker commented Nov 5, 2020

bendrucker commented Nov 10, 2020

pselle Nov 17, 2020

pselle Nov 17, 2020

pselle Nov 17, 2020

pselle commented Nov 17, 2020 •

edited

Loading

bendrucker commented Nov 17, 2020

bendrucker commented Dec 6, 2020

bendrucker commented Dec 14, 2020

pselle left a comment

bendrucker commented Jan 7, 2021

drdamour commented Jan 8, 2021

bendrucker commented Jan 8, 2021

drdamour commented Jan 8, 2021

drdamour commented Jan 8, 2021

pselle commented Jan 8, 2021

drdamour commented Jan 8, 2021

pselle commented Jan 8, 2021

ghost commented Feb 7, 2021

		@@ -48,6 +48,13 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi

		configBody := buildProviderConfig(ctx, n.Addr, n.ProviderConfig())

		// if provider config is empty, return early

validate: ignore providers with no configuration #24896

validate: ignore providers with no configuration #24896

Conversation

bendrucker commented May 8, 2020 • edited Loading

Child Module

Root Module

codecov bot commented May 8, 2020 • edited Loading

Codecov Report

jsf9k commented May 13, 2020

bendrucker commented May 20, 2020

pselle commented Nov 5, 2020

bendrucker commented Nov 5, 2020

bendrucker commented Nov 10, 2020

pselle Nov 17, 2020

Choose a reason for hiding this comment

pselle Nov 17, 2020

Choose a reason for hiding this comment

pselle Nov 17, 2020

Choose a reason for hiding this comment

pselle commented Nov 17, 2020 • edited Loading

bendrucker commented Nov 17, 2020

bendrucker commented Dec 6, 2020

bendrucker commented Dec 14, 2020

pselle left a comment

Choose a reason for hiding this comment

bendrucker commented Jan 7, 2021

drdamour commented Jan 8, 2021

bendrucker commented Jan 8, 2021

drdamour commented Jan 8, 2021

drdamour commented Jan 8, 2021

pselle commented Jan 8, 2021

drdamour commented Jan 8, 2021

pselle commented Jan 8, 2021

ghost commented Feb 7, 2021

bendrucker commented May 8, 2020 •

edited

Loading

codecov bot commented May 8, 2020 •

edited

Loading

pselle commented Nov 17, 2020 •

edited

Loading