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

Feature request: raise(error_msg) function #24269

Closed
aaronsteers opened this issue Mar 4, 2020 · 15 comments
Closed

Feature request: raise(error_msg) function #24269

aaronsteers opened this issue Mar 4, 2020 · 15 comments
Labels
config custom-conditions Feedback on variable validation, preconditions. postconditions, checks, and test assertions enhancement

Comments

@aaronsteers
Copy link

aaronsteers commented Mar 4, 2020

UPDATE (2): Terraform have opened a 'discuss' topic for this capability: https://discuss.hashicorp.com/t/terraform-core-research-ability-to-raise-an-error/35818

UPDATE: This item is not yet prioritized/approved from Hashicorp, but we now have PRs open: hashicorp/hcl#384 for the HCL components and #25088 for Terraform. Please watch and vote those as well. Thank you!


Current Terraform Version

Terraform v0.12.21

Use-cases

  • To provide simple and robust error handling in a way that's easy to read and debug
  • To provide compatibility with DRY principles - in general, but also specifically as it relates to effective use of locals working variables

Attempted Solutions

  • validation at the variable level is too early - they do not have access to other input variables or to calculated interim variables (locals in terraform-speak)
  • failures at the resource level are too late - they are rarely meaningful or helpful in debugging, and they cannot expose things early on in a module such as user most provide 'var_a' or 'var_b', but not both.

Proposal

TL;DR:

  • a new function raise() which accepts a single error_msg argument and which alerts the user of the failure, printing the provided error message (and the same normal call stack info for other errors, per usual)

Details:

  • The proposal is simple but also very robust when combined with if-then statements and the new try() and can() functions.
  • The function could be used:
    1. inline with if-then-else statements: {the-world-makes-sense} ? {something-sane} : raise("something is definitely wrong")
    2. as the final statement in a try() function: try({this-often-fails}, raise("Here's is why it failed: 'reasons'."))
    3. as the final statement in a coalesce() statement to handle non-nullables: coalesce({first-attempt}, {second-attempt}, ..., raise("Oh geez, I have no value. Danger, Will Robinson!"))
    4. in combination with can() in an if-then-else statement to catch otherwise hard-to-define errors: can(100/{denominator}) ? {denominator} : raise("Gah - something is wrong! Did you pass a denominator of zero???")
    5. in combination with interpolation (optionally) to provide very specific messages: contains(my_map, my_key) ? my_map[my_key] : raise("Error. The key '${my_key}' is suspiciously absent")

Benefits:

  • Importantly, the function just "feels" right and is stylistically in line with the new (and very helpful) try() and can() functions. It encourages easy-to-read code, and it does not require any new complex structures or new syntax to be memorized.
  • At least functionally (if not stylistically), it meets all use cases of the new experimental variables validation, and could indeed fully replace that functionality - since anything that can be validated at the variable level can also be validated at the locals level. (Not saying it should replace it, just that it does not appear there are any functional validation requirements that the variable-level solution meets which this could not also meet.)
  • Architecture-wise, there are no new dag-complexity issues to resolve. Whereas embedding these validations directly into input variable contructs adds the potential for cyclical dependencies - since a common case is that 'a or b should exist but not both' or 'a should be contained in b, but not in c'. All of the potential nightmares of interdependency are erased because the validation happens one-step removed in the dependency DAG: either in a 'locals' block or in the input to another module or resource that's already clearly "downstream" from the variables themselves.

References

@jbardin jbardin added the config label Mar 4, 2020
@apparentlymart apparentlymart added the custom-conditions Feedback on variable validation, preconditions. postconditions, checks, and test assertions label Mar 4, 2020
@aaronsteers aaronsteers changed the title Feature request: function raise({error_msg}) Feature request: raise({error_msg}) function Mar 4, 2020
@aaronsteers aaronsteers changed the title Feature request: raise({error_msg}) function Feature request: raise(error_msg) function Mar 4, 2020
@aaronsteers
Copy link
Author

aaronsteers commented Mar 5, 2020

The question came up on #24223 if this would allow multiple distinct error messages. Below is my response based on the spec - at least in theory. Someone from TF team would need to confirm if this is correct, but I believe this proposal does give the option of providing multiple error messages for the same input variable, with multiple tests all being able to be "reached" as long as they are not downstream from one another.


Yes - that should work for what I was looking for. Also, is there a limit on the number of raises...? Can I raise more than once (provided I do so in a separate block?)

In theory, I expect that this code would produce two separate errors since both "Calc A" and "Calc B" can be reached, and they both raise an error:

locals {
  my_int    = "asdf"
  my_calc_a = try(local.my_int * 5, raise("Can't perform calc A. Is the int really an int?")
  my_calc_b = try(local.my_int / 5, raise("Can't perform calc B. Is the int really an int?")
}

While this would produce a single error on "Calc A":

locals {
  my_int    = "asdf"
  my_calc_a = try(local.my_int * 5, raise("Can't perform calc A. Is the int really an int?")
  my_calc_c = try(local.my_calc_a * 5, raise("Can't perform calc C. Is the int really an int?")
}

Because Calc C depends on an already failed value ("Calc A"), there's no way to execute it and it would not report an error.

@aaronsteers
Copy link
Author

aaronsteers commented Mar 5, 2020

If it were desirable in the above case to only ever receive a single error message, you could accomplish that in this way:

locals {
  my_int    = "asdf"
  validated_int = (
    can(local.my_int *5) ? 
    local.my_int : 
    raise("Can't perform any calculations on my_int. Is the int really an int?")
  )
  my_calc_a = try(local.validated_int * 5, raise("Can't perform calc A. Is the int really an int?")
  my_calc_b = try(local.validated_int / 5, raise("Can't perform calc B. Is the int really an int?")
}

Although the above is error-handling overkill, there are cases where it would make sense to take this approach, and the fact that calculations A and B reference validated_int in this example means that they would only raise a single error message. If we change calculation B to used the int as a denominator instead of a numerator, then it's still possible that if passed 0 (zero) the validation would pass it's initial test, pass Calc A and then fail on Calc B. And I think this is also in line with how we'd expect errors to be handled.

@apparentlymart
Copy link
Contributor

Hi @aaronsteers,

We are planning to do some deeper thinking about this overall proposal, but for now just to address your direct question in your second comment:

Because functions are executed only during expression validation, and expression validation is part of Terraform's graph walk, it's unfortunately undefined whether something like the example above would produce one error or two, because it depends on how the concurrent graph walk plays out:

  • If and only if Terraform were to start evaluating local.my_calc_a and local.my_calc_b concurrently, they could both potentially fail and produce two error messages. However, given that local value evaluation is entirely computation and therefore relatively fast, there is only a very small window for those two things to happen concurrently.
  • More likely, one of them would get an opportunity to evaluate first, and if it were to then produce an error Terraform would halt walking the graph once any pending nodes have finished evaluating, preventing the other one from getting a chance to generate an error.

I expect this is also true for the existing variable-specific validation experiment, because it also happens during the validation graph walk. Anything that explicitly depends on the failing variable would not get a chance to generate any additional errors, and a pair of variables that do not depend on one another at all could potentially evaluate concurrently, but probably would not.

It may be possible to mark these errors (in both cases) as a sort of "soft error" that doesn't prevent the graph walk from continuing. I expect that's marginally harder to do for the raise(...) function proposal than the existing variable values mechanism because an error emerging from a function evaluation has to go through a lot more indirection and machinery before it reaches the graph walk code, and so it would likely require some non-trivial refactoring to bring that "soft error" metadata out along with the error. However, I've not investigated either case in any detail yet, so I may very well be wrong about that.

@aaronsteers
Copy link
Author

aaronsteers commented Mar 5, 2020

@apparentlymart - Great! Thanks so very much for the update, and I'll look forward to updates after team discussions. Regarding the answer and clarification of how many errors are generated, this is likewise very helpful. The response basically confirms my expectation that:

  1. multiple error messages in theory are possible but not guaranteed (depending upon implementation details and other technical matters of the particular instance)
  2. regardless of those details though, hiding one validation behind another (such as the validated_int example above) would still be an effective way to minimize redundant errors when desired - since the graph walk would effectively stop at the point of already failed variable declaration.

No surprises here, per se, and I still think this meets most/all the my own desired validation requirements - and most of the main use cases I've been able to understand from the related threads.

@dmrzzz
Copy link

dmrzzz commented Mar 5, 2020

FWIW I (and I suspect many others) would be very happy to use this in practice even if the two-reachable-errors case remains nondeterministic. In the relatively infrequent case where I'm simultaneously doing two separate things wrong, I don't mind having to correct them one at a time.

@aaronsteers
Copy link
Author

Hello, @apparentlymart - just wanted to check in on this. I find myself wishing for / dreaming of a raise() function on almost a weekly basis now - and how much they could improve the usability of our terraform modules. Is there any chance this may be added in the next terraform release?

@DanielFallon
Copy link

DanielFallon commented Apr 30, 2020

commenting here to help others until this is implemented:
As a workaround that produces a relatively clear output value, I started using this specific version of the the regex workaround:

locals {
  error = regex((<boolean_conditional>) ? "(INTENTIONALLY RAISE ERROR":""),"<Error message text>")
}

It produces the still awkward but definitely clear error message that looks like this:

on variables.tf line 20, in locals:
  20:   error = regex((<boolean_conditional>) ? "(INTENTIONALLY RAISE ERROR":""),"<Error Message Text>")
    |----------------
    | <variable name> is "<variable value>"

Invalid value for "pattern" parameter: invalid regexp pattern: missing closing
) in (INTENTIONALLY RAISE ERROR.

I felt like creating a scenario where the regex parser fails with the characters (INTENTIONALLY RAISE ERROR in its buffer ensured that the regex error is just nonsensical enough to not be misidentified.

@aaronsteers
Copy link
Author

Many have posted workarounds that get Terraform to "appear" as if it has this functionality without actually having it.

Any chance we can include this with the 0.13.0 release ( #25016 ) ? 🙏

@aaronsteers
Copy link
Author

aaronsteers commented May 31, 2020

@apparentlymart - I finally had some time to work on this and I understand golang well enough now to start contributing. Do you mind taking a look at these PRs:

Ditto - to anyone else who knows golang and/or the Terraform/HCL platform, I would welcome feedback and/or contributions on the above.

@tomasbackman
Copy link

I don't really know the stuff good enough yet so can't really contribute on this, other than to say, nice @aaronsteers! This would be a good feature I hope this or something very similar gets added soon!

@aaronsteers
Copy link
Author

@apparentlymart, @jbardin - Could I please request a review on the HCL PR here: hashicorp/hcl#384

The code is feature complete and tests are passing. Would kindly appreciate your review and feedback. Thanks very much!

@brandon-fryslie
Copy link

This feature would lead to vastly improved Terraform code. Currently we're just hacking around this issue by trying to read a non-existent file. It's really a bummer because Terraform is so clean and and elegant except for a few cases like this

@timblaktu
Copy link

+1 what @brandon-fryslie said. I would like to be able to assert in a way that doesn't fail terraform validate runs (which happens with my file("invalid file name") hack.)

@apparentlymart
Copy link
Contributor

Hi all!

It looks like we missed this one when we were looking for issues related to the use-cases that motivated the Custom Condition Checks features in Terraform v1.2. The design effort which led there caused us to conclude that explicitly monitoring conditions as side-effects met the apparent requirements better than what was achievable with just a function, and so the final design uses conditions associated with resources and output values (both of which already have explicit side-effects for the conditions to guard) rather than the function design proposed here.

Because of that, I'm going to close this issue. If you are using Terraform v1.2 or later, you can use Preconditions and Postconditions to cause Terraform to return an error if some arbitrary condition doesn't hold.

Thanks for proposing this, and sorry we missed closing it after the v1.2 release!

@apparentlymart apparentlymart closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2022
@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 Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
config custom-conditions Feedback on variable validation, preconditions. postconditions, checks, and test assertions enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants