diff --git a/.changes/unreleased/BUG FIXES-20231023-165712.yaml b/.changes/unreleased/BUG FIXES-20231023-165712.yaml new file mode 100644 index 000000000..5cf6594ca --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20231023-165712.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'resource: Add `Private` field to `DeleteResource` type, which was missing to + allow provider logic to update private state on errors' +time: 2023-10-23T16:57:12.447739-04:00 +custom: + Issue: "863" diff --git a/.changes/unreleased/BUG FIXES-20231023-165814.yaml b/.changes/unreleased/BUG FIXES-20231023-165814.yaml new file mode 100644 index 000000000..c69900c60 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20231023-165814.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'resource: Prevented private state data loss if resource destruction returned + an error' +time: 2023-10-23T16:58:14.585227-04:00 +custom: + Issue: "863" diff --git a/internal/fwserver/server_applyresourcechange_test.go b/internal/fwserver/server_applyresourcechange_test.go index 4539736b7..59e10d76c 100644 --- a/internal/fwserver/server_applyresourcechange_test.go +++ b/internal/fwserver/server_applyresourcechange_test.go @@ -638,6 +638,90 @@ func TestServerApplyResourceChange(t *testing.T) { }), Schema: testSchema, }, + Private: testEmptyPrivate, + }, + }, + "delete-response-private": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ApplyResourceChangeRequest{ + PriorState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + ResourceSchema: testSchema, + Resource: &testprovider.Resource{ + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + }, + }, + expectedResponse: &fwserver.ApplyResourceChangeResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "error summary", + "error detail", + ), + }, + NewState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + Private: testPrivateProvider, + }, + }, + "delete-response-private-updated": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ApplyResourceChangeRequest{ + PlannedPrivate: testPrivateFramework, + PriorState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + ResourceSchema: testSchema, + Resource: &testprovider.Resource{ + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + }, + }, + expectedResponse: &fwserver.ApplyResourceChangeResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "error summary", + "error detail", + ), + }, + NewState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + Private: testPrivate, }, }, "delete-response-newstate": { diff --git a/internal/fwserver/server_deleteresource.go b/internal/fwserver/server_deleteresource.go index a7c6ebd0a..01fdc635c 100644 --- a/internal/fwserver/server_deleteresource.go +++ b/internal/fwserver/server_deleteresource.go @@ -82,8 +82,18 @@ func (s *Server) DeleteResource(ctx context.Context, req *DeleteResourceRequest, deleteReq.ProviderMeta = *req.ProviderMeta } + privateProviderData := privatestate.EmptyProviderData(ctx) + + deleteReq.Private = privateProviderData + deleteResp.Private = privateProviderData + if req.PlannedPrivate != nil { - deleteReq.Private = req.PlannedPrivate.Provider + if req.PlannedPrivate.Provider != nil { + deleteReq.Private = req.PlannedPrivate.Provider + deleteResp.Private = req.PlannedPrivate.Provider + } + + resp.Private = req.PlannedPrivate } logging.FrameworkTrace(ctx, "Calling provider defined Resource Delete") @@ -91,10 +101,23 @@ func (s *Server) DeleteResource(ctx context.Context, req *DeleteResourceRequest, logging.FrameworkTrace(ctx, "Called provider defined Resource Delete") if !deleteResp.Diagnostics.HasError() { - logging.FrameworkTrace(ctx, "No provider defined Delete errors detected, ensuring State is cleared") + logging.FrameworkTrace(ctx, "No provider defined Delete errors detected, ensuring State and Priavate are cleared") deleteResp.State.RemoveResource(ctx) + + // Preserve prior behavior of always returning nil. + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/863 + deleteResp.Private = nil + resp.Private = nil } resp.Diagnostics = deleteResp.Diagnostics resp.NewState = &deleteResp.State + + if deleteResp.Private != nil { + if resp.Private == nil { + resp.Private = &privatestate.Data{} + } + + resp.Private.Provider = deleteResp.Private + } } diff --git a/internal/fwserver/server_deleteresource_test.go b/internal/fwserver/server_deleteresource_test.go index 5f7f13523..95024d9a1 100644 --- a/internal/fwserver/server_deleteresource_test.go +++ b/internal/fwserver/server_deleteresource_test.go @@ -81,12 +81,35 @@ func TestServerDeleteResource(t *testing.T) { TestProviderMetaAttribute types.String `tfsdk:"test_provider_meta_attribute"` } + testPrivateFrameworkMap := map[string][]byte{ + ".frameworkKey": []byte(`{"fk": "framework value"}`), + } + testProviderKeyValue := privatestate.MustMarshalToJson(map[string][]byte{ "providerKeyOne": []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`), }) testProviderData := privatestate.MustProviderData(context.Background(), testProviderKeyValue) + testPrivate := &privatestate.Data{ + Framework: testPrivateFrameworkMap, + Provider: testProviderData, + } + + testPrivateFramework := &privatestate.Data{ + Framework: testPrivateFrameworkMap, + } + + testPrivateProvider := &privatestate.Data{ + Provider: testProviderData, + } + + testEmptyProviderData := privatestate.EmptyProviderData(context.Background()) + + testEmptyPrivate := &privatestate.Data{ + Provider: testEmptyProviderData, + } + testCases := map[string]struct { server *fwserver.Server request *fwserver.DeleteResourceRequest @@ -245,6 +268,7 @@ func TestServerDeleteResource(t *testing.T) { }), Schema: testSchema, }, + Private: testEmptyPrivate, }, }, "resource-configure-data": { @@ -310,6 +334,89 @@ func TestServerDeleteResource(t *testing.T) { NewState: testEmptyState, }, }, + "response-private": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.DeleteResourceRequest{ + PriorState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + ResourceSchema: testSchema, + Resource: &testprovider.Resource{ + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + }, + }, + expectedResponse: &fwserver.DeleteResourceResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "error summary", + "error detail", + ), + }, + NewState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + Private: testPrivateProvider, + }, + }, + "response-private-updated": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.DeleteResourceRequest{ + PlannedPrivate: testPrivateFramework, + PriorState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + ResourceSchema: testSchema, + Resource: &testprovider.Resource{ + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKeyOne", []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + }, + }, + expectedResponse: &fwserver.DeleteResourceResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "error summary", + "error detail", + ), + }, + NewState: &tfsdk.State{ + Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + Schema: testSchema, + }, + Private: testPrivate, + }, + }, } for name, testCase := range testCases { diff --git a/internal/proto5server/server_applyresourcechange_test.go b/internal/proto5server/server_applyresourcechange_test.go index df7bc520f..92e0a1b24 100644 --- a/internal/proto5server/server_applyresourcechange_test.go +++ b/internal/proto5server/server_applyresourcechange_test.go @@ -696,6 +696,130 @@ func TestServerApplyResourceChange(t *testing.T) { }), }, }, + "delete-response-private": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.Resource{ + SchemaMethod: func(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + resp.Schema = testSchema + }, + MetadataMethod: func(_ context.Context, _ resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = "test_resource" + }, + CreateMethod: func(_ context.Context, _ resource.CreateRequest, resp *resource.CreateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Create") + }, + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKey", []byte(`{"key": "value"}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + UpdateMethod: func(_ context.Context, _ resource.UpdateRequest, resp *resource.UpdateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Update") + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov5.ApplyResourceChangeRequest{ + PlannedState: &testEmptyDynamicValue, + PriorState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + TypeName: "test_resource", + }, + expectedResponse: &tfprotov5.ApplyResourceChangeResponse{ + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "error summary", + Detail: "error detail", + }, + }, + Private: privatestate.MustMarshalToJson(map[string][]byte{ + "providerKey": []byte(`{"key": "value"}`), + }), + NewState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + }, + }, + "delete-response-private-updated": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.Resource{ + SchemaMethod: func(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + resp.Schema = testSchema + }, + MetadataMethod: func(_ context.Context, _ resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = "test_resource" + }, + CreateMethod: func(_ context.Context, _ resource.CreateRequest, resp *resource.CreateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Create") + }, + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKey", []byte(`{"key": "value"}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + UpdateMethod: func(_ context.Context, _ resource.UpdateRequest, resp *resource.UpdateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Update") + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov5.ApplyResourceChangeRequest{ + PlannedPrivate: privatestate.MustMarshalToJson(map[string][]byte{ + ".frameworkKey": []byte(`{"frameworkKey": "framework value"}`), + }), + PlannedState: &testEmptyDynamicValue, + PriorState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + TypeName: "test_resource", + }, + expectedResponse: &tfprotov5.ApplyResourceChangeResponse{ + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "error summary", + Detail: "error detail", + }, + }, + Private: privatestate.MustMarshalToJson(map[string][]byte{ + ".frameworkKey": []byte(`{"frameworkKey": "framework value"}`), + "providerKey": []byte(`{"key": "value"}`), + }), + NewState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + }, + }, "delete-response-newstate": { server: &Server{ FrameworkServer: fwserver.Server{ diff --git a/internal/proto6server/server_applyresourcechange_test.go b/internal/proto6server/server_applyresourcechange_test.go index 3b235252d..463371cb7 100644 --- a/internal/proto6server/server_applyresourcechange_test.go +++ b/internal/proto6server/server_applyresourcechange_test.go @@ -696,6 +696,130 @@ func TestServerApplyResourceChange(t *testing.T) { }), }, }, + "delete-response-private": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.Resource{ + SchemaMethod: func(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + resp.Schema = testSchema + }, + MetadataMethod: func(_ context.Context, _ resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = "test_resource" + }, + CreateMethod: func(_ context.Context, _ resource.CreateRequest, resp *resource.CreateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Create") + }, + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKey", []byte(`{"key": "value"}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + UpdateMethod: func(_ context.Context, _ resource.UpdateRequest, resp *resource.UpdateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Update") + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov6.ApplyResourceChangeRequest{ + PlannedState: &testEmptyDynamicValue, + PriorState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + TypeName: "test_resource", + }, + expectedResponse: &tfprotov6.ApplyResourceChangeResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "error summary", + Detail: "error detail", + }, + }, + Private: privatestate.MustMarshalToJson(map[string][]byte{ + "providerKey": []byte(`{"key": "value"}`), + }), + NewState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + }, + }, + "delete-response-private-updated": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.Resource{ + SchemaMethod: func(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + resp.Schema = testSchema + }, + MetadataMethod: func(_ context.Context, _ resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = "test_resource" + }, + CreateMethod: func(_ context.Context, _ resource.CreateRequest, resp *resource.CreateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Create") + }, + DeleteMethod: func(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + diags := resp.Private.SetKey(ctx, "providerKey", []byte(`{"key": "value"}`)) + + resp.Diagnostics.Append(diags...) + + // Must return error to prevent automatic private state clearing + resp.Diagnostics.AddError("error summary", "error detail") + }, + UpdateMethod: func(_ context.Context, _ resource.UpdateRequest, resp *resource.UpdateResponse) { + resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Delete, Got: Update") + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov6.ApplyResourceChangeRequest{ + PlannedPrivate: privatestate.MustMarshalToJson(map[string][]byte{ + ".frameworkKey": []byte(`{"frameworkKey": "framework value"}`), + }), + PlannedState: &testEmptyDynamicValue, + PriorState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + TypeName: "test_resource", + }, + expectedResponse: &tfprotov6.ApplyResourceChangeResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "error summary", + Detail: "error detail", + }, + }, + Private: privatestate.MustMarshalToJson(map[string][]byte{ + ".frameworkKey": []byte(`{"frameworkKey": "framework value"}`), + "providerKey": []byte(`{"key": "value"}`), + }), + NewState: testNewDynamicValue(t, testSchemaType, map[string]tftypes.Value{ + "test_computed": tftypes.NewValue(tftypes.String, nil), + "test_required": tftypes.NewValue(tftypes.String, "test-priorstate-value"), + }), + }, + }, "delete-response-newstate": { server: &Server{ FrameworkServer: fwserver.Server{ diff --git a/resource/delete.go b/resource/delete.go index 5c2aa83a6..ab81a6c92 100644 --- a/resource/delete.go +++ b/resource/delete.go @@ -37,6 +37,14 @@ type DeleteResponse struct { // should be set during the resource's Update operation. State tfsdk.State + // Private is the private state resource data following the Delete + // operation. This field is pre-populated from DeleteRequest.Private and + // can be modified during the resource's Delete operation in cases where + // an error diagnostic is being returned. Otherwise if no error diagnostic + // is being returned, indicating that the resource was successfully deleted, + // this data will be automatically cleared to prevent Terraform errors. + Private *privatestate.ProviderData + // Diagnostics report errors or warnings related to deleting the // resource. An empty slice indicates a successful operation with no // warnings or errors generated.