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

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

Closed
apparentlymart opened this issue Feb 23, 2017 · 23 comments
Closed

Comments

@apparentlymart
Copy link
Contributor

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
Copy link

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
Copy link

Quentin-M commented Mar 24, 2017

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.

alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this issue Mar 24, 2017
* don't condition the creation of the VPC resource
  (hashicorp/hil#50)
* pass external subnet IDs into the VPC module
alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this issue Mar 24, 2017
* don't condition the creation of the VPC resource
  (hashicorp/hil#50)
* pass external subnet IDs into the VPC module
alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this issue Mar 24, 2017
* don't condition the creation of the VPC resource
  (hashicorp/hil#50)
* pass external subnet IDs into the VPC module
alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this issue Mar 24, 2017
* don't condition the creation of the VPC resource
  (hashicorp/hil#50)
* pass external subnet IDs into the VPC module
@Quentin-M
Copy link

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}"

alexsomesan added a commit to coreos/tectonic-installer that referenced this issue Mar 27, 2017
Fixes for BYO VPC:
* pass external subnet IDs into the VPC module
* diabolical use of join() to circumvent conditional operator limitation on missing resources
   (hashicorp/hil#50)
alexsomesan pushed a commit to coreos/tectonic-installer that referenced this issue Mar 27, 2017
* modules/bootkube: fix error when a CA cert/key is provided
* modules/bootkube: add missing wiring for provided CA cert in templating

Due to hashicorp/hil#50 the `join()` interpolation function is used to work around it.
@fatmcgav
Copy link

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) : ""} 

metral added a commit to metral/tectonic-installer that referenced this issue May 4, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 4, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 5, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
metral added a commit to metral/tectonic-installer that referenced this issue May 9, 2017
- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
coreos@7ab31b0)
s-urbaniak pushed a commit to coreos/tectonic-installer that referenced this issue May 9, 2017
* platforms/azure: add missing master_count to the tectonic module

* modules/azure: Enable the use of external master & worker subnets

- Only create master & worker subnets if no external vnets exist
- The `join()` interpolation function is used to work around
hashicorp/hil#50 when the subnets are conditionally
created. For more detail, see:
7ab31b0)
@pikeas
Copy link

pikeas commented May 16, 2017

Looks like #53 addresses this?

jnoss added a commit to FitnessKeeper/terraform-aws-s3-cloudfront that referenced this issue Nov 6, 2017
This allows to have multiple CloudFront distributions pointing to the
same S3 bucket, such as to different paths.

Note, due to the way the conditional parser works in current version of
Terraform we're using the workaround using join() and the .*. operator,
see hashicorp/hil#50
jnoss added a commit to FitnessKeeper/terraform-aws-s3-cloudfront that referenced this issue Nov 7, 2017
* allow not creating/managing the S3 bucket

This allows to have multiple CloudFront distributions pointing to the
same S3 bucket, such as to different paths.

Note, due to the way the conditional parser works in current version of
Terraform we're using the workaround using join() and the .*. operator,
see hashicorp/hil#50

* update readme

* improve handling CloudFront origin access identity

This allows to still create the CloudFront origin access identity even
if not creating the S3 bucket, and will output the iam_arn of the newly
created CloudFront origin access identity. Addresses issue #7.
@dan-rose
Copy link

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

neomantra added a commit to neomantra/terraform-google-lb-http that referenced this issue Mar 31, 2018
This change sets the `ports` list to [""] when `backend_params` is empty.

Ideally this would be done with an expression like:
```
"${length(var.backend_params) == 0 ? "" : element(split(",", element(var.backend_params, count.index)), 2)}"
```

But because of a limitation with HCL, it cannot be done and we've resorted
to the trick described in this issue:

hashicorp/hil#50 (comment)

The pipe `|` delimiter is used because it is not valid to use in GCP names.

Contributed according to the Google CLA.
neomantra added a commit to neomantra/terraform-google-lb-http that referenced this issue Jun 27, 2018
This change sets the `ports` list to [""] when `backend_params` is empty.

Ideally this would be done with an expression like:
```
"${length(var.backend_params) == 0 ? "" : element(split(",", element(var.backend_params, count.index)), 2)}"
```

But because of a limitation with HCL, it cannot be done and we've resorted
to the trick described in this issue:

hashicorp/hil#50 (comment)

The pipe `|` delimiter is used because it is not valid to use in GCP names.

Contributed according to the Google CLA.
@cbarbour
Copy link

cbarbour commented Jul 31, 2018

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
Copy link
Contributor Author

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
Copy link

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

@MalloZup
Copy link

is this fixed with TF12?

@debugmaster
Copy link

Yes @MalloZup. Check hashicorp/terraform#15605.

@Ranger-X
Copy link

Ranger-X commented Apr 7, 2020

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
Copy link

dhoepelman commented Apr 8, 2020

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
Copy link

In most programming languages you cannot do

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

@apparentlymart
Copy link
Contributor Author

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.

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

No branches or pull requests