From 01af165decf3f7bc2c239e4c350c9cdae89c7f2f Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Thu, 3 Apr 2025 11:51:50 +0200 Subject: [PATCH 1/6] support importing by resource identity --- helper/schema/grpc_provider.go | 52 ++++++++++++++++++++++++++++++++-- helper/schema/provider.go | 18 ++++++++++++ helper/schema/resource.go | 2 +- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index 843314f3ba6..c1424f10770 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -1566,7 +1566,27 @@ func (s *GRPCProviderServer) ImportResourceState(ctx context.Context, req *tfpro return resp, nil } - newInstanceStates, err := s.provider.ImportState(ctx, info, req.ID) + var identity map[string]string + // parse identity data if available + if req.Identity != nil && req.Identity.IdentityData != nil { + // convert req.Identity to flat map identity structure + // Step 1: Turn JSON into cty.Value based on schema + identityBlock, err := s.getResourceIdentitySchemaBlock(req.TypeName) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("getting identity schema failed for resource '%s': %w", req.TypeName, err)) + return resp, nil + } + + identityVal, err := msgpack.Unmarshal(req.Identity.IdentityData.MsgPack, identityBlock.ImpliedType()) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) + return resp, nil + } + // Step 2: Turn cty.Value into flatmap representation + identity = hcl2shim.FlatmapValueFromHCL2(identityVal) + } + + newInstanceStates, err := s.provider.ImportStateWithIdentity(ctx, info, req.ID, identity) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) return resp, nil @@ -1622,12 +1642,40 @@ func (s *GRPCProviderServer) ImportResourceState(ctx context.Context, req *tfpro return resp, nil } + var identityData *tfprotov5.ResourceIdentityData + if is.Identity != nil { + identityBlock, err := s.getResourceIdentitySchemaBlock(resourceType) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("getting identity schema failed for resource '%s': %w", req.TypeName, err)) + return resp, nil + } + + newIdentityVal, err := hcl2shim.HCL2ValueFromFlatmap(is.Identity, identityBlock.ImpliedType()) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) + return resp, nil + } + + newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType()) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) + return resp, nil + } + + identityData = &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: newIdentityMP, + }, + } + } + importedResource := &tfprotov5.ImportedResource{ TypeName: resourceType, State: &tfprotov5.DynamicValue{ MsgPack: newStateMP, }, - Private: meta, + Private: meta, + Identity: identityData, } resp.ImportedResources = append(resp.ImportedResources, importedResource) diff --git a/helper/schema/provider.go b/helper/schema/provider.go index ad258517eac..cb7e2a89813 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -476,6 +476,14 @@ func (p *Provider) ImportState( ctx context.Context, info *terraform.InstanceInfo, id string) ([]*terraform.InstanceState, error) { + return p.ImportStateWithIdentity(ctx, info, id, nil) +} + +func (p *Provider) ImportStateWithIdentity( + ctx context.Context, + info *terraform.InstanceInfo, + id string, + identity map[string]string) ([]*terraform.InstanceState, error) { // Find the resource r, ok := p.ResourcesMap[info.Type] if !ok { @@ -492,6 +500,16 @@ func (p *Provider) ImportState( data.SetId(id) data.SetType(info.Type) + if data.identitySchema != nil { + identityData, err := data.Identity() + if err != nil { + return nil, err // this should not happen, as we checked above + } + identityData.raw = identity // is this too hacky / unexpected? + } else if identity != nil { + return nil, fmt.Errorf("resource %s doesn't support identity import", info.Type) + } + // Call the import function results := []*ResourceData{data} if r.Importer.State != nil || r.Importer.StateContext != nil { diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 077a6ddb09b..c206df6c15e 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -1398,7 +1398,7 @@ func isReservedResourceFieldName(name string) bool { // // This function is useful for unit tests and ResourceImporter functions. func (r *Resource) Data(s *terraform.InstanceState) *ResourceData { - result, err := schemaMap(r.SchemaMap()).Data(s, nil) + result, err := schemaMapWithIdentity{r.SchemaMap(), r.Identity.SchemaMap()}.Data(s, nil) if err != nil { // At the time of writing, this isn't possible (Data never returns // non-nil errors). We panic to find this in the future if we have to. From 9dea0078fc69b2f67b57a67566541b152039884c Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Tue, 15 Apr 2025 12:14:45 +0200 Subject: [PATCH 2/6] add todo note --- helper/schema/resource_importer.go | 1 + 1 file changed, 1 insertion(+) diff --git a/helper/schema/resource_importer.go b/helper/schema/resource_importer.go index ad9a5c3b9c6..e0c637aa0b1 100644 --- a/helper/schema/resource_importer.go +++ b/helper/schema/resource_importer.go @@ -77,6 +77,7 @@ func ImportStatePassthrough(d *ResourceData, m interface{}) ([]*ResourceData, er // ImportStatePassthroughContext is an implementation of StateContextFunc that can be // used to simply pass the ID directly through. This should be used only // in the case that an ID-only refresh is possible. +// TODO: this does not work with identity, since we also need to be able to set the id at the same time and that can't happen automatically, since the id probably is a combination of the identity attributes (this could work if there's only one attribute though, but practitioners should be able to write their own function that handles this then) func ImportStatePassthroughContext(ctx context.Context, d *ResourceData, m interface{}) ([]*ResourceData, error) { return []*ResourceData{d}, nil } From 0ee75aa9078d8d0a5123dd7cd20558584ffef16f Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 16 Apr 2025 15:02:00 +0200 Subject: [PATCH 3/6] add tests for ImportResourceState with identity --- helper/schema/grpc_provider_test.go | 163 ++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index 600a4ade34e..cdf97a30f13 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -7715,6 +7715,169 @@ func TestImportResourceState(t *testing.T) { }, }, }, + "basic-import-from-identity": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test_string": { + Type: TypeString, + Computed: true, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + Importer: &ResourceImporter{ + StateContext: func(ctx context.Context, d *ResourceData, meta interface{}) ([]*ResourceData, error) { + identity, err := d.Identity() + if err != nil { + t.Fatalf("failed to get identity: %v", err) + } + result, exists := identity.GetOk("id") + if !exists { + t.Fatalf("expected id to exist in identity") + } + + err = d.Set("test_string", "new-imported-val") + if err != nil { + return nil, err + } + + d.SetId(result.(string)) + + return []*ResourceData{d}, nil + }, + }, + }, + }, + }), + req: &tfprotov5.ImportResourceStateRequest{ + TypeName: "test", + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("imported-id"), + }), + ), + }, + }, + }, + expected: &tfprotov5.ImportResourceStateResponse{ + ImportedResources: []*tfprotov5.ImportedResource{ + { + TypeName: "test", + State: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test_string": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("imported-id"), + "test_string": cty.StringVal("new-imported-val"), + }), + ), + }, + Private: []byte(`{"schema_version":"1"}`), + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("imported-id"), + }), + ), + }, + }, + }, + }, + }, + }, + "basic-import-from-identity-no-id": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test_string": { + Type: TypeString, + Computed: true, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + Importer: &ResourceImporter{ + // Note: this does not set the Id on the ResourceData which results in an error that this test expects + StateContext: func(ctx context.Context, d *ResourceData, meta interface{}) ([]*ResourceData, error) { + err := d.Set("test_string", "new-imported-val") + if err != nil { + return nil, err + } + + return []*ResourceData{d}, nil + }, + }, + }, + }, + }), + req: &tfprotov5.ImportResourceStateRequest{ + TypeName: "test", + Identity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("imported-id"), + }), + ), + }, + }, + }, + expected: &tfprotov5.ImportResourceStateResponse{ + ImportedResources: nil, + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "The provider returned a resource missing an identifier during ImportResourceState. This is generally a bug in the resource implementation for import. Resource import code should not call d.SetId(\"\") or create an empty ResourceData. If the resource is missing, instead return an error. Please report this to the provider developers.", + }, + }, + }, + }, "resource-doesnt-exist": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ From 2e38f579f24ac4d1b52a25586bd8abe0905e9189 Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 16 Apr 2025 15:12:06 +0200 Subject: [PATCH 4/6] turn todo into comment --- helper/schema/resource_importer.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/helper/schema/resource_importer.go b/helper/schema/resource_importer.go index e0c637aa0b1..e2c4d08b4f7 100644 --- a/helper/schema/resource_importer.go +++ b/helper/schema/resource_importer.go @@ -77,7 +77,9 @@ func ImportStatePassthrough(d *ResourceData, m interface{}) ([]*ResourceData, er // ImportStatePassthroughContext is an implementation of StateContextFunc that can be // used to simply pass the ID directly through. This should be used only // in the case that an ID-only refresh is possible. -// TODO: this does not work with identity, since we also need to be able to set the id at the same time and that can't happen automatically, since the id probably is a combination of the identity attributes (this could work if there's only one attribute though, but practitioners should be able to write their own function that handles this then) +// Please note that this implementation does not work when using resource identity as +// an Id still has to be set and the identity might contain multiple fields +// that are not the same as the ID. func ImportStatePassthroughContext(ctx context.Context, d *ResourceData, m interface{}) ([]*ResourceData, error) { return []*ResourceData{d}, nil } From 458e9f6682c252d2aa775e62ae42023595c16193 Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 16 Apr 2025 15:32:37 +0200 Subject: [PATCH 5/6] add tests for ImportStateWithIdentity --- helper/schema/provider_test.go | 158 +++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index 0087c61d4c0..2426a6c189a 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -2271,6 +2271,164 @@ func TestProviderImportState(t *testing.T) { } } +func TestProviderImportStateWithIdentity(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + provider *Provider + info *terraform.InstanceInfo + id string + identity map[string]string + expectedStates []*terraform.InstanceState + expectedErr error + }{ + "error-missing-identity-schema": { + provider: &Provider{ + ResourcesMap: map[string]*Resource{ + "test_resource": { + Importer: &ResourceImporter{}, + // no identity schema defined + }, + }, + }, + identity: map[string]string{ + "id": "test-id", + }, + info: &terraform.InstanceInfo{ + Type: "test_resource", + }, + expectedErr: fmt.Errorf("resource test_resource doesn't support identity import"), + }, + "error-missing-ResourceData-Id": { + provider: &Provider{ + ResourcesMap: map[string]*Resource{ + "test_resource": { + Importer: &ResourceImporter{ + StateContext: func(_ context.Context, d *ResourceData, _ interface{}) ([]*ResourceData, error) { + // Example for handling import based on identity but not + // setting the id even though it's still required to be set + d.SetId("") + return []*ResourceData{d}, nil + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + }, + }, + }, + info: &terraform.InstanceInfo{ + Type: "test_resource", + }, + identity: map[string]string{ + "id": "test-id", + }, + expectedErr: fmt.Errorf("The provider returned a resource missing an identifier during ImportResourceState."), + }, + "Importer-StateContext-from-identity": { + provider: &Provider{ + ResourcesMap: map[string]*Resource{ + "test_resource": { + Importer: &ResourceImporter{ + StateContext: func(_ context.Context, d *ResourceData, meta interface{}) ([]*ResourceData, error) { + if d.Id() != "" { + return nil, fmt.Errorf("expected d.Id() to be empty, got: %s", d.Id()) + } + + identity, err := d.Identity() + if err != nil { + return nil, fmt.Errorf("error getting identity: %s", err) + } + id, exists := identity.GetOk("id") + if !exists { + return nil, fmt.Errorf("expected identity to contain key id") + } + if id != "test-id" { + return nil, fmt.Errorf("expected identity id %q, got: %s", "test-id", id) + } + + // set region as we act as if it's derived from provider defaults + err = identity.Set("region", "eu-central-1") + if err != nil { + return nil, fmt.Errorf("error setting identity region: %s", err) + } + // set the id as well + d.SetId(id.(string)) + + return []*ResourceData{d}, nil + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + RequiredForImport: true, + }, + "region": { + Type: TypeString, + OptionalForImport: true, + }, + } + }, + }, + }, + }, + }, + info: &terraform.InstanceInfo{ + Type: "test_resource", + }, + identity: map[string]string{ + "id": "test-id", + }, + expectedStates: []*terraform.InstanceState{ + { + Attributes: map[string]string{"id": "test-id"}, + Ephemeral: terraform.EphemeralState{Type: "test_resource"}, + ID: "test-id", + Identity: map[string]string{"id": "test-id", "region": "eu-central-1"}, + Meta: map[string]interface{}{"schema_version": "0"}, + }, + }, + }, + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + states, err := testCase.provider.ImportStateWithIdentity(context.Background(), testCase.info, testCase.id, testCase.identity) + + if err != nil { + if testCase.expectedErr == nil { + t.Fatalf("unexpected error: %s", err) + } + + if !strings.Contains(err.Error(), testCase.expectedErr.Error()) { + t.Fatalf("expected error %q, got: %s", testCase.expectedErr, err) + } + } + + if err == nil && testCase.expectedErr != nil { + t.Fatalf("expected error %q, got none", testCase.expectedErr) + } + + if diff := cmp.Diff(states, testCase.expectedStates); diff != "" { + t.Fatalf("unexpected states difference: %s", diff) + } + }) + } +} + func TestProviderMeta(t *testing.T) { p := new(Provider) if v := p.Meta(); v != nil { From 94a605f8d3255fc991af6a5d991ec4cc6a086d5d Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Thu, 17 Apr 2025 11:31:36 +0200 Subject: [PATCH 6/6] fix logging_http_transport_test.go after terraform.io started redirecting recently --- helper/logging/logging_http_transport_test.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/helper/logging/logging_http_transport_test.go b/helper/logging/logging_http_transport_test.go index a7b84e110dc..73c3515cb60 100644 --- a/helper/logging/logging_http_transport_test.go +++ b/helper/logging/logging_http_transport_test.go @@ -31,7 +31,7 @@ func TestNewLoggingHTTPTransport(t *testing.T) { reqBody := `An example multiline request body` - req, _ := http.NewRequest("GET", "https://www.terraform.io", bytes.NewBufferString(reqBody)) + req, _ := http.NewRequest("GET", "https://developer.hashicorp.com/terraform", bytes.NewBufferString(reqBody)) res, err := client.Do(req.WithContext(ctx)) if err != nil { t.Fatalf("request failed: %v", err) @@ -40,7 +40,7 @@ func TestNewLoggingHTTPTransport(t *testing.T) { entries, err := tflogtest.MultilineJSONDecode(loggerOutput) if err != nil { - t.Fatalf("log outtput parsing failed: %v", err) + t.Fatalf("log output parsing failed: %v", err) } if len(entries) != 2 { @@ -67,12 +67,12 @@ func TestNewLoggingHTTPTransport(t *testing.T) { "@module": "provider", "tf_http_op_type": "request", "tf_http_req_method": "GET", - "tf_http_req_uri": "/", + "tf_http_req_uri": "/terraform", "tf_http_req_version": "HTTP/1.1", "tf_http_req_body": "An example multiline request body", "tf_http_trans_id": transId, "Accept-Encoding": "gzip", - "Host": "www.terraform.io", + "Host": "developer.hashicorp.com", "User-Agent": "Go-http-client/1.1", "Content-Length": "37", }); diff != "" { @@ -122,7 +122,7 @@ func TestNewSubsystemLoggingHTTPTransport(t *testing.T) { reqBody := `An example multiline request body` - req, _ := http.NewRequest("GET", "https://www.terraform.io", bytes.NewBufferString(reqBody)) + req, _ := http.NewRequest("GET", "https://developer.hashicorp.com/terraform", bytes.NewBufferString(reqBody)) res, err := client.Do(req.WithContext(ctx)) if err != nil { t.Fatalf("request failed: %v", err) @@ -158,12 +158,12 @@ func TestNewSubsystemLoggingHTTPTransport(t *testing.T) { "@module": "provider.test-subsystem", "tf_http_op_type": "request", "tf_http_req_method": "GET", - "tf_http_req_uri": "/", + "tf_http_req_uri": "/terraform", "tf_http_req_version": "HTTP/1.1", "tf_http_req_body": "An example multiline request body", "tf_http_trans_id": transId, "Accept-Encoding": "gzip", - "Host": "www.terraform.io", + "Host": "developer.hashicorp.com", "User-Agent": "Go-http-client/1.1", "Content-Length": "37", }); diff != "" { @@ -201,7 +201,7 @@ func TestNewSubsystemLoggingHTTPTransport(t *testing.T) { func TestNewLoggingHTTPTransport_LogMasking(t *testing.T) { ctx, loggerOutput := setupRootLogger() ctx = tflog.MaskFieldValuesWithFieldKeys(ctx, "tf_http_op_type") - ctx = tflog.MaskAllFieldValuesRegexes(ctx, regexp.MustCompile(`(?s).*`)) + ctx = tflog.MaskAllFieldValuesRegexes(ctx, regexp.MustCompile(`(?s).*`)) ctx = tflog.MaskMessageStrings(ctx, "Request", "Response") transport := logging.NewLoggingHTTPTransport(http.DefaultTransport) @@ -210,7 +210,7 @@ func TestNewLoggingHTTPTransport_LogMasking(t *testing.T) { Timeout: 10 * time.Second, } - req, _ := http.NewRequest("GET", "https://www.terraform.io", nil) + req, _ := http.NewRequest("GET", "https://developer.hashicorp.com/terraform", nil) res, err := client.Do(req.WithContext(ctx)) if err != nil { t.Fatalf("request failed: %v", err) @@ -266,7 +266,7 @@ func TestNewLoggingHTTPTransport_LogOmitting(t *testing.T) { Timeout: 10 * time.Second, } - req, _ := http.NewRequest("GET", "https://www.terraform.io", nil) + req, _ := http.NewRequest("GET", "https://developer.hashicorp.com/terraform", nil) res, err := client.Do(req.WithContext(ctx)) if err != nil { t.Fatalf("request failed: %v", err)