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

Experimental custom validation rules for input variables #23794

Merged
merged 4 commits into from
Jan 10, 2020

Conversation

apparentlymart
Copy link
Contributor

This set of commits introduces, as an opt-in experiment only for now, the capability to write custom validation rules for input variables in a module:

variable "ami_id" {
  type = string

  validation {
    condition     = can(regex("^ami-", var.ami_id))
    error_message = "Argument ami_id must be a valid AMI id, starting with \"ami-\"."
  }
}

The condition argument inside this new validation block takes an expression that must return either true to indicate that the value is valid, or false to indicate that it is invalid. To avoid introducing new graph edges, condition is constrained such that it can refer only to the variable that is being validated, which is var.ami_id in the example above.

By attaching the validation rule directly to the variable, Terraform can therefore report the given error in context, identifying the value provided by the caller as the subject of the error:

Error: Invalid value for variable

  on example.tf line 5, in module "example":
   5:   ami_id = "amo-abc123"

Argument ami_id must be a valid AMI id, starting with "ami-".

This was checked by the validation rule at modules/example/variables.tf:7,3-13.

I'm introducing this as an opt-in experiment using our new language experiments mechanism, both so that we can make this easier for folks to try out and give feedback and also as a first iteration of using our language experiments process with a relatively self-contained language feature so we can hopefully iron out any process difficulties before we try to use it with more complex language improvements.

Consequently the new validation block will, for now, be accepted only in modules with an explicit experiment opt-in:

terraform {
  experiments = [variable_validation]
}

The intent of this mechanism is to make it very clear that this is not yet a stable language feature and it's subject to breaking changes even in minor releases until it's finalized. Our hope, then, is that folks will give it a try in an experimental way and give feedback that may allow the feature to be adjusted and improved prior to final release. (If you are reading this because you do have feedback after this has been included in a release, please open a new issue to record your feedback, rather than adding comments to this one, so that we can keep a separate issue for each thread of discussion.)

Unless subsequent feedback and discussion raises a show-stopper for the design of the feature, we can hopefully make it stable (not requiring the experiment opt-in) in a future release.


This also includes two related enhancements:

  • A can function to allow using the success or failure of an expression as a validation condition. I expect this will be the most concise way to express a lot of validation conditions, because validation is primarily used to guard against similar errors downstream and give better feedback to the module caller: we can use it to say "if this operation would fail downstream, return an error early here instead".
  • A try function, which is not directly in service of validation but uses the same underlying techniques as can to allow concisely writing normalization expressions that adapt arbitrary data derived from elsewhere (like JSON/YAML from outside sources) for use in a Terraform configuration.

The documentation pages for these functions, included as part of this PR, give more information on the usage and intended use-cases for each of the functions. These two functions are not part of the experiment, but are bundled in here because can is important for the usability of validation blocks.

@apparentlymart apparentlymart requested a review from a team January 6, 2020 21:51
@apparentlymart apparentlymart self-assigned this Jan 6, 2020
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Jan 6, 2020
@apparentlymart
Copy link
Contributor Author

The test failure here seems to be a bug upstream in cty, so I'll take a closer look at that another time. However, I would still appreciate review of the Terraform changes here, since the upstream cty fix is unrelated to what I was working on here and should not change anything about this PR except for which cty release is vendored.

// in the module defining the validation rule, not an error in the caller.
Condition hcl.Expression

// ErrorMessage is one or more full sentences, usually in English for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually in English

I'm going to nitpick here, but my more recent experience writing terraform code was for a team that I'm confident will write error messages in Dutch.

There's no guarantee that a user will want to write their error messages in english. It might be more accurate to indicate that it's assumed to be english and looksLikeSentences will complain if it doesn't like the punctuation.

I feel rather (oddly) strongly about this, even though it's a tiny detail, but not at all in a blocking way if you disagree (super helpful, I'm sure!).

Copy link
Contributor Author

@apparentlymart apparentlymart Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "usually in English" here is coming from the fact that a message written in Dutch would end up looking like this in the UI (though presumbly written in correct Dutch, not this Google Translate mess for the sake of example):

Error: Invalid value for variable

  on example.tf line 5, in module "example":
   5:   ami_id = "amo-abc123"

Het argument "ami_id" moet een geldige AMI-ID zijn, beginnend met "ami-".

This was checked by the validation rule at modules/example/variables.tf:7,3-13.

I'm guessing that as long as Terraform's own UI is only available in English the UI of most modules will be in English too. There's no code here preventing you from writing messages in other languages of course (as long as they use the sentence-terminal punctuation we're expecting), if the author is happy to see some non-English text mixed in with predominately-English. The "usually" is an assumption about how it will be commonly used, not a usage requirement, and I wrote it here specifically to acknowledge that our i18n story for this feature (and Terraform as a whole) is poor.

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm excited about this new feature! I left some very nitpicky thoughts and questions, but they aren't meant to block you.

On the other hand, since (I believe) this shouldn't be merged as-is until the upstream cty bug is fixed and it wasn't clear if you planned on holding off until then, I'm going to leave the angry red comment of doom (Request Changes) for now. If I've misunderstood your intent I'll change it to an approval and let you handle it from there.

Tangential to the PR: what do you think about a HashiCorp blog post covering experiments and the new feature together? This is something we should show off!

// (This assumes that if a sentence ends with quotes then the period
// will be outside the quotes, which is consistent with Terraform's UI
// writing style.)
return last == '.' || last == '?'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include ! for completeness here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered it but excluded it because exclamation points are not really part of our usual UI writing style. I suppose it can't hurt, though. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thread above reminded me that there are currently no validations to fail, so this is enforcing a particular style form the start. I think this is perfectly fine since it's there's nothing to break 👍

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Jan 9, 2020

Indeed I don't plan to merge this without the cty fix, because I expect it would cause the terraform provider (and possibly other things too, not covered by tests here) to panic. Was just hoping to address that and the review feedback together in a single session, to minimize context-switching.

@apparentlymart
Copy link
Contributor Author

The upstream cty error here is captured as zclconf/go-cty#35.

@radeksimko
Copy link
Member

zclconf/go-cty#35 is now ready for review

This brings in the new HCL extension functions "try", "can", and
"convert", along with the underlying HCL and cty infrastructure that allow
them to work.
These are intended to make it easier to work with arbitrary data
structures whose shape might not be known statically, such as the result
of jsondecode(...) or yamldecode(...) of data from a separate system.

For example, in an object value which has attributes that may or may not
be set we can concisely provide a fallback value to use when the attribute
isn't set:

    try(local.example.foo, "fallback-foo")

Using a "try to evaluate" model rather than explicit testing fits better
with the usual programming model of the Terraform language where values
are normally automatically converted to the necessary type where possible:
the given expression is subject to all of the same normal type conversions,
which avoids inadvertently creating a more restrictive evaluation model
as might happen if this were handled using checks like a hypothetical
isobject(...) function, etc.
The existing "type" argument allows specifying a type constraint that
allows for some basic validation, but often there are more constraints on
a variable value than just its type.

This new feature (requiring an experiment opt-in for now, while we refine
it) allows specifying arbitrary validation rules for any variable which
can then cause custom error messages to be returned when a caller provides
an inappropriate value.

    variable "example" {
      validation {
        condition = var.example != "nope"
        error_message = "Example value must not be \"nope\"."
      }
    }

The core parts of this are designed to do as little new work as possible
when no validations are specified, and thus the main new checking codepath
here can therefore only run when the experiment is enabled in order to
permit having validations.
@radeksimko
Copy link
Member

I suspect this may close #2847 ?

@apparentlymart
Copy link
Contributor Author

@radeksimko it's related to a few different issues but I don't plan to close them until something related to this transitions from experimental to final. (Note that for this PR in particular it's an experimental feature requiring an opt-in, subject to breaking changes in future releases.)

@apparentlymart apparentlymart merged commit ff4ea04 into master Jan 10, 2020
@apparentlymart apparentlymart deleted the f-try-func branch January 10, 2020 23:23
@apparentlymart
Copy link
Contributor Author

If you have feedback on this experimental feature once it's released, please open a feature request issue, using the template to describe what you were trying to do and what problems you faced, if any.

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
config enhancement sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants