Skip to content

Avoid roundtrip to json when converting Cty to map[string]any #3023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Avoid roundtrip to json when converting Cty to map[string]any #3023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f93f37b
Avoid roundtrip to json when converting Cty to map[string]any
VenelinMartinov Apr 23, 2025
ce3c10f
fix nil handling
VenelinMartinov Apr 23, 2025
327c740
add tests
VenelinMartinov May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 0 additions & 207 deletions pkg/tfshim/sdk-v2/cty_json.go
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions pkg/tfshim/sdk-v2/instance_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,10 @@
package sdkv2

import (
"bytes"
"encoding/json"
"strings"

"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
diff_reader "github.com/pulumi/terraform-diff-reader/sdk-v2"

shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
Expand Down Expand Up @@ -67,34 +63,6 @@ func (s v2InstanceState) Object(sch shim.SchemaMap) (map[string]interface{}, err
return s.objectV1(sch)
}

// This is needed because json.Unmarshal uses float64 for numbers by default which truncates int64 numbers.
func unmarshalJSON(data []byte, v interface{}) error {
dec := json.NewDecoder(bytes.NewReader(data))
dec.UseNumber()
return dec.Decode(v)
}

// objectFromCtyValue takes a cty.Value and converts it to JSON object.
Copy link
Member

@t0yv0 t0yv0 May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this comment removed?

// We do not care about type checking the values, we just want to do our best to recursively convert
// the cty.Value to the underlying value
//
// NOTE: one of the transforms this needs to handle is converting unknown values.
// cty.Value that are also unknown cannot be converted to their underlying value. To get
// around this we just convert to a sentinel, which so far does not seem to cause any issues downstream
func objectFromCtyValue(v cty.Value) map[string]interface{} {
var path cty.Path
buf := &bytes.Buffer{}
// The round trip here to JSON is redundant, we could instead convert from cty to map[string]interface{} directly
err := marshal(v, v.Type(), path, buf)
contract.AssertNoErrorf(err, "Failed to marshal cty.Value to a JSON string value")

var m map[string]interface{}
err = unmarshalJSON(buf.Bytes(), &m)
contract.AssertNoErrorf(err, "failed to unmarshal: %s", buf.String())

return m
}

// The legacy version of Object used custom Pulumi code forked from TF sources.
func (s v2InstanceState) objectV1(sch shim.SchemaMap) (map[string]interface{}, error) {
obj := make(map[string]interface{})
Expand Down
60 changes: 60 additions & 0 deletions pkg/tfshim/sdk-v2/object_from_cty.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package sdkv2

import (
"github.com/hashicorp/go-cty/cty"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)

func objectFromCtyValue(val cty.Value) map[string]any {
Copy link
Member

@t0yv0 t0yv0 May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pre-existing problem but if we're editing it let's annotate how this functionality is used in the project. Doing a quick search, it is used to implement

pulumi-terraform-bridge/pkg/tfshim/shim.go

Line 31 in 9284425

Object(sch SchemaMap) (map[string]interface{}, error)
which is only used by MakeTerraformResult, so the resulting map[string]any is passed to MakeTerraformOutputs and it is its only job in fact. This could be helpful to annotate at least in a comment.

So the ultimate purpose of this code is to convert cty.Value received from TF to the PropertyMap Pulumi domain.

Couple of questions:

  • why do you believe DynamicPseudoType is unused or will be unused in the future? It is quite plausible that TF providers would return a DynamicPseudoType-encoded cty.Values and this PR is deciding to drop support, why is it a good idea?

  • similarly for marks and capsules, why is it that that upstream providers would never use them? We found a very interesting use case for sensitive markers with @corymhall recently when working on the Terraform module project, it helped us propagate secret bits through the TF engine using sensitive() functions - https://github.com/pulumi/pulumi-terraform-module/blob/19814c58e25c402bc3ec8135d1c9817ec0118307/pkg/tfsandbox/tf_file.go#L10 - I have a hunch that these may end up being Marks on the cty.Value wire. There is a possibility that these will be received from TF providers.

Copy link
Member

@t0yv0 t0yv0 May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other bit that makes this code somewhat more difficult to maintain than necessary is that we are using a map[string]any type which is not tight enough on what values are admissible, this means changes here need to scrutinize whether MakeTerraformOutputs is going to interpret the code correctly to finish translating to PropertyMap. What if we undertook a refactor that replaced map[string]interface{} with a stricter type when communicating from the shim layer to MakeTerraformOutputs? It could make a few things less ambiguous in the code especially:

  • how unknowns are represented
  • how numbers are represented (reading this I'm still worried MakeTerraformOutputs will not always transform the string back to a PropertyMap number here, this is just not great)

res := objectFromCtyValueInner(val)
if res == nil {
return nil
}
return res.(map[string]any)
}

func objectFromCtyValueInner(val cty.Value) any {
contract.Assertf(!val.IsMarked(), "value has marks, so it cannot be serialized")
if val.IsNull() {
return nil
}

if !val.IsKnown() {
return terraformUnknownVariableValue
}

switch {
case val.Type().IsPrimitiveType():
switch val.Type() {
case cty.String:
return val.AsString()
case cty.Number:
return val.AsBigFloat().Text('f', -1)
Copy link
Member

@t0yv0 t0yv0 May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an observable change right, or it isn't? I like this I've found the val.AsBigFloat().Text('f', -1) is a good method we use elsewhere to preserve precision. It might work better than what this code had before. But it's different. Should the type be json.Number instead of string? Why is it a string that we are returning here?

case cty.Bool:
return val.True()
default:
contract.Failf("unsupported primitive type: %s", val.Type().FriendlyName())
}
case val.Type().IsListType(), val.Type().IsSetType(), val.Type().IsTupleType():
l := make([]interface{}, 0, val.LengthInt())
it := val.ElementIterator()
for it.Next() {
_, ev := it.Element()
elem := objectFromCtyValueInner(ev)
l = append(l, elem)
}
return l
case val.Type().IsObjectType(), val.Type().IsMapType():
l := make(map[string]interface{})
it := val.ElementIterator()
for it.Next() {
ek, ev := it.Element()
cv := objectFromCtyValueInner(ev)
l[ek.AsString()] = cv
}
return l
}

contract.Failf("unsupported type: %s", val.Type().FriendlyName())
return nil
}
Loading
Loading