From ce3618efa4076b74a314d73e097847d462cef05b Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Tue, 1 Apr 2025 17:02:28 -0400 Subject: [PATCH 1/3] initial import implementation with some TODOs --- internal/fromproto5/importresourcestate.go | 9 +- .../fromproto5/importresourcestate_test.go | 64 +++++++- internal/fromproto6/importresourcestate.go | 9 +- .../fromproto6/importresourcestate_test.go | 64 +++++++- .../fwserver/server_importresourcestate.go | 58 ++++++- .../server_importresourcestate.go | 10 +- .../server_importresourcestate_test.go | 142 ++++++++++++++++++ .../server_importresourcestate.go | 10 +- .../server_importresourcestate_test.go | 142 ++++++++++++++++++ .../resourcewithidentityandimportstate.go | 43 ++++++ internal/toproto5/importedresource.go | 5 + internal/toproto5/importedresource_test.go | 95 ++++++++++++ internal/toproto6/importedresource.go | 5 + internal/toproto6/importedresource_test.go | 95 ++++++++++++ resource/import_state.go | 38 ++++- 15 files changed, 780 insertions(+), 9 deletions(-) create mode 100644 internal/testing/testprovider/resourcewithidentityandimportstate.go diff --git a/internal/fromproto5/importresourcestate.go b/internal/fromproto5/importresourcestate.go index ec40c2119..04034b584 100644 --- a/internal/fromproto5/importresourcestate.go +++ b/internal/fromproto5/importresourcestate.go @@ -18,7 +18,7 @@ import ( // ImportResourceStateRequest returns the *fwserver.ImportResourceStateRequest // equivalent of a *tfprotov5.ImportResourceStateRequest. -func ImportResourceStateRequest(ctx context.Context, proto5 *tfprotov5.ImportResourceStateRequest, reqResource resource.Resource, resourceSchema fwschema.Schema) (*fwserver.ImportResourceStateRequest, diag.Diagnostics) { +func ImportResourceStateRequest(ctx context.Context, proto5 *tfprotov5.ImportResourceStateRequest, reqResource resource.Resource, resourceSchema fwschema.Schema, identitySchema fwschema.Schema) (*fwserver.ImportResourceStateRequest, diag.Diagnostics) { if proto5 == nil { return nil, nil } @@ -45,10 +45,17 @@ func ImportResourceStateRequest(ctx context.Context, proto5 *tfprotov5.ImportRes Schema: resourceSchema, }, ID: proto5.ID, + IdentitySchema: identitySchema, Resource: reqResource, TypeName: proto5.TypeName, ClientCapabilities: ImportStateClientCapabilities(proto5.ClientCapabilities), } + identity, identityDiags := ResourceIdentity(ctx, proto5.Identity, identitySchema) + + diags.Append(identityDiags...) + + fw.Identity = identity + return fw, diags } diff --git a/internal/fromproto5/importresourcestate_test.go b/internal/fromproto5/importresourcestate_test.go index 39b5fdfa4..737d9a80d 100644 --- a/internal/fromproto5/importresourcestate_test.go +++ b/internal/fromproto5/importresourcestate_test.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/fwschema" "github.com/hashicorp/terraform-plugin-framework/internal/fwserver" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" ) @@ -31,6 +32,30 @@ func TestImportResourceStateRequest(t *testing.T) { }, } + testIdentityProto5Type := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_identity_attribute": tftypes.String, + }, + } + + testIdentityProto5Value := tftypes.NewValue(testIdentityProto5Type, map[string]tftypes.Value{ + "test_identity_attribute": tftypes.NewValue(tftypes.String, "id-123"), + }) + + testIdentityProto5DynamicValue, err := tfprotov5.NewDynamicValue(testIdentityProto5Type, testIdentityProto5Value) + + if err != nil { + t.Fatalf("unexpected error calling tfprotov5.NewDynamicValue(): %s", err) + } + + testIdentitySchema := identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_identity_attribute": identityschema.StringAttribute{ + RequiredForImport: true, + }, + }, + } + testFwEmptyState := tfsdk.State{ Raw: tftypes.NewValue(testFwSchema.Type().TerraformType(context.Background()), nil), Schema: testFwSchema, @@ -39,6 +64,7 @@ func TestImportResourceStateRequest(t *testing.T) { testCases := map[string]struct { input *tfprotov5.ImportResourceStateRequest resourceSchema fwschema.Schema + identitySchema fwschema.Schema resource resource.Resource expected *fwserver.ImportResourceStateRequest expectedDiagnostics diag.Diagnostics @@ -67,6 +93,42 @@ func TestImportResourceStateRequest(t *testing.T) { ), }, }, + "identity-missing-schema": { + input: &tfprotov5.ImportResourceStateRequest{ + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &testIdentityProto5DynamicValue, + }, + }, + resourceSchema: testFwSchema, + expected: &fwserver.ImportResourceStateRequest{ + EmptyState: testFwEmptyState, + }, + expectedDiagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Unable to Convert Resource Identity", + "An unexpected error was encountered when converting the resource identity from the protocol type. "+ + "Identity data was sent in the protocol to a resource that doesn't support identity.\n\n"+ + "This is always a problem with Terraform or terraform-plugin-framework. Please report this to the provider developer.", + ), + }, + }, + "identity": { + input: &tfprotov5.ImportResourceStateRequest{ + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &testIdentityProto5DynamicValue, + }, + }, + resourceSchema: testFwSchema, + identitySchema: testIdentitySchema, + expected: &fwserver.ImportResourceStateRequest{ + EmptyState: testFwEmptyState, + IdentitySchema: testIdentitySchema, + Identity: &tfsdk.ResourceIdentity{ + Raw: testIdentityProto5Value, + Schema: testIdentitySchema, + }, + }, + }, "id": { input: &tfprotov5.ImportResourceStateRequest{ ID: "test-id", @@ -122,7 +184,7 @@ func TestImportResourceStateRequest(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - got, diags := fromproto5.ImportResourceStateRequest(context.Background(), testCase.input, testCase.resource, testCase.resourceSchema) + got, diags := fromproto5.ImportResourceStateRequest(context.Background(), testCase.input, testCase.resource, testCase.resourceSchema, testCase.identitySchema) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/internal/fromproto6/importresourcestate.go b/internal/fromproto6/importresourcestate.go index 7070901cd..5e62da5c7 100644 --- a/internal/fromproto6/importresourcestate.go +++ b/internal/fromproto6/importresourcestate.go @@ -18,7 +18,7 @@ import ( // ImportResourceStateRequest returns the *fwserver.ImportResourceStateRequest // equivalent of a *tfprotov6.ImportResourceStateRequest. -func ImportResourceStateRequest(ctx context.Context, proto6 *tfprotov6.ImportResourceStateRequest, reqResource resource.Resource, resourceSchema fwschema.Schema) (*fwserver.ImportResourceStateRequest, diag.Diagnostics) { +func ImportResourceStateRequest(ctx context.Context, proto6 *tfprotov6.ImportResourceStateRequest, reqResource resource.Resource, resourceSchema fwschema.Schema, identitySchema fwschema.Schema) (*fwserver.ImportResourceStateRequest, diag.Diagnostics) { if proto6 == nil { return nil, nil } @@ -45,10 +45,17 @@ func ImportResourceStateRequest(ctx context.Context, proto6 *tfprotov6.ImportRes Schema: resourceSchema, }, ID: proto6.ID, + IdentitySchema: identitySchema, Resource: reqResource, TypeName: proto6.TypeName, ClientCapabilities: ImportStateClientCapabilities(proto6.ClientCapabilities), } + identity, identityDiags := ResourceIdentity(ctx, proto6.Identity, identitySchema) + + diags.Append(identityDiags...) + + fw.Identity = identity + return fw, diags } diff --git a/internal/fromproto6/importresourcestate_test.go b/internal/fromproto6/importresourcestate_test.go index 6b385bb1a..c022b3258 100644 --- a/internal/fromproto6/importresourcestate_test.go +++ b/internal/fromproto6/importresourcestate_test.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/fwschema" "github.com/hashicorp/terraform-plugin-framework/internal/fwserver" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" ) @@ -31,6 +32,30 @@ func TestImportResourceStateRequest(t *testing.T) { }, } + testIdentityProto6Type := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_identity_attribute": tftypes.String, + }, + } + + testIdentityProto6Value := tftypes.NewValue(testIdentityProto6Type, map[string]tftypes.Value{ + "test_identity_attribute": tftypes.NewValue(tftypes.String, "id-123"), + }) + + testIdentityProto6DynamicValue, err := tfprotov6.NewDynamicValue(testIdentityProto6Type, testIdentityProto6Value) + + if err != nil { + t.Fatalf("unexpected error calling tfprotov6.NewDynamicValue(): %s", err) + } + + testIdentitySchema := identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_identity_attribute": identityschema.StringAttribute{ + RequiredForImport: true, + }, + }, + } + testFwEmptyState := tfsdk.State{ Raw: tftypes.NewValue(testFwSchema.Type().TerraformType(context.Background()), nil), Schema: testFwSchema, @@ -39,6 +64,7 @@ func TestImportResourceStateRequest(t *testing.T) { testCases := map[string]struct { input *tfprotov6.ImportResourceStateRequest resourceSchema fwschema.Schema + identitySchema fwschema.Schema resource resource.Resource expected *fwserver.ImportResourceStateRequest expectedDiagnostics diag.Diagnostics @@ -67,6 +93,42 @@ func TestImportResourceStateRequest(t *testing.T) { ), }, }, + "identity-missing-schema": { + input: &tfprotov6.ImportResourceStateRequest{ + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: &testIdentityProto6DynamicValue, + }, + }, + resourceSchema: testFwSchema, + expected: &fwserver.ImportResourceStateRequest{ + EmptyState: testFwEmptyState, + }, + expectedDiagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Unable to Convert Resource Identity", + "An unexpected error was encountered when converting the resource identity from the protocol type. "+ + "Identity data was sent in the protocol to a resource that doesn't support identity.\n\n"+ + "This is always a problem with Terraform or terraform-plugin-framework. Please report this to the provider developer.", + ), + }, + }, + "identity": { + input: &tfprotov6.ImportResourceStateRequest{ + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: &testIdentityProto6DynamicValue, + }, + }, + resourceSchema: testFwSchema, + identitySchema: testIdentitySchema, + expected: &fwserver.ImportResourceStateRequest{ + EmptyState: testFwEmptyState, + IdentitySchema: testIdentitySchema, + Identity: &tfsdk.ResourceIdentity{ + Raw: testIdentityProto6Value, + Schema: testIdentitySchema, + }, + }, + }, "id": { input: &tfprotov6.ImportResourceStateRequest{ ID: "test-id", @@ -122,7 +184,7 @@ func TestImportResourceStateRequest(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - got, diags := fromproto6.ImportResourceStateRequest(context.Background(), testCase.input, testCase.resource, testCase.resourceSchema) + got, diags := fromproto6.ImportResourceStateRequest(context.Background(), testCase.input, testCase.resource, testCase.resourceSchema, testCase.identitySchema) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/internal/fwserver/server_importresourcestate.go b/internal/fwserver/server_importresourcestate.go index 4abe204cd..44c65a667 100644 --- a/internal/fwserver/server_importresourcestate.go +++ b/internal/fwserver/server_importresourcestate.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/internal/fwschema" "github.com/hashicorp/terraform-plugin-framework/internal/logging" "github.com/hashicorp/terraform-plugin-framework/internal/privatestate" "github.com/hashicorp/terraform-plugin-framework/resource" @@ -18,14 +19,29 @@ import ( // ImportedResource represents a resource that was imported. type ImportedResource struct { Private *privatestate.Data + Identity *tfsdk.ResourceIdentity State tfsdk.State TypeName string } // ImportResourceStateRequest is the framework server request for the // ImportResourceState RPC. +// +// Either ID or Identity will be supplied depending on how the resource is being imported. type ImportResourceStateRequest struct { - ID string + // ID will come from the import CLI command or an import config block with the "id" attribute assigned. + // + // This ID field is a special string identifier that can be parsed however the provider deems fit. + ID string + + // Identity will come from an import config block with the "identity" attribute assigned and will conform + // to the identity schema defined by the resource. (Terraform v1.12+) + // + // All attributes marked as RequiredForImport will be populated (enforced by Terraform core) and OptionalForImport + // attributes may be null, but could have a config value. + Identity *tfsdk.ResourceIdentity + IdentitySchema fwschema.Schema + Resource resource.Resource // EmptyState is an empty State for the resource schema. This is used to @@ -132,6 +148,29 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta Private: privateProviderData, } + // If the resource supports identity and we are not importing by identity, pre-populate with a null value. + // TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one? + if req.Identity == nil && req.IdentitySchema != nil { + nullTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) + + req.Identity = &tfsdk.ResourceIdentity{ + Schema: req.IdentitySchema, + Raw: nullTfValue.Copy(), + } + } + + if req.Identity != nil { + importReq.Identity = &tfsdk.ResourceIdentity{ + Schema: req.Identity.Schema, + Raw: req.Identity.Raw.Copy(), + } + + importResp.Identity = &tfsdk.ResourceIdentity{ + Schema: req.Identity.Schema, + Raw: req.Identity.Raw.Copy(), + } + } + logging.FrameworkTrace(ctx, "Calling provider defined Resource ImportState") resourceWithImportState.ImportState(ctx, importReq, &importResp) logging.FrameworkTrace(ctx, "Called provider defined Resource ImportState") @@ -154,7 +193,11 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta importResp.State.Raw = modifiedState - if importResp.State.Raw.Equal(req.EmptyState.Raw) { + // TODO:ResourceIdentity: Now you can reasonably import using just the identity field and no state. However this still feels like not a great idea, because it's possible + // to import via ID with a client that doesn't support identity, so the "Read" logic for providers will always have to account for both scenarios. + // + // A potential improvement on this could be to check if identity AND state are empty. That is the only true error state because then no data would be transferred from import to read. + if req.ID != "" && importResp.State.Raw.Equal(req.EmptyState.Raw) { resp.Diagnostics.AddError( "Missing Resource Import State", "An unexpected error was encountered when importing the resource. This is always a problem with the provider. Please give the following information to the provider developer:\n\n"+ @@ -169,10 +212,21 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta private.Provider = importResp.Private } + if importResp.Identity != nil && req.IdentitySchema == nil { + resp.Diagnostics.AddError( + "Unexpected ImportState Response", + "An unexpected error was encountered when creating the import response. New identity data was returned by the provider import operation, but the resource does not indicate identity support.\n\n"+ + "This is always a problem with the provider and should be reported to the provider developer.", + ) + + return + } + resp.Deferred = importResp.Deferred resp.ImportedResources = []ImportedResource{ { State: importResp.State, + Identity: importResp.Identity, TypeName: req.TypeName, Private: private, }, diff --git a/internal/proto5server/server_importresourcestate.go b/internal/proto5server/server_importresourcestate.go index 5d89dc908..9baaa8056 100644 --- a/internal/proto5server/server_importresourcestate.go +++ b/internal/proto5server/server_importresourcestate.go @@ -36,7 +36,15 @@ func (s *Server) ImportResourceState(ctx context.Context, proto5Req *tfprotov5.I return toproto5.ImportResourceStateResponse(ctx, fwResp), nil } - fwReq, diags := fromproto5.ImportResourceStateRequest(ctx, proto5Req, resource, resourceSchema) + identitySchema, diags := s.FrameworkServer.ResourceIdentitySchema(ctx, proto5Req.TypeName) + + fwResp.Diagnostics.Append(diags...) + + if fwResp.Diagnostics.HasError() { + return toproto5.ImportResourceStateResponse(ctx, fwResp), nil + } + + fwReq, diags := fromproto5.ImportResourceStateRequest(ctx, proto5Req, resource, resourceSchema, identitySchema) fwResp.Diagnostics.Append(diags...) diff --git a/internal/proto5server/server_importresourcestate_test.go b/internal/proto5server/server_importresourcestate_test.go index 268853c7d..e2b539199 100644 --- a/internal/proto5server/server_importresourcestate_test.go +++ b/internal/proto5server/server_importresourcestate_test.go @@ -16,7 +16,9 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/testing/testprovider" "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/types" ) func TestServerImportResourceState(t *testing.T) { @@ -30,12 +32,34 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentityType := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_id": tftypes.String, + "test_other_id": tftypes.String, + }, + } + + testRequestIdentityValue := testNewDynamicValue(t, testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "test_other_id": tftypes.NewValue(tftypes.String, nil), + }) + + testImportedResourceIdentityDynamicValue := testNewDynamicValue(t, testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "test_other_id": tftypes.NewValue(tftypes.String, "new-value-123"), + }) + testStateDynamicValue := testNewDynamicValue(t, testType, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "test-id"), "optional": tftypes.NewValue(tftypes.String, nil), "required": tftypes.NewValue(tftypes.String, nil), }) + testEmptyStateDynamicValue, err := tfprotov5.NewDynamicValue(testType, tftypes.NewValue(testType, nil)) + if err != nil { + t.Fatalf("unexpected error calling tfprotov5.NewDynamicValue(): %s", err) + } + testSchema := schema.Schema{ Attributes: map[string]schema.Attribute{ "id": schema.StringAttribute{ @@ -50,6 +74,17 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentitySchema := identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.StringAttribute{ + RequiredForImport: true, + }, + "test_other_id": identityschema.StringAttribute{ + OptionalForImport: true, + }, + }, + } + testCases := map[string]struct { server *Server request *tfprotov5.ImportResourceStateRequest @@ -99,6 +134,64 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "request-identity": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.ResourceWithIdentityAndImportState{ + Resource: &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" + }, + }, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + var identityData struct { + TestID types.String `tfsdk:"test_id"` + TestOtherID types.String `tfsdk:"test_other_id"` + } + + resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...) + + if identityData.TestID.ValueString() != "id-123" { + resp.Diagnostics.AddError("Unexpected req.Identity", identityData.TestID.ValueString()) + } + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov5.ImportResourceStateRequest{ + TypeName: "test_resource", + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + }, + expectedResponse: &tfprotov5.ImportResourceStateResponse{ + ImportedResources: []*tfprotov5.ImportedResource{ + { + State: &testEmptyStateDynamicValue, + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + TypeName: "test_resource", + }, + }, + }, + }, "response-diagnostics": { server: &Server{ FrameworkServer: fwserver.Server{ @@ -184,6 +277,55 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "response-importedresources-identity": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.ResourceWithIdentityAndImportState{ + Resource: &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" + }, + }, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("test_other_id"), types.StringValue("new-value-123"))...) + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov5.ImportResourceStateRequest{ + TypeName: "test_resource", + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + }, + expectedResponse: &tfprotov5.ImportResourceStateResponse{ + ImportedResources: []*tfprotov5.ImportedResource{ + { + State: &testEmptyStateDynamicValue, + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: testImportedResourceIdentityDynamicValue, + }, + TypeName: "test_resource", + }, + }, + }, + }, "response-importedresources-private": { server: &Server{ FrameworkServer: fwserver.Server{ diff --git a/internal/proto6server/server_importresourcestate.go b/internal/proto6server/server_importresourcestate.go index 46a58c633..aaf7cf1b8 100644 --- a/internal/proto6server/server_importresourcestate.go +++ b/internal/proto6server/server_importresourcestate.go @@ -36,7 +36,15 @@ func (s *Server) ImportResourceState(ctx context.Context, proto6Req *tfprotov6.I return toproto6.ImportResourceStateResponse(ctx, fwResp), nil } - fwReq, diags := fromproto6.ImportResourceStateRequest(ctx, proto6Req, resource, resourceSchema) + identitySchema, diags := s.FrameworkServer.ResourceIdentitySchema(ctx, proto6Req.TypeName) + + fwResp.Diagnostics.Append(diags...) + + if fwResp.Diagnostics.HasError() { + return toproto6.ImportResourceStateResponse(ctx, fwResp), nil + } + + fwReq, diags := fromproto6.ImportResourceStateRequest(ctx, proto6Req, resource, resourceSchema, identitySchema) fwResp.Diagnostics.Append(diags...) diff --git a/internal/proto6server/server_importresourcestate_test.go b/internal/proto6server/server_importresourcestate_test.go index 8c67646f0..f59f287b4 100644 --- a/internal/proto6server/server_importresourcestate_test.go +++ b/internal/proto6server/server_importresourcestate_test.go @@ -16,7 +16,9 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/testing/testprovider" "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/types" ) func TestServerImportResourceState(t *testing.T) { @@ -30,12 +32,34 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentityType := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_id": tftypes.String, + "test_other_id": tftypes.String, + }, + } + + testRequestIdentityValue := testNewDynamicValue(t, testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "test_other_id": tftypes.NewValue(tftypes.String, nil), + }) + + testImportedResourceIdentityDynamicValue := testNewDynamicValue(t, testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "test_other_id": tftypes.NewValue(tftypes.String, "new-value-123"), + }) + testStateDynamicValue := testNewDynamicValue(t, testType, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "test-id"), "optional": tftypes.NewValue(tftypes.String, nil), "required": tftypes.NewValue(tftypes.String, nil), }) + testEmptyStateDynamicValue, err := tfprotov6.NewDynamicValue(testType, tftypes.NewValue(testType, nil)) + if err != nil { + t.Fatalf("unexpected error calling tfprotov6.NewDynamicValue(): %s", err) + } + testSchema := schema.Schema{ Attributes: map[string]schema.Attribute{ "id": schema.StringAttribute{ @@ -50,6 +74,17 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentitySchema := identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.StringAttribute{ + RequiredForImport: true, + }, + "test_other_id": identityschema.StringAttribute{ + OptionalForImport: true, + }, + }, + } + testCases := map[string]struct { server *Server request *tfprotov6.ImportResourceStateRequest @@ -99,6 +134,64 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "request-identity": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.ResourceWithIdentityAndImportState{ + Resource: &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" + }, + }, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + var identityData struct { + TestID types.String `tfsdk:"test_id"` + TestOtherID types.String `tfsdk:"test_other_id"` + } + + resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...) + + if identityData.TestID.ValueString() != "id-123" { + resp.Diagnostics.AddError("Unexpected req.Identity", identityData.TestID.ValueString()) + } + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov6.ImportResourceStateRequest{ + TypeName: "test_resource", + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + }, + expectedResponse: &tfprotov6.ImportResourceStateResponse{ + ImportedResources: []*tfprotov6.ImportedResource{ + { + State: &testEmptyStateDynamicValue, + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + TypeName: "test_resource", + }, + }, + }, + }, "response-diagnostics": { server: &Server{ FrameworkServer: fwserver.Server{ @@ -184,6 +277,55 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "response-importedresources-identity": { + server: &Server{ + FrameworkServer: fwserver.Server{ + Provider: &testprovider.Provider{ + ResourcesMethod: func(_ context.Context) []func() resource.Resource { + return []func() resource.Resource{ + func() resource.Resource { + return &testprovider.ResourceWithIdentityAndImportState{ + Resource: &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" + }, + }, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("test_other_id"), types.StringValue("new-value-123"))...) + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + } + }, + } + }, + }, + }, + }, + request: &tfprotov6.ImportResourceStateRequest{ + TypeName: "test_resource", + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: testRequestIdentityValue, + }, + }, + expectedResponse: &tfprotov6.ImportResourceStateResponse{ + ImportedResources: []*tfprotov6.ImportedResource{ + { + State: &testEmptyStateDynamicValue, + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: testImportedResourceIdentityDynamicValue, + }, + TypeName: "test_resource", + }, + }, + }, + }, "response-importedresources-private": { server: &Server{ FrameworkServer: fwserver.Server{ diff --git a/internal/testing/testprovider/resourcewithidentityandimportstate.go b/internal/testing/testprovider/resourcewithidentityandimportstate.go new file mode 100644 index 000000000..9e26dbf7c --- /dev/null +++ b/internal/testing/testprovider/resourcewithidentityandimportstate.go @@ -0,0 +1,43 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package testprovider + +import ( + "context" + + "github.com/hashicorp/terraform-plugin-framework/resource" +) + +var _ resource.Resource = &ResourceWithIdentityAndImportState{} +var _ resource.ResourceWithIdentity = &ResourceWithIdentityAndImportState{} +var _ resource.ResourceWithImportState = &ResourceWithIdentityAndImportState{} + +// Declarative resource.ResourceWithIdentityAndImportState for unit testing. +type ResourceWithIdentityAndImportState struct { + *Resource + + // ResourceWithIdentity interface methods + IdentitySchemaMethod func(context.Context, resource.IdentitySchemaRequest, *resource.IdentitySchemaResponse) + + // ResourceWithImportState interface methods + ImportStateMethod func(context.Context, resource.ImportStateRequest, *resource.ImportStateResponse) +} + +// IdentitySchema implements resource.ResourceWithIdentity. +func (p *ResourceWithIdentityAndImportState) IdentitySchema(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + if p.IdentitySchemaMethod == nil { + return + } + + p.IdentitySchemaMethod(ctx, req, resp) +} + +// ImportState satisfies the resource.ResourceWithImportState interface. +func (r *ResourceWithIdentityAndImportState) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + if r.ImportStateMethod == nil { + return + } + + r.ImportStateMethod(ctx, req, resp) +} diff --git a/internal/toproto5/importedresource.go b/internal/toproto5/importedresource.go index 1a208db64..0b515dbd7 100644 --- a/internal/toproto5/importedresource.go +++ b/internal/toproto5/importedresource.go @@ -27,6 +27,11 @@ func ImportedResource(ctx context.Context, fw *fwserver.ImportedResource) (*tfpr proto5.State = state + identity, identityDiags := ResourceIdentity(ctx, fw.Identity) + + diags = append(diags, identityDiags...) + proto5.Identity = identity + newPrivate, privateDiags := fw.Private.Bytes(ctx) diags = append(diags, privateDiags...) diff --git a/internal/toproto5/importedresource_test.go b/internal/toproto5/importedresource_test.go index 58a1e860b..4d2ef533b 100644 --- a/internal/toproto5/importedresource_test.go +++ b/internal/toproto5/importedresource_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/fwserver" "github.com/hashicorp/terraform-plugin-framework/internal/privatestate" "github.com/hashicorp/terraform-plugin-framework/internal/toproto5" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" ) @@ -50,6 +51,22 @@ func TestImportResourceStateResponse(t *testing.T) { t.Fatalf("unexpected error calling tfprotov5.NewDynamicValue(): %s", err) } + testIdentityProto5Type := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_id": tftypes.String, + }, + } + + testIdentityProto5Value := tftypes.NewValue(testIdentityProto5Type, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + }) + + testIdentityProto5DynamicValue, err := tfprotov5.NewDynamicValue(testIdentityProto5Type, testIdentityProto5Value) + + if err != nil { + t.Fatalf("unexpected error calling tfprotov5.NewDynamicValue(): %s", err) + } + testState := tfsdk.State{ Raw: testProto5Value, Schema: schema.Schema{ @@ -85,6 +102,28 @@ func TestImportResourceStateResponse(t *testing.T) { testProviderData := privatestate.MustProviderData(context.Background(), testProviderKeyValue) + testIdentity := &tfsdk.ResourceIdentity{ + Raw: testIdentityProto5Value, + Schema: identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.StringAttribute{ + RequiredForImport: true, + }, + }, + }, + } + + testIdentityInvalid := &tfsdk.ResourceIdentity{ + Raw: testIdentityProto5Value, + Schema: identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.BoolAttribute{ + RequiredForImport: true, + }, + }, + }, + } + testCases := map[string]struct { input *fwserver.ImportResourceStateResponse expected *tfprotov5.ImportResourceStateResponse @@ -154,6 +193,42 @@ func TestImportResourceStateResponse(t *testing.T) { }, }, }, + "diagnostics-invalid-identity": { + input: &fwserver.ImportResourceStateResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewWarningDiagnostic("test warning summary", "test warning details"), + diag.NewErrorDiagnostic("test error summary", "test error details"), + }, + ImportedResources: []fwserver.ImportedResource{ + { + State: testState, + Identity: testIdentityInvalid, + }, + }, + }, + expected: &tfprotov5.ImportResourceStateResponse{ + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityWarning, + Summary: "test warning summary", + Detail: "test warning details", + }, + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "test error summary", + Detail: "test error details", + }, + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "Unable to Convert Resource Identity", + Detail: "An unexpected error was encountered when converting the resource identity to the protocol type. " + + "This is always an issue in terraform-plugin-framework used to implement the provider and should be reported to the provider developers.\n\n" + + "Please report this to the provider developer:\n\n" + + "Unable to create DynamicValue: AttributeName(\"test_id\"): unexpected value type string, tftypes.Bool values must be of type bool", + }, + }, + }, + }, "newstate": { input: &fwserver.ImportResourceStateResponse{ ImportedResources: []fwserver.ImportedResource{ @@ -170,6 +245,26 @@ func TestImportResourceStateResponse(t *testing.T) { }, }, }, + "identity": { + input: &fwserver.ImportResourceStateResponse{ + ImportedResources: []fwserver.ImportedResource{ + { + State: testState, + Identity: testIdentity, + }, + }, + }, + expected: &tfprotov5.ImportResourceStateResponse{ + ImportedResources: []*tfprotov5.ImportedResource{ + { + State: &testProto5DynamicValue, + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &testIdentityProto5DynamicValue, + }, + }, + }, + }, + }, "private": { input: &fwserver.ImportResourceStateResponse{ ImportedResources: []fwserver.ImportedResource{ diff --git a/internal/toproto6/importedresource.go b/internal/toproto6/importedresource.go index 28fea4b58..a57ed6319 100644 --- a/internal/toproto6/importedresource.go +++ b/internal/toproto6/importedresource.go @@ -27,6 +27,11 @@ func ImportedResource(ctx context.Context, fw *fwserver.ImportedResource) (*tfpr proto6.State = state + identity, identityDiags := ResourceIdentity(ctx, fw.Identity) + + diags = append(diags, identityDiags...) + proto6.Identity = identity + newPrivate, privateDiags := fw.Private.Bytes(ctx) diags = append(diags, privateDiags...) diff --git a/internal/toproto6/importedresource_test.go b/internal/toproto6/importedresource_test.go index 248d0d437..53eb11690 100644 --- a/internal/toproto6/importedresource_test.go +++ b/internal/toproto6/importedresource_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/internal/fwserver" "github.com/hashicorp/terraform-plugin-framework/internal/privatestate" "github.com/hashicorp/terraform-plugin-framework/internal/toproto6" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" ) @@ -44,6 +45,22 @@ func TestImportResourceStateResponse(t *testing.T) { t.Fatalf("unexpected error calling tfprotov6.NewDynamicValue(): %s", err) } + testIdentityProto6Type := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_id": tftypes.String, + }, + } + + testIdentityProto6Value := tftypes.NewValue(testIdentityProto6Type, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + }) + + testIdentityProto6DynamicValue, err := tfprotov6.NewDynamicValue(testIdentityProto6Type, testIdentityProto6Value) + + if err != nil { + t.Fatalf("unexpected error calling tfprotov6.NewDynamicValue(): %s", err) + } + testEmptyProto6DynamicValue, err := tfprotov6.NewDynamicValue(testEmptyProto6Type, testEmptyProto6Value) if err != nil { @@ -85,6 +102,28 @@ func TestImportResourceStateResponse(t *testing.T) { testProviderData := privatestate.MustProviderData(context.Background(), testProviderKeyValue) + testIdentity := &tfsdk.ResourceIdentity{ + Raw: testIdentityProto6Value, + Schema: identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.StringAttribute{ + RequiredForImport: true, + }, + }, + }, + } + + testIdentityInvalid := &tfsdk.ResourceIdentity{ + Raw: testIdentityProto6Value, + Schema: identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.BoolAttribute{ + RequiredForImport: true, + }, + }, + }, + } + testCases := map[string]struct { input *fwserver.ImportResourceStateResponse expected *tfprotov6.ImportResourceStateResponse @@ -154,6 +193,42 @@ func TestImportResourceStateResponse(t *testing.T) { }, }, }, + "diagnostics-invalid-identity": { + input: &fwserver.ImportResourceStateResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewWarningDiagnostic("test warning summary", "test warning details"), + diag.NewErrorDiagnostic("test error summary", "test error details"), + }, + ImportedResources: []fwserver.ImportedResource{ + { + State: testState, + Identity: testIdentityInvalid, + }, + }, + }, + expected: &tfprotov6.ImportResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityWarning, + Summary: "test warning summary", + Detail: "test warning details", + }, + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "test error summary", + Detail: "test error details", + }, + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Unable to Convert Resource Identity", + Detail: "An unexpected error was encountered when converting the resource identity to the protocol type. " + + "This is always an issue in terraform-plugin-framework used to implement the provider and should be reported to the provider developers.\n\n" + + "Please report this to the provider developer:\n\n" + + "Unable to create DynamicValue: AttributeName(\"test_id\"): unexpected value type string, tftypes.Bool values must be of type bool", + }, + }, + }, + }, "newstate": { input: &fwserver.ImportResourceStateResponse{ ImportedResources: []fwserver.ImportedResource{ @@ -170,6 +245,26 @@ func TestImportResourceStateResponse(t *testing.T) { }, }, }, + "identity": { + input: &fwserver.ImportResourceStateResponse{ + ImportedResources: []fwserver.ImportedResource{ + { + State: testState, + Identity: testIdentity, + }, + }, + }, + expected: &tfprotov6.ImportResourceStateResponse{ + ImportedResources: []*tfprotov6.ImportedResource{ + { + State: &testProto6DynamicValue, + Identity: &tfprotov6.ResourceIdentityData{ + IdentityData: &testIdentityProto6DynamicValue, + }, + }, + }, + }, + }, "private": { input: &fwserver.ImportResourceStateResponse{ ImportedResources: []fwserver.ImportedResource{ diff --git a/resource/import_state.go b/resource/import_state.go index 63f1b9fc5..bdf1aba4a 100644 --- a/resource/import_state.go +++ b/resource/import_state.go @@ -34,8 +34,19 @@ type ImportStateRequest struct { // as an Attribute. However, this identifier can also be treated as // its own type of value and parsed during import. This value // is not stored in the state unless the provider explicitly stores it. + // + // This ID field is supplied in the "terraform import" CLI command or in the import config block "id" attribute. + // Either ID or Identity must be supplied by the practitioner, depending on the method used to import the resource. ID string + // Identity is the configuration data provided by the practitioner in the import config block "identity" attribute. This + // configuration data will conform to the identity schema defined by the managed resource. If the resource does not support identity, + // this value will not be set. + // + // The "identity" attribute in the import block is only supported in Terraform 1.12 and later. + // Either ID or Identity must be supplied by the practitioner, depending on the method used to import the resource. + Identity *tfsdk.ResourceIdentity + // ClientCapabilities defines optionally supported protocol features for the // ImportResourceState RPC, such as forward-compatible Terraform behavior changes. ClientCapabilities ImportStateClientCapabilities @@ -56,6 +67,14 @@ type ImportStateResponse struct { // refresh the resource, e.g. call the Resource Read method. State tfsdk.State + // Identity is the identity of the resource following the Import operation. + // This field is pre-populated from ImportStateRequest.Identity and + // should be set during the resource's Import operation. + // + // If the resource does not support identity, this value will not be set and will + // raise a diagnostic if set by the resource's Import operation. + Identity *tfsdk.ResourceIdentity + // Private is the private state resource data following the Import operation. // This field is not pre-populated as there is no pre-existing private state // data during the resource's Import operation. @@ -75,6 +94,10 @@ type ImportStateResponse struct { // ImportStatePassthroughID is a helper function to set the import // identifier to a given state attribute path. The attribute must accept a // string value. +// +// This method will also automatically pass through the Identity field if provided. (Terraform 1.12+ and later) +// In this scenario where identity is provided instead of the string ID, the state field defined at `attrPath` will +// be set to null. func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req ImportStateRequest, resp *ImportStateResponse) { if attrPath.Equal(path.Empty()) { resp.Diagnostics.AddError( @@ -84,5 +107,18 @@ func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req Impor ) } - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, attrPath, req.ID)...) + // If the import is using the ID string identifier, (either via the "terraform import" CLI command, or a config block with the "id" attribute set) + // pass through the ID to the designated state attribute. + if req.ID != "" { + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, attrPath, req.ID)...) + } + + // TODO:ResourceIdentity: Should we implement another pass-through? called like ImportStatePassthrough that would work "better" with both ID and Identity + // + // We need to decide how we want to handle "existing id" string fields that need to be set to state when an identity is provided. + // Perhaps we can look at the identity schema and automatically use those values to set state? If we did that, we should probably rename this function + // since it's doing much more than just passing through ID. We wouldn't rename it, maybe just deprecate/create a new function. + // + // Since providers will likely be supporting versions of Terraform that don't support identity, they will likely won't want to write multiple ways of reading + // (read with identity, read with state "id" field) } From 55bd0f12c025409aa9c0e01ff5b35d67df2f9fc0 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Thu, 3 Apr 2025 12:00:05 -0400 Subject: [PATCH 2/3] add tests for fwserver --- .../fwserver/server_importresourcestate.go | 6 +- .../server_importresourcestate_test.go | 172 ++++++++++++++++++ resource/import_state.go | 16 +- 3 files changed, 178 insertions(+), 16 deletions(-) diff --git a/internal/fwserver/server_importresourcestate.go b/internal/fwserver/server_importresourcestate.go index 44c65a667..23961aacf 100644 --- a/internal/fwserver/server_importresourcestate.go +++ b/internal/fwserver/server_importresourcestate.go @@ -193,10 +193,8 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta importResp.State.Raw = modifiedState - // TODO:ResourceIdentity: Now you can reasonably import using just the identity field and no state. However this still feels like not a great idea, because it's possible - // to import via ID with a client that doesn't support identity, so the "Read" logic for providers will always have to account for both scenarios. - // - // A potential improvement on this could be to check if identity AND state are empty. That is the only true error state because then no data would be transferred from import to read. + // If we are importing by ID, we should ensure that something in the import stub state has been populated, + // otherwise the resource doesn't actually support import, which is a provider issue. if req.ID != "" && importResp.State.Raw.Equal(req.EmptyState.Raw) { resp.Diagnostics.AddError( "Missing Resource Import State", diff --git a/internal/fwserver/server_importresourcestate_test.go b/internal/fwserver/server_importresourcestate_test.go index 0f8481eea..ef9a7ac36 100644 --- a/internal/fwserver/server_importresourcestate_test.go +++ b/internal/fwserver/server_importresourcestate_test.go @@ -18,8 +18,10 @@ import ( "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/provider" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/identityschema" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-framework/types" ) func TestServerImportResourceState(t *testing.T) { @@ -33,6 +35,13 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentityType := tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test_id": tftypes.String, + "other_test_id": tftypes.String, + }, + } + testTypeWriteOnly := tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "id": tftypes.String, @@ -61,6 +70,16 @@ func TestServerImportResourceState(t *testing.T) { "required": tftypes.NewValue(tftypes.String, nil), }) + testRequestIdentityValue := tftypes.NewValue(testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "other_test_id": tftypes.NewValue(tftypes.String, nil), + }) + + testImportedResourceIdentityValue := tftypes.NewValue(testIdentityType, map[string]tftypes.Value{ + "test_id": tftypes.NewValue(tftypes.String, "id-123"), + "other_test_id": tftypes.NewValue(tftypes.String, "new-value-123"), + }) + testSchema := schema.Schema{ Attributes: map[string]schema.Attribute{ "id": schema.StringAttribute{ @@ -75,6 +94,17 @@ func TestServerImportResourceState(t *testing.T) { }, } + testIdentitySchema := identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "test_id": identityschema.StringAttribute{ + RequiredForImport: true, + }, + "other_test_id": identityschema.StringAttribute{ + OptionalForImport: true, + }, + }, + } + testSchemaWriteOnly := schema.Schema{ Attributes: map[string]schema.Attribute{ "id": schema.StringAttribute{ @@ -105,11 +135,21 @@ func TestServerImportResourceState(t *testing.T) { Schema: testSchema, } + testRequestIdentity := &tfsdk.ResourceIdentity{ + Raw: testRequestIdentityValue, + Schema: testIdentitySchema, + } + testState := &tfsdk.State{ Raw: testStateValue, Schema: testSchema, } + testImportedResourceIdentity := &tfsdk.ResourceIdentity{ + Raw: testImportedResourceIdentityValue, + Schema: testIdentitySchema, + } + testProviderKeyValue := privatestate.MustMarshalToJson(map[string][]byte{ "providerKeyOne": []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`), }) @@ -202,6 +242,47 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "request-identity": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ImportResourceStateRequest{ + EmptyState: *testEmptyState, + Identity: testRequestIdentity, + IdentitySchema: testIdentitySchema, + Resource: &testprovider.ResourceWithIdentityAndImportState{ + Resource: &testprovider.Resource{}, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + var identityData struct { + TestID types.String `tfsdk:"test_id"` + OtherTestID types.String `tfsdk:"other_test_id"` + } + + resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...) + + if identityData.TestID.ValueString() != "id-123" { + resp.Diagnostics.AddError("unexpected req.Identity value: %s", identityData.TestID.ValueString()) + } + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + }, + TypeName: "test_resource", + }, + expectedResponse: &fwserver.ImportResourceStateResponse{ + ImportedResources: []fwserver.ImportedResource{ + { + State: *testEmptyState, + Identity: testRequestIdentity, + TypeName: "test_resource", + Private: testEmptyPrivate, + }, + }, + }, + }, "request-resourcetype-importstate-not-implemented": { server: &fwserver.Server{ Provider: &testprovider.Provider{}, @@ -323,6 +404,97 @@ func TestServerImportResourceState(t *testing.T) { }, }, }, + "response-importedresources-identity": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ImportResourceStateRequest{ + EmptyState: *testEmptyState, + Identity: testRequestIdentity, + IdentitySchema: testIdentitySchema, + Resource: &testprovider.ResourceWithIdentityAndImportState{ + Resource: &testprovider.Resource{}, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("other_test_id"), types.StringValue("new-value-123"))...) + + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { + resp.IdentitySchema = testIdentitySchema + }, + }, + TypeName: "test_resource", + }, + expectedResponse: &fwserver.ImportResourceStateResponse{ + ImportedResources: []fwserver.ImportedResource{ + { + State: *testEmptyState, + Identity: testImportedResourceIdentity, + TypeName: "test_resource", + Private: testEmptyPrivate, + }, + }, + }, + }, + "response-importedresources-identity-supported": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ImportResourceStateRequest{ + EmptyState: *testEmptyState, + ID: "test-id", + IdentitySchema: testIdentitySchema, + Resource: &testprovider.ResourceWithImportState{ + Resource: &testprovider.Resource{}, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("test_id"), types.StringValue("id-123"))...) + resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("other_test_id"), types.StringValue("new-value-123"))...) + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + }, + TypeName: "test_resource", + }, + expectedResponse: &fwserver.ImportResourceStateResponse{ + ImportedResources: []fwserver.ImportedResource{ + { + State: *testState, + Identity: testImportedResourceIdentity, + TypeName: "test_resource", + Private: testEmptyPrivate, + }, + }, + }, + }, + "response-importedresources-invalid-identity": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ImportResourceStateRequest{ + EmptyState: *testEmptyState, + ID: "test-id", + Resource: &testprovider.ResourceWithImportState{ + Resource: &testprovider.Resource{}, + ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + // This resource doesn't indicate identity support (via a schema), so this should raise a diagnostic. + resp.Identity = &tfsdk.ResourceIdentity{ + Raw: testImportedResourceIdentityValue, + Schema: testIdentitySchema, + } + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + }, + }, + TypeName: "test_resource", + }, + expectedResponse: &fwserver.ImportResourceStateResponse{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Unexpected ImportState Response", + "An unexpected error was encountered when creating the import response. New identity data was returned by the provider import operation, but the resource does not indicate identity support.\n\n"+ + "This is always a problem with the provider and should be reported to the provider developer.", + ), + }, + }, + }, "response-importedresources-deferral-automatic": { server: &fwserver.Server{ Provider: &testprovider.Provider{ diff --git a/resource/import_state.go b/resource/import_state.go index bdf1aba4a..1ca670448 100644 --- a/resource/import_state.go +++ b/resource/import_state.go @@ -95,9 +95,10 @@ type ImportStateResponse struct { // identifier to a given state attribute path. The attribute must accept a // string value. // -// This method will also automatically pass through the Identity field if provided. (Terraform 1.12+ and later) -// In this scenario where identity is provided instead of the string ID, the state field defined at `attrPath` will -// be set to null. +// This method will also automatically pass through the Identity field if imported by +// the identity attribute of a import config block (Terraform 1.12+ and later). In this +// scenario where identity is provided instead of the string ID, the state field defined +// at `attrPath` will be set to null. func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req ImportStateRequest, resp *ImportStateResponse) { if attrPath.Equal(path.Empty()) { resp.Diagnostics.AddError( @@ -112,13 +113,4 @@ func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req Impor if req.ID != "" { resp.Diagnostics.Append(resp.State.SetAttribute(ctx, attrPath, req.ID)...) } - - // TODO:ResourceIdentity: Should we implement another pass-through? called like ImportStatePassthrough that would work "better" with both ID and Identity - // - // We need to decide how we want to handle "existing id" string fields that need to be set to state when an identity is provided. - // Perhaps we can look at the identity schema and automatically use those values to set state? If we did that, we should probably rename this function - // since it's doing much more than just passing through ID. We wouldn't rename it, maybe just deprecate/create a new function. - // - // Since providers will likely be supporting versions of Terraform that don't support identity, they will likely won't want to write multiple ways of reading - // (read with identity, read with state "id" field) } From c2faa63985a798c9f500b9cc55dfd553bd470d0f Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Thu, 3 Apr 2025 12:18:14 -0400 Subject: [PATCH 3/3] add changelog for beta --- .changes/unreleased/NOTES-20250403-121229.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changes/unreleased/NOTES-20250403-121229.yaml diff --git a/.changes/unreleased/NOTES-20250403-121229.yaml b/.changes/unreleased/NOTES-20250403-121229.yaml new file mode 100644 index 000000000..e383dcddc --- /dev/null +++ b/.changes/unreleased/NOTES-20250403-121229.yaml @@ -0,0 +1,8 @@ +kind: NOTES +body: This beta pre-release continues the implementation of managed resource identity, which should now be used with Terraform v1.12.0-beta1. + Managed resources now can support import by identity during plan and apply workflows. Managed resources that already support import via the + `resource.ResourceWithImportState` interface will automatically pass-through identity data to the `Read` method. The `RequiredForImport` and + `OptionalForImport` fields on the identity schema can be used to control the validation that Terraform core will apply to the import config block. +time: 2025-04-03T12:12:29.323193-04:00 +custom: + Issue: "1126"