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

guineveresaenger · 2023-10-31T23:44:04Z

This pull request looks at the Terraform Diff Attributes, checks if they exist in the Pulumi diff generated by olds and news, and adds any additional diff information to Pulumi diff.

Points of concern:

This change is not yet under test; suggestions or help modeling a schema map and schema info are welcome.
How detailed we can make the property diff on each schema path? There's room for improvement here.

Addresses #1491.

Add resulting diff information to Pulumi diff

iwahbe

This will need to be under test to merge. To start, I would write table-driven unit tests for parseTFPath. You can use shim/schema.* to mock shim.Schema* interfaces:

pulumi-terraform-bridge/pkg/tfshim/schema/schema.go

Lines 140 to 142 in b83370c

    
           //nolint:revive 
        
           type SchemaMap map[string]shim.Schema

iwahbe · 2023-11-01T00:41:03Z

pkg/tfbridge/diff.go

+			}
+			// reset the schema map to point at the nested schema
+			if nestedSchema != nil {
+				if res, isres := nestedSchema.Elem().(shim.Resource); isres {


If !isres, then tfs should be set to nil. A misalignment doesn't mean that no traversal happened.

Also, it's not actually a resource:

Suggested change

if res, isres := nestedSchema.Elem().(shim.Resource); isres {

if res, isObj := nestedSchema.Elem().(shim.Resource); isObj {

iwahbe · 2023-11-01T00:43:36Z

pkg/tfbridge/diff.go

+		nestedSchema, found := tfs.GetOk(field)
+		if found {


Tightening the scoping makes it harder to misuse these variables.

Suggested change

nestedSchema, found := tfs.GetOk(field)

if found {

if nestedSchema, found := tfs.GetOk(field); found {

iwahbe · 2023-11-01T00:46:10Z

pkg/tfbridge/diff.go

+		} else {
+			// check if the field is a number
+			// the error value for strconv.Atoi is 0, so we look at the error.
+			_, err := strconv.Atoi(field) // the error value here is 0, so we look at the error.
+			if err == nil {
+				// we found an index
+				pulumiPath = pulumiPath + "[" + field + "]"
+			} else {
+				// Ignore special characters from Terraform


I think this would read clearer if we reduce the nesting.

Suggested change

} else {

// check if the field is a number

// the error value for strconv.Atoi is 0, so we look at the error.

_, err := strconv.Atoi(field) // the error value here is 0, so we look at the error.

if err == nil {

// we found an index

pulumiPath = pulumiPath + "[" + field + "]"

} else {

// Ignore special characters from Terraform

continue

}

// check if the field is a number

// the error value for strconv.Atoi is 0, so we look at the error.

_, err := strconv.Atoi(field) // the error value here is 0, so we look at the error.

if err == nil {

// we found an index

pulumiPath = pulumiPath + "[" + field + "]"

continue

}

// Ignore special characters from Terraform

iwahbe · 2023-11-01T00:47:46Z

pkg/tfbridge/diff.go

+				// TODO: Using PropertyDiff_ADD is not entirely accurate in every case.
+				// TODO: (cont) We may want to pass on a more generic UPDATE strategy here - less precise, less error prone.


Can you give an example for the case there PropertyDiff_ADD is the wrong case? What is the effect of getting the kind wrong (is it just display)?

It could be a PropertyDiff_UPDATE, on an already-existing object key.

t0yv0

Thank you for looking into this!

Making changes to this code is currently an extremely high-risk area, and existing coverage is very inadequate to verify changes. If you're contributing to the expectation is to have 100% coverage on changes but also back-filling missing coverage.

While it is possible to continue with the chosen approach and get there, it is going to be require some work.

Can we discuss alternative approaches that avoid high-risk edits? Perhaps we can start with a repro of the original problem?

t0yv0 · 2023-11-01T16:50:51Z

pkg/tfbridge/diff.go

 	changes := pulumirpc.DiffResponse_DIFF_NONE
 	if len(diff) > 0 || *forceDiff {
 		changes = pulumirpc.DiffResponse_DIFF_SOME
 	}
 	return diff, changes
 }
+
+func parseTFPath(path string, tfs shim.SchemaMap, ps map[string]*SchemaInfo) (string, bool) {


Do we understand the semantics of paths here, both the TF paths and Pulumi paths?

What happens with set elements? (represented as hashes in TF paths). Should these be mapped to Pulumi numbers as steps are represented as lists?

What happens with flattening (when Pulumi paths are shorter than TF paths with MaxItems=1).

t0yv0 · 2023-11-01T16:52:03Z

pkg/tfbridge/diff.go

+				diff[pulumiPath] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
+			}
+		}
+	}
 	changes := pulumirpc.DiffResponse_DIFF_NONE


What would happen if we considered changing the changes only without populating diff[pulumiPath] at all? Is there a way such an approach could get us to passing the bug, possibly with incorrect detailed diff displayed to the user?

t0yv0 · 2023-11-01T17:38:18Z

Linking related issue #1498

t0yv0 · 2023-11-01T17:39:54Z

pkg/tfbridge/diff.go

@@ -309,9 +309,71 @@ func makeDetailedDiff(
 		makePropertyDiff(ctx, en, string(k), v, tfDiff, diff, forceDiff, etf, eps, true, useRawNames(etf))
 	}

+	// In some cases, the terraform diff will have additional attributes not covered by Pulumi's olds and news.


For my education can you spell it out as a full example for the GCP problem. I understand this is related to GCP provider-level labels, what's the attribute name and is it possible to show a printout of tfDiff in question. Much appreciated.

Here is a tfDiff.Attributes() when adding the new GCP labels:

"effective_labels.%": { Old: "3", New: "4", NewComputed: false, NewRemoved: false, NewExtra: nil, RequiresNew: false, Sensitive: false, Type: 0x0, }, "effective_labels.new": { Old: "", New: "defaultLabel", NewComputed: false, NewRemoved: false, NewExtra: nil, RequiresNew: false, Sensitive: false, Type: 0x0, }, "terraform_labels.%": { Old: "3", New: "4", NewComputed: false, NewRemoved: false, NewExtra: nil, RequiresNew: false, Sensitive: false, Type: 0x0, }, "terraform_labels.new": { Old: "", New: "defaultLabel", NewComputed: false, NewRemoved: false, NewExtra: nil, RequiresNew: false, Sensitive: false, Type: 0x0, }, }

EvanBoyle · 2023-11-03T01:41:36Z

Should we close this given that we merged #1502?

t0yv0 · 2023-11-03T15:27:46Z

I have filed #1504 detailing remaining work to consider here, and why we should consider finishing this up. I will close for now. I think there's a substantial amount of work required to get this right; the code in this PR will still be linked to 1504 if we need to find it again.

guineveresaenger added 3 commits October 31, 2023 16:27

Parse Terraform Path into Pulumi Path

02ff6b9

Add resulting diff information to Pulumi diff

fix explicit if statement

3a852ad

run gomt -s -w

4f9dac3

iwahbe self-requested a review November 1, 2023 00:48

iwahbe reviewed Nov 1, 2023

View reviewed changes

t0yv0 self-requested a review November 1, 2023 15:59

t0yv0 requested changes Nov 1, 2023

View reviewed changes

t0yv0 reviewed Nov 1, 2023

View reviewed changes

t0yv0 mentioned this pull request Nov 3, 2023

Fix makeDetailedDiff inaccuracies #1504

Closed

t0yv0 closed this Nov 3, 2023

guineveresaenger deleted the guin/labels-diff branch February 23, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

guineveresaenger commented Oct 31, 2023

iwahbe left a comment

iwahbe Nov 1, 2023

iwahbe Nov 1, 2023

iwahbe Nov 1, 2023

iwahbe Nov 1, 2023

guineveresaenger Nov 1, 2023

t0yv0 left a comment

t0yv0 Nov 1, 2023

t0yv0 Nov 1, 2023

t0yv0 commented Nov 1, 2023

t0yv0 Nov 1, 2023

guineveresaenger Nov 1, 2023

EvanBoyle commented Nov 3, 2023

t0yv0 commented Nov 3, 2023

	if res, isres := nestedSchema.Elem().(shim.Resource); isres {
	if res, isObj := nestedSchema.Elem().(shim.Resource); isObj {

	nestedSchema, found := tfs.GetOk(field)
	if found {
	if nestedSchema, found := tfs.GetOk(field); found {

		// TODO: Using PropertyDiff_ADD is not entirely accurate in every case.
		// TODO: (cont) We may want to pass on a more generic UPDATE strategy here - less precise, less error prone.

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

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

Conversation

guineveresaenger commented Oct 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a 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 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanBoyle commented Nov 3, 2023

t0yv0 commented Nov 3, 2023