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

Pre-flight Check on Resource Creation #4170

Closed
apparentlymart opened this issue Dec 4, 2015 · 4 comments
Closed

Pre-flight Check on Resource Creation #4170

apparentlymart opened this issue Dec 4, 2015 · 4 comments

Comments

@apparentlymart
Copy link
Contributor

A large number of resources within Terraform create objects that must have unique names, or other such constraints that can't be checked without calls to a backing API.

For example, aws_s3_bucket takes a name that must be globally-unique across the whole of AWS. Several other AWS resources such as aws_db_instance require account-local or account-region-local identifiers to be provided by the user.

When applying a config containing a new aws_s3_bucket instance, the plan stage will accept any syntactically-valid name, but when the plan is applied a dynamic error can be raised saying that the bucket name is in use.

It is always frustrating when an error arises during apply, since it leaves the system in an incomplete state. It is impossible to completely prevent such issues, since the world continues to change around Terraform in the time period between plan and apply, but this proposal aims to reduce the likelihood of such errors by allowing resources to optionally perform a "pre-flight check" for a create diff.

The intent of the proposal is that the conflicting S3 bucket name would be caught during plan, so it would look like this from the user's perspective:

$ terraform plan
... yada, yada, yada ...

Error:
* aws_s3_bucket.foo: A bucket named 'foo' already exists.

It would be nice, though perhaps unfeasible, to also be able to check references to ids that are presented literally in the configuration vs. interpolated from other resources. For example, the hard-coded instance type and AMI id in the following example could in theory be checked via AWS API calls and validated at plan time:

resource "aws_instance" "foo" {
    instance_type = "t12.micro"
    ami = "ami-12345"
}
$ terraform plan
... yada, yada, yada ...

Error:
* aws_instance.foo: "t12.micro" is not a valid instance_type. For currently-valid instance types, see:
    http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-types.html

...but that is not possible when either of them contain interpolations from resources that are not yet created. It is less likely that an id derived from another Terraform-managed resource would be incorrect, but the this difference in behavior may be difficult to explain and justify to users.

Implementation Ideas

Lightweight Implementation

It is possible to implement something like the above UX with only minimal changes to the provider plugin interface by making use of the existing Refresh method.

With this approach, any time plan encounters a resource that does not yet exist in the state, it would call Refresh on that resource with an empty-string instance id. If the returned, updated instance state has an id set, then we treat it as the resource already existing and return an error.

helper/schema would then be altered so that the provider implementation can opt-in to being "pre-refreshed". When Refresh is called with an empty id, helper/schema would check this flag and then:

  • If it's set, call the Read implementation for the resource with a ResourceData whose id is empty and the attributes populated with the subset of the configuration that is non-computed. The provider implementation is then expected to call d.SetId(...) with a non-empty id if it detects that the resource already exists.
  • If it's not set, schema/helper immediately returns an empty instance state with no id set, signalling that the resource does not yet exist. The resource implementation will therefore not have to be altered to deal with this new "empty id" scenario.

This approach is attractive because it builds on the existing Refresh/Read concepts. However, it has the limitation that providers can't customize the error message returned when a resource already exists, which would require a more generic error message:

$ terraform plan
... yada, yada, yada ...

Error:
* aws_s3_bucket.foo: A resource with id 'foo' already exists.

...where "foo" here is the id from the InstanceState. In this case it's a reasonable id, but in other cases like aws_route53_record it's an opaque composite of various attributes that is less user-friendly.

This approach also doesn't allow for other kinds of pre-create check, such as the ami and instance_type checks described above.

More-comprehensive Implementation

Adding a new CheckCreate function to the provider interface would allow providers to implement arbitrary checks against any non-computed value in the configuration, like the instance_type and ami cases in the motivation above.

The model here is trickier and requires more thought, since currently the ResourceData model in helper/schema expects that all computed values have been dealt with, and so it's not clear how the non-yet-resolved configuration would be presented to the provider to check.

@phinze
Copy link
Contributor

phinze commented Dec 7, 2015

Yep we've talked a bunch about adding richer plan-time checks and I think this proposal is a good start to moving this forward.

I think it's definitely better to add a first-class concept for this rather than piggybacking onto refresh. I have always thought of this as a "resource level Validate" myself, so I wonder if there's room for adding this into the validateWalk we do. Would require some investigation, since providers need to be configured to perform this level of validation, which would be a change from existing validate implementation IIRC.

@apparentlymart
Copy link
Contributor Author

Doing API requests during validateWalk seems a bit heavy, so I think I'd lean towards keeping validateWalk for light syntax-ish checks and instead saving this heavier semantic validation for planWalk.

A more specific idea of how this could work, having mulled it over more in the mean time:

  • Terraform produces a plan as normal, by diffing the state against the config.
  • A new provider function ValidatePlan is called, which helper/schema handles by creating a ResourceData populated from the plan and passing it into a ValidatePlan callback on the resource.
  • The ValidatePlan implementation can use d.Get and d.GetChange to see both the new values and the diff, thus allowing it to verify both. (For example, it might be valid to increase a particular number but not to decrease it.)
  • If ValidatePlan returns an error, plan creation fails. Otherwise it proceeds as normal.

Definitely still some details to figure out, though:

  • What do we do if there are still unresolved interpolations in the resource configuration?
  • Do we need to add new hooks so that the UI can explain that it's validating a resource? Will all the extra chatter make terraform plan feel slower/heavier/scarier?

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Mar 29, 2017

#8769 has since subsumed this proposal, so I'm going to close this one.

@ghost
Copy link

ghost commented Apr 14, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants