From 3933b619a27ff79a8bddf1b232f86caf555a08f1 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 14 Feb 2023 09:17:29 -0500 Subject: [PATCH] helper/schema: Remove timeouts during ImportResourceState (#1146) Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1145 Reference: https://github.com/hashicorp/terraform/pull/32463 Terraform 1.3.8 and later now correctly handles null values for single nested blocks. This means Terraform will now report differences between a null block and known block with null values. This SDK only supported single nested blocks via its timeouts functionality. This change is a very targeted removal of any potential `timeouts` block values in a resource state from the `ImportResourceState` RPC. Since configuration is not available during that RPC, it is never valid to return any data beyond null for that block. This will prevent unexpected differences on the first plan after import, where Terraform will report the block removal for configurations which do not contain the block. New unit test failure prior to code updates: ``` --- FAIL: TestImportResourceState_Timeouts_Removed (0.00s) /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider_test.go:1159: unexpected difference: cty.Value( - { - ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}}, - v: map[string]any{"id": string("test"), "string_attribute": nil, "timeouts": nil}, - }, + { + ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}}, + v: map[string]any{ + "id": string("test"), + "string_attribute": nil, + "timeouts": map[string]any{"create": nil, "read": nil}, + }, + }, ) ``` --- .../unreleased/BUG FIXES-20230213-150146.yaml | 6 + helper/schema/grpc_provider.go | 16 ++ helper/schema/grpc_provider_test.go | 154 ++++++++++++++++++ 3 files changed, 176 insertions(+) create mode 100644 .changes/unreleased/BUG FIXES-20230213-150146.yaml diff --git a/.changes/unreleased/BUG FIXES-20230213-150146.yaml b/.changes/unreleased/BUG FIXES-20230213-150146.yaml new file mode 100644 index 00000000000..d3839ec7590 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20230213-150146.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'helper/schema: Prevented unexpected difference for timeouts on first plan after + import' +time: 2023-02-13T15:01:46.441029-05:00 +custom: + Issue: "1146" diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index ec3ed07c102..17a0de4cc63 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -1110,6 +1110,22 @@ func (s *GRPCProviderServer) ImportResourceState(ctx context.Context, req *tfpro // Normalize the value and fill in any missing blocks. newStateVal = objchange.NormalizeObjectFromLegacySDK(newStateVal, schemaBlock) + // Ensure any timeouts block is null in the imported state. There is no + // configuration to read from during import, so it is never valid to + // return a known value for the block. + // + // This is done without modifying HCL2ValueFromFlatmap or + // NormalizeObjectFromLegacySDK to prevent other unexpected changes. + // + // Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1145 + newStateType := newStateVal.Type() + + if newStateVal != cty.NilVal && !newStateVal.IsNull() && newStateType.IsObjectType() && newStateType.HasAttribute(TimeoutsConfigKey) { + newStateValueMap := newStateVal.AsValueMap() + newStateValueMap[TimeoutsConfigKey] = cty.NullVal(newStateType.AttributeType(TimeoutsConfigKey)) + newStateVal = cty.ObjectVal(newStateValueMap) + } + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index 326161216a3..e0c42d14b0a 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -1006,6 +1006,160 @@ func TestApplyResourceChange_bigint(t *testing.T) { } } +// Timeouts should never be present in imported resources. +// Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1145 +func TestImportResourceState_Timeouts_None(t *testing.T) { + t.Parallel() + + resourceDefinition := &Resource{ + Importer: &ResourceImporter{ + StateContext: ImportStatePassthroughContext, + }, + Schema: map[string]*Schema{ + "string_attribute": { + Type: TypeString, + Optional: true, + }, + }, + } + resourceTypeName := "test" + + server := NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + resourceTypeName: resourceDefinition, + }, + }) + + schema := resourceDefinition.CoreConfigSchema() + + // Import shim state should not require all attributes. + stateVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("test"), + })) + + if err != nil { + t.Fatalf("unable to coerce state value: %s", err) + } + + testReq := &tfprotov5.ImportResourceStateRequest{ + ID: "test", + TypeName: resourceTypeName, + } + + resp, err := server.ImportResourceState(context.Background(), testReq) + + if err != nil { + t.Fatalf("unexpected error during ImportResourceState: %s", err) + } + + if resp == nil { + t.Fatal("expected ImportResourceState response") + } + + if len(resp.Diagnostics) > 0 { + var diagnostics []string + + for _, diagnostic := range resp.Diagnostics { + diagnostics = append(diagnostics, fmt.Sprintf("%s: %s: %s", diagnostic.Severity, diagnostic.Summary, diagnostic.Detail)) + } + + t.Fatalf("unexpected ImportResourceState diagnostics: %s", strings.Join(diagnostics, " | ")) + } + + if len(resp.ImportedResources) != 1 { + t.Fatalf("expected 1 ImportedResource, got: %#v", resp.ImportedResources) + } + + gotStateVal, err := msgpack.Unmarshal(resp.ImportedResources[0].State.MsgPack, schema.ImpliedType()) + + if err != nil { + t.Fatalf("unexpected error during MessagePack unmarshal: %s", err) + } + + if diff := cmp.Diff(stateVal, gotStateVal, valueComparer); diff != "" { + t.Errorf("unexpected difference: %s", diff) + } +} + +// Timeouts should never be present in imported resources. +// Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1145 +func TestImportResourceState_Timeouts_Removed(t *testing.T) { + t.Parallel() + + resourceDefinition := &Resource{ + Importer: &ResourceImporter{ + StateContext: ImportStatePassthroughContext, + }, + Schema: map[string]*Schema{ + "string_attribute": { + Type: TypeString, + Optional: true, + }, + }, + Timeouts: &ResourceTimeout{ + Create: DefaultTimeout(10 * time.Minute), + Read: DefaultTimeout(10 * time.Minute), + }, + } + resourceTypeName := "test" + + server := NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + resourceTypeName: resourceDefinition, + }, + }) + + schema := resourceDefinition.CoreConfigSchema() + + // Import shim state should not require all attributes. + stateVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("test"), + })) + + if err != nil { + t.Fatalf("unable to coerce state value: %s", err) + } + + testReq := &tfprotov5.ImportResourceStateRequest{ + ID: "test", + TypeName: resourceTypeName, + } + + resp, err := server.ImportResourceState(context.Background(), testReq) + + if err != nil { + t.Fatalf("unexpected error during ImportResourceState: %s", err) + } + + if resp == nil { + t.Fatal("expected ImportResourceState response") + } + + if len(resp.Diagnostics) > 0 { + var diagnostics []string + + for _, diagnostic := range resp.Diagnostics { + diagnostics = append(diagnostics, fmt.Sprintf("%s: %s: %s", diagnostic.Severity, diagnostic.Summary, diagnostic.Detail)) + } + + t.Fatalf("unexpected ImportResourceState diagnostics: %s", strings.Join(diagnostics, " | ")) + } + + if len(resp.ImportedResources) != 1 { + t.Fatalf("expected 1 ImportedResource, got: %#v", resp.ImportedResources) + } + + gotStateVal, err := msgpack.Unmarshal(resp.ImportedResources[0].State.MsgPack, schema.ImpliedType()) + + if err != nil { + t.Fatalf("unexpected error during MessagePack unmarshal: %s", err) + } + + if diff := cmp.Diff(stateVal, gotStateVal, valueComparer); diff != "" { + t.Errorf("unexpected difference: %s", diff) + } +} + func TestReadDataSource(t *testing.T) { t.Parallel()