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

Ability to prevent taints on create errors #330

Open
danawillow opened this issue Feb 19, 2020 · 8 comments
Open

Ability to prevent taints on create errors #330

danawillow opened this issue Feb 19, 2020 · 8 comments
Labels
enhancement New feature or request upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol

Comments

@danawillow
Copy link
Contributor

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk",
  "Version": "v1.4.0"
}

Use-cases

Some resources in the Google provider have multiple steps that happen on create. For example, google_project creates a project, but then it also adds a billing account to it and potentially deletes a network from the project. If the project gets created successfully but one of the other steps fails, the resource is tainted, which means retrying the apply after fixing whatever caused it to fail will destroy the project and recreate it, even though it's a perfectly good project. See terraform-google-modules/terraform-google-project-factory#373 for further discussion.

Attempted Solutions

d.SetPartial seems vaguely relevant to what we want, but it's been deprecated (and I'm not positive that it would have actually worked).

Proposal

Basically anything where we, in the provider code, can tell Terraform to please not taint the resource (just like how a failed update doesn't taint it).

References

hashicorp/terraform#21652 (which was solved in a different way)

@danawillow danawillow added the enhancement New feature or request label Feb 19, 2020
@paultyng
Copy link
Contributor

paultyng commented Feb 20, 2020

I'm not sure if it would work in this case, but would the ability to just return a warning and not an error solve your need? The warning lets you persist the state without a taint but still message to the users (FYI 0.12 diagnostics/warnings are not exposed directly in the SDK yet but are on our near term list to do so, see #74).

@paultyng
Copy link
Contributor

I guess in this case you probably want to halt further downstream usage of computed attributes though, which a warning will not do.

@paultyng paultyng self-assigned this Feb 20, 2020
@danawillow
Copy link
Contributor Author

Exactly- a warning would be a very good solution, but not a fully comprehensive one. This FR becomes more of a nice-to-have if warnings are coming down the road, though. Looking forward to that :)

@alexng-canuck
Copy link

alexng-canuck commented Feb 21, 2020

Just as an FYI, we have also noticed in the OCI provider that tainting on create errors leads to other side-effects if the tainted resource no longer exists in the service. Upon a destroy, the refresh operation invalidates the resource, but then causes config validation errors if other downstream resources were dependent on the tainted resource: hashicorp/terraform#23886

@alexng-canuck
Copy link

After we have called SetId on a resource, is there a reason why it must be tainted even for create errors? What purpose does the tainting serve?

A warning instead of error would be nice.

Not sure if it would cover the situation where we must inform the user with a terminating error that something went wrong and that the Terraform apply shouldn't make any progress, but we still want Terraform to track the resources that may have been created (for a future destroy).

@paultyng
Copy link
Contributor

@alexng-canuck the main problem as I understand it is that provisioners haven't been run and there is nowhere that information is tracked, as they are just run for create, so we'd need a mechanism to track that info to make this work without the taint I think.

@damodhar22
Copy link

What is the status on this enhancement. We have a similar requirement to conditionally avoid taint on create errors. As our create operation is an asynchronous operation, it first returns the id and we poll on it with id for the status.
Once we have the id of the resource we do not want to taint it even if the operation fails. We still want to send error for create operation, but we do not want to taint the resource.

fho added a commit to simplesurance/terraform-provider-bunny that referenced this issue Jan 31, 2022
When a new hostname with a certificate was created and uploading the certificate
failed, the local state was invalid.
The hostname was created successfully at bunny.net but it was missing from
the state. This resulted in hostname already exist errors on following
applies for the hostname.

Change the provider to not return an error immediately when uploading a
certificate fails. Following independent API operations for the same hostname
are still run, the state is set according and at the end the error about the
failed certificate upload is returned.
This results in a correct state.
The hostname resource will still be tainted by Terraform. This has the unwanted
effect that also when the broken certificate section is removed and therefore
the state and definition are the same the hostname resource is unnecessary
recreated. This can prevented manually by the user by untainting the resource.
There seems to be no good solution for this kind of issue[1].
Returning a warning instead of an error will result in a misleading success and
the warning would be easy to miss.

- [1]: hashicorp/terraform-plugin-sdk#330.
fho added a commit to simplesurance/terraform-provider-bunny that referenced this issue Jan 31, 2022
When a new hostname with a certificate was created and uploading the certificate
failed, the local state was invalid.
The hostname was created successfully at bunny.net but it was missing from
the state. This resulted in hostname already exist errors on following
applies for the hostname.

Change the provider to not return an error immediately when uploading a
certificate fails. Following independent API operations for the same hostname
are still run, the state is set according and at the end the error about the
failed certificate upload is returned.
This results in a correct state.
The hostname resource will still be tainted by Terraform. This has the unwanted
effect that also when the broken certificate section is removed and therefore
the state and definition are the same the hostname resource is unnecessary
recreated. This can prevented manually by the user by untainting the resource.
There seems to be no good solution for this kind of issue[1].
Returning a warning instead of an error will result in a misleading success and
the warning would be easy to miss.

- [1]: hashicorp/terraform-plugin-sdk#330.
@bflad bflad added the upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol label Mar 30, 2022
@bflad
Copy link
Contributor

bflad commented Jan 23, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol
Projects
None yet
Development

No branches or pull requests

5 participants