Normalize block values before passing to TF provider #1971

VenelinMartinov · 2024-05-14T16:49:22Z

part of #1785

This change adds a normalisation step for collections when recovering cty values to pass to terraform. This ensures we represent them similarly to terraform.

In practice this means that all block collections need to be passed to TF providers as an empty collection instead of nil. This should get rid of quite a few subtle discrepancies in the data we pass to the TF provider code. These sometimes result in panics since we pass unexpected nils.

This gets rid of all known input discrepancies discovered so far through cross-testing.

The full rules for what is a block are here. It is essentially properties with schema: type List or Sets with a Resource Elem.

fixes #1970
fixes #1915
fixes #1964
fixes #1767
fixes #1762

TODO: [DONE] remove the MaxItemsOne default hacks introduced in #1725 (opened #2049)

codecov · 2024-05-14T16:55:34Z

Codecov Report

Attention: Patch coverage is 76.25000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 61.46%. Comparing base (1c3797a) to head (e99690d).

Files	Patch %	Lines
pkg/tfshim/sdk-v2/provider2.go	75.32%	14 Missing and 5 partials ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           vvm/unknown_tests    #1971      +/-   ##
=====================================================
+ Coverage              61.41%   61.46%   +0.05%     
=====================================================
  Files                    334      334              
  Lines                  44951    45022      +71     
=====================================================
+ Hits                   27605    27672      +67     
- Misses                 15819    15820       +1     
- Partials                1527     1530       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/tests/cross-tests/input_cross_test.go

t0yv0

Good test case to add, continues to be surprising.

VenelinMartinov · 2024-05-14T18:28:11Z

Pretty sure this is happening in the else block here:

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/cty.go

Lines 220 to 233 in 7aa41da

    
           } 
        
           attrs := map[string]cty.Value{} 
        
           for attrName, attrType := range aT { 
        
           	if v, ok := value[attrName]; ok { 
        
           		var err error 
        
           		attrs[attrName], err = recoverCtyValue(attrType, v) 
        
           		if err != nil { 
        
           			return cty.NilVal, err 
        
           		} 
        
           	} else { 
        
           		attrs[attrName] = cty.NullVal(attrType) 
        
           	} 
        
           } 
        
           return cty.ObjectVal(attrs), nil

panic: here!

goroutine 201 [running]:
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2.recoverCtyValueOfObjectType({{0x104741db8, 0x14000782f20}}, 0x14000b64ea0)
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfshim/sdk-v2/cty.go:232 +0x2b0
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2.recoverCtyValue({{0x104741db8, 0x14000782f20}}, {0x104336680, 0x14000b64ea0})
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfshim/sdk-v2/cty.go:122 +0x264
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2.recoverAndCoerceCtyValueWithSchema({0x104730060, 0x14000ecbf20}, {0x104336680, 0x14000b64ea0})
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfshim/sdk-v2/cty.go:94 +0x40
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2.(*planResourceChangeImpl).Diff(0x14000996470, {0x1047419e8, 0x14000703a40}, {0x10392c6f9, 0x15}, {0x0?, 0x0?}, {0x10471c9a0?, 0x14000f29ef0?}, {0x0?, ...})
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go:147 +0x10c
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2.(*providerWithPlanResourceChangeDispatch).Diff(0x1400077eb70, {0x1047419e8, 0x14000703a40}, {0x10392c6f9, 0x15}, {0x0, 0x0}, {0x10471c9a0, 0x14000f29ef0}, {0x0?, ...})
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go:688 +0xb0
github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge.(*Provider).Create(0x14000bb9988, {0x1047419e8?, 0x14000703380?}, 0x14000777450)
	/Users/vvm/code/pulumi-terraform-bridge/pkg/tfbridge/provider.go:1099 +0x5c0
github.com/pulumi/pulumi/sdk/v3/proto/go._ResourceProvider_Create_Handler({0x1046c0e40, 0x14000bb9988}, {0x1047419e8, 0x14000703380}, 0x14000a79080, 0x0)
	/Users/vvm/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.115.2/proto/go/provider_grpc.pb.go:586 +0x1c0
google.golang.org/grpc.(*Server).processUnaryRPC(0x14000aa2a00, {0x1047419e8, 0x140007032f0}, {0x1047553e0, 0x1400117a180}, 0x140003acd80, 0x14000ac47b0, 0x105986f00, 0x0)
	/Users/vvm/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1369 +0xb58
google.golang.org/grpc.(*Server).handleStream(0x14000aa2a00, {0x1047553e0, 0x1400117a180}, 0x140003acd80)
	/Users/vvm/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1780 +0xb20
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	/Users/vvm/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1019 +0x8c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 191
	/Users/vvm/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1030 +0x13c
exit status 2
FAIL	github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/cross-tests	1.771s

t0yv0 · 2024-05-14T18:32:59Z

attrs[attrName] = cty.NullVal(attrType) is a correct representation AFAIK, digging in.

VenelinMartinov · 2024-05-14T18:33:24Z

In the cross tests this is flagged as a diff:

TF value cty.ObjectVal(map[string]cty.Value{"f0":cty.SetValEmpty(cty.Object(map[string]cty.Type{"x":cty.String})), "id":cty.UnknownVal(cty.String)})
PU value cty.ObjectVal(map[string]cty.Value{"f0":cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{"x":cty.String}))), "id":cty.NullVal(cty.String)})

t0yv0 · 2024-05-14T18:38:57Z

Yeah this is plausible.

    input_check.go:41: TF value cty.ObjectVal(map[string]cty.Value{"f0":cty.SetValEmpty(cty.Object(map[string]cty.Type{"x":cty.String})), "id":cty.NullVal(cty.String)})
    input_check.go:42: PU value cty.ObjectVal(map[string]cty.Value{"f0":cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{"x":cty.String}))), "id":cty.NullVal(cty.String)})

Perhaps this comes back to the fact that null values aren't representable in TF for set-nested blocks. Therefore it's an automatic empty set aka [].

VenelinMartinov · 2024-05-14T18:42:05Z

Yeah and I think this is were it gets really messy with SchemaConfigMode, since they are representable if ConfigModeAttr is set.

VenelinMartinov · 2024-05-14T18:56:14Z

I ran into some trouble with proposed_new.go - not obvious how to convert the element type between hcty and cty

t0yv0 · 2024-05-14T18:56:16Z

True - this is just saying the fix need to accommodate both for both cases:

your code is testing a Set-nested block
if is also possible to have a Set({foo: string}) attribute in TF, we should test that we pass that too

I tried a quick fix quickly - sorry accidentally pushed a commit to this branch. Meant a separate branch t0yv0/empty_set_cross_test

I can get the quick fix to pass RawConfig test but curiously NOT the RawPlan test. This makes me wonder what's going on there. Could this be because we'e linking in outdated code for planning the change compared to TF CLI? It's a bit unfortunate that in this test setup we cannot debug into TF CLI to see what is it doing .

t0yv0 · 2024-05-14T18:56:57Z

I'd also wager my fix doesn't cut it for the attribute case which might need to actually return null not [].

t0yv0 · 2024-05-14T18:58:02Z

BTW the reason this may be less impactful than it seems is that few providers consult RawPlan and RawConfig on Create, they mostly consult the ResourceData. So issues of this sort remain unfixed.

VenelinMartinov · 2024-05-14T18:58:07Z

I think the attribute case is completely unhandled but let's fix that seprately.

Isn't there the same issue in

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/proposed_new.go

Line 99 in 26a2938

case val.IsNull():

t0yv0 · 2024-05-14T19:02:19Z

Well that one looks fine? hcty2ctyWithType converts between two equivalent representations cty.Value and hcty.Value, converting null to null is reasonable, why do we have null passed into that in the first place would be the question?

Perhaps we need to fix at the level of MakeTerraformInputs?

What is not yet tested that's really interesting to test, even more fundamentally important than RawConfig/RawPLan parity, is whether the two paths get the same answer to this:

tfResData.Get("f0")
tfResData.GetChange("f0")

…ross_test

VenelinMartinov · 2024-05-16T14:15:05Z

I believe I found two other places where the state is populated with nulls instead of empty collections:

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go

Line 307 in 012ac65

stateValue: cty.NullVal(ty),

and

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go

Line 334 in 012ac65

newInstanceState, err := upgradeResourceState(ctx, p.tf, res, instanceState)

The first one is fairly straight-forward to fix but the upgradeState stuff might be better fixed by calling a higher level function in the plugin-sdk -it looks to me like we are doing multiple passes back and forth between instanceState and cty values both in the bridge code and then in the plugin-sdk.

Investigating...

VenelinMartinov · 2024-05-16T14:40:11Z

Fairly sure we are loosing the value here:

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/upgrade_state.go

Line 74 in 22fdb29

newState, err := res.ShimInstanceStateFromValue(v)

the v returned from NormalizeObjectFromLegacySDK is correct but then newState becomes newState=&terraform.InstanceState{}

which is then converted to stateValue=cty.ObjectVal(map[string]cty.Value{"f0":cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{"x":cty.String}))), "id":cty.NullVal(cty.String)})

VenelinMartinov · 2024-05-16T14:43:05Z

Ah, I think we are missing a null normalization after calling AttrsAsObjectValue

In the plugin-sdk: https://github.com/hashicorp/terraform-plugin-sdk/blob/70fb6b9b15e8e5fc73f424e24084c28fedd1e013/helper/schema/grpc_provider.go#L1100-L1106

StateValueFromInstanceState is just a wrapper around AttrsAsObjectValue and note that normalizeNullValues is called after.

But in provider2.go:

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go

Line 342 in 012ac65

stateValue, err := newInstanceState.AttrsAsObjectValue(ty)

we don't then normalize the nulls to empty collections after calling AttrsAsObjectValue

VenelinMartinov · 2024-06-04T11:10:10Z

The most worrisome case to me is replacing unknowns with empty collections, basically testing this at an integration level, since I believe we have several subsystems fighting over it (MakeTerraformInputs, and now normalizeCollections). Can we find out how the bridged provider behavior is changing in that case, and importantly, why, is this what we want?

I've removed the code which modified unknowns - it was wrong and unused.

I've also added GRPC tests for handling unknowns both in PlanResourceChange and non-PlanResourceChange. I've verified these have not changed between master and this PR. Very interesting how this has changed for PlanResourceChange but unrelated to this PR.

I also added targeted regression tests for the fixed issues.

Let me know if this makes sense.

pkg/tfbridge/provider_test.go

t0yv0 · 2024-06-04T14:18:07Z

pkg/tfbridge/provider_test.go

+		"response": {
+			"properties":{
+				"id":"",
+				"nestedListProps":["04da6b54-80e4-46f7-96ec-b56ff0331ba9"]


What's going on here with the unknown floating, curious?

very curious but unrelated to the changes in this PR - I've verified the same behaviour on master

t0yv0 · 2024-06-04T14:21:45Z

pkg/tfbridge/provider_test.go

+				"id":"04da6b54-80e4-46f7-96ec-b56ff0331ba9",
+				"stringProp":null,
+				"setProps":null,
+				"listProps":"04da6b54-80e4-46f7-96ec-b56ff0331ba9",


The unknown is floating from [unk] to unk, strange. Let me debug this.

t0yv0 · 2024-06-04T14:22:01Z

pkg/tfbridge/provider_test.go

+				"setProps":null,
+				"listProps":null,
+				"maxItemsOneProp":null,
+				"nestedListProps":["04da6b54-80e4-46f7-96ec-b56ff0331ba9"]


Another case of floating.

t0yv0 · 2024-06-04T15:32:26Z

Verified that [unk] to unk floating happens inside PlanResourceChange method so this is something programmed in TF it seems, we cannot easily avoid this without digging further. Let's not block on it but accept as is for now.

t0yv0 · 2024-06-04T15:33:00Z

Let's check that "listBlockProps":"04da6b54-80e4-46f7-96ec-b56ff0331ba9" top-level unknown gives something reasonable and if so LGTM!

VenelinMartinov · 2024-06-04T16:09:43Z

I've extracted the unknown tests in #2054

There are some failures in the new code related to unknowns - I'll address these next.

Appreciate the input here 🙏

Should be fixed - we no longer panic on unknown collections - instead we return the unknown as is.

VenelinMartinov · 2024-06-04T16:25:22Z

pkg/tfbridge/provider_test.go

@@ -5484,7 +5484,7 @@ func TestPlanResourceChangeUnknowns(t *testing.T) {
 				"maxItemsOneProp":null,
 				"setBlockProps":[],
 				"listBlockProps":[],
-				"nestedListBlockProps":[{"nestedProps":null}],
+				"nestedListBlockProps":[{"nestedProps":[]}],


This is the only change in behaviour to current master related to unknowns and it seems reasonable to me - nestedProps is a list type so should not be nil.

Yeah looks like MakeTerraformInputs substituted "04da6b54-80e4-46f7-96ec-b56ff0331ba9" into "nestedListBlockProps":[{"nestedProps":null}], and then the newly added pass corrected that to []. This is fine.. we're already in heuristic land in this case. Good to have it covered!!

t0yv0 · 2024-06-04T16:37:07Z

empty sets cross test

5528bab

VenelinMartinov requested review from t0yv0, guineveresaenger and iwahbe May 14, 2024 16:49

t0yv0 reviewed May 14, 2024

View reviewed changes

pkg/tests/cross-tests/input_cross_test.go Show resolved Hide resolved

VenelinMartinov changed the title ~~Empty sets cross test~~ Missing set cross test May 14, 2024

t0yv0 self-requested a review May 14, 2024 17:23

t0yv0 approved these changes May 14, 2024

View reviewed changes

Toward a fix

fba73e1

VenelinMartinov added 6 commits May 16, 2024 13:26

Add additional checks on input cross tests for ResourceData.Get values

9c69721

remove debug output

9c998f0

compare change vals too

95aa4ff

Merge branch 'vvm/cross_test_inputs_compare_get' into vvm/empty_set_c…

5eac370

…ross_test

revert error formatting

1e6a0c2

Merge branch 'vvm/cross_test_inputs_compare_get' into vvm/empty_set_c…

3458348

…ross_test

VenelinMartinov added 3 commits June 4, 2024 11:42

add non-plan resource change test

933d5ca

max items one unknown

e6098cd

targeted regression tests

7613c39

Merge branch 'master' into vvm/empty_set_cross_test

51d9bf4

t0yv0 reviewed Jun 4, 2024

View reviewed changes

pkg/tfbridge/provider_test.go Outdated Show resolved Hide resolved

t0yv0 self-requested a review June 4, 2024 14:15

t0yv0 reviewed Jun 4, 2024

View reviewed changes

t0yv0 self-requested a review June 4, 2024 14:23

VenelinMartinov added 3 commits June 4, 2024 15:58

expand unknown tests to cover block props

0215d28

typo

19c6dc8

add missing skip

b351202

t0yv0 approved these changes Jun 4, 2024

View reviewed changes

finish unknown test matrix

fe9707d

VenelinMartinov added 3 commits June 4, 2024 17:10

remove tests

634d56f

unknown tests

4b4e705

don't panic on block unknowns

11df84b

VenelinMartinov changed the base branch from master to vvm/unknown_tests June 4, 2024 16:21

VenelinMartinov added 2 commits June 4, 2024 17:22

Merge branch 'vvm/unknown_tests' into vvm/empty_set_cross_test

18b286a

minor refactor

8e0fd52

VenelinMartinov commented Jun 4, 2024

View reviewed changes

VenelinMartinov added 2 commits June 4, 2024 17:28

rename accidentally repeated test name

c998809

Merge branch 'vvm/unknown_tests' into vvm/empty_set_cross_test

e99690d

VenelinMartinov mentioned this pull request Jun 4, 2024

Unknown tests #2054

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize block values before passing to TF provider #1971

Normalize block values before passing to TF provider #1971

VenelinMartinov commented May 14, 2024 •

edited

codecov bot commented May 14, 2024 •

edited

t0yv0 left a comment

VenelinMartinov commented May 14, 2024 •

edited

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024 •

edited

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024

VenelinMartinov commented May 14, 2024

t0yv0 commented May 14, 2024

t0yv0 commented May 14, 2024

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024

t0yv0 commented May 14, 2024

VenelinMartinov commented May 16, 2024 •

edited

VenelinMartinov commented May 16, 2024

VenelinMartinov commented May 16, 2024 •

edited

VenelinMartinov commented Jun 4, 2024 •

edited

t0yv0 Jun 4, 2024 •

edited

VenelinMartinov Jun 4, 2024

t0yv0 Jun 4, 2024

t0yv0 Jun 4, 2024

t0yv0 commented Jun 4, 2024

t0yv0 commented Jun 4, 2024

VenelinMartinov commented Jun 4, 2024 •

edited

VenelinMartinov Jun 4, 2024

t0yv0 Jun 4, 2024

t0yv0 commented Jun 4, 2024

Normalize block values before passing to TF provider #1971

Are you sure you want to change the base?

Normalize block values before passing to TF provider #1971

Conversation

VenelinMartinov commented May 14, 2024 • edited

codecov bot commented May 14, 2024 • edited

Codecov Report

t0yv0 left a comment

Choose a reason for hiding this comment

VenelinMartinov commented May 14, 2024 • edited

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024 • edited

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024

VenelinMartinov commented May 14, 2024

t0yv0 commented May 14, 2024

t0yv0 commented May 14, 2024

t0yv0 commented May 14, 2024

VenelinMartinov commented May 14, 2024

t0yv0 commented May 14, 2024

VenelinMartinov commented May 16, 2024 • edited

VenelinMartinov commented May 16, 2024

VenelinMartinov commented May 16, 2024 • edited

VenelinMartinov commented Jun 4, 2024 • edited

t0yv0 Jun 4, 2024 • edited

Choose a reason for hiding this comment

VenelinMartinov Jun 4, 2024

Choose a reason for hiding this comment

t0yv0 Jun 4, 2024

Choose a reason for hiding this comment

t0yv0 Jun 4, 2024

Choose a reason for hiding this comment

t0yv0 commented Jun 4, 2024

t0yv0 commented Jun 4, 2024

VenelinMartinov commented Jun 4, 2024 • edited

VenelinMartinov Jun 4, 2024

Choose a reason for hiding this comment

t0yv0 Jun 4, 2024

Choose a reason for hiding this comment

t0yv0 commented Jun 4, 2024

VenelinMartinov commented May 14, 2024 •

edited

codecov bot commented May 14, 2024 •

edited

VenelinMartinov commented May 14, 2024 •

edited

VenelinMartinov commented May 14, 2024 •

edited

VenelinMartinov commented May 16, 2024 •

edited

VenelinMartinov commented May 16, 2024 •

edited

VenelinMartinov commented Jun 4, 2024 •

edited

t0yv0 Jun 4, 2024 •

edited

VenelinMartinov commented Jun 4, 2024 •

edited