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 (TestInputsEmptyCollections and TestInputsNestedBlocksEmpty)
fixes #1915 (TestInputsEmptyCollections and TestInputsNestedBlocksEmpty)
fixes #1964 (TestInputsEmptyCollections and TestInputsNestedBlocksEmpty)
fixes #1767 (TestInputsEmptyCollections and TestInputsNestedBlocksEmpty)
fixes #1762 (TestInputsConfigModeEqual)

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

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

Codecov Report

Attention: Patch coverage is 73.75000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 61.40%. Comparing base (1f9028f) to head (bb2db02).
Report is 2 commits behind head on master.

Files	Patch %	Lines
pkg/tfshim/sdk-v2/provider2.go	72.72%	15 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1971      +/-   ##
==========================================
+ Coverage   61.39%   61.40%   +0.01%     
==========================================
  Files         334      334              
  Lines       44950    45020      +70     
==========================================
+ Hits        27595    27644      +49     
- Misses      15829    15844      +15     
- Partials     1526     1532       +6

☔ 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/[email protected]/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/[email protected]/server.go:1369 +0xb58
google.golang.org/grpc.(*Server).handleStream(0x14000aa2a00, {0x1047553e0, 0x1400117a180}, 0x140003acd80)
	/Users/vvm/go/pkg/mod/google.golang.org/[email protected]/server.go:1780 +0xb20
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	/Users/vvm/go/pkg/mod/google.golang.org/[email protected]/server.go:1019 +0x8c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 191
	/Users/vvm/go/pkg/mod/google.golang.org/[email protected]/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

extracted from #1971 This adds tests for ConfigModeAttr, Timeout and nested blocks. `SkipCompareRawConfig` is used for the parts which need 1971

VenelinMartinov · 2024-05-31T15:48:42Z

Most of the downstream updates which weren't there before look like #2047

Empty maps refresh dirty now.

EDIT: Actually this is present on master too.

The type checking test failures are not significant as the tests assert on the exact panic message - not meaningful here.

The other test failures are also issues with the tests, not the changes.

The downstream failures look like they are all present on master as well.

I believe this is good to go now.

Kicking off another round of downstream tests from 4f24fa4

iwahbe

I'm exited to get this in. If the downstream tests go well, does that mean we are ready to merge?

pkg/tfshim/sdk-v2/provider2.go

VenelinMartinov · 2024-05-31T17:26:10Z

Yeah, the PR should be good to go if downstream tests pass.

Co-authored-by: Ian Wahbe <[email protected]>

This is a pure refactor. Reverts the options added in #1725 as these were the wrong way to do the change. #1971 should be the proper fix to the issues encountered there, now backed by cross-tests. This should simplify the code around makeTerraformInputs a bit.

VenelinMartinov · 2024-06-03T14:39:29Z

Tested pulumi-aws after the ComputeID fixes - it flaked on one test but is otherwise fine: https://github.com/pulumi/pulumi-aws/actions/runs/9350339814/job/25736323726?pr=4012

I think this is good to go. I've seen to failures in downstream tests aside from the repos with ComputeID issues.

pkg/tfshim/sdk-v2/provider.go

pkg/tfshim/sdk-v2/provider2.go

t0yv0 · 2024-06-03T15:08:58Z

pkg/tfshim/sdk-v2/provider2.go

+func normalizeIterable(blockVal cty.Value, blockRes *schema.Resource) []cty.Value {
+ contract.Assertf(blockVal.Type().IsListType() || blockVal.Type().IsSetType(),
+ "normalizeIterable: Expected list or set type, got %v", blockVal.Type().GoString())
+ if blockVal.IsNull() || !blockVal.IsKnown() {


Are we certain about the !blockVal.IsKnown() case? This seems like a bigger deal. Tested this well?

I've opened #2032 to follow up with an investigation of unknown blocks, which is inexpressible in TF.

It should not happen but if it does we can't do anything about it in that function, so I've opted to keep it as is for now.

t0yv0 · 2024-06-03T15:18:44Z

I would like to understand test coverage here at the integration level.

Something changed with the cross-tests, perhaps for the better, but it's not visible in the diff. Can we get a summary? Perhaps double up on some of these with targeted test cases?

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

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?

#1885 related here but might not be the end of the story.

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 Outdated 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

Merge branch 'master' into vvm/empty_set_cross_test

59f174c

VenelinMartinov mentioned this pull request May 28, 2024

Cross test additional tests #2035

Merged

VenelinMartinov added a commit that referenced this pull request May 29, 2024

Cross test additional tests (#2035)

579ec6b

extracted from #1971 This adds tests for ConfigModeAttr, Timeout and nested blocks. `SkipCompareRawConfig` is used for the parts which need 1971

t0yv0 mentioned this pull request May 29, 2024

[EPIC] Rollout PlanResourceChange #1785

Open

5 tasks

VenelinMartinov added 2 commits May 31, 2024 17:54

revert unconditional plan resource change

36f24f3

Merge branch 'master' into vvm/empty_set_cross_test

4f24fa4

iwahbe reviewed May 31, 2024

View reviewed changes

pkg/tfshim/sdk-v2/provider2.go Outdated Show resolved Hide resolved

VenelinMartinov mentioned this pull request May 31, 2024

remove max items one defaults hacks #2049

Merged

Update pkg/tfshim/sdk-v2/provider2.go

9ebe718

Co-authored-by: Ian Wahbe <[email protected]>

VenelinMartinov marked this pull request as ready for review May 31, 2024 19:47