Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mandatory "features" block is breaking best-practice usage of "terraform validate" #7359

Closed
embik opened this issue Jun 17, 2020 · 4 comments
Assignees
Labels
question upstream/terraform This issue is blocked on an upstream issue within Terraform (Terraform Core/CLI, The Plugin SDK etc)

Comments

@embik
Copy link

embik commented Jun 17, 2020

This is a follow-up issue to #5866 since the discussion there has not seen any follow-up before the bot closed it. I believe the provider behavior violates several best-practices and de-facto behaviors in Terraform providers with the mandatory "features" block.

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform (and AzureRM Provider) Version

Terraform v0.12.26
azurerm-provider: v2.14.0

Affected Resource(s)

  • azurerm Provider

Terraform Configuration Files

Any self-contained Terraform module that contains:

terraform {
  required_providers {
    azurerm = "= 2.14.0"
  }
}

But not an explicit provider block.

Expected Behavior

terraform validate should validate the syntax and typing of my Terraform module successfully.

I expect this to work for several reasons. For one, terraform validate explicitly should only validate syntactic validity, see this comment:

validation means "check that the syntax is valid and types/values are being used correctly", which means validation reports on whether the module is valid regardless of particular variable values.

Using the workaround described in #5866 (comment), I am required to provide specific variable values to variables of my Terraform module when importing it to pass the explicit provider configuration, breaking the intended use of terraform validate. It is validating a different kind of thing! Previously, validating a Terraform module using AzureRM was possible, now I need to validate a root module that imports another module. That is not the same thing.

Moreover, the previously linked issue on hashicorp/terraform mentions adjusting terraform validate to ignore provider behavior like this: hashicorp/terraform#21408 (comment)

I think the trick here will be to introduce a new subtlety into the validation: the terraform validate command should only validate a provider configuration if there's an explicit provider block. If there's no explicit block (and thus an empty configuration is implied), terraform validate would skip validating the configuration and leave that for terraform plan to check. This is acceptable because terraform validate doesn't actually need to configure the provider itself, so (unlike with other commands) skipping a provider with an implied configuration won't prevent other validation from completing.

This means that the intended behavior for terraform validatein this case would be to work, not fail. The provider is breaking intended behavior of Terraform here in my opinion and it could be easily fixed without changes to core Terraform, at least in the AzureRM case.

Last but not least there is no technical reason for requiring this option, since all examples of it are setting it to a default value (empty). The requirement for setting a default value explicitly doesn't make sense and is violating the convention over configuration paradigm, a concept that is frequently applied within Terraform. I understand that this was to make the change from 1.x to 2.x explicit, but 2.x has been out for quite some time now.

Actual Behavior

terraform validate fails when running on a Terraform module:

 $ terraform validate ${MODULE_DIR}
Error: "features": required field is not set

Steps to Reproduce

  • run terraform validate on a Terraform module

Important Factoids

References

@embik
Copy link
Author

embik commented Jun 17, 2020

I want to add a note of appreciation that apart from this, the 1.x -> 2.x upgrade of the AzureRM provider is super painless. I'm really glad about that, good job on the 2.x release line!

@tombuildsstuff tombuildsstuff self-assigned this Jun 24, 2020
@tombuildsstuff tombuildsstuff added the upstream/terraform This issue is blocked on an upstream issue within Terraform (Terraform Core/CLI, The Plugin SDK etc) label Jun 24, 2020
@tombuildsstuff
Copy link
Contributor

hey @embik

Thanks for opening this issue.

Previously, validating a Terraform module using AzureRM was possible, now I need to validate a root module that imports another module.

Whilst previously Terraform Core would infer a Provider block if one was omitted, where all required fields could be provided via Environment Variables - since the features block configures behaviours, we need to be explicit about which provider block is being used to prevent unexpected behaviours (e.g. deleting a disk when it wasn't expected).

As mentioned in #5866 one option would be to introduce an ARM_FEATURES environment variable - however that just brings us back to the same issue, which caused a large number of issues/confusion previously - as such unfortunately that isn't something we plan to support at this time.

For context, the AWS Provider (by default) requires a "region" argument is specified on the Provider block - whereas the 1.x versions of the Azure Provider (by default) didn't require anything (since we can run from Azure CLI auth/Env Variables) - as such this issue mostly affects users who are new to Terraform (who are most likely to be affected by this bug).


Since terraform validate doesn't require a Provider block - one option would be to remove the requirement for this to exist at that stage, as proposed in this issue (with a PR tracking that here) - however that's one for the Terraform Core team.


It's worth noting that due to the nature of the Azure API's (where these API's are generic, and can have behavioural changes depending on the Subscription/Location being used), unfortunately Terraform's validation functionality can only go so far.

When building standalone modules, one option would be to provide end-to-end examples/tests to confirm this provisions infrastructure as you expect it to be used (either with terraform validate, terraform plan and terraform apply - or with something like terratest). In this instance it's possible to pass in a Provider block from the example configuration which gets around this, to give an example (using Terraform 0.13-Beta2):

$ cat main.tf
terraform {
  required_providers {
    azurerm = {
      source = "hashicorp/azurerm"
      version = "2.15.0"
    }
  }
}

provider "azurerm" {
  features {}
}

module "example" {
  source   = "./module"
  name     = "issue7359"
  location = "West Europe"
  providers = {
    azurerm.default = azurerm
  }
}

And the module:

$ cat module/main.tf
terraform {
  required_providers {
    azurerm = "= 2.15.0"
  }
}

resource "azurerm_resource_group" "test" {
  name     = "${var.name}-resources"
  location = var.location
}

variable "name" {}

variable "location" {}

When (in this case running from the root level) - terraform validate works as expected:

$ ./terraform validate
Success! The configuration is valid.

Ultimately this is a workaround in this instance until the upstream issue on Terraform Core is solved - as you've mentioned that's being tracked in hashicorp/terraform#21408.

As such whilst I'd like to thank you for opening this issue I'm going to close this in favour of the upstream issue tracking this bug in terraform validate (hashicorp/terraform#21408) - which will ultimately fix this.

Thanks!

@embik
Copy link
Author

embik commented Jun 24, 2020

Thank you for the comprehensive answer @tombuildsstuff! I would have preferred to see this change in providers after Terraform upstream fixed this, but I understand your point and hope that the validation part is fixed soon.

@ghost
Copy link

ghost commented Jul 24, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question upstream/terraform This issue is blocked on an upstream issue within Terraform (Terraform Core/CLI, The Plugin SDK etc)
Projects
None yet
Development

No branches or pull requests

3 participants