lang/funcs: Add "alltrue" function

[artburkart]

What

This commit adds an all function to Terraform configuration. A
reason we might want this function is because it will enable more
powerful custom variable validations. For example:

variable "amis" {
  type = list(object({
    id = string
  }))

  validation {
    condition = (all([
      for a in var.amis : length(a.id) > 4 && substr(a.id, 0, 4) == "ami-"
    ]))
    error_message = "The ID of at least one AMI was invalid."
  }
}

If this is not the intended use of custom variable validation, the all
function may still prove useful in other contexts where we, for example,
create a resource conditionally.

[codecov]

Codecov Report

Merging #25656 into master will increase coverage by 0.01%.
The diff coverage is 89.47%.

Impacted Files	Coverage Δ
lang/funcs/collection.go	78.35% <88.88%> (+0.75%)	⬆️
lang/functions.go	93.38% <100.00%> (+0.05%)	⬆️
terraform/node_resource_plan.go	92.23% <0.00%> (-1.95%)	⬇️
helper/resource/state.go	84.68% <0.00%> (-0.91%)	⬇️
states/statefile/version4.go	55.45% <0.00%> (+0.30%)	⬆️
dag/marshal.go	54.79% <0.00%> (+1.36%)	⬆️

[artburkart]

@apparentlymart – I suppose you'd be the one who'd review a change like this?

[artburkart]

It seems like this function could have utility in Terraform. Is there any way to get it attention?

[alisdair]

Thanks for working on this function! This seems like a good implementation to me. I think we've seen enough questions about the need for all and any functions for variable validation that I think this is worth including.

I pondered a few design decisions and concluded that I agree with them:

• Choosing to accept "true" seems pretty much in line with the common behaviour of aggressively unifying arguments. I can't find any precedent for this specific choice with bool, but it seems reasonable to me.
• Similarly, working with all collections makes sense
• I think all([]) == true is the correct choice, so long as a future any function is defined as any([]) == false

I have one inline comment. The only other change I'd like to see is a test covering cty.TupleVal([]cty.Value{cty.True, cty.StringVal("true")}), just for clarity. Should this function be added, I think it would be great to see any included in a follow-up, for symmetry.

I'm also requesting a second opinion from another core team member—there may be good reasons why we shouldn't accept these functions. If you'd like to wait for the second thumbs up before responding, no problem.

Thanks again!

[alisdair]

I'm curious about these commented out test cases. Why are they here?

[artburkart]

Ah, I missed your message.

---

{
 cty.ListVal([]cty.Value{cty.ListValEmpty(cty.String)}),
 cty.False,
 false,
},

At the time of writing, I couldn't get this to pass. It seems to pass now. ¯_(ツ)_/¯

---

{
 cty.ListVal([]cty.Value{cty.True, cty.StringVal("true")}),
 cty.True,
 false,
},

When I wrote this, I didn't know ListVal requires all list element types be consistent. My intent was to ask someone on your team how to test this scenario. You offered the solution in your review: test with TupleVal.

---

{
 cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
 cty.False,
 false,
},

This test scenario fails with the following message:

lang/funcs TestAllTrue/alltrue(cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)})) (0.00s)
    TestAllTrue/alltrue(cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)})): collection_test.go:225: unexpected error: panic in function implementation: interface conversion: interface {} is *cty.unknownType, not bool

I wanted to check with you to see if this is expected. I don't really understand the usage of UnknownVal.

[alisdair]

Ah, that failure makes sense—you can't attempt to convert an unknown value to bool.

Generally we want to propagate unknownness. If the list contains unknown values, then it is unknown if all of them are true—so we defer that decision until they become known by returning an unknown value of the correct type. In this case, that means that the return value should be cty.UnknownVal(cty.Bool) with no error, not cty.False.

You can use v.IsKnown() to determine the unknownness of each element value.

[artburkart]

Cool. The test case has been updated to look like this:

		{
			cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
			cty.UnknownVal(cty.Bool),
			false,
		},

And the code looks like this now:

    tobool := MakeToFunc(cty.Bool)
    for it := args[0].ElementIterator(); it.Next(); {
      _, v := it.Element()
+     if !v.IsKnown() {
+       return cty.UnknownVal(cty.Bool), nil
+     }
      got, err := tobool.Call([]cty.Value{v})
      if err != nil {
        return cty.False, nil
      }
      eq, err := stdlib.Equal(got, cty.True)
      if err != nil {
        return cty.NilVal, err
      }
      if eq.False() {
        return cty.False, nil
      }
    }
    return cty.True, nil

What is a scenario where we might run into this? During a terraform plan?

[alisdair]

Yes, when an element which is part of the input to the function is computed and can't be known until during apply.

[mildwonkey]

🤔 I have a concern about the name function, and I'd like to hear what others think. It's not intuitive (to me) that functions named any and all would evaluate the existence of bools in a list. I know that python has the same functions, but I worry that it wouldn't be intuitive for most terraform users.

Before looking at the code (and before remembering that python has such functions), I expected that the function would take a second parameter that is the value you are looking for in the list:

all(var.list, true)
all(var.list, false)

I'm not necessarily opposed to the functionality, but I'm concerned by this name. any and all are broad terms (for users who don't know python), and this is a very specific function.

---

For what it's worth, you can get the same result with contains() and a conditional (which admittedly requires a bit more code):

 validation {
    condition = contains([
      for ami in var.amis: 
        length(ami.id) > 4 && substr(ami.id, 0, 4) == "ami-"
    ], false) ? false: true
    error_message = "The ID of at least one AMI was invalid."
  }

This returns false (which is to say, validation failed) if the list contains any falses.

[seh]

There's a long history of names in families of functions like these.

• Common Lisp
  • every
  • some
  • notevery
  • notany
• Clojure
  • every?
  • some
    Where's that question mark suffix?
  • not-every?
  • not-any?
• Ruby
  Missing is the converse of any?: Are there any that don't satisfy the predicate?
  • all?
  • any?
  • none?

[artburkart]

I know that python has the same functions, but I worry that it wouldn't be intuitive for most terraform users.

You're spot on the money in the assumption that I based this implementation verbatim on how Python implements "all", and I have another branch lying around on my machine that implements the corresponding "any" function as well. I didn't implement them both in the same PR, because I didn't want to muddy the waters with implementation details of two separate functions. I think my implementation of "any" leverages Terraform's "contains" function. I'd have to double check.

I agree that it would be neater if the all function took some sort of predicate. In Haskell, the all function takes such a predicate, and you're able to do things equivalent to this (please pardon my pseudo-HCL):

> all([1,3,5,7,9], "<10")
false

and this:

> any([0,1,2,3,4,5], "== 1")
true

I had thought about this some, and thought it would be really cool if we could do this, but I didn't know of a good way to implement such a thing. We don't have any functions in Terraform that take strings that are meant to be interpolated, do we? I could follow that model if we do.

In the absence of such interpolated syntax, I figured we could use Terraform's list comprehension syntax to achieve something equivalent:

all([for i in [1,3,5,7,9] : i < 10])

any([for i in [0,1,2,3,4,5] : i == 1])

[apparentlymart]

It is possible in theory to do some funny tricks with function arguments where they can be evaluated in unusual ways... the can and try functions are doing that, for example, so that they can handle errors in a different way than would be typical for the language.

However, I'm not sure how best to employ that capability here, because a callback function like this would require defining a new symbol (like the variable declarations after the for keyword do in a for expression) but there's no direct HCL syntax for defining what you might think of as a lambda/arrow function in another language, and implicitly defining a new variable with a hard-coded name would be tricky because Terraform's static dependency analyzer would think it was an attempt to reference a resource of that name, due to having no syntactic prompt that the symbol is being shadowed by a local declaration.

Using a for expression to produce an array of bools was the compromise I had originally imagined here, because (as you've shown in your comment) that already includes a means to define a local value and produce a value for each element, though I share the concern that it might not be obvious what it means to a reader that isn't already very familiar with Terraform and has already looked up what the all function does.

Perhaps we could compromise by giving them a more specific name, making it slightly less concise but also hopefully giving an extra clue as to what's going on. For example, we could call them alltrue and anytrue, so that what it's testing for is embedded in the name:

alltrue([for i in [1,3,5,7,9] : i < 10])
anytrue([for i in [0,1,2,3,4,5] : i == 1])

What do you all think about that compromise?

(This continues the existing precedent of just ramming multiple lowercase words together without any intervening underscores, which is a historical convention from early Terraform that we don't love today but that we've remained consistent with so that it's not necessary to remember which functions use underscores and which do not.)

[mildwonkey]

@apparentlymart I won't fight against any and all, if that's what we decide makes most sense, but I do like your suggestion of anytrue and alltrue. We generally prioritize readability of terraform configuration (and can't assume that folks reading config are programmers), and I think based on that they are better options.

(EDIT: I just read this back to myself, and it feel like I'm explaining terraform to my own lead. I'm not, just writing the full thought out, but sorry if it sounds bad).

[artburkart]

The branch is rebased on master and the function name is changed from "all" to "alltrue". There is still an outstanding question from @alisdair, and a response has been given. Once the question is addressed, will be happy to make the final adjustments. Thanks, team, for discussing this!

[alisdair]

Updates look great, thanks for being so responsive!

[artburkart]

Thanks!

I'll follow up with the corresponding anytrue function soon. Do you think it's possible to get both to land in the same release?

[alisdair]

Yes, I'd expect so! Everything being merged now is targeting 0.14.0, the first beta is several weeks away, and I imagine the anytrue PR will be an easy one to review.

[artburkart]

I coded up an implementation for anytrue, but when I started writing the commit message for it, I couldn't think of a scenario where anytrue would be a valuable function to introduce. Every scenario I contrived seemed to be just as succinctly addressed with a for expression paired with the length function or contains function.

Even in the example I provided earlier, this appears to be true.

anytrue([for i in [0,1,2,3,4,5] : i == 1])

vs

contains([0,1,2,3,4,5], 1)

vs

length(for i in [0,1,2,3,4,5] : i==1) > 0

I still maintain that the introduction of the alltrue function makes configuration clearer, but the anytrue function appears to be a less versatile contains function.

Thoughts?

[apparentlymart]

Thanks for raising that, @artburkart!

In the earlier discussion I got the sense that the alltrue function was, in a sense, a dynamic version of the && operator for situations where the number of tests isn't known until runtime, often because a test is being run for each element of a collection.

With that framing, anytrue could be considered to be the corresponding dynamic version of the || operator, but indeed that doesn't answer the question of whether there's actually a use case for a "dynamic OR" that isn't already better served by a more specialized function.

With that said, the trick of comparing the length of the result of a for expression is something I'd ended up employing for an alltrue use-case before too (I compared the length of the result to the length of the original input), and I found myself preferring alltrue because it seemed to me to be more direct statement of the desired intent. I've not yet encountered a situation where I've needed the > 0 variant of that, as you showed here, because I've also found that the contains function has been a more concise way to express what I wanted to say. But if I did encounter such a situation, I'd rather use anytrue than the length test you showed here, because it's a more direct representation of the intended meaning.

If you've already written it and the implementation is relatively straightforward then I wouldn't mind merging it since it seems like it would not represent a significant additional maintenance burden, and it seems relatively easy to describe it in the documentation. Would you mind opening a new PR for what you implemented and then we can talk about the pros and cons of introducing it in that PR?

[artburkart]

I've submitted a draft PR here: #26498
👍

[ghost]

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.