From 6cfe57fc528dbf898669cd6aeca0619be3460dd0 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 27 Jul 2022 08:47:23 +0100 Subject: [PATCH 1/4] Using UnmarshalWithOpts in order to ignore undefined attributes (#415) --- .../fwserver/server_upgraderesourcestate.go | 13 +++++++-- .../server_upgraderesourcestate_test.go | 29 ++++++++++--------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/internal/fwserver/server_upgraderesourcestate.go b/internal/fwserver/server_upgraderesourcestate.go index 93905b6ea..b744dd450 100644 --- a/internal/fwserver/server_upgraderesourcestate.go +++ b/internal/fwserver/server_upgraderesourcestate.go @@ -4,10 +4,12 @@ import ( "context" "fmt" + "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-go/tftypes" + "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/internal/logging" "github.com/hashicorp/terraform-plugin-framework/tfsdk" - "github.com/hashicorp/terraform-plugin-go/tfprotov6" ) // UpgradeResourceStateRequest is the framework server request for the @@ -57,12 +59,19 @@ func (s *Server) UpgradeResourceState(ctx context.Context, req *UpgradeResourceS // version which would never get called, the framework can introduce a // unit test helper. // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/113 + // + // UnmarshalWithOpts allows optionally ignoring instances in which elements being + // do not have a corresponding attribute within the schema. if req.Version == req.ResourceSchema.Version { logging.FrameworkTrace(ctx, "UpgradeResourceState request version matches current Schema version, using framework defined passthrough implementation") resourceSchemaType := req.ResourceSchema.TerraformType(ctx) - rawStateValue, err := req.RawState.Unmarshal(resourceSchemaType) + rawStateValue, err := req.RawState.UnmarshalWithOpts(resourceSchemaType, tftypes.UnmarshalOpts{ + JSONOpts: tftypes.JSONOpts{ + IgnoreUndefinedAttributes: true, + }, + }) if err != nil { resp.Diagnostics.AddError( diff --git a/internal/fwserver/server_upgraderesourcestate_test.go b/internal/fwserver/server_upgraderesourcestate_test.go index e8ed00716..ef1c7b931 100644 --- a/internal/fwserver/server_upgraderesourcestate_test.go +++ b/internal/fwserver/server_upgraderesourcestate_test.go @@ -7,14 +7,15 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-go/tftypes" + "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/internal/fwserver" "github.com/hashicorp/terraform-plugin-framework/internal/testing/testprovider" "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/tfsdk" "github.com/hashicorp/terraform-plugin-framework/types" - "github.com/hashicorp/terraform-plugin-go/tfprotov6" - "github.com/hashicorp/terraform-plugin-go/tftypes" ) func TestServerUpgradeResourceState(t *testing.T) { @@ -599,9 +600,11 @@ func TestServerUpgradeResourceState(t *testing.T) { Provider: &testprovider.Provider{}, }, request: &fwserver.UpgradeResourceStateRequest{ - RawState: &tfprotov6.RawState{ - JSON: []byte(`{"nonexistent_attribute":"value"}`), - }, + 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) { @@ -615,15 +618,13 @@ func TestServerUpgradeResourceState(t *testing.T) { Version: 1, // Must match current tfsdk.Schema version to trigger framework implementation }, expectedResponse: &fwserver.UpgradeResourceStateResponse{ - Diagnostics: diag.Diagnostics{ - diag.NewErrorDiagnostic( - "Unable to Read Previously Saved State for UpgradeResourceState", - "There was an error reading the saved resource state using the current resource schema.\n\n"+ - "If this resource state was last refreshed with Terraform CLI 0.11 and earlier, it must be refreshed or applied with an older provider version first. "+ - "If you manually modified the resource state, you will need to manually modify it to match the current resource schema. "+ - "Otherwise, please report this to the provider developer:\n\n"+ - "ElementKeyValue(tftypes.String): unsupported attribute \"nonexistent_attribute\"", - ), + 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, }, }, }, From 489ea99e166fe64c06acea85d682605ff3b3064a Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Thu, 28 Jul 2022 07:48:50 +0100 Subject: [PATCH 2/4] 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{}, From c412148dcacf40937ccf96f9c34ef84a5b2567ab Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Thu, 28 Jul 2022 16:16:13 +0100 Subject: [PATCH 3/4] Bumping terraform-plugin-go to v0.13.0 (#415) --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index d468b1d89..065ff1d25 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.17 require ( github.com/google/go-cmp v0.5.8 - github.com/hashicorp/terraform-plugin-go v0.12.0 + github.com/hashicorp/terraform-plugin-go v0.13.0 github.com/hashicorp/terraform-plugin-log v0.7.0 ) diff --git a/go.sum b/go.sum index c945c7e9b..2f4601777 100644 --- a/go.sum +++ b/go.sum @@ -66,6 +66,8 @@ github.com/hashicorp/go-uuid v1.0.3/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/b github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/terraform-plugin-go v0.12.0 h1:6wW9mT1dSs0Xq4LR6HXj1heQ5ovr5GxXNJwkErZzpJw= github.com/hashicorp/terraform-plugin-go v0.12.0/go.mod h1:kwhmaWHNDvT1B3QiSJdAtrB/D4RaKSY/v3r2BuoWK4M= +github.com/hashicorp/terraform-plugin-go v0.13.0 h1:Zm+o91HUOcTLotaEu3X2jV/6wNi6f09gkZwGg/MDvCk= +github.com/hashicorp/terraform-plugin-go v0.13.0/go.mod h1:NYGFEM9GeRdSl52txue3RcBDFt2tufaqS22iURP8Bxs= github.com/hashicorp/terraform-plugin-log v0.6.0/go.mod h1:p4R1jWBXRTvL4odmEkFfDdhUjHf9zcs/BCoNHAc7IK4= github.com/hashicorp/terraform-plugin-log v0.7.0 h1:SDxJUyT8TwN4l5b5/VkiTIaQgY6R+Y2BQ0sRZftGKQs= github.com/hashicorp/terraform-plugin-log v0.7.0/go.mod h1:p4R1jWBXRTvL4odmEkFfDdhUjHf9zcs/BCoNHAc7IK4= From 750038270d88f5296b8a62f27454c172d123a7b6 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Thu, 28 Jul 2022 17:43:51 +0100 Subject: [PATCH 4/4] go mod tidy (#415) --- go.sum | 3 --- 1 file changed, 3 deletions(-) diff --git a/go.sum b/go.sum index 2f4601777..bb27976c3 100644 --- a/go.sum +++ b/go.sum @@ -64,11 +64,8 @@ github.com/hashicorp/go-plugin v1.4.4/go.mod h1:viDMjcLJuDui6pXb8U4HVfb8AamCWhHG github.com/hashicorp/go-uuid v1.0.3 h1:2gKiV6YVmrJ1i2CKKa9obLvRieoRGviZFL26PcT/Co8= github.com/hashicorp/go-uuid v1.0.3/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= -github.com/hashicorp/terraform-plugin-go v0.12.0 h1:6wW9mT1dSs0Xq4LR6HXj1heQ5ovr5GxXNJwkErZzpJw= -github.com/hashicorp/terraform-plugin-go v0.12.0/go.mod h1:kwhmaWHNDvT1B3QiSJdAtrB/D4RaKSY/v3r2BuoWK4M= github.com/hashicorp/terraform-plugin-go v0.13.0 h1:Zm+o91HUOcTLotaEu3X2jV/6wNi6f09gkZwGg/MDvCk= github.com/hashicorp/terraform-plugin-go v0.13.0/go.mod h1:NYGFEM9GeRdSl52txue3RcBDFt2tufaqS22iURP8Bxs= -github.com/hashicorp/terraform-plugin-log v0.6.0/go.mod h1:p4R1jWBXRTvL4odmEkFfDdhUjHf9zcs/BCoNHAc7IK4= github.com/hashicorp/terraform-plugin-log v0.7.0 h1:SDxJUyT8TwN4l5b5/VkiTIaQgY6R+Y2BQ0sRZftGKQs= github.com/hashicorp/terraform-plugin-log v0.7.0/go.mod h1:p4R1jWBXRTvL4odmEkFfDdhUjHf9zcs/BCoNHAc7IK4= github.com/hashicorp/terraform-registry-address v0.0.0-20220623143253-7d51757b572c h1:D8aRO6+mTqHfLsK/BC3j5OAoogv1WLRWzY1AaTo3rBg=