From ea35eb2d33e12857b4a80a2b5680cc5e68b63389 Mon Sep 17 00:00:00 2001 From: VenelinMartinov Date: Mon, 25 Nov 2024 15:23:48 +0000 Subject: [PATCH] Fix PF provider_server detailed diff handling (#2628) This change fixes an issue with the `provider_server` implementation's detailed diff handling. Previously passing a detailed diff to it would result in the previews being deleted. This is tested as part of https://github.com/pulumi/pulumi-terraform-bridge/pull/2629 The problem was that we were re-calculating the `diffs` and `replaces` keys for the GRPC Diff protocol in the `provider_server` implementation but also doing that incorrectly. Instead this change now makes `provider_server`'s `marshalDiff` just pass through the `diffs` and `replaces` which we have already calculated in https://github.com/pulumi/pulumi-terraform-bridge/blob/1d6b032f3e376af4667c6c4d80a65eff072df807/pkg/pf/tfbridge/provider_diff.go#L108-L109 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2620 --- pkg/pf/internal/plugin/provider_server.go | 81 +++++++++--------- .../internal/plugin/provider_server_test.go | 82 +++++++++++++++++++ 2 files changed, 124 insertions(+), 39 deletions(-) create mode 100644 pkg/pf/internal/plugin/provider_server_test.go diff --git a/pkg/pf/internal/plugin/provider_server.go b/pkg/pf/internal/plugin/provider_server.go index 61b488c05..4254c3309 100644 --- a/pkg/pf/internal/plugin/provider_server.go +++ b/pkg/pf/internal/plugin/provider_server.go @@ -23,9 +23,9 @@ import ( pbempty "github.com/golang/protobuf/ptypes/empty" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" "github.com/pulumi/pulumi/sdk/v3/go/common/resource/config" - "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" pl "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" + "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -76,82 +76,85 @@ func (p *providerServer) checkNYI(method string, err error) error { return err } +func pluginDiffKindToRPC(kind pl.DiffKind) pulumirpc.PropertyDiff_Kind { + switch kind { + case pl.DiffAdd: + return pulumirpc.PropertyDiff_ADD + case pl.DiffAddReplace: + return pulumirpc.PropertyDiff_ADD_REPLACE + case pl.DiffDelete: + return pulumirpc.PropertyDiff_DELETE + case pl.DiffDeleteReplace: + return pulumirpc.PropertyDiff_DELETE_REPLACE + case pl.DiffUpdate: + return pulumirpc.PropertyDiff_UPDATE + case pl.DiffUpdateReplace: + return pulumirpc.PropertyDiff_UPDATE_REPLACE + default: + contract.Assertf(false, "unknown diff kind: %v", kind) + return pulumirpc.PropertyDiff_ADD + } +} + func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffResponse, error) { - changes := pulumirpc.DiffResponse_DIFF_UNKNOWN + var changes pulumirpc.DiffResponse_DiffChanges switch diff.Changes { case pl.DiffNone: changes = pulumirpc.DiffResponse_DIFF_NONE case pl.DiffSome: changes = pulumirpc.DiffResponse_DIFF_SOME + case pl.DiffUnknown: + changes = pulumirpc.DiffResponse_DIFF_UNKNOWN } // Infer the result from the detailed diff. var diffs, replaces []string var detailedDiff map[string]*pulumirpc.PropertyDiff - if len(diff.DetailedDiff) == 0 { - diffs = make([]string, len(diff.ChangedKeys)) - for i, k := range diff.ChangedKeys { - diffs[i] = string(k) - } - replaces = make([]string, len(diff.ReplaceKeys)) - for i, k := range diff.ReplaceKeys { - replaces[i] = string(k) - } - } else { - changes = pulumirpc.DiffResponse_DIFF_SOME - + if len(diff.DetailedDiff) != 0 { detailedDiff = make(map[string]*pulumirpc.PropertyDiff) for path, diff := range diff.DetailedDiff { - diffs = append(diffs, path) - - var kind pulumirpc.PropertyDiff_Kind - switch diff.Kind { - case pl.DiffAdd: - kind = pulumirpc.PropertyDiff_ADD - case pl.DiffAddReplace: - kind, replaces = pulumirpc.PropertyDiff_ADD_REPLACE, append(replaces, path) - case pl.DiffDelete: - kind = pulumirpc.PropertyDiff_DELETE - case pl.DiffDeleteReplace: - kind, replaces = pulumirpc.PropertyDiff_DELETE, append(replaces, path) - case pl.DiffUpdate: - kind = pulumirpc.PropertyDiff_UPDATE - case pl.DiffUpdateReplace: - kind, replaces = pulumirpc.PropertyDiff_UPDATE_REPLACE, append(replaces, path) - } - detailedDiff[path] = &pulumirpc.PropertyDiff{ - Kind: kind, + Kind: pluginDiffKindToRPC(diff.Kind), InputDiff: diff.InputDiff, } } } + diffs = make([]string, len(diff.ChangedKeys)) + for i, k := range diff.ChangedKeys { + diffs[i] = string(k) + } + replaces = make([]string, len(diff.ReplaceKeys)) + for i, k := range diff.ReplaceKeys { + replaces[i] = string(k) + } + return &pulumirpc.DiffResponse{ Replaces: replaces, DeleteBeforeReplace: diff.DeleteBeforeReplace, Changes: changes, Diffs: diffs, DetailedDiff: detailedDiff, + HasDetailedDiff: len(detailedDiff) > 0, }, nil } type forwardServer struct { - plugin.UnimplementedProvider + pl.UnimplementedProvider - parameterize func(context.Context, plugin.ParameterizeRequest) (plugin.ParameterizeResponse, error) + parameterize func(context.Context, pl.ParameterizeRequest) (pl.ParameterizeResponse, error) } func (p forwardServer) Parameterize( - ctx context.Context, req plugin.ParameterizeRequest, -) (plugin.ParameterizeResponse, error) { + ctx context.Context, req pl.ParameterizeRequest, +) (pl.ParameterizeResponse, error) { return p.parameterize(ctx, req) } func (p *providerServer) Parameterize( ctx context.Context, req *pulumirpc.ParameterizeRequest, ) (*pulumirpc.ParameterizeResponse, error) { - return plugin.NewProviderServer(&forwardServer{ + return pl.NewProviderServer(&forwardServer{ parameterize: p.provider.ParameterizeWithContext, }).Parameterize(ctx, req) } @@ -167,7 +170,7 @@ func (p *providerServer) GetSchema(ctx context.Context, } subpackageVersion = &ver } - schema, err := p.provider.GetSchemaWithContext(ctx, plugin.GetSchemaRequest{ + schema, err := p.provider.GetSchemaWithContext(ctx, pl.GetSchemaRequest{ Version: req.GetVersion(), SubpackageName: req.SubpackageName, SubpackageVersion: subpackageVersion, diff --git a/pkg/pf/internal/plugin/provider_server_test.go b/pkg/pf/internal/plugin/provider_server_test.go new file mode 100644 index 000000000..dcac92f09 --- /dev/null +++ b/pkg/pf/internal/plugin/provider_server_test.go @@ -0,0 +1,82 @@ +package plugin + +import ( + "testing" + + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" + pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" + "github.com/stretchr/testify/require" +) + +func TestMarshalDiff(t *testing.T) { + t.Parallel() + + runTest := func(t *testing.T, diff plugin.DiffResult) *pulumirpc.DiffResponse { + server := providerServer{} + resp, err := server.marshalDiff(diff) + require.NoError(t, err) + return resp + } + + t.Run("no diffs", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffNone, + ReplaceKeys: []resource.PropertyKey{}, + ChangedKeys: []resource.PropertyKey{}, + DetailedDiff: map[string]plugin.PropertyDiff{}, + } + + require.Equal(t, &pulumirpc.DiffResponse{ + Replaces: []string{}, + Changes: pulumirpc.DiffResponse_DIFF_NONE, + Diffs: []string{}, + }, runTest(t, diff)) + }) + + t.Run("diff without detailed diff", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"replace"}, + ChangedKeys: []resource.PropertyKey{"change"}, + } + + require.Equal(t, &pulumirpc.DiffResponse{ + Replaces: []string{"replace"}, + Changes: pulumirpc.DiffResponse_DIFF_SOME, + Diffs: []string{"change"}, + }, runTest(t, diff)) + }) + + t.Run("diff with detailed diff", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"replace"}, + ChangedKeys: []resource.PropertyKey{"change", "replace"}, + DetailedDiff: map[string]plugin.PropertyDiff{ + "change": { + Kind: plugin.DiffAdd, + }, + "replace": { + Kind: plugin.DiffDeleteReplace, + }, + }, + } + + require.Equal(t, &pulumirpc.DiffResponse{ + Replaces: []string{ + "replace", + }, + Changes: pulumirpc.DiffResponse_DIFF_SOME, + Diffs: []string{ + "change", + "replace", + }, + DetailedDiff: map[string]*pulumirpc.PropertyDiff{ + "change": {}, + "replace": {Kind: pulumirpc.PropertyDiff_DELETE_REPLACE}, + }, + HasDetailedDiff: true, + }, runTest(t, diff)) + }) +}