Pass `PlannedPrivate` from PlanResourceChange to ApplyResourceChange #2407

flostadler · 2024-09-09T17:48:08Z

The PlannedPrivate field of the PRC response includes metadata that's required for correctly re-assembling the diff.
For example, it includes NewExtra which includes the original value for attributes with a StateFunc.
Without PlannedPrivate, those attributes get saved to state encoded twice. This can lead to mangled data in state and perma-diffs.

Fixes #2408

codecov · 2024-09-09T17:54:41Z

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.11%. Comparing base (fddce46) to head (1e1ac96).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/tfshim/sdk-v2/provider2.go	90.47%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2407   +/-   ##
=======================================
  Coverage   57.10%   57.11%           
=======================================
  Files         369      369           
  Lines       50593    50604   +11     
=======================================
+ Hits        28891    28902   +11     
  Misses      20124    20124           
  Partials     1578     1578

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

The `PlannedPrivate` field of the PRC response includes metadata that's required for correctly re-assembling the diff. For example, it includes `NewExtra` which includes the original value for attributes with a `StateFunc`. Without `PlannedPrivate`, those attributes get saved to state encoded twice. This can lead to mangled data in state and perma-diffs.

flostadler · 2024-09-10T11:56:29Z

pkg/tfbridge/provider_test.go

@@ -5113,7 +5113,8 @@ func TestPlanResourceChangeUnknowns(t *testing.T) {
 				"setBlockProps":[],
 				"listBlockProps":[],
 				"nestedListBlockProps":[],
-				"maxItemsOneBlockProp":null
+				"maxItemsOneBlockProp":null,
+				"__meta":"{\"_new_extra_shim\":{}}"


I wonder if this will cause pain somewhere else? I.e. re-recording of upgrade tests?

We'll likely have to fix a bunch of GRPC tests everywhere but that's fine.

This reverts commit a807b14.

This reverts commit 9e1ebdd.

This reverts commit 0b7c7d5.

flostadler · 2024-09-10T15:36:00Z

pkg/tfshim/sdk-v2/provider2.go

-	PlannedMeta  map[string]interface{}
-	PlannedDiff  *terraform.InstanceDiff
+	PlannedState   cty.Value
+	PlannedPrivate map[string]interface{}


This is actually PlannedPrivate not PlannedMeta. Renaming this so it aligns better with terraform

VenelinMartinov · 2024-09-10T16:00:14Z

Is the third parameter on CreateContext/UpdateContext the PlannedPrivate in TF? If so, I think we can write cross-tests here similar to

pulumi-terraform-bridge/pkg/tests/cross-tests/input_cross_test.go

Line 13 in a61fbc4

func TestInputsEqualStringBasic(t *testing.T) {

to make sure that what we pass to the TF provider matches what TF passes to the TF provider.

flostadler · 2024-09-10T16:05:45Z

Is the third parameter on CreateContext/UpdateContext the PlannedPrivate in TF? If so, I think we can write cross-tests here similar to

pulumi-terraform-bridge/pkg/tests/cross-tests/input_cross_test.go

Line 13 in a61fbc4

func TestInputsEqualStringBasic(t *testing.T) {

to make sure that what we pass to the TF provider matches what TF passes to the TF provider.

Sadly it's not :(

iwahbe

It's hard for me to review, just because it's hard to know what the correct behavior is.

I'd leave this for @VenelinMartinov and @t0yv0 unless I need to spin up on this.

VenelinMartinov · 2024-09-10T16:59:34Z

Sadly it's not :(

Where is the PlannedPrivate picked up on the TF side? Is there a way for us to hook into that and write a cross-test for that? That'd give us more confidence that we are matching the TF behaviour here.

Don't want to block the PR on this since the changes look sensible but it'd be nice if we can be confident here.

VenelinMartinov · 2024-09-10T17:05:08Z

How does the TF provider access the PlannedPrivate?

t0yv0 · 2024-09-10T17:05:47Z

I was working with this on Florian a bit this morning, I can review. It looked safe enough but we were exploring how it's tangled to timeouts and there's a little bit of residual uncertainty I have around how TF handles timeout etc encoding in PlannedPrivate as well as whether we're doing a good job of mapping planned private to Pulumi protocol. It's a bit subtle.

flostadler · 2024-09-10T17:14:17Z

@VenelinMartinov
What we're interested in is this bit here: https://github.com/hashicorp/terraform-plugin-sdk/blob/527cfe4ccf1738d4efcfd6006bbd2068f4a35a06/helper/schema/grpc_provider.go#L1111-L1122

It's reconstructing the NewExtra field of the ResourceAttrDiff using data in PlannedPrivate.
NewExtra contains the original value the attribute was set to before the StateFunc was applied (even though the name NewExtra is misleading).

The resource access methods (e.g. schema.ResourceData::GetOk) either return the value in state or NewExtra if it's set.
Right now the regression test is asserting what's stored in state.
Can we craft a cross-test out of this?

VenelinMartinov · 2024-09-10T17:37:01Z

Maybe not without bigger changes - this property seems to be hidden from the provider too and only accessible from within the plugin-sdk. The provider only sees the .Get result change depending on if this is set or not.

Is this correct? If so, then we'd need to add a hook in the plugin-sdk for this which is certainly out of scope here.

flostadler · 2024-09-10T17:40:52Z

@VenelinMartinov Yeah you're understanding is correct. It's hidden in the ResourceData and the .Get result changes are the only thing providers see.

t0yv0 · 2024-09-10T19:06:24Z

Cross-test would be very helpful if just to debug the TF side of things.

iwahbe · 2024-09-11T08:33:47Z

Why couldn't we write a cross test that asserts on the results of .Get? We don't need a cross-test to assert the internals, only the provider visible section.

VenelinMartinov · 2024-09-11T09:40:50Z

Yeah, that bit is possible - a test like

pulumi-terraform-bridge/pkg/tests/cross-tests/input_cross_test.go

Line 13 in a61fbc4

func TestInputsEqualStringBasic(t *testing.T) {

but with the StateFunc and the modifications as in the existing test.

Unfortunately that wouldn't cover the full contents of the PlannedPrivate field.

flostadler · 2024-09-11T13:33:29Z

@t0yv0 @VenelinMartinov I added a cross test, please have a look

VenelinMartinov

LGTM, thanks for digging in here!

pulumi-bot · 2024-09-20T09:50:12Z

This PR has been shipped in release v3.91.0.

Add repro

1e0b4b2

flostadler self-assigned this Sep 9, 2024

lint

5cbec08

flostadler mentioned this pull request Sep 9, 2024

StateFunc applied twice for resources under PRC #2408

Closed

flostadler force-pushed the flostadler/prc-statefunc-bug branch from 852d21f to b807281 Compare September 10, 2024 11:25

flostadler changed the title ~~PRC StateFunc applied twice repro~~ Pass PlannedPrivate from PlanResourceChange to ApplyResourceChange Sep 10, 2024

flostadler added 2 commits September 10, 2024 13:43

Update tests

a807b14

Update tests

9e1ebdd

flostadler commented Sep 10, 2024

View reviewed changes

flostadler requested review from t0yv0, guineveresaenger, iwahbe, VenelinMartinov and a team September 10, 2024 11:57

More test updates

0b7c7d5

flostadler marked this pull request as ready for review September 10, 2024 13:31

flostadler added 6 commits September 10, 2024 16:27

Add dedicated property for PlannedMeta

6fd8596

Revert "Update tests"

9c4661a

This reverts commit a807b14.

Revert "Update tests"

00d0c33

This reverts commit 9e1ebdd.

Revert "More test updates"

3323bb0

This reverts commit 0b7c7d5.

make lint_fix

6a52c4c

Merge plannedPrivate and Meta to make timeouts work

06db5d1

flostadler commented Sep 10, 2024

View reviewed changes

iwahbe reviewed Sep 10, 2024

View reviewed changes

Add cross test for StateFunc

1e1ac96

VenelinMartinov approved these changes Sep 11, 2024

View reviewed changes

flostadler merged commit 7864bb0 into master Sep 11, 2024
11 checks passed

flostadler deleted the flostadler/prc-statefunc-bug branch September 11, 2024 15:00

t0yv0 mentioned this pull request Sep 11, 2024

Refactor custom timeouts support to match TF #2411

Open

mjeffryes added this to the 0.110 milestone Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass `PlannedPrivate` from PlanResourceChange to ApplyResourceChange #2407

Pass `PlannedPrivate` from PlanResourceChange to ApplyResourceChange #2407

flostadler commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

flostadler Sep 10, 2024

VenelinMartinov Sep 10, 2024

flostadler Sep 10, 2024

VenelinMartinov commented Sep 10, 2024

flostadler commented Sep 10, 2024

iwahbe left a comment

VenelinMartinov commented Sep 10, 2024

VenelinMartinov commented Sep 10, 2024

t0yv0 commented Sep 10, 2024

flostadler commented Sep 10, 2024

VenelinMartinov commented Sep 10, 2024

flostadler commented Sep 10, 2024

t0yv0 commented Sep 10, 2024

iwahbe commented Sep 11, 2024

VenelinMartinov commented Sep 11, 2024

flostadler commented Sep 11, 2024

VenelinMartinov left a comment

pulumi-bot commented Sep 20, 2024

Pass PlannedPrivate from PlanResourceChange to ApplyResourceChange #2407

Pass PlannedPrivate from PlanResourceChange to ApplyResourceChange #2407

Conversation

flostadler commented Sep 9, 2024 • edited Loading

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

flostadler Sep 10, 2024

Choose a reason for hiding this comment

VenelinMartinov Sep 10, 2024

Choose a reason for hiding this comment

flostadler Sep 10, 2024

Choose a reason for hiding this comment

VenelinMartinov commented Sep 10, 2024

flostadler commented Sep 10, 2024

iwahbe left a comment

Choose a reason for hiding this comment

VenelinMartinov commented Sep 10, 2024

VenelinMartinov commented Sep 10, 2024

t0yv0 commented Sep 10, 2024

flostadler commented Sep 10, 2024

VenelinMartinov commented Sep 10, 2024

flostadler commented Sep 10, 2024

t0yv0 commented Sep 10, 2024

iwahbe commented Sep 11, 2024

VenelinMartinov commented Sep 11, 2024

flostadler commented Sep 11, 2024

VenelinMartinov left a comment

Choose a reason for hiding this comment

pulumi-bot commented Sep 20, 2024

Pass `PlannedPrivate` from PlanResourceChange to ApplyResourceChange #2407

Pass `PlannedPrivate` from PlanResourceChange to ApplyResourceChange #2407

flostadler commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading