Switch upgradeResourceState to use SDKv2's gRPC method #1735

iwahbe · 2024-03-06T16:51:36Z

This PR switches out the default implementation of upgradeResourceState to schema.GRPCProviderServer.UpgradeResourceState.

I explored this PR as part of root causing pulumi/pulumi-github#586. The root cause of pulumi/pulumi-github#586 is #1667.

This change is a pure refactor, but it clarifies which behavior is directly from TF and which behavior is part of Pulumi's bridge requests. It also guarantees that if TF changes it's implementation, we will copy the new behavior when we upgrade the linked SDKv2.

codecov · 2024-03-06T17:20:00Z

Codecov Report

Attention: Patch coverage is 68.11594% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 60.81%. Comparing base (67fc311) to head (16cc874).

Files	Patch %	Lines
pkg/tfshim/sdk-v2/upgrade_state.go	62.71%	15 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
- Coverage   60.82%   60.81%   -0.01%     
==========================================
  Files         332      332              
  Lines       44727    44751      +24     
==========================================
+ Hits        27206    27217      +11     
- Misses      15998    16008      +10     
- Partials     1523     1526       +3

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

pkg/tfshim/sdk-v2/upgrade_state.go

t0yv0 · 2024-03-07T18:48:20Z

pkg/tfshim/sdk-v2/upgrade_state.go

+	case CtyInstanceState:
+		rawState, err = upgradeResourceStateNonNormalizing(ctx, m, p, int(version), res)
+	default:
+		rawState, err = upgradeResourceStateRPC(ctx, typeName, m, p, res, version)


This is a very tedious spot-the-differences exercise hiding here. I've read the before/after code line by line and they seem to be following very closely.

Before:

json, _, _ := UpgradeFlatmapState(_, _, m, _, _) json, _ = UpgradeJSONState(json) RemoveAttributes(_, json, _) v, _ = JSONMapToStateValue(json, configBlock) v, _ = CoerceValue(v) v = schema.NormalizeObjectFromLegacySDK(v, configBlock)

After:

func (s *GRPCProviderServer) UpgradeResourceState( s.upgradeFlatmapState(ctx, version, req.RawState.Flatmap, res) jsonMap, err = s.upgradeJSONState(ctx, version, jsonMap, res) RemoveAttributes(ctx, jsonMap, schemaBlock.ImpliedType()) val, err := JSONMapToStateValue(jsonMap, schemaBlock) val, err = schemaBlock.CoerceValue(val) val = objchange.NormalizeObjectFromLegacySDK(val, schemaBlock) // serialize to msgpack // deserialize from msgpack var newState cty.Value newState, err := res.ShimInstanceStateFromValue(rawState)

It seems that the serialization of cty.Value to msgpack and back is the only addition. That one I think should not be lossy.

It seems like the previous method was an inlining of the logic in the gRPC provider.

That is my understanding, which I why this is a no-op refactor, not a bug fix.

t0yv0 · 2024-03-07T18:53:09Z

Have you looked at how this code is covered?

If we had #1736 in place evaluating safety here would have been an easy conversation. I think downstream checks will not test this very well at all, we need good coverage in the bridge.

I found this:
https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tests/regress_923_test.go#L29

But are the more tests covering the upgrade specifically?

iwahbe · 2024-03-11T14:10:52Z

I found this: https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tests/regress_923_test.go#L29

But are the more tests covering the upgrade specifically?

There are not. I tried turning off upgradeResourceState (making it a no-op) and that was the only test that failed.

t0yv0 · 2024-03-11T20:22:17Z

Ahh yes, very unfortunate.. Well it seems like we should consider throwing some more simple tests specifically designed at covering upgrading resources basics before changing the code further. Especially if this is mostly a forward-looking refactor and we do not have customers waiting on a change of behavior here.

This ensures that our behavior aligns with TF's behavior.

pkg/tfshim/sdk-v2/upgrade_state.go

t0yv0 · 2024-05-20T20:28:16Z

pkg/tfshim/sdk-v2/upgrade_state.go

@@ -35,13 +39,18 @@ func upgradeResourceState(ctx context.Context, typeName string, p *schema.Provid
 		version, hasVersion = v, true
 	}

-	// Now, we perform the UpgradeResourceState operation by re-implementing TF's UpgradeResourceState.
+	json, err := ctyjson.Marshal(instanceState.RawState, res.CoreConfigSchema().ImpliedType())


Eyes on this line please, I think res.CoreConfigSchema().ImpliedType() comes from the current provider schema but instanceState.RawState may be on prev provider schema in the case of actual upgrade. We could try inferring the type from the value. I think it's about tradeoffs both approaches slightly problematic. If this is exactly how it worked before perhaps can leave that.

We should be able to change that to

json, err := ctyjson.Marshal(instanceState.RawState, res.RawState.Type())

I'll add a test where we do a more substantial upgrade to verify that this works as expected.

It didn't work without the change. Good catch! I have added the fix and a test to go with it.

This includes pulumi/terraform-plugin-sdk@28b1672. This change is necessary for full fidelity round tripping of numbers.

VenelinMartinov · 2024-05-21T10:31:58Z

pkg/tfshim/sdk-v2/upgrade_state.go

 		}
-		version, hasVersion = int(v), true
+		version, hasVersion = v, true
 	}


I don't see a check for downgrades here - pretty sure TF doesn't allow downgrades, see #1998 (comment)

iwahbe · 2024-05-21T21:16:30Z

Closing in favor of #2005.

This PR extracts the non provider2 specific part of `planResourceChangeImpl.upgradeState` into it's own function: `upgradeResourceStateGRPC`. This part is a pure refactor. The PR then re-implements `upgradeResourceState` in terms of `upgradeResourceStateGRPC`. The driving motivation for this change is to reduce complexity and allow both upgraders to handle large JSON numbers. Replaces #1735

iwahbe requested review from t0yv0 and a team March 6, 2024 16:51

iwahbe self-assigned this Mar 6, 2024

iwahbe force-pushed the iwahbe/grpc-based-sdkv2-uprgade-state branch from eff83e8 to f585004 Compare March 6, 2024 17:04

iwahbe enabled auto-merge (squash) March 6, 2024 17:53

t0yv0 reviewed Mar 7, 2024

View reviewed changes

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

t0yv0 reviewed Mar 7, 2024

View reviewed changes

iwahbe force-pushed the iwahbe/grpc-based-sdkv2-uprgade-state branch from f585004 to 6e72de9 Compare March 11, 2024 13:50

iwahbe added 3 commits May 20, 2024 10:34

Switch upgradeResourceState to a grcp method

efb15f3

This ensures that our behavior aligns with TF's behavior.

improve diag handling

42810fc

inline upgradeResourceStateRPC

abeedba

iwahbe force-pushed the iwahbe/grpc-based-sdkv2-uprgade-state branch from 6626748 to abeedba Compare May 20, 2024 17:34

VenelinMartinov reviewed May 20, 2024

View reviewed changes

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

iwahbe added 2 commits May 20, 2024 10:44

Show that Flatmap doesn't support full round tripping

e79f389

Use JSON for sending data

c9ea19c

iwahbe force-pushed the iwahbe/grpc-based-sdkv2-uprgade-state branch 2 times, most recently from b76bf11 to ad30565 Compare May 20, 2024 19:24

iwahbe requested review from t0yv0 and VenelinMartinov May 20, 2024 19:25

t0yv0 reviewed May 20, 2024

View reviewed changes

Upgrade to upstream-v2.33.0-with-272df8b

1832fb7

This includes pulumi/terraform-plugin-sdk@28b1672. This change is necessary for full fidelity round tripping of numbers.

iwahbe force-pushed the iwahbe/grpc-based-sdkv2-uprgade-state branch from ad30565 to 1832fb7 Compare May 20, 2024 22:54

iwahbe added 2 commits May 20, 2024 16:15

Recover RawState from Attributes if necessary

c54e9cd

Fix deserializing from old states

16cc874

iwahbe requested a review from t0yv0 May 21, 2024 01:04

VenelinMartinov reviewed May 21, 2024

View reviewed changes

iwahbe mentioned this pull request May 21, 2024

Unify upgradeResourceState between provider and provider2 #2005

Merged

iwahbe closed this May 21, 2024

auto-merge was automatically disabled May 21, 2024 21:16
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch upgradeResourceState to use SDKv2's gRPC method #1735

Switch upgradeResourceState to use SDKv2's gRPC method #1735

iwahbe commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

t0yv0 Mar 7, 2024

iwahbe Mar 11, 2024

t0yv0 commented Mar 7, 2024

iwahbe commented Mar 11, 2024

t0yv0 commented Mar 11, 2024

t0yv0 May 20, 2024 •

edited

Loading

iwahbe May 21, 2024

iwahbe May 21, 2024

VenelinMartinov May 21, 2024

iwahbe commented May 21, 2024

Switch upgradeResourceState to use SDKv2's gRPC method #1735

Switch upgradeResourceState to use SDKv2's gRPC method #1735

Conversation

iwahbe commented Mar 6, 2024 • edited Loading

codecov bot commented Mar 6, 2024 • edited Loading

Codecov Report

t0yv0 Mar 7, 2024

Choose a reason for hiding this comment

iwahbe Mar 11, 2024

Choose a reason for hiding this comment

t0yv0 commented Mar 7, 2024

iwahbe commented Mar 11, 2024

t0yv0 commented Mar 11, 2024

t0yv0 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

iwahbe May 21, 2024

Choose a reason for hiding this comment

iwahbe May 21, 2024

Choose a reason for hiding this comment

VenelinMartinov May 21, 2024

Choose a reason for hiding this comment

iwahbe commented May 21, 2024

iwahbe commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

t0yv0 May 20, 2024 •

edited

Loading