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

Terraform 0.14 crashes, Terraform 1.0 reports inconsistent data type only when array is in conditional statement #30866

Closed
AndreiPotemkin opened this issue Apr 13, 2022 · 10 comments · Fixed by #30879
Assignees
Labels
bug config confirmed a Terraform Core team member has reproduced this issue explained a Terraform Core team member has described the root cause of this issue in code

Comments

@AndreiPotemkin
Copy link

AndreiPotemkin commented Apr 13, 2022

I think there might be an issue with the way Terraform handles list elements when in conditional statement compared to unconditional logic.
Actual problem has been encountered with Azure network security rule definitions but simplified code reproduces the problem.

Consider 2 local variables:

  a = {
    val = 1, arr = null
  }
  b = {
    val = null, arr = [1]
  }

if these 2 values are placed in an array unconditionally

c = [local.a, local.b]

it works fine but if same array is built conditionally

c = local.v ? [local.a, local.b] : []

it crashes Terraform 0.14 and in later versions reports error
│ │ local.a is object with 2 attributes
│ │ local.b is object with 2 attributes

To reproduce the issue attempt to execute 'terraform plan' on following:

locals {
  v = true
  a = {
    val = 1, arr = null
  }
  b = {
    val = null, arr = [1]
  }
  c = concat(
    [local.a,local.b],
    local.v ? [local.a,local.b]: []
  )
}

When v = false plan succeeds, when v = true plan fails

Possible solution to change definition of a to

  a = {
    val = 1, arr = []
  }

would pass Terraform check but will fail on apply as Azure network security rule only supports one of the 2 values to be provided, for example source_port_range and source_port_ranges (https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/network_security_rule#source_port_range)

@laszlocsomor
Copy link

laszlocsomor commented Apr 14, 2022

I believe this is the same bug I just saw with Terraform 1.1.7 on Linux x86-64.

Actual encounter was with jsonencode( { widgets = [ ... ] } ) with a computed list of widgets; simplified repro is:

locals {
  value = concat(
    [{ x = 0 }],
    var.some_input_var == null ? [] : [
      {
        key1 = [
          [
            { x = 1 }
          ]
        ]
      },
      {
        key2 = ["y"]
      },
  ])
}

This gives:

│ The true and false result expressions must have consistent types. The given expressions are tuple and tuple, respectively.

Replacing the true branch with null gives a different error, I guess originating from concat:

│ Invalid value for "seqs" parameter: argument must not be null.

Removing the conditional and keeping either branch makes the problem vanish.

@crw crw added bug new new issue not yet triaged labels Apr 14, 2022
@apparentlymart
Copy link
Member

Hi @AndreiPotemkin,

Aside from this error message being confusing, this does seem like correct behavior to me. Both the true and false arms of a conditional expression must have the same type so that Terraform can determine the result type even if the condition isn't known yet, and a tuple with two elements is a different type than a tuple with no elements.

One possible answer is to convert them both to be lists, because (unlike for tuple types) the numby of elements is part of the value of a list, not part of the type: a list of two strings is the same type as a list of zero strings.

That would not work for @laszlocsomor's example though, because in that case there really is no single type that both the true and false expressions could be converted to: the false arm itself can't be a list because the two elements of the tuple are of different types. That problem will require a different solution that doesn't use a conditional expression, though I'm not sure what to suggest since the example seems to be contrived rather than a real configuration.

@apparentlymart
Copy link
Member

(regarding the confusing error message: Terraform really wants to say that the two tuple types have different numbers/types of elements, but it's only reporting the top level type "tuple" and not describing the elements inside.)

@AndreiPotemkin
Copy link
Author

@apparentlymart, unless I misunderstand your response I would disagree with your point about how handling of arms of conditional expression plays into this problem.
As I mentioned above if arr attribute is defined as a list in both a and b definitions then Terraform does not have any problem evaluating conditional expression

locals {
  v = true
  a = {
    val = 1, arr = []
  }
  b = {
    val = null, arr = [1]
  }
  c = concat(
    [local.a,local.b],
    local.v ? [local.a,local.b]: []
  )
}

However if attribute arr is defined as a list in one variable but as null in the other one it causes the problem only if list [local.a, local.b] is constructed in one of the arms of conditional statement.

Maybe you can suggest an alternative solution to the actual problem I am trying to solve which can be described as follows:
I am building a list of network rules to be created where each rule is an object with multiple attributes and I would like to be able to conditionally include certain rules in the list hence my attempt to use conditional

list = concat(
   [unconditional rules],
   condition : [rules for condition] : []
)

Problem arises from scenario where some rules may require a single port to be supplied and others require a list of ports meaning that some objects being added into a list have ports attribute set to a list and other have same attribute set to null which my simplified code simulates.

@laszlocsomor
Copy link

Both the true and false arms of a conditional expression must have the same type so that Terraform can determine the result type even if the condition isn't known yet, and a tuple with two elements is a different type than a tuple with no elements.

Thanks for the explanation @apparentlymart. This makes sense.

I'm not sure what to suggest since the example seems to be contrived rather than a real configuration.

That's OK, thanks anyway! It was an actual example -- a JSON string passed to aws_cloudwatch_dashboard.dashboard_body's widgets element. To work around the behavior, I ended up separately jsonencode()'ing both arms of the conditional.

@apparentlymart
Copy link
Member

Thanks for clarifying your concern, @laszlocsomor! The underlying concern here was obscured a bit by you only sharing a part of the error message, making me think you were reporting that the error message was incorrect rather than that the behavior was incorrect.

However, I can see one specific misbehavior here which I think is the root cause of the problem. Trying some expressions in terraform console:

> type(tolist([1, 2]))
list(number)
> type(tolist([1, null]))
list(number)
> type(tostring(null))
dynamic

The first two of these are correct:

  • Converting tuple([number, number]) to a list produces a list(number) because all of the tuple elements have the same type.
  • Converting tuple([number, any]) to a list produces a list(number) because any can convert to number. (Note that the inherent type of null is "any" because it doesn't specify any type information itself; Terraform infers the type of a null value only by its context.)
  • However, converting any to a string as we're doing in tostring(null) should also produce type string, for the same reason as the previous conversion worked, and yet tostring is actually returning "dynamic", which is just another way of saying any when we're talking about the type of a value rather than a type constraint. The information added by the tostring call here is therefore lost.

I can see the likely cause of this unexpected result in the generic definition that we share across all of these to... functions:

{
Name: "v",
// We use DynamicPseudoType rather than wantTy here so that
// all values will pass through the function API verbatim and
// we can handle the conversion logic within the Type and
// Impl functions. This allows us to customize the error
// messages to be more appropriate for an explicit type
// conversion, whereas the cty function system produces
// messages aimed at _implicit_ type conversions.
Type: cty.DynamicPseudoType,
AllowNull: true,
AllowMarked: true,
},

The above states that the argument can be of any type (cty.DynamicPseudoType is the internal representation of any), it's okay for it to be null, and that the function is able to handle the function being "marked" (which is how Terraform tracks a value being "sensitive"). However, a crucial omission here is that this definition doesn't include AllowDynamicType: true, which tells the language runtime that this function isn't capable of handling a non-precisely-typed value itself and so the language runtime should just immediately return "dynamic" whenever such an argument is presented.

I think that just adding AllowDynamicType: true here would be sufficient to fix this inconsistency, because the function itself is already just delegating to the generic type conversion functionality and that already knows how to convert an untyped null into a typed null.

Doing the above would not make the original configuration as presented immediately work, because the given values are ambiguous enough about the intended result type that the automatic type inference algorithm prefers to give up and return an error rather than make an non-confident guess what the author intended.

However, I believe fixing the above so that tostring(null) returns a string-typed null would allow this to work with the addition of some explicit type conversions to guide the language runtime toward converting the values toward a single result type:

locals {
  v = true
  a = {
    val = 1, arr = tolist(null)
  }
  b = {
    val = tonumber(null), arr = tolist([1])
  }
  c = concat(
    [local.a,local.b],
    local.v ? [local.a,local.b] : []
  )
}

The tonumber(null) and tolist(null) are the most relevant to what I've described above -- that will make the val attribute have type number in both objects and the arr attribute in both cases be a list, which still doesn't exactly match but I expect would be close enough that the automatic type inference would be satisfied that it understands what you intend and thus be able to convert both [local.a, local.b] and [] to list(object({ val = number, arr = list(number) })) so that the conditional expression would be valid and produce that type as its result.

@apparentlymart apparentlymart added config confirmed a Terraform Core team member has reproduced this issue explained a Terraform Core team member has described the root cause of this issue in code and removed new new issue not yet triaged labels Apr 15, 2022
@apparentlymart apparentlymart self-assigned this Apr 15, 2022
@AndreiPotemkin
Copy link
Author

@apparentlymart, I tried few more scenarios in Terraform console and it does appear [] construct is handled differently in conditional statement.

> [{a:1, b:[]},{a:2, b:null}]
[
  {
    "a" = 1
    "b" = []
  },
  {
    "a" = 2
    "b" = null
  },
]

works however

> true ? [{a:1, b:[]},{a:2, b:null}] :[]
╷
│ Error: Inconsistent conditional result types
│ 
│   on <console-input> line 1:
│   (source code not available)
│ 
│ The true result value has the wrong type: element types must all match for conversion to list.
╵

fails

Further analysis shows this interesting behavior

> [{a:1, b:[]},{a:2, b:[1]}]
[
  {
    "a" = 1
    "b" = []
  },
  {
    "a" = 2
    "b" = [
      1,
    ]
  },
]

evaluates b in both elements to [] however when used in the conditional the same construct evaluates to tolist conversion

> true ? [{a:1, b:[]},{a:2, b:[1]}] :[]
tolist([
  {
    "a" = 1
    "b" = tolist([])
  },
  {
    "a" = 2
    "b" = tolist([
      1,
    ])
  },
])

And finally when b array in both objects has same number of elements it does not produce tolist conversion

> true ? [{a:1, b:[3]},{a:2, b:[1]}] :[]
tolist([
  {
    "a" = 1
    "b" = [
      3,
    ]
  },
  {
    "a" = 2
    "b" = [
      1,
    ]
  },
])

I sure hope PR #30879 properly handles presented failing case

@apparentlymart
Copy link
Member

Hi @AndreiPotemkin,

Yes, the conditional operator does make some attempts to automatically fix inconsistencies between the types of the true and false results through automatic type inference and conversion. This typically works as long as at least one expression is precisely typed enough that Terraform can see how to convert others which contain unknown-typed values like null to match.

For example, if given a tuple where all elements are strings and a tuple with no elements, Terraform will assume that you intended both to be off type list(string) and automatically convert them. This is possible even though the empty tuple provides no specific element type information.

However, Terraform will give up and return an error if there isn't a clear path to automatic conversion to match both results. In that case the best approach is to add more type information using the type conversion functions. The associated pull request fixes an issue where the type conversion functions don't add type information to null values passed directly, and so after that is merged it will be possible to use the type conversion functions to explain what type you intended a null value to have when Terraform either cannot guess or it guesses incorrectly.

@apparentlymart
Copy link
Member

#30879 addresses this issue by fixing a bug in the type conversion functions which was previously making it impossible to give Terraform the type information necessary to resolve type inference for the ambiguous input shown in the original report.

The original input still returns the same error, which is a correct error because the untyped nulls in the local.a and local.b values don't give enough information for Terraform to confidently infer that a single type was intended for conversion to a list:

╷
│ Error: Inconsistent conditional result types
│ 
│   on fun-with-tuples.tf line 11, in locals:
│   11:     local.v ? [local.a,local.b]: []
│     ├────────────────
│     │ local.a is object with 2 attributes
│     │ local.b is object with 2 attributes
│ 
│ The true result value has the wrong type: element types must all match for conversion to list.
╵

However, with that bug fixed we can use tonumber and tolist strategically to explain to Terraform the intended type of the resulting value, so that Terraform has to make fewer guesses:

locals {
  v = true
  a = {
    val = 1, arr = tolist(null)
  }
  b = {
    val = tonumber(null), arr = tolist([1])
  }
  c = concat(
    [local.a,local.b],
    local.v ? [local.a,local.b]: []
  )
}

output "result" {
  value = local.c
}
Changes to Outputs:
  + result = [
      + {
          + arr = null
          + val = 1
        },
      + {
          + arr = [
              + 1,
            ]
          + val = null
        },
      + {
          + arr = null
          + val = 1
        },
      + {
          + arr = [
              + 1,
            ]
          + val = null
        },
    ]

In the final output from terraform apply there are some additional annotations on the output value which show what result types Terraform concluded:

result = [
  {
    "arr" = tolist(null) /* of dynamic */
    "val" = 1
  },
  {
    "arr" = tolist([
      1,
    ])
    "val" = tonumber(null)
  },
  {
    "arr" = tolist(null) /* of number */
    "val" = 1
  },
  {
    "arr" = tolist([
      1,
    ])
    "val" = tonumber(null)
  },
]

Since the overall result of local.c is a tuple type there is no need for the first two elements to match, but the final two must match in order for that sublist to be of type list(object({ arr = list(number), val = number })), which is a type that both [local.a, local.b] and [] can convert to in order for the conditional expression to be valid and also return a value of that type.


We also have another change pending over in the HCL repository to improve the error message about the conditional result types being inconsistent. We plan to adopt that into Terraform once it's merged, but we'll keep track of that separately since it's only an error message improvement and so can be updated at any time (including in a patch release) without changing the behavior of anything in the language.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug config confirmed a Terraform Core team member has reproduced this issue explained a Terraform Core team member has described the root cause of this issue in code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants