hashicorp / hil

HIL is a small embedded language for string interpolations.
Mozilla Public License 2.0
396 stars 34 forks source link

Conditional operator doesn't short-circuit evaluation based on result #50

Closed apparentlymart closed 4 years ago

apparentlymart commented 7 years ago

To avoid extensive refactoring of the evaluator while implementing the conditional syntax, we accepted the limitation that it would evaluate both "sides" and then discard the inappropriate one when given syntax like this:

foo ? bar : baz

This is in most cases just a small waste of compute to evaluate a result that will never be used, but it's troublesome in cases where one of the expressions can be evaluated without error only if its associated condition state is true. For example:

length(foo) > 0 ? foo[0] : "default"

This doesn't work as intended because we fail on trying to index into the empty list foo in spite of the conditional.

The correct behavior is for the evaluator to evaluate the boolean expression first and then visit only one of the given expressions.

tomdavidson commented 7 years ago

Not complaining, just noting that it is more than cpu cycles. For example, my module was intended to have var.accounts optional.

  group = "${lookup(var.accounts, "unset", "") != ""
    ? element(aws_iam_group.default.*.id, count.index)
    : aws_iam_group.default.id}"

aws_iam_group.default does not exist when var.accounts is set.

Quentin-M commented 7 years ago

Completely breaks patterns such as:

ca_cert = "${length(var.tectonic_ca_cert_path) > 0 ? file(var.tectonic_ca_cert_path) : ""}"

or

ca_cert = "${var.ca_cert == "" ? tls_self_signed_cert.ca.cert_pem : var.ca_cert}"

Which is not so great because it means there's literally no way as far as I know to support generating OR importing a certificate. Except if you always create the CA resource, use heredocs to specify the existing PEM-Encoded CA cert in tfvars then use either. Hacks all over the place.

Is there a timeline for that one? It's quite incapacitating.

Quentin-M commented 7 years ago

Actually, here is a wacky workaround for the second use-case:

ca_cert = "${var.ca_cert == "" ? join(" ", tls_self_signed_cert.ca.*.cert_pem) : var.ca_cert}"
fatmcgav commented 7 years ago

Just to add a scenario that I've just attempted that's hit this issue:

# CloudInit config
data "template_cloudinit_config" "instance_userdata" {
  count = "${length(var.userdata_template) > 0 ? var.count * length(split(",", lookup(var.common, "azs"))): 0}"

  /* count = "${var.count * length(split(",", lookup(var.common, "azs")))}" */

  part {
    content_type = "text/cloud-config"
    content      = "${element(data.template_file.instance_userdata.*.rendered, count.index)}"
  }
}

# Create required instance(s)
resource "openstack_compute_instance_v2" "instance" {
  count               = "${var.count * length(split(",", lookup(var.common, "azs")))}"
  ... snipped for brevity...

  user_data = "${length(var.userdata_template) > 0 ? element(data.template_cloudinit_config.instance_userdata.*.rendered, count.index) : ""}"
  ... snip ...
}

You can see my commented count in the template_cloudinit_config resource, which worked fine when if I'm always providing a userdata_template, however I want it to be optional, hence the user of length.

However the above code fails with:

 Error running plan: 1 error(s) occurred:

* module.instance_test.openstack_compute_instance_v2.instance: 2 error(s) occurred:

* module.instance_test.openstack_compute_instance_v2.instance[0]: element: element() may not be used with an empty list in:

${length(var.userdata_template) > 0 ? element(data.template_cloudinit_config.instance_userdata.*.rendered, count.index) : ""}
* module.instance_test.openstack_compute_instance_v2.instance[1]: element: element() may not be used with an empty list in:

${length(var.userdata_template) > 0 ? element(data.template_cloudinit_config.instance_userdata.*.rendered, count.index) : ""} 
pikeas commented 7 years ago

Looks like https://github.com/hashicorp/hil/pull/53 addresses this?

apparentlymart commented 7 years ago

Sadly not, @pikeas. #53 is about the unknown values produced in Terraform when a new resource is created, and is not related to the conditional operator.

andresvia commented 7 years ago

On 0.9.8 module output with resource count = 0 and splat operator, works if Terraform env is NOT default.

value = "${var.with_resource == "true" ? random_pet.default.0.id : var.with_resource}"
mootpt commented 7 years ago

I hit this when trying to source secrets from vault using the vault_generic_secret data source. It seems when you are using a data source like:

foo = "${var.vault_path == "" ? var.foo : data.vault_generic_secret.certs.data["foo"]}

Unlike any other resource, I can't seem to use splat, coalesce, etc. I even attempted putting it into a null_data_source, which also proved unhelpful. Any workarounds here?

jaygorrell commented 7 years ago

@mootpt I think for those you can use a depends_on with basically any other resource to force evaluation to fall back to runtime instead of plan-time. We're using similar for other resources that fail evalutation at plan-time.

mootpt commented 7 years ago

@jaygorrell, sorry should have been more clear. The vault_generic_secret uses count to determine if vault_path had been set. In this case when it's not set, the vault_generic_secret does not exist so it seems ordering doesn't really matter in this case.

data "vault_generic_secret" "certs" {
  count = "${var.vault_path == "" ? 0 : 1}"
  path = "${var.vault_path}"
}
mootpt commented 7 years ago

Okay, got to the bottom of this. Do not use data.vault_generic_secret.certs.data["foo"], you can get away with using data.vault_generic_secret.certs.data.foo and there for the splat/join hack:

foo = "${var.vault_path == "" ? var.foo : join(" ", data.vault_generic_secret.certs.*.data.foo)}
ooesili commented 7 years ago

I found a nasty hack for the length(foo) > 0 ? foo[0] : "default" pattern.

I have a variable called snapshot_ids that defaults to an empty list, and a variable called count that specifies how many EBS volumes I want to create. If no snapshots are specified, default to an empty string. I had something to the effect of this.

resource "aws_ebs_volume" "volumes" {
  count = "${var.count}"
  snapshot_id = "${length(var.snapshot_ids) == var.count ? element(var.snapshot_ids, count.index) : ""}"
}

But I kept getting element() may not be used with an empty list. Here is the hack I came up with.

resource "aws_ebs_volume" "volumes" {
  count = "${var.count}"
  snapshot_id = "${element(split(" ", join("", list(join(" ", var.snapshot_ids), format("%*s", var.count, "")))), count.index)}"
}

Breakdown

Join snapshots by spaces.

=> "snap-1 snap-2 snap3"

Append string of count number of spaces.

=> "snap-1 snap-2 snap3   "

Split on spaces.

=> ["snap-1", "snap-2", "snap-3", "", "", ""]

This gives us at least count number elements. If three snapshots are specified, the first three elements are what you expect.

=> ["snap-1", "snap-2", "snap-3"]

If no snapshots are specified, the first three elements will be empty strings.

=> ["", "", ""]

The requires three things, that you know upper bound of index into the list (which is the case when using count and count.index), that the default value is an empty string, and that there are no spaces in the elements.

You can get around the default value and no-space constraints with a little string substitution. Here is an example with a separator of "arbitrary-separator" and a default value of "default".

"${element(split("arbitrary-separator", join("", list(join("arbitrary-separator", var.snapshot_ids), replace(format("%*s", var.count, ""), " ", "arbitrary-separatordefault")))), count.index)}"
olenm commented 7 years ago

workaround worked for me:

module creating instance (either 0 or 1 of them), instance id output via: output "i_id" { value = "${join("", aws_instance.i.*.id)}" } and am able to use module.bla.i_id in my output which now properly returns outs = or outs = i-blabla

dan-rose commented 6 years ago

We hit this problem with this evaluation,

name_prefix = "${length(var.customer_name) < 5 ? "${var.customer_name}-" : "${substr(var.customer_name, 0, 5)}-"}"

substr: 'offset + length' cannot be larger than the length of the string

cbarbour commented 6 years ago

length(foo) > 0 ? foo[0] : "default"

@apparentlymart: Your problem can be solved without conditional logic:

element(concat(foo, list("default"), 0)

This returns the first element of the list foo, which will be "default" if foo is empty.

More complex cases, such as @ooesili 's case can be solved using this basic pattern. His example:

resource "aws_ebs_volume" "volumes" {
  count = "${var.count}"
  snapshot_id = "${length(var.snapshot_ids) == var.count ? element(var.snapshot_ids, count.index) : ""}"
}

And the solution:

resource "aws_ebs_volume" "volumes" {
  count = "${var.count}"
  snapshot_id = "${element(concat(var.snapshot_ids, list("")), min(length(var.snapshot_ids) + 1, count.index))}"
}

With this approach, the last element of the list is an empty string. We use min() to ensure that element() will stop there rather than re-iterating through the entire list.

These solutions are entirely declarative. Arguably, they are more correct than using conditional logic.

apparentlymart commented 6 years ago

Thanks for sharing all the workarounds!

Please note that this is an issue about HIL itself rather than about Terraform, so (for the benefit of other readers) many of these solutions are Terraform-specific and wouldn't apply to other users of HIL unless they were to replicate some of Terraform's functions.

However, it is unlikely that this issue will ever be addressed in this codebase because Terraform is moving away from separate HIL to the new version of HCL that has HIL-like functionality integrated into it, and has a fully-functional conditional operator. That is coming in the next major version of Terraform and this particular change has its own article.

If you'd like to get updates on this being fixed in the context of Terraform, I suggest instead tracking hashicorp/terraform#15605.

If anyone monitoring this is the maintainer of another caller of the HIL library, I would suggest to investigate the new version of HCL, whose template syntax is the spiritual successor of HIL and includes the fully-featured conditional operator and a number of other enhancements.

cbarbour commented 6 years ago

Totally understood. My suggestions aren't intended to close the ticket, but instead to help anyone who runs into this particular problem. 👍

MalloZup commented 5 years ago

is this fixed with TF12?

debugmaster commented 5 years ago

Yes @MalloZup. Check hashicorp/terraform#15605.

Ranger-X commented 4 years ago

Yes @MalloZup. Check hashicorp/terraform#15605.

No, this still wont work :-(

$ terraform version
Terraform v0.12.24

$ terraform console
> false ? aws_security_group.test : null

Error: Reference to undeclared resource

  on <console-input> line 1:
  (source code not available)

A managed resource "aws_security_group" "test" has not been declared in the
> root module.
dhoepelman commented 4 years ago

Can confirm this is fixed

> false ? null.x : 1
1
> true ? null.x : 1
>
Error: Attempt to get attribute from null value

  on <console-input> line 1:
  (source code not available)

This value is null, so it does not have any attributes.

@Ranger-X it complains about the resource not being defined, that's a name binding issue not an evaluation issue. In most programming languages you cannot do false ? undeclared_variable : "default" either

graywolf-at-work commented 4 years ago

In most programming languages you cannot do

You can do that both in javascript and ruby, so it's not that unreasonable expectation.

apparentlymart commented 4 years ago

Hi all,

As I noted in my previous comment, Terraform no longer uses HIL as of Terraform 0.12, so this is not the right place to discuss Terraform's behaviors. If you have feedback about the behavior of the Terraform language I'd suggest opening an issue in the Terraform repository and then the Terraform team can route bug reports and enhancement requests upstream to Terraform's dependencies as appropriate.

I also noted before that this issue is unlikely to be fixed in HIL, because HIL has been superseded by the second-generation implementation of HCL now incorporating a HIL-like expression language. HIL's featureset is therefore frozen and there are no plans to change its behavior except for minor maintenance in support of any remaining applications that are still calling into it. For that reason, I'm going to close this issue as a "wont fix". If you are the maintainer of an application that is using HIL and would like conditional evaluation to behave as this issue describes, your likely path forward would be to transition to using the template language in HCL 2.