The true and false result expressions must have consistent types. The 'true' value is set of dynamic, but the 'false' value is object. #31039
Comments
|
Thanks for reporting this, @FernandoMiguel! At first glance this seems like a correct error: your conditional expression is indeed defined to produce a set in one case but an empty object in another, and those types are not compatible. Therefore I think our first step here would be to understand why this didn't return an error in v1.1.9. It suggests that there was some sort of bug in that previous version that is now fixed, so I think we will need to understand what that was to see what is a reasonable path forward here. We'll take a closer look at this soon. Thanks again! |
apparentlymart
added
the
v1.2
|
This one was a pretty interesting one to create a minimal reproduction for, because it seems that it's crucial that the set itself be unknown and of unknown type, leading to what this error message clumsily refers to as a "set of dynamic". Therefore my reproduction case is pretty contrived and relies on some tricks to synthesize a similar result without relying on a Kubernetes server: resource "null_resource" "example" {
}
locals {
list = null_resource.example.id[*]
set = toset(local.list)
pred = null_resource.example.id == "well well well"
maybe = local.pred ? local.set : {}
}Some notes about what's going on here:
With all of that said then, this sufficiently recreates the relevant situation to produce an equivalent error on Terraform v1.2.0-rc2: On Terraform v1.1.9 it succeeds during planning and during the subsequent apply step: We do typically consider it an improvement if an error that was previously detected only during apply were now detected during plan instead due to Terraform's analysis being more precise, but this one is concerning because somehow Terraform v1.1.9 wasn't even detecting this problem during the apply step, at which point Terraform should be able to clearly see that the true and false arms of the conditional are of incompatible types. I don't yet know why Terraform v1.1.9 wasn't detecting this error, but hopefully the simpler reproduction case above will make it easier to dig in and get to the bottom of it. |
|
One further thing I noticed is that even though the first (Notice that this is the old-style "Inconsistent conditional result types" message, where says "are set of string and object, respectively" instead of "The 'true' value is set of string, but the false value is object" as v1.2 would've produced.) It is not clear to me at the moment why (The "Refreshing state..." message did cause me to wonder if the refresh step was the key here, but sadly there's no difference in behavior when I do the subsequent plan with |
apparentlymart
added
confirmed
|
The change which resulted in this new behaviour is in #30879, where we corrected the conversion functions to cope with values of unknown type. This means that calling I've verified this by cherry-picking the commit from #30879 onto v1.1, which results in the same behaviour as the 1.2 branch. To achieve the goal I think you're aiming for, the for_each = (
var.fargate_create == true && var.otel_fargate_container_insights_enabled == true ?
toset(data.kubectl_path_documents.otel_fargate_container_insights[0].documents) :
toset([])
)That is, changing Based on our reproduction, we don't currently believe this is breaking previously-valid behaviour, as while the first Thanks again for testing the release candidate and reporting the issue! |
alisdair
added
the
explained
|
From looking closely at the TRACE logs for the apply step, I can see why Since I used local values for my reproduction, that made them subject to pruning during the apply step and therefore Terraform didn't evaluate them at all. I expect the same would not apply to a resource (It's interesting to note that this pruning does appear to allow invalid local values to go undetected as long as they remain unknown until the apply step, which is concerning now that I see it, but is a different concern from what this issue is discussing. An unknown value that never gets used is an edge case anyway, so it's not super concerning but still something we might want to revisit and consider.) |
|
I did try to reproduce this with I find myself unsure about how to write a reproduction that involves ...and so it seems like if the condition hadn't failed type checking then the @FernandoMiguel, when you tried this with Terraform v1.1.9, did you do so in the same context as you tried v1.2.0-rc2? In particular, I'm wondering if you tried v1.2.0-rc2 in a situation where there was no prior state but you tried Terraform v1.1.9 in a situation where there was already a prior state, and therefore Terraform was able to decide a known answer for |
all of my plans/applies were of real code with already existing states, and just by switching |
|
After talking with @apparentlymart in slack , I ended up going with Still, I expect this change of behaviour to impact in some manner a large set of codebases that did not cast type prior. although looking at https://github.com/search?l=&p=3&q=for_each++%3D+var.+%3A+%7B%7D+language%3AHCL+-dynamic+-toset&ref=advsearch&type=Code , that may not prove true my statement... oh well |
|
@FernandoMiguel do you have a sense of why I'm trying to find a realistic reproduction of what you saw that I can run on my system (without access to your Kubernetes 😀) and it seems like exactly how you were running Terraform in each case may have made a difference here, but I'm not sure exactly what differed. Thanks! |
|
this is the code for it I dont recall now, but there is a slight chance it was missing in the downstream module, and was added in between runs. |
|
also, using |
|
Hi @FernandoMiguel! Thanks for sharing that. I'm sorry I wasn't clear about what I was asking; I was actually hoping to see the assignment of an expression to that variable in the |
|
main.tf variable "fargate_create" {
description = "Controls if fargate resources should be created"
type = bool
default = false
}
module "base_system" {
source = "/Users/fernando/work/TF-Modules/tf_modules_eks_base_system"
vpc_tag = var.vpc_tag
cluster_name = var.cluster_name
fargate_create = var.fargate_create
eks_managed_node_groups_create = var.eks_managed_node_groups_create
[...]dev.tfvars fargate_create = true
[...]hth |
|
Thanks for that extra context, @FernandoMiguel! I'm not yet understanding how " It does seem though like whatever is going on here is originating somewhere different than where the error message is appearing. Something elsewhere in your configuration seems to be causing Terraform to never evaluate this incorrect expression before, but it's now correctly validating it in v1.2.0. I don't think we yet have enough information to pursue a fix here, since it isn't clear where the problem is, but hopefully someone else will encounter a similar problem soon enough and maybe we'll have a second example to compare with and see what you both have in common, and thus hopefully narrow it down enough that we can have another attempt at a realistic reproduction case. Thanks again! |
apparentlymart
added
waiting for reproduction
|
to get around issues like this, you can re-write the expression to be a comprehension rather than a ternary (Note: we can shorten Ternary form: for_each = var.fargate_create && var.otel_fargate_container_insights_enabled ? toset(data.kubectl_path_documents.otel_fargate_container_insights[0].documents) : {}Comprehension form: for_each = {for k,v in toset(data.kubectl_path_documents.otel_fargate_container_insights[0].documents : k => v if var.fargate_create && var.otel_fargate_container_insights_enabled}Note: I did not test the comprehension form but it looks like you might encounter a secondary error that would raise a |
|
In my opinion this validation seems very much like an anti-feature. It is yet another thing in Terraform which makes handling conditional values harder, and conditional handling in Terraform really is terrible already. For instance I have a scenario where I conditionally try to set up replication with the https://registry.terraform.io/modules/terraform-aws-modules/s3-bucket/aws/latest module. If I have a value that looks like this: This only works if replication is true, if So, let's have the false value be the same as the modules default value, which is: What makes it even worse is that the check is only performed if the conditional returns true. If I set Terraform version: |
replication_configuration = {for k,v in {
role = aws_iam_role.replication[0].arn
rules = [{ [...] }]
} : k => v if var.replication } |
|
We hit this issue regularly, and I saw the locals {
environment = "dev"
services_with_postgresql = local.environment == "dev" ? {
tools = {}
big-tools = {
shape = "PostgreSQL.VM.Standard.E4.Flex.4.64GB"
configuration = {
autovacuum = 0
effective_io_concurrency = 1
}
}
} : {}
}
resource "terraform_data" "cluster" {
for_each = local.services_with_postgresql
input = {
name = each.key
shape = lookup(each.value, "shape", "PostgreSQL.VM.Standard.E4.Flex.2.32GB")
}
}
Running terraform validate: I'm using: Hope this helps. |
|
Sorry in advance for jumping in the bandwagon, but I tend to agree that conditional handling experience in terraform is quite bad. I'd say maybe it's time to rethink some design decisions there. I think these type of problems arise because terraform blends two concepts that are antagonic: Both Strongly typed and Weakly typed language behaviorIf you make implicit type conversions and don't allow type definitions, strict type checks doesn't make senseTake the example below, without applying dirty hacks, problems like the below happen all the time:
I understand that this can prevent certain errors (such as a reference to the missing field), but that's no the right way to prevent a problem. The paradigm mix is. To make problem worse, there is no simple way to intersect these objects. You have to project one based on the other, just to make it run. If we want to behave like javascript, then this needs to be a run-time/apply-time error, if applicable. In this case, wouldn't necessarily be so. Boolean expressions should work like every other languageAs soon as a boolean expression encounters a false evaluation, it should not continue evaluating the rest of the expression.
instead of this:
No support for user defined functionIf there was support for user defined functions, things such as custom map projections or other ways to handle incompatible types becomes a pain staking error prone process. Most of the time (in my experience), current handling of types are a hurdle that does not solve a real problem and introduces others. I know the mileage may vary and type checking prevent some issues depending on who is writing the code, but the way it is designed needs rethinking as it pulls the carpet on one side to cover the other. Final thoughtsBy no means I am unappreciative of all the work put in terraform ecosystem (to me the best IaC framework), but sometimes certain parts of a design don't fit model and I think this is one of those cases. This is the only point I have in favor of some other products that have been showing up and allow the user to use his language of choice for the IaC (which in my opinion is a mistake in itself, like trying to use C++ instead of HTML for the frontend). |
Terraform Version
Terraform Configuration Files
works fine with
The text was updated successfully, but these errors were encountered: