Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

guineveresaenger · 2023-11-02T17:57:16Z

This PR presents an alternative fix for #1491.

Because makeDetailedDiff can distort the expected changes for a diff set via custom diffs in the upstream provider logic, we should not, in all cases, rely on it to provide us with an accurate pulumirpc.DiffResponse.Changes result. This PR does two things:

Rely on the terraform Diff Attributes length to tell the engine that there are changes via the diff
Gate this behavior behind a new feature flag, SkipDetailedDiffForChanges.

This code will continue to rely on makeDetailedDiff for setting the DetailedDiff field in all cases.

I have verified that this succeeds against these test cases when the GCP provider sets SkipDetailedDiffForChanges to True, and fails when set to False.

Fixes #1491 in the context of unblocking the GCP release.

…DiffForChanges feature flag is set

codecov · 2023-11-02T18:01:02Z

Codecov Report

Merging #1502 (3aa4ce5) into master (b83370c) will decrease coverage by 4.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1502      +/-   ##
==========================================
- Coverage   61.93%   57.91%   -4.02%     
==========================================
  Files         184      285     +101     
  Lines       32836    39629    +6793     
==========================================
+ Hits        20336    22953    +2617     
- Misses      11362    15339    +3977     
- Partials     1138     1337     +199

Files	Coverage Δ
internal/testprovider/schema.go	`75.45% <100.00%> (+0.54%)`	⬆️
pkg/tfbridge/info.go	`93.31% <ø> (ø)`
pkg/tfbridge/provider.go	`48.64% <100.00%> (+0.48%)`	⬆️

... and 102 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

t0yv0 · 2023-11-02T18:13:15Z

pkg/tfbridge/provider.go

@@ -661,6 +661,15 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
 	doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff)
 	detailedDiff, changes := makeDetailedDiff(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, diff)

+	// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
+	// to changes being dropped by Pulumi.
+	// Until we address https://github.com/pulumi/pulumi-terraform-bridge/issues/1501, it is safer to refer


This change literally addresses 1501

It does; but I'm under the impression we'll want to keep it open so we can fix the bugs in makeDetailedDiff "for real".

I'll amend the wording to reflect more nuance.

I don't think so, I'm happy to close. We can open new issues for makeDetailedDiff referencing the closed issue.

iwahbe

I'd like at least 1 synthetic test case to make sure this is hooked up (and to prevent regressions).

iwahbe · 2023-11-02T18:13:37Z

pkg/tfbridge/info.go

+	// Disables using detailed diff to determine diff changes and falls back on the length of TF Diff Attributes.
+	//
+	// See https://github.com/pulumi/pulumi-terraform-bridge/issues/1501
+	SkipDetailedDiffForChanges bool


I don't think we want to support this feature until v4.0.0. When we close #1501 for real, we will want to remove this flag.

Suggested change

// Disables using detailed diff to determine diff changes and falls back on the length of TF Diff Attributes.

//

// See https://github.com/pulumi/pulumi-terraform-bridge/issues/1501

SkipDetailedDiffForChanges bool

// Disables using detailed diff to determine diff changes and falls back on the length of TF Diff Attributes.

//

// See https://github.com/pulumi/pulumi-terraform-bridge/issues/1501

//

// This flag is experimental and may be removed in a future bridge release.

XSkipDetailedDiffForChanges bool

iwahbe · 2023-11-02T18:14:04Z

pkg/tfbridge/provider.go

+	// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
+	// to changes being dropped by Pulumi.
+	// Until we address https://github.com/pulumi/pulumi-terraform-bridge/issues/1501, it is safer to refer
+	// to the Terraform Diff attribute length for setting the DiffResponse.
+	// We will still use `detailedDiff` for diff display purposes.


I really appreciate the comment explaining what is going on!

t0yv0 · 2023-11-02T18:57:04Z

pkg/tfbridge/provider.go

+	// to the Terraform Diff attribute length for setting the DiffResponse.
+	// We will still use `detailedDiff` for diff display purposes.
+	if p.info.SkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
+		changes = pulumirpc.DiffResponse_DIFF_SOME


Noting that it doesn't do the inverse, that is if len(diff.Attributes()) == 0 but makeDetailedDiff returned CHANGES, it does not override it back to NO_CHANGES. Is that perhaps a degenerate/ non-existent situation? Reading the code seems to be the case, so we're probably fine.

We have not verified such a situation as of today to the best of my knowledge.

t0yv0 · 2023-11-02T19:55:47Z

Also fixes AWS P1.. LGTM once tested, consider #1498 test cases I believe this change makes them pass, adding empty tags to map at least certainly should pass.

…ction and when XSkipDetailedDiff is set Adds a new test provider that has a CuztomizeDiff field This change does NOT add a test for the False() -> DIFF_NONE case which occurs today but is a bug we need to fix.

iwahbe

1 nit on the test, but LGTM!

iwahbe · 2023-11-02T20:21:29Z

internal/testprovider/schema.go

@@ -880,3 +880,54 @@ func AssertProvider(f func(data *schemav2.ResourceData)) *schemav2.Provider {
 		},
 	}
 }
+
+func AssertCustomizedDiffProvider(f func(data *schemav2.ResourceData)) *schemav2.Provider {


Since we don't care about calling f here, we can just remove it.

Suggested change

func AssertCustomizedDiffProvider(f func(data *schemav2.ResourceData)) *schemav2.Provider {

func CustomizedDiffProvider() *schemav2.Provider {

t0yv0 · 2023-11-02T20:23:36Z

internal/testprovider/schema.go

+		Schema: map[string]*schemav2.Schema{},
+		ResourcesMap: map[string]*schemav2.Resource{
+			"test_resource": {
+				Schema: map[string]*schemav2.Schema{


You could probably delete a lot here the test will only exercise CustomizeDiff

Adds a `DiffEqualDecisionOverride` to the shim layer which allows provider implementations to specify a diff decision instead of relying on the shim layer to do that. Also adds a `DiffEqualDecisionOverride` to the PRC sdkv2 implementation which opentofu does. Also adds a feature flag `EnableAccurateBridgePreview` which guards this feature. will fix once rolled out: #2293 will fix once rolled out: #1501 will undo once rolled out: #1502 as the underlying issue was fixed during the PRC work. Stacked on #2380

Use TF Diff Attributes to set diff response changes when SkipDetailed…

4e8190d

…DiffForChanges feature flag is set

guineveresaenger requested review from iwahbe and t0yv0 November 2, 2023 17:59

t0yv0 reviewed Nov 2, 2023

View reviewed changes

iwahbe reviewed Nov 2, 2023

View reviewed changes

t0yv0 reviewed Nov 2, 2023

View reviewed changes

make feature flag Xperimental and tweak explanation

18ac83f

t0yv0 mentioned this pull request Nov 2, 2023

Fix adding empty values to maps #1498

Closed

Add test for detecting a diff when a provider has a CustomizeDiff fun…

840d8dd

…ction and when XSkipDetailedDiff is set Adds a new test provider that has a CuztomizeDiff field This change does NOT add a test for the False() -> DIFF_NONE case which occurs today but is a bug we need to fix.

iwahbe self-requested a review November 2, 2023 20:19

iwahbe approved these changes Nov 2, 2023

View reviewed changes

t0yv0 reviewed Nov 2, 2023

View reviewed changes

t0yv0 approved these changes Nov 2, 2023

View reviewed changes

address review comments

3aa4ce5

guineveresaenger enabled auto-merge November 2, 2023 20:59

guineveresaenger merged commit ed35c1e into master Nov 2, 2023
8 checks passed

guineveresaenger deleted the guin/skipdetailedDiff branch November 2, 2023 20:59

EvanBoyle mentioned this pull request Nov 3, 2023

Refer to Terraform diff to generate a more precise diff #1499

Closed

guineveresaenger mentioned this pull request Nov 11, 2023

Verify behavior of upstream Labels pulumi/pulumi-gcp#1275

Closed

t0yv0 mentioned this pull request Aug 9, 2024

Disentangle deciding DiffResponse from detailedDiff #2293

Closed

VenelinMartinov mentioned this pull request Sep 17, 2024

Separate diff decision from presentation #2379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

guineveresaenger commented Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

t0yv0 Nov 2, 2023

guineveresaenger Nov 2, 2023 •

edited

Loading

t0yv0 Nov 2, 2023

iwahbe left a comment

iwahbe Nov 2, 2023

iwahbe Nov 2, 2023

t0yv0 Nov 2, 2023

guineveresaenger Nov 2, 2023

t0yv0 commented Nov 2, 2023

iwahbe left a comment

iwahbe Nov 2, 2023

t0yv0 Nov 2, 2023

	func AssertCustomizedDiffProvider(f func(data schemav2.ResourceData)) schemav2.Provider {
	func CustomizedDiffProvider() *schemav2.Provider {

Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

Conversation

guineveresaenger commented Nov 2, 2023

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

guineveresaenger Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Nov 2, 2023

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2023 •

edited

Loading

guineveresaenger Nov 2, 2023 •

edited

Loading