terraform-linters / tflint

A Pluggable Terraform Linter
Mozilla Public License 2.0
4.98k stars 357 forks source link

Incorrectly handling complex conditional #1814

Closed alex-harvey-z3q closed 1 year ago

alex-harvey-z3q commented 1 year ago

Summary

Given a complext TF conditional, TFLint is incorrectly producing an error. Details below. It may be related to not properly understanding the meaning of the any type.

Command

tflint

Terraform Configuration

variable "cloudwatch_schedule_expression" {
  description = "Define the aws cloudwatch event rule schedule expression. This is expected to be in UTC time (default) or AEDT time if is_aedt_timezone is true or AEST time if is_aest_timezone is true"
  type        = any
  default     = "cron(0 22 ? * MON-FRI *)"
}

variable "is_aedt_timezone" {
  description = "Use AEDT timezone (Terraform will redeploy twice a year with a correction for DST) instead of UTC in cloudwatch_schedule_expression"
  type        = bool
  default     = false
}

variable "is_aest_timezone" {
  description = "Use AEST timezone (no correction for DST) instead of UTC in cloudwatch_schedule_expression"
  type        = bool
  default     = false
}

locals {
  # We want to create a list of schedule expressions based on different conditions.
  # The ternary conditional operator '?' is used to choose between two alternatives.
  # 
  # If either 'is_aedt_timezone' or 'is_aest_timezone' is true, we take the
  # comma-separated schedule expression string from the external data source, and
  # split it into a list.
  # 
  # Otherwise, we first check the type of 'var.cloudwatch_schedule_expression' using
  # the 'typeof' function.
  #
  # If 'var.cloudwatch_schedule_expression' is a string, we create a single-element
  # list with this string.
  #
  # If 'var.cloudwatch_schedule_expression' is already a list, we use it as is.
  #
  cloudwatch_schedule_expressions = (
    var.is_aedt_timezone || var.is_aest_timezone
    ? split(",", data.external.schedule_expression[0].result["schedule_expression"])
    : (
      typeof(var.cloudwatch_schedule_expression) == "string"
      ? [var.cloudwatch_schedule_expression]
      : var.cloudwatch_schedule_expression
    )
  )
}

TFLint Configuration

Default.

Output

When running TFLint we get this output:

Inconsistent conditional result types; The true and false result expressions must have consistent types. The 'true' value is tuple, but the 'false' value is string., and 1 other diagnostic(s)

TFLint Version

0.47.0

Terraform Version

latest

Operating System

bendrucker commented 1 year ago

Isolated reproductions are always helpful here as there are a multitude of factors to separate before identifying a bug.

The default value is a string which TFLint identifies as invalid. TFLint only knows about variables with defaults or that you explicitly set via config/flags. It cannot evaluate runtime constructs (resources, data sources). In this case it is passing on the legitimate error that the provided configuration is invalid as passed. It may be (and seems) that your configuration is valid sometimes based on the data and variables but given the inputs to TFLint it isn't.

bendrucker commented 1 year ago

Misread the expression but the conclusion remains the same. The false condition must still have the right type, as the error indicates. I'm not sure what the best workaround is here--dynamic type behavior with any is tricky and full of sharp edges. You might be able to use flatten to handle this in a way that always produces the same type on both branches.

alex-harvey-z3q commented 1 year ago

@bendrucker No, you have misunderstood I think. The expression produces a List of Strings 100% of the time. It's clearly a bug in TFLint.

alex-harvey-z3q commented 1 year ago

Here's some more info. A part of the problem, as I mentioned already, is that there is no way in Terraform to tell the type checker that the input can only be a String or a List of Strings. Screenshot 2023-07-31 at 12 56 24 pm

wata727 commented 1 year ago

Interestingly, the typeof function does not exist in Terraform.

> typeof(var.cloudwatch_schedule_expression) == "string" ? 1 : 2
╷
│ Error: Call to unknown function
│
│   on <console-input> line 1:
│   (source code not available)
│
│ There is no function named "typeof". Did you mean "type"?

Is this a valid configuration in Terraform?

By the way, I get the same error when I use type instead of typeof in Terraform v1.5.4.

> type(var.cloudwatch_schedule_expression) == "string" ? [var.cloudwatch_schedule_expression] : var.cloudwatch_schedule_expression
╷
│ Error: Inconsistent conditional result types
│
│   on <console-input> line 1:
│   (source code not available)
│
│ The true and false result expressions must have consistent types. The 'true' value is tuple, but the 'false' value is string.
╵
bendrucker commented 1 year ago

The default value is a string which TFLint identifies as invalid.

This part was the mis-read. Because it's not a well-isolated reproduction, I thought this default value was falling through and actually producing a different type.

The expression produces a List of Strings 100% of the time.

Doesn't matter. Per the error, Terraform requires that both sides of a ternary expression evaluate to the same type. It doesn't matter that the expression will always produce the same type from the branch of the ternary that is actually followed given the evaluation of the condition.

While Terraform introduced some partial evaluation of ternaries prior to 1.0, apparently this only applies to the evaluation of the actual value, and not the type checks.

It's clearly a bug in TFLint.

Instead of restating the original claim more forcefully, you'd be better served by trying to understand the issue through an isolated reproduction.

locals {
  ternary = true ? [] : ""
}

Just like your example, while this will in theory produce a tuple 100% of the time, it is not a valid configuration/expression. In either Terraform or TFLint.

╷
│ Error: Inconsistent conditional result types
│
│   on main.tf line 2, in locals:
│    2:   ternary = true ? [] : ""
│
│ The true and false result expressions must have consistent types. The 'true'
│ value is tuple, but the 'false' value is string.
alex-harvey-z3q commented 1 year ago

Hi @bendrucker my apologies, I had been working on the assumption that the code in Terraform worked and only the linter was complaining. As you say the real issue is that Terraform itself is finnicky about this. For the record, the solution I finally found that works is this one:

locals {
  # We want to create a list of schedule expressions based on different conditions.
  # The ternary conditional operator '?' is used to choose between two alternatives.
  # 
  # If either 'is_aedt_timezone' or 'is_aest_timezone' is true, we take the
  # comma-separated schedule expression string from the external data source, and
  # split it into a list.
  # 
  # Otherwise, we return the original input flattened to a list of strings.
  #
  cloudwatch_schedule_expressions = (
    var.is_aedt_timezone || var.is_aest_timezone
    ? split(",", data.external.schedule_expression[0].result["schedule_expression"])
    : flatten([var.cloudwatch_schedule_expression])
  )
}
alex-harvey-z3q commented 1 year ago

@wata727

Interestingly, the typeof function does not exist in Terraform.

Yes. Not only was typeof a typo, but the type function only is available in terraform console. See my update above otherwise.

bendrucker commented 1 year ago

Nice! That flatten usage is exactly what I had in mind. The function handles the conditional type-specific logic and is always guaranteed to output the same type.