From e3804810a9c95fe590f6eb3807a12d86a0ecf76e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Oct 2020 15:05:49 -0400 Subject: [PATCH 1/2] Accept JSON encoded dynamic values from providers Core was previously ignoring JSON-encoded dynamic values, but these are technically supported, so we must either error or accept the value. Since we already have the decoder for Json state, it's minimal effort to support this on all plugin methods too. This change also gives providers an easy way to implement the UpgradeResourceState method. The obvious implementation of returning the same JSON-encoded value has tripped up a few providers not using the legacy SDK already, and we should have at least indicated that the value was being lost. --- plugin/grpc_provider.go | 99 +++++++------- plugin/grpc_provider_test.go | 249 +++++++++++++++++++++++++++++++++++ 2 files changed, 299 insertions(+), 49 deletions(-) diff --git a/plugin/grpc_provider.go b/plugin/grpc_provider.go index 3f29e0f10..60586d399 100644 --- a/plugin/grpc_provider.go +++ b/plugin/grpc_provider.go @@ -12,6 +12,7 @@ import ( proto "github.com/hashicorp/terraform/internal/tfplugin5" "github.com/hashicorp/terraform/plugin/convert" "github.com/hashicorp/terraform/providers" + ctyjson "github.com/zclconf/go-cty/cty/json" "github.com/zclconf/go-cty/cty/msgpack" "google.golang.org/grpc" ) @@ -185,13 +186,10 @@ func (p *GRPCProvider) PrepareProviderConfig(r providers.PrepareProviderConfigRe return resp } - config := cty.NullVal(ty) - if protoResp.PreparedConfig != nil { - config, err = msgpack.Unmarshal(protoResp.PreparedConfig.Msgpack, ty) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + config, err := decodeDynamicValue(protoResp.PreparedConfig, ty) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resp.PreparedConfig = config @@ -270,16 +268,19 @@ func (p *GRPCProvider) UpgradeResourceState(r providers.UpgradeResourceStateRequ } resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) - state := cty.NullVal(resSchema.Block.ImpliedType()) - if protoResp.UpgradedState != nil { - state, err = msgpack.Unmarshal(protoResp.UpgradedState.Msgpack, resSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + ty := resSchema.Block.ImpliedType() + resp.UpgradedState = cty.NullVal(ty) + if protoResp.UpgradedState == nil { + return resp } + state, err := decodeDynamicValue(protoResp.UpgradedState, ty) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp + } resp.UpgradedState = state + return resp } @@ -361,13 +362,10 @@ func (p *GRPCProvider) ReadResource(r providers.ReadResourceRequest) (resp provi } resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) - state := cty.NullVal(resSchema.Block.ImpliedType()) - if protoResp.NewState != nil { - state, err = msgpack.Unmarshal(protoResp.NewState.Msgpack, resSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + state, err := decodeDynamicValue(protoResp.NewState, resSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resp.NewState = state resp.Private = protoResp.Private @@ -423,13 +421,10 @@ func (p *GRPCProvider) PlanResourceChange(r providers.PlanResourceChangeRequest) } resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) - state := cty.NullVal(resSchema.Block.ImpliedType()) - if protoResp.PlannedState != nil { - state, err = msgpack.Unmarshal(protoResp.PlannedState.Msgpack, resSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + state, err := decodeDynamicValue(protoResp.PlannedState, resSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resp.PlannedState = state @@ -492,13 +487,10 @@ func (p *GRPCProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques resp.Private = protoResp.Private - state := cty.NullVal(resSchema.Block.ImpliedType()) - if protoResp.NewState != nil { - state, err = msgpack.Unmarshal(protoResp.NewState.Msgpack, resSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + state, err := decodeDynamicValue(protoResp.NewState, resSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resp.NewState = state @@ -529,13 +521,10 @@ func (p *GRPCProvider) ImportResourceState(r providers.ImportResourceStateReques } resSchema := p.getResourceSchema(resource.TypeName) - state := cty.NullVal(resSchema.Block.ImpliedType()) - if imported.State != nil { - state, err = msgpack.Unmarshal(imported.State.Msgpack, resSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + state, err := decodeDynamicValue(imported.State, resSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resource.State = state resp.ImportedResources = append(resp.ImportedResources, resource) @@ -579,13 +568,10 @@ func (p *GRPCProvider) ReadDataSource(r providers.ReadDataSourceRequest) (resp p } resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) - state := cty.NullVal(dataSchema.Block.ImpliedType()) - if protoResp.State != nil { - state, err = msgpack.Unmarshal(protoResp.State.Msgpack, dataSchema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) - return resp - } + state, err := decodeDynamicValue(protoResp.State, dataSchema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp } resp.State = state @@ -613,3 +599,18 @@ func (p *GRPCProvider) Close() error { p.PluginClient.Kill() return nil } + +// Decode a DynamicValue from either the JSON or MsgPack encoding. +func decodeDynamicValue(v *proto.DynamicValue, ty cty.Type) (cty.Value, error) { + // always return a valid value + res := cty.NullVal(ty) + if v == nil { + return res, nil + } + + if len(v.Msgpack) > 0 { + return msgpack.Unmarshal(v.Msgpack, ty) + } + + return ctyjson.Unmarshal(v.Json, ty) +} diff --git a/plugin/grpc_provider_test.go b/plugin/grpc_provider_test.go index 5abbff51a..3746ae220 100644 --- a/plugin/grpc_provider_test.go +++ b/plugin/grpc_provider_test.go @@ -177,6 +177,37 @@ func TestGRPCProvider_UpgradeResourceState(t *testing.T) { } } +func TestGRPCProvider_UpgradeResourceStateJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + client.EXPECT().UpgradeResourceState( + gomock.Any(), + gomock.Any(), + ).Return(&proto.UpgradeResourceState_Response{ + UpgradedState: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + }, nil) + + resp := p.UpgradeResourceState(providers.UpgradeResourceStateRequest{ + TypeName: "resource", + Version: 0, + RawStateJSON: []byte(`{"old_attr":"bar"}`), + }) + checkDiags(t, resp.Diagnostics) + + expected := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }) + + if !cmp.Equal(expected, resp.UpgradedState, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expected, resp.UpgradedState, typeComparer, valueComparer, equateEmpty)) + } +} + func TestGRPCProvider_Configure(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{ @@ -246,6 +277,39 @@ func TestGRPCProvider_ReadResource(t *testing.T) { } } +func TestGRPCProvider_ReadResourceJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + client.EXPECT().ReadResource( + gomock.Any(), + gomock.Any(), + ).Return(&proto.ReadResource_Response{ + NewState: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + }, nil) + + resp := p.ReadResource(providers.ReadResourceRequest{ + TypeName: "resource", + PriorState: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("foo"), + }), + }) + + checkDiags(t, resp.Diagnostics) + + expected := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }) + + if !cmp.Equal(expected, resp.NewState, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expected, resp.NewState, typeComparer, valueComparer, equateEmpty)) + } +} + func TestGRPCProvider_PlanResourceChange(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{ @@ -309,6 +373,69 @@ func TestGRPCProvider_PlanResourceChange(t *testing.T) { } } +func TestGRPCProvider_PlanResourceChangeJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + expectedPrivate := []byte(`{"meta": "data"}`) + + client.EXPECT().PlanResourceChange( + gomock.Any(), + gomock.Any(), + ).Return(&proto.PlanResourceChange_Response{ + PlannedState: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + RequiresReplace: []*proto.AttributePath{ + { + Steps: []*proto.AttributePath_Step{ + { + Selector: &proto.AttributePath_Step_AttributeName{ + AttributeName: "attr", + }, + }, + }, + }, + }, + PlannedPrivate: expectedPrivate, + }, nil) + + resp := p.PlanResourceChange(providers.PlanResourceChangeRequest{ + TypeName: "resource", + PriorState: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("foo"), + }), + ProposedNewState: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }), + Config: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }), + }) + + checkDiags(t, resp.Diagnostics) + + expectedState := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }) + + if !cmp.Equal(expectedState, resp.PlannedState, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expectedState, resp.PlannedState, typeComparer, valueComparer, equateEmpty)) + } + + expectedReplace := `[]cty.Path{cty.Path{cty.GetAttrStep{Name:"attr"}}}` + replace := fmt.Sprintf("%#v", resp.RequiresReplace) + if expectedReplace != replace { + t.Fatalf("expected %q, got %q", expectedReplace, replace) + } + + if !bytes.Equal(expectedPrivate, resp.PlannedPrivate) { + t.Fatalf("expected %q, got %q", expectedPrivate, resp.PlannedPrivate) + } +} + func TestGRPCProvider_ApplyResourceChange(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{ @@ -355,6 +482,52 @@ func TestGRPCProvider_ApplyResourceChange(t *testing.T) { t.Fatalf("expected %q, got %q", expectedPrivate, resp.Private) } } +func TestGRPCProvider_ApplyResourceChangeJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + expectedPrivate := []byte(`{"meta": "data"}`) + + client.EXPECT().ApplyResourceChange( + gomock.Any(), + gomock.Any(), + ).Return(&proto.ApplyResourceChange_Response{ + NewState: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + Private: expectedPrivate, + }, nil) + + resp := p.ApplyResourceChange(providers.ApplyResourceChangeRequest{ + TypeName: "resource", + PriorState: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("foo"), + }), + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }), + Config: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }), + PlannedPrivate: expectedPrivate, + }) + + checkDiags(t, resp.Diagnostics) + + expectedState := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }) + + if !cmp.Equal(expectedState, resp.NewState, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expectedState, resp.NewState, typeComparer, valueComparer, equateEmpty)) + } + + if !bytes.Equal(expectedPrivate, resp.Private) { + t.Fatalf("expected %q, got %q", expectedPrivate, resp.Private) + } +} func TestGRPCProvider_ImportResourceState(t *testing.T) { client := mockProviderClient(t) @@ -399,6 +572,49 @@ func TestGRPCProvider_ImportResourceState(t *testing.T) { t.Fatal(cmp.Diff(expectedResource, imported, typeComparer, valueComparer, equateEmpty)) } } +func TestGRPCProvider_ImportResourceStateJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + expectedPrivate := []byte(`{"meta": "data"}`) + + client.EXPECT().ImportResourceState( + gomock.Any(), + gomock.Any(), + ).Return(&proto.ImportResourceState_Response{ + ImportedResources: []*proto.ImportResourceState_ImportedResource{ + { + TypeName: "resource", + State: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + Private: expectedPrivate, + }, + }, + }, nil) + + resp := p.ImportResourceState(providers.ImportResourceStateRequest{ + TypeName: "resource", + ID: "foo", + }) + + checkDiags(t, resp.Diagnostics) + + expectedResource := providers.ImportedResource{ + TypeName: "resource", + State: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }), + Private: expectedPrivate, + } + + imported := resp.ImportedResources[0] + if !cmp.Equal(expectedResource, imported, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expectedResource, imported, typeComparer, valueComparer, equateEmpty)) + } +} func TestGRPCProvider_ReadDataSource(t *testing.T) { client := mockProviderClient(t) @@ -432,3 +648,36 @@ func TestGRPCProvider_ReadDataSource(t *testing.T) { t.Fatal(cmp.Diff(expected, resp.State, typeComparer, valueComparer, equateEmpty)) } } + +func TestGRPCProvider_ReadDataSourceJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + client.EXPECT().ReadDataSource( + gomock.Any(), + gomock.Any(), + ).Return(&proto.ReadDataSource_Response{ + State: &proto.DynamicValue{ + Json: []byte(`{"attr":"bar"}`), + }, + }, nil) + + resp := p.ReadDataSource(providers.ReadDataSourceRequest{ + TypeName: "data", + Config: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("foo"), + }), + }) + + checkDiags(t, resp.Diagnostics) + + expected := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("bar"), + }) + + if !cmp.Equal(expected, resp.State, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expected, resp.State, typeComparer, valueComparer, equateEmpty)) + } +} From 353937411de45abbe5ffaf7aa0d357377f831a49 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Oct 2020 09:31:37 -0400 Subject: [PATCH 2/2] handle empty json --- plugin/grpc_provider.go | 11 +++++++---- plugin/grpc_provider_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/plugin/grpc_provider.go b/plugin/grpc_provider.go index 60586d399..495347bfd 100644 --- a/plugin/grpc_provider.go +++ b/plugin/grpc_provider.go @@ -603,14 +603,17 @@ func (p *GRPCProvider) Close() error { // Decode a DynamicValue from either the JSON or MsgPack encoding. func decodeDynamicValue(v *proto.DynamicValue, ty cty.Type) (cty.Value, error) { // always return a valid value + var err error res := cty.NullVal(ty) if v == nil { return res, nil } - if len(v.Msgpack) > 0 { - return msgpack.Unmarshal(v.Msgpack, ty) + switch { + case len(v.Msgpack) > 0: + res, err = msgpack.Unmarshal(v.Msgpack, ty) + case len(v.Json) > 0: + res, err = ctyjson.Unmarshal(v.Json, ty) } - - return ctyjson.Unmarshal(v.Json, ty) + return res, err } diff --git a/plugin/grpc_provider_test.go b/plugin/grpc_provider_test.go index 3746ae220..9cf2dca31 100644 --- a/plugin/grpc_provider_test.go +++ b/plugin/grpc_provider_test.go @@ -310,6 +310,38 @@ func TestGRPCProvider_ReadResourceJSON(t *testing.T) { } } +func TestGRPCProvider_ReadEmptyJSON(t *testing.T) { + client := mockProviderClient(t) + p := &GRPCProvider{ + client: client, + } + + client.EXPECT().ReadResource( + gomock.Any(), + gomock.Any(), + ).Return(&proto.ReadResource_Response{ + NewState: &proto.DynamicValue{ + Json: []byte(``), + }, + }, nil) + + obj := cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("foo"), + }) + resp := p.ReadResource(providers.ReadResourceRequest{ + TypeName: "resource", + PriorState: obj, + }) + + checkDiags(t, resp.Diagnostics) + + expected := cty.NullVal(obj.Type()) + + if !cmp.Equal(expected, resp.NewState, typeComparer, valueComparer, equateEmpty) { + t.Fatal(cmp.Diff(expected, resp.NewState, typeComparer, valueComparer, equateEmpty)) + } +} + func TestGRPCProvider_PlanResourceChange(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{