From 489ea99e166fe64c06acea85d682605ff3b3064a Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Thu, 28 Jul 2022 07:48:50 +0100 Subject: [PATCH] Updates following code review (#415) --- .changelog/426.txt | 3 + .../fwserver/server_upgraderesourcestate.go | 19 +++-- .../server_upgraderesourcestate_test.go | 85 +++++++++++++++++++ 3 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 .changelog/426.txt diff --git a/.changelog/426.txt b/.changelog/426.txt new file mode 100644 index 000000000..cbf4996a8 --- /dev/null +++ b/.changelog/426.txt @@ -0,0 +1,3 @@ +```release-note:bug +internal/fwserver: Ensured `UpgradeResourceState` calls from Terraform 0.12 properly ignored attributes not defined in the schema +``` diff --git a/internal/fwserver/server_upgraderesourcestate.go b/internal/fwserver/server_upgraderesourcestate.go index b744dd450..504757263 100644 --- a/internal/fwserver/server_upgraderesourcestate.go +++ b/internal/fwserver/server_upgraderesourcestate.go @@ -44,6 +44,15 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS return } + // Define options to be used when unmarshalling raw state. + // IgnoreUndefinedAttributes will silently skip over fields in the JSON + // that do not have a matching entry in the schema. + unmarshalOpts := tfprotov6.UnmarshalOpts{ + ValueFromJSONOpts: tftypes.ValueFromJSONOpts{ + IgnoreUndefinedAttributes: true, + }, + } + // Terraform CLI can call UpgradeResourceState even if the stored state // version matches the current schema. Presumably this is to account for // the previous terraform-plugin-sdk implementation, which handled some @@ -54,7 +63,7 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS // detail for provider developers. Instead, the framework will attempt to // roundtrip the prior RawState to a State matching the current Schema. // - // TODO: To prevent provider developers from accidentially implementing + // TODO: To prevent provider developers from accidentally implementing // ResourceWithUpgradeState with a version matching the current schema // version which would never get called, the framework can introduce a // unit test helper. @@ -67,11 +76,7 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS resourceSchemaType := req.ResourceSchema.TerraformType(ctx) - rawStateValue, err := req.RawState.UnmarshalWithOpts(resourceSchemaType, tftypes.UnmarshalOpts{ - JSONOpts: tftypes.JSONOpts{ - IgnoreUndefinedAttributes: true, - }, - }) + rawStateValue, err := req.RawState.UnmarshalWithOpts(resourceSchemaType, unmarshalOpts) if err != nil { resp.Diagnostics.AddError( @@ -147,7 +152,7 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS priorSchemaType := resourceStateUpgrader.PriorSchema.TerraformType(ctx) - rawStateValue, err := req.RawState.Unmarshal(priorSchemaType) + rawStateValue, err := req.RawState.UnmarshalWithOpts(priorSchemaType, unmarshalOpts) if err != nil { resp.Diagnostics.AddError( diff --git a/internal/fwserver/server_upgraderesourcestate_test.go b/internal/fwserver/server_upgraderesourcestate_test.go index ef1c7b931..796099862 100644 --- a/internal/fwserver/server_upgraderesourcestate_test.go +++ b/internal/fwserver/server_upgraderesourcestate_test.go @@ -466,6 +466,91 @@ func TestServerUpgradeResourceState(t *testing.T) { }, }, }, + "PriorSchema-and-State-json-mismatch": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.UpgradeResourceStateRequest{ + RawState: testNewRawState(t, map[string]interface{}{ + "id": "test-id-value", + "required_attribute": true, + "nonexistent_attribute": "value", + }), + ResourceSchema: schema, + ResourceType: &testprovider.ResourceType{ + GetSchemaMethod: func(_ context.Context) (tfsdk.Schema, diag.Diagnostics) { + return schema, nil + }, + NewResourceMethod: func(_ context.Context, _ tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) { + return &testprovider.ResourceWithUpgradeState{ + Resource: &testprovider.Resource{}, + UpgradeStateMethod: func(ctx context.Context) map[int64]tfsdk.ResourceStateUpgrader { + return map[int64]tfsdk.ResourceStateUpgrader{ + 0: { + PriorSchema: &tfsdk.Schema{ + Attributes: map[string]tfsdk.Attribute{ + "id": { + Type: types.StringType, + Computed: true, + }, + "optional_attribute": { + Type: types.BoolType, + Optional: true, + }, + "required_attribute": { + Type: types.BoolType, + Required: true, + }, + }, + }, + StateUpgrader: func(ctx context.Context, req tfsdk.UpgradeResourceStateRequest, resp *tfsdk.UpgradeResourceStateResponse) { + var priorStateData struct { + Id string `tfsdk:"id"` + OptionalAttribute *bool `tfsdk:"optional_attribute"` + RequiredAttribute bool `tfsdk:"required_attribute"` + } + + resp.Diagnostics.Append(req.State.Get(ctx, &priorStateData)...) + + if resp.Diagnostics.HasError() { + return + } + + upgradedStateData := struct { + Id string `tfsdk:"id"` + OptionalAttribute *string `tfsdk:"optional_attribute"` + RequiredAttribute string `tfsdk:"required_attribute"` + }{ + Id: priorStateData.Id, + RequiredAttribute: fmt.Sprintf("%t", priorStateData.RequiredAttribute), + } + + if priorStateData.OptionalAttribute != nil { + v := fmt.Sprintf("%t", *priorStateData.OptionalAttribute) + upgradedStateData.OptionalAttribute = &v + } + + resp.Diagnostics.Append(resp.State.Set(ctx, upgradedStateData)...) + }, + }, + } + }, + }, nil + }, + }, + Version: 0, + }, + expectedResponse: &fwserver.UpgradeResourceStateResponse{ + UpgradedState: &tfsdk.State{ + Raw: tftypes.NewValue(schemaType, map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "test-id-value"), + "optional_attribute": tftypes.NewValue(tftypes.String, nil), + "required_attribute": tftypes.NewValue(tftypes.String, "true"), + }), + Schema: schema, + }, + }, + }, "UpgradedState-missing": { server: &fwserver.Server{ Provider: &testprovider.Provider{},