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

Check dynamic block contents during "terraform validate" even when the for_each expression is unknown #35173

Open
anilkumarmyla opened this issue May 17, 2024 · 3 comments

Comments

@anilkumarmyla
Copy link

Terraform Version

% terraform version
Terraform v1.8.3
on darwin_arm64

Terraform Configuration Files

...Nothing special...

Debug Output

.

Expected Behavior

terraform validate should fail

Actual Behavior

terraform validate is successful

Steps to Reproduce

Following 3 files are needed for reproduction, one change in cluster.tf triggers the correct behavior, other does not

% cat versions.tf 
terraform {
  required_version = ">= 1.8.3"

  required_providers {
    google = {
      source  = "hashicorp/google"
      version = ">= 5.29.0"
    }
    google-beta = {
      source  = "hashicorp/google-beta"
      version = ">= 5.29.0"
    }
  }
}
% cat variables.tf 
variable "name" {
  type        = string
  description = "The name of the cluster (required)"
}

variable "cluster_dns_provider" {
  type        = string
  default     = "CLOUD_DNS"
}

variable "cluster_dns_scope" {
  type        = string
  default     = "CLUSTER_SCOPE"
}

variable "cluster_dns_domain" {
  type        = string
  default     = ""
}

variable "enable_additive_vpc_scope_dns" {
  type        = bool 
  default     = false
}
% cat cluster.tf 
resource "google_container_cluster" "primary" {
  provider = google-beta

  name                = var.name

  dynamic "dns_config" {
    // terraform validate is unsuccessful, fails with below error
    // An input variable with the name "invalid" has not been declared
    //for_each = [1]

    // terraform validate is successful, incorrect behavior
    for_each = var.cluster_dns_provider == "CLOUD_DNS" ? [1] : []
    content {
      additive_vpc_scope_dns_domain = var.enable_additive_vpc_scope_dns ? "${var.invalid}.vpc.private" : ""
      cluster_dns                   = var.cluster_dns_provider
      cluster_dns_scope             = var.cluster_dns_scope
      cluster_dns_domain            = var.cluster_dns_domain
    }
  }
}

Run as-is with above files

% terraform validate
Success! The configuration is valid.

Change cluster.tf and uncomment //for_each = [1] and comment for_each = var.cluster_dns_provid...

% terraform validate
╷
│ Error: Reference to undeclared input variable
│ 
│   on cluster.tf line 14, in resource "google_container_cluster" "primary":
│   14:       additive_vpc_scope_dns_domain = var.enable_additive_vpc_scope_dns ? "${var.invalid}.vpc.private" : ""
│ 
│ An input variable with the name "invalid" has not been declared. This variable can be declared with a variable "invalid" {} block.

Additional Context

No response

References

No response

@anilkumarmyla anilkumarmyla added bug new new issue not yet triaged labels May 17, 2024
@anilkumarmyla anilkumarmyla changed the title terraform validate reports success for invalid code terraform validate reports success for code referencing invalid variable in certain cases May 17, 2024
@apparentlymart
Copy link
Member

Hi @anilkumarmyla! Thanks for reporting this.

Input variable values are specified as part of the planning options, and so input variables do not have known values during the validation phase.

When a dynamic block has an unknown value as its for_each, Terraform skips expanding it and instead just treats the result as unknown. The assumption is that Terraform will get an opportunity to check this part of the configuration in a later phase. In this case, I would expect Terraform to detect it in the planning phase once a value has been assigned to var.cluster_dns_provider.

terraform validate checks as much as it is able to with the information known at that phase, but anything whose evaluation depends on the dynamic value of an input variable will typically not be fully checked during the validation phase, because input variables are a plan-time concept.

Terraform could, in principle, make a different tradeoff here: it could decide that if the for_each for a dynamic block is unknown then it will assume that the final result would have at least one element and so evaluate the content block once with the iterator symbols set to unknown values, to see if that evaluation produces any errors.

However, we'd need to check first if that change would violate backward compatibility. The key question is: if your dynamic block had for_each = [] -- that is, no elements at all -- would Terraform currently detect the invalid variable reference during the plan or apply phases? Our rule for compatibility in this area is that anything that is already invalid in a later phase can switch to being detected in an earlier phase in a future version -- we typically consider that to be an improvement rather than a regression -- but if a partial configuration can currently reach the end of the apply phase without raising any errors then it must continue doing so for as long as the compatibility promises remain in effect.

I believe Terraform is currently working as designed as long as this error is still being detected in ether the plan or apply phases when there is at least one generated block. We can consider also detecting the error when the number of generated blocks is unknown, but that is a new capability that requires design work to make sure it doesn't break any existing configurations already considered valid and so I'm going to relabel this as an enhancement to represent that design work is required. (We use "bug" only for situations where Terraform's behavior doesn't match what it was designed to do and therefore it's obvious that the required work is to change the implementation to better match the design.)

Thanks again!

@apparentlymart apparentlymart added enhancement config and removed bug new new issue not yet triaged labels May 17, 2024
@apparentlymart apparentlymart changed the title terraform validate reports success for code referencing invalid variable in certain cases Check dynamic block contents during "terraform validate" even when the for_each expression is unknown May 17, 2024
@anilkumarmyla
Copy link
Author

terraform validate checks as much as it is able to with the information known at that phase, but anything whose evaluation depends on the dynamic value of an input variable will typically not be fully checked during the validation phase, because input variables are a plan-time concept.

@apparentlymart one other observation is that the dynamic code block is somewhat validated, I cannot have a random foo="bar" inside the dynamic block and it fails the validation as below

% terraform validate

│ Error: Unsupported argument

│   on cluster.tf line 18, in resource "google_container_cluster" "primary":
│   18:       foo                           = "bar"

│ An argument named "foo" is not expected here.

The only case it doesn't catch is usage of undeclared variables

@apparentlymart
Copy link
Member

Hi @anilkumarmyla,

Indeed, this is a quirk of static vs. dynamic configuration constructs. Terraform typically treats arguments inside blocks as a "static" concern, because their presence cannot possibly vary based on anything else in the configuration.

But evaluating expressions is trickier because their evaluation behavior can vary considerably depending on context and depending on what else is in the expression, and because evaluation can encounter unknown values that are placeholders for information Terraform won't know until a later phase.

When Terraform encounters an unknown value in a location that would normally decide whether or not something else would be evaluated, it typically skips evaluating that other thing because that is what allows progressing to the next phase where more information might be available.

It remains to be seen whether this is a situation where Terraform can proactively validate. The main thing we need to check is whether you'd be able to plan and apply something like this (assuming nothing else in your configuration has changed):

resource "google_container_cluster" "primary" {
  provider = google-beta

  name = var.name

  dynamic "dns_config" {
    for_each = [] # NOTE: No instances at all!
    content {
      additive_vpc_scope_dns_domain = var.enable_additive_vpc_scope_dns ? "${var.invalid}.vpc.private" : ""
      cluster_dns                   = var.cluster_dns_provider
      cluster_dns_scope             = var.cluster_dns_scope
      cluster_dns_domain            = var.cluster_dns_domain
    }
  }
}

In this case there would be no dns_config blocks generated. I don't recall off the top of my head whether Terraform treats the var.invalid reference as an error during planning in this case. If Terraform currently raises an error for this during planning then we may be able to make it also check during validation. If Terraform currently allows the above, despite the reference to an undeclared variable, then unfortunately we cannot change Terraform in the way you suggested because that would make a currently-acceptable configuration become invalid retroactively.

If you're able to quickly test this with the configuration you were using to prepare this issue -- making sure to test both terraform validate and terraform plan, at least -- then that might allow us to quickly determine whether what you've proposed is possible to implement. No pressure to do that, though; if you aren't able then we can leave this issue open for now and investigate further once someone on the team is available to research if further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants