From c975862f43cfa2964e0a89718d3a441bd4288198 Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Tue, 16 Jul 2024 15:52:53 -0700 Subject: [PATCH] [PF] Allow overriding Number with String for PF fields (#2155) This is motivated by [`terraform-provider-vantage blocked on numeric ID support #1198`](https://github.com/pulumi/pulumi-terraform-bridge/issues/1198). My plan for #1198 is to override the "id" fields of each resource to be a `"string"`, which is how we currently handle SDK based providers with the same problem. --- pf/internal/check/checks.go | 15 ++- pf/internal/check/not_supported.go | 4 +- .../pulumi-resource-testbridge/schema.json | 24 ++++ pf/tests/internal/testprovider/testbridge.go | 7 ++ .../testbridge_resource_int_id.go | 109 ++++++++++++++++++ pf/tests/provider_check_test.go | 24 ++++ pf/tests/provider_create_test.go | 21 ++++ pf/tests/provider_update_test.go | 26 +++++ pkg/convert/adapter.go | 89 ++++++++++++++ pkg/convert/encoding.go | 18 ++- pkg/convert/encoding_test.go | 51 ++++++++ pkg/convert/schema_context.go | 8 ++ 12 files changed, 390 insertions(+), 6 deletions(-) create mode 100644 pf/tests/internal/testprovider/testbridge_resource_int_id.go create mode 100644 pkg/convert/adapter.go diff --git a/pf/internal/check/checks.go b/pf/internal/check/checks.go index fb31bfe57..aad95f0e9 100644 --- a/pf/internal/check/checks.go +++ b/pf/internal/check/checks.go @@ -19,6 +19,7 @@ import ( "fmt" "github.com/pulumi/pulumi/sdk/v3/go/common/diag" + "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" "github.com/pulumi/pulumi-terraform-bridge/pf/internal/muxer" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" @@ -57,7 +58,7 @@ func checkIDProperties(sink diag.Sink, info tfbridge.ProviderInfo, isPFResource if resourceHasComputeID(info, rname) { return true } - ok, reason := resourceHasRegularID(resource) + ok, reason := resourceHasRegularID(resource, info.Resources[rname]) if ok { return true } @@ -77,12 +78,20 @@ func checkIDProperties(sink diag.Sink, info tfbridge.ProviderInfo, isPFResource return nil } -func resourceHasRegularID(resource shim.Resource) (bool, string) { +func resourceHasRegularID(resource shim.Resource, resourceInfo *tfbridge.ResourceInfo) (bool, string) { idSchema, gotID := resource.Schema().GetOk("id") if !gotID { return false, `no "id" attribute` } - if idSchema.Type() != shim.TypeString { + var typeOverride tokens.Type + if resourceInfo != nil { + if id := resourceInfo.Fields["id"]; id != nil { + typeOverride = id.Type + } + } + + // If the user over-rode the type to be a string, don't reject. + if idSchema.Type() != shim.TypeString && typeOverride != "string" { return false, `"id" attribute is not of type String` } if idSchema.Sensitive() { diff --git a/pf/internal/check/not_supported.go b/pf/internal/check/not_supported.go index 9fcf3cf6f..34b8a1148 100644 --- a/pf/internal/check/not_supported.go +++ b/pf/internal/check/not_supported.go @@ -115,7 +115,9 @@ func (u *notSupportedUtil) resource(path string, res *tfbridge.ResourceInfo) { } func (u *notSupportedUtil) schema(path string, schema *tfbridge.SchemaInfo) { - u.assertIsZero(path+".Type", schema.Type) + if schema.Type != "string" { + u.assertIsZero(path+".Type", schema.Type) + } u.assertIsZero(path+".AltTypes", schema.AltTypes) u.assertIsZero(path+".NestedType", schema.NestedType) u.assertIsZero(path+".Transform", schema.Transform) diff --git a/pf/tests/internal/testprovider/cmd/pulumi-resource-testbridge/schema.json b/pf/tests/internal/testprovider/cmd/pulumi-resource-testbridge/schema.json index bc6e5ee63..fa87449e9 100644 --- a/pf/tests/internal/testprovider/cmd/pulumi-resource-testbridge/schema.json +++ b/pf/tests/internal/testprovider/cmd/pulumi-resource-testbridge/schema.json @@ -281,6 +281,30 @@ } }, "resources": { + "testbridge:index/intID:IntID": { + "properties": { + "name": { + "type": "string" + } + }, + "required": [ + "name" + ], + "inputProperties": { + "name": { + "type": "string" + } + }, + "stateInputs": { + "description": "Input properties used for looking up and filtering IntID resources.\n", + "properties": { + "name": { + "type": "string" + } + }, + "type": "object" + } + }, "testbridge:index/testnest:Testnest": { "properties": { "rules": { diff --git a/pf/tests/internal/testprovider/testbridge.go b/pf/tests/internal/testprovider/testbridge.go index 73b3583fb..1076f515e 100644 --- a/pf/tests/internal/testprovider/testbridge.go +++ b/pf/tests/internal/testprovider/testbridge.go @@ -98,6 +98,12 @@ func SyntheticTestBridgeProvider() tfbridge.ProviderInfo { "testbridge_privst": {Tok: "testbridge:index/testres:Privst"}, "testbridge_autoname_res": {Tok: "testbridge:index/testres:AutoNameRes"}, + "testbridge_int_id_res": { + Tok: "testbridge:index/intID:IntID", + Fields: map[string]*tfbridge.SchemaInfo{ + "id": {Type: "string"}, + }, + }, }, DataSources: map[string]*tfbridge.DataSourceInfo{ @@ -208,5 +214,6 @@ func (p *syntheticProvider) Resources(context.Context) []func() resource.Resourc newTestDefaultInfoRes, newPrivst, newAutoNameRes, + newIntIDRes, } } diff --git a/pf/tests/internal/testprovider/testbridge_resource_int_id.go b/pf/tests/internal/testprovider/testbridge_resource_int_id.go new file mode 100644 index 000000000..356b5ffc6 --- /dev/null +++ b/pf/tests/internal/testprovider/testbridge_resource_int_id.go @@ -0,0 +1,109 @@ +// Copyright 2016-2023, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package testprovider + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/path" + "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/schema" + rschema "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-go/tftypes" +) + +type intIDRes struct{} + +var _ resource.Resource = &intIDRes{} + +func newIntIDRes() resource.Resource { + return &intIDRes{} +} + +func (*intIDRes) schema() rschema.Schema { + return rschema.Schema{ + Attributes: map[string]rschema.Attribute{ + "id": schema.Int64Attribute{ + Computed: true, + PlanModifiers: []planmodifier.Int64{ + int64planmodifier.UseStateForUnknown(), + }, + }, + "name": schema.StringAttribute{ + Required: true, + }, + }, + } +} + +func (e *intIDRes) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_int_id_res" +} + +func (e *intIDRes) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + resp.Schema = e.schema() +} + +func (e *intIDRes) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { + resp.State.Raw = req.Plan.Raw.Copy() // Copy plan to state. + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("id"), 1234)...) +} + +func (e *intIDRes) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) { +} + +func (e *intIDRes) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { + var id int64 + resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root("id"), &id)...) + if resp.Diagnostics.HasError() { + return + } + if id != 1234 { + resp.Diagnostics.AddAttributeError(path.Root("id"), "unexpected value", + fmt.Sprintf("expected 1234, found %d", id)) + } + + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, path.Root("id"), &id)...) + if resp.Diagnostics.HasError() { + return + } + if id != 5678 { + resp.Diagnostics.AddAttributeError(path.Root("id"), "unexpected value", + fmt.Sprintf("expected 5678, found %d", id)) + } + + resp.State.Raw = req.Plan.Raw.Copy() // Copy plan to state. + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("id"), 90)...) +} + +func (e *intIDRes) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + resp.State = e.nilState(ctx) +} + +func (e *intIDRes) nilState(ctx context.Context) tfsdk.State { + typ := e.terraformType(ctx) + return tfsdk.State{ + Raw: tftypes.NewValue(typ, nil), + Schema: e.schema(), + } +} + +func (e *intIDRes) terraformType(ctx context.Context) tftypes.Type { + return e.schema().Type().TerraformType(ctx) +} diff --git a/pf/tests/provider_check_test.go b/pf/tests/provider_check_test.go index 4b56affae..dcbef844a 100644 --- a/pf/tests/provider_check_test.go +++ b/pf/tests/provider_check_test.go @@ -28,6 +28,7 @@ import ( testutils "github.com/pulumi/providertest/replay" "github.com/pulumi/pulumi-terraform-bridge/pf/tests/internal/providerbuilder" + "github.com/pulumi/pulumi-terraform-bridge/pf/tests/internal/testprovider" "github.com/pulumi/pulumi-terraform-bridge/pf/tfbridge" tfbridge0 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" ) @@ -352,3 +353,26 @@ func TestCheck(t *testing.T) { }) } } + +func TestCheckWithIntID(t *testing.T) { + server := newProviderServer(t, testprovider.SyntheticTestBridgeProvider()) + testCase := ` + { + "method": "/pulumirpc.ResourceProvider/Check", + "request": { + "urn": "urn:pulumi:test-stack::basicprogram::testbridge:index/intID:IntID::r1", + "news": { + "name": "name" + }, + "olds": {}, + "randomSeed": "wqZZaHWVfsS1ozo3bdauTfZmjslvWcZpUjn7BzpS79c=" + }, + "response": { + "inputs": { + "name": "name" + } + } + } + ` + testutils.Replay(t, server, testCase) +} diff --git a/pf/tests/provider_create_test.go b/pf/tests/provider_create_test.go index be74d9ea8..2143f6171 100644 --- a/pf/tests/provider_create_test.go +++ b/pf/tests/provider_create_test.go @@ -48,6 +48,27 @@ func TestCreateWithComputedOptionals(t *testing.T) { testutils.Replay(t, server, testCase) } +func TestCreateWithIntID(t *testing.T) { + server := newProviderServer(t, testprovider.SyntheticTestBridgeProvider()) + testCase := ` + { + "method": "/pulumirpc.ResourceProvider/Create", + "request": { + "urn": "urn:pulumi:test-stack::basicprogram::testbridge:index/intID:IntID::r1", + "properties": {}, + "preview": false + }, + "response": { + "id": "1234", + "properties": { + "id": "1234" + } + } + } + ` + testutils.Replay(t, server, testCase) +} + func TestCreateWritesSchemaVersion(t *testing.T) { server := newProviderServer(t, testprovider.RandomProvider()) diff --git a/pf/tests/provider_update_test.go b/pf/tests/provider_update_test.go index cd4ab0e45..babe91ab2 100644 --- a/pf/tests/provider_update_test.go +++ b/pf/tests/provider_update_test.go @@ -58,3 +58,29 @@ func TestUpdateWritesSchemaVersion(t *testing.T) { } `) } + +func TestUpdateWithIntID(t *testing.T) { + server := newProviderServer(t, testprovider.SyntheticTestBridgeProvider()) + testCase := ` + { + "method": "/pulumirpc.ResourceProvider/Update", + "request": { + "id": "1234", + "olds": { + "id": "1234" + }, + "news": { + "id": "5678" + }, + "urn": "urn:pulumi:test-stack::basicprogram::testbridge:index/intID:IntID::r1", + "preview": false + }, + "response": { + "properties": { + "id": "90" + } + } + } + ` + testutils.Replay(t, server, testCase) +} diff --git a/pkg/convert/adapter.go b/pkg/convert/adapter.go new file mode 100644 index 000000000..848d9cb0f --- /dev/null +++ b/pkg/convert/adapter.go @@ -0,0 +1,89 @@ +// Copyright 2016-2024, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package convert + +import ( + "fmt" + "math/big" + "strconv" + + "github.com/hashicorp/terraform-plugin-go/tftypes" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" +) + +// adaptedEncoder wraps an encoder in an adapter during encoding. +// +// Given [resource.PropertyValue] types P_i and P_j and an encoder P_j -> T, the adapter +// function should translate P_i -> P_j. +type adaptedEncoder[T Encoder] struct { + adapter func(resource.PropertyValue) (resource.PropertyValue, error) + encoder T +} + +func (e adaptedEncoder[T]) fromPropertyValue(v resource.PropertyValue) (tftypes.Value, error) { + adapted, err := e.adapter(v) + if err != nil { + return tftypes.Value{}, fmt.Errorf("failed to adapt for %T: %w", e.encoder, err) + } + return e.encoder.fromPropertyValue(adapted) +} + +type adaptedDecoder[T Decoder] struct { + adapter func(tftypes.Value) (tftypes.Value, error) + decoder T +} + +func (d adaptedDecoder[T]) toPropertyValue(v tftypes.Value) (resource.PropertyValue, error) { + adapted, err := d.adapter(v) + if err != nil { + return resource.PropertyValue{}, fmt.Errorf("failed to adapt for %T: %w", d.decoder, err) + } + return d.decoder.toPropertyValue(adapted) +} + +func newIntOverrideStringEncoder() Encoder { + return adaptedEncoder[*numberEncoder]{ + adapter: func(v resource.PropertyValue) (resource.PropertyValue, error) { + if v.IsString() { + f, err := strconv.ParseFloat(v.StringValue(), 64) + if err != nil { + return resource.PropertyValue{}, err + } + return resource.NewProperty(f), nil + } + return v, nil + }, + encoder: &numberEncoder{}, + } +} + +func newStringOverIntDecoder() Decoder { + return adaptedDecoder[*stringDecoder]{ + adapter: func(v tftypes.Value) (tftypes.Value, error) { + if !v.Type().Is(tftypes.Number) { + return v, nil + } + if !v.IsKnown() { + return tftypes.NewValue(tftypes.String, tftypes.UnknownValue), nil + } + var f big.Float + if err := v.As(&f); err != nil { + return tftypes.Value{}, err + } + return tftypes.NewValue(tftypes.String, f.Text('f', -1)), nil + }, + decoder: &stringDecoder{}, + } +} diff --git a/pkg/convert/encoding.go b/pkg/convert/encoding.go index 869f3973d..f72365b88 100644 --- a/pkg/convert/encoding.go +++ b/pkg/convert/encoding.go @@ -167,7 +167,12 @@ func deriveEncoder(pctx *schemaPropContext, t tftypes.Type) (Encoder, error) { case t.Is(tftypes.String): return newStringEncoder(), nil case t.Is(tftypes.Number): - return newNumberEncoder(), nil + switch pctx.TypeInfo() { + case "string": + return newIntOverrideStringEncoder(), nil + default: + return newNumberEncoder(), nil + } case t.Is(tftypes.Bool): return newBoolEncoder(), nil } @@ -239,11 +244,20 @@ func deriveDecoder(pctx *schemaPropContext, t tftypes.Type) (Decoder, error) { case t.Is(tftypes.String): return newStringDecoder(), nil case t.Is(tftypes.Number): - return newNumberDecoder(), nil + switch pctx.TypeInfo() { + case "string": + return newStringOverIntDecoder(), nil + default: + return newNumberDecoder(), nil + } case t.Is(tftypes.Bool): return newBoolDecoder(), nil } + if to := pctx.TypeInfo(); to != "" { + return nil, fmt.Errorf("unable to apply type override %s to upstream type %s", to, t) + } + switch tt := t.(type) { case tftypes.Object: mctx, err := pctx.Object() diff --git a/pkg/convert/encoding_test.go b/pkg/convert/encoding_test.go index 65bfd2053..7e8ffe5ab 100644 --- a/pkg/convert/encoding_test.go +++ b/pkg/convert/encoding_test.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/hexops/autogold/v2" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" @@ -631,3 +632,53 @@ func TestTupleDerivations(t *testing.T) { }) } } + +func TestAdapter(t *testing.T) { + tests := []struct { + name string + input resource.PropertyValue + expected tftypes.Value + error bool + }{ + { + name: "valid", + input: resource.NewProperty("123"), + expected: tftypesNewValue(tftypes.Number, 123), + }, + { + name: "invalid", + input: resource.NewProperty("abc"), + error: true, + }, + { + name: "computed-output", + input: resource.NewOutputProperty(resource.Output{}), + expected: tftypes.NewValue(tftypes.Number, tftypes.UnknownValue), + }, + } + + for _, tt := range tests { + t.Run("", func(t *testing.T) { + t.Run("encoder", func(t *testing.T) { + v, err := newIntOverrideStringEncoder().fromPropertyValue(tt.input) + if !tt.error { + assert.NoError(t, err) + assert.True(t, v.Equal(tt.expected)) + } else { + assert.Error(t, err) + } + }) + t.Run("decoder", func(t *testing.T) { + if tt.error { + t.Logf("skipping since the encoder should error") + return + } + v, err := newStringOverIntDecoder().toPropertyValue(tt.expected) + assert.NoError(t, err) + if !assert.True(t, v.DeepEquals(tt.input)) { + assert.Equal(t, v, tt.input) + } + }) + }) + } +} diff --git a/pkg/convert/schema_context.go b/pkg/convert/schema_context.go index 53f5df252..ed5c093e4 100644 --- a/pkg/convert/schema_context.go +++ b/pkg/convert/schema_context.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" @@ -111,6 +112,13 @@ func (pc *schemaPropContext) Secret() bool { return false } +func (pc *schemaPropContext) TypeInfo() tokens.Type { + if pc.schemaInfo == nil { + return "" + } + return pc.schemaInfo.Type +} + func (pc *schemaPropContext) Element() (*schemaPropContext, error) { step := walk.NewSchemaPath().Element() var s shim.Schema