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

Private State is lost after a failed Deletion #863

Closed
AmadeusK525 opened this issue Oct 23, 2023 · 10 comments · Fixed by #864
Closed

Private State is lost after a failed Deletion #863

AmadeusK525 opened this issue Oct 23, 2023 · 10 comments · Fixed by #864
Assignees
Labels
bug Something isn't working
Milestone

Comments

@AmadeusK525
Copy link

AmadeusK525 commented Oct 23, 2023

Module version

v1.3.4

Relevant provider source code

func (r *MyResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
	result, err, d := performOperation(...)
	resp.Diagnostics.Append(d...)
	if err != nil {
	        return
	}

	d = applyState(&resp.State, result)
	resp.Diagnostics.Append(d...)
	if resp.Diagnostics.HasError() {
	        return
	}

	d = resp.Private.SetKey(ctx, createResultKey, result)
	resp.Diagnostics.Append(d...)
}

func (r *MyResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
        // Do stuff, has access to 'createResultKey' via req.Private.GetKey
}

func (r *MyResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
        // Do stuff, has access to 'createResultKey' via req.Private.GetKey
}

func (r *MyResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
        // This works fine on the first call to Delete
        createResult, d := req.Private.GetKey(ctx, createResultKey)
        resp.Diagnostics.Append(d...)
        if resp.Diagnostics.HasError() {
                return
        }
        
        result, err, d := performOperation(...)
        resp.Diagnostics.Append(d...)
        if err != nil {
                return
        }
        
        // Add a dummy error to signal that 'Delete' failed.
        resp.Diagnostics.AddError("dummy error", "this is a forced error")
}

Terraform Configuration Files

...

Debug Output

Expected Behavior

When saving private state resources, they should be accessible by the CRUD methods and should not be dropped/altered if the operation that modified them return with error diagnostics (just like normal states) for further use/re-use

Actual Behavior

If the Delete (I haven't tested other CRUD methods to see if the behavior is the same) call is made and fails (thus, the resource was NOT deleted), the Private state is completely wiped and can't be accessed in further calls to the CRUD methods

Steps to Reproduce

  • Create a dummy Resource and save some dummy data on the Private state in the CRU calls
  • In the Delete call, retrieve that Private State
  • Check if everything went correctly (it should have)
  • Force fail the Delete call
  • Make the Delete call again and see that retrieving the Private State will no longer work
@AmadeusK525 AmadeusK525 added the bug Something isn't working label Oct 23, 2023
@bflad
Copy link
Contributor

bflad commented Oct 23, 2023

Hi @AmadeusK525 👋 Thank you for reporting this and sorry you are running into trouble with this.

We'll need to confirm whether this is something the framework is causing and can be fixed in this repository or if Terraform core is dropping this data in this situation. I believe the framework should be automatically setting the ApplyResourceChange response private state data if the request private data is set:

if req.PlannedPrivate != nil {
deleteReq.Private = req.PlannedPrivate.Provider
}

If you're able, could you try running Terraform with the TF_LOG_SDK_PROTO_DATA_DIR environment variable set while the resource generates an error during destroy, to see if it leaves an ApplyResourceChange_Response_Private file? This would confirm that we are sending back the data to Terraform core and that we might need a fix upstream. Thanks!

@bflad
Copy link
Contributor

bflad commented Oct 23, 2023

😅 Looking at that code there right after I posted it, it actually is setting the request data but not the response data. We might need to decide the right thing to do in this case, but my hunch is that we might need to populate the data except when there are no error diagnostics from the provider logic.

To immediately fix your situation, you might want to temporarily have your provider logic copy the private state data from the request to the response.

@AmadeusK525
Copy link
Author

Hey @bflad, sorry for the lack of info on the issue, I posted it by accident prior to writing everything out lol I've been collecting the Trace output and was about to encrypt it and actually describe everything in the issue. Anyway, thanks for responding quickly, I'll get back to you in a sec

@AmadeusK525
Copy link
Author

Heyo @bflad, I tried running the deletion with the env var set but the dir was unchanged.
I did try setting the Private data in the DeleteResponse but not by copying, but by using SetKey again and it was still wiped. Maybe that's not enough? I'll try literally copying the state and see if it works

@AmadeusK525
Copy link
Author

Oh, DeleteResponse has no Private field. Do you mean copying the Private state from the DeleteRequest to the DeleteResponse's regular State?

@victor-accarini
Copy link

It has no Private field since we can't set private data in the Delete function. docs

Maybe this bug is an unintended side-effect of the rule?

@bflad
Copy link
Contributor

bflad commented Oct 23, 2023

It's possible that when we originally designed the feature, we expected Terraform to keep the private data in the case of an error diagnostic instead of needing the provider to return it, since that is what happens with "regular" state data. I reached out to the Terraform core folks and it does appear that the core logic is just always using the response private data field regardless of errors being returned.

To fix this what we'll likely do is:

  • Introduce a Private field to the resource.DeleteResponse type
  • Automatically copy any request private data to that response private data to prevent unexpected data loss
  • Then to preserve the current behavior and prevent other potential errors, if the provider logic returns without error diagnostic, empty out the response private data before responding back to Terraform

I'll tag this for a bug fix release sooner rather than later, given the data loss situation.

@bflad bflad added this to the v1.4.2 milestone Oct 23, 2023
@bflad bflad self-assigned this Oct 23, 2023
@AmadeusK525
Copy link
Author

Thanks a lot! 😄

bflad added a commit that referenced this issue Oct 23, 2023
Reference: #863

The previous implementation assumed that Terraform core would preserve private state similar to "regular" state when a provider returned an error diagnostic while destroying a resource. The core implementation instead always uses the response data, which the framework was previously always discarding. This change corrects the errant design while also enabling provider logic to potentially update the private state in this situation.
bflad added a commit that referenced this issue Oct 24, 2023
Reference: #863

The previous implementation assumed that Terraform core would preserve private state similar to "regular" state when a provider returned an error diagnostic while destroying a resource. The core implementation instead always uses the response data, which the framework was previously always discarding. This change corrects the errant design while also enabling provider logic to potentially update the private state in this situation.
@bflad
Copy link
Contributor

bflad commented Oct 24, 2023

Hi again 👋 terraform-plugin-framework v1.4.2 has been released which should resolve this issue. Please reach out in a new issue if you are still running into unexpected behaviors. Thank you!

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 Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
3 participants