Detailed diff duplicated collection diffs #2234
Assignees
Labels
Comments
VenelinMartinov
added
kind/bug
|
Can we construct an AWS-based test case here? I can help. |
import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
const config = new pulumi.Config();
const step = config.requireNumber("step");
const lt = new aws.ec2.LaunchTemplate("lt", {
blockDeviceMappings: step === 0 ? [] : [
{deviceName: "/dev/sdf",
ebs: {volumeSize: 20}},
],
});
export const ltId = lt.id;Getting: Which appears reasonable. Any ideas why this doesn't reproduce on LaunchTemplate? |
VenelinMartinov
added a commit
that referenced
this issue
Aug 20, 2024
This adds integration tests around detailed diff for string, list, set and map attributes. Quite a few issues with: - Handling empty collections being removed/added - these are not shown in most cases - Adding/removing elements being duplicated in the detailed diff output - Outright wrong diff for `set element removed middle` and `set element added front` similar to #2103 Opened follow-up issues: - [Detailed diff empty collection diffs not shown](#2233) - [Detailed diff duplicated collection diffs](#2234) - [Detailed diff for set element changes shows more additions/removals than present](#2235) Related to #1895
|
Two things:
{
"method": "/pulumirpc.ResourceProvider/Create",
"request": {
"urn": "urn:pulumi:dev::bridge_2234_aws::aws:ec2/launchTemplate:LaunchTemplate::lt",
"properties": {
"__defaults": [
"name"
],
"name": "lt-da69eeb"
}
},
"response": {
"id": "lt-015036d7e87cf914d",
"properties": {
"arn": "arn:aws:ec2:us-east-1:616138583583:launch-template/lt-015036d7e87cf914d",
"blockDeviceMappings": [],
"capacityReservationSpecification": null,
"cpuOptions": null,
"defaultVersion": 1,
"description": "",
"disableApiStop": false,
"disableApiTermination": false,
"ebsOptimized": "",
"elasticGpuSpecifications": [],
"elasticInferenceAccelerator": null,
"enclaveOptions": null,
"hibernationOptions": null,
"iamInstanceProfile": null,
"id": "lt-015036d7e87cf914d",
"imageId": "",
"instanceInitiatedShutdownBehavior": "",
"instanceMarketOptions": null,
"instanceRequirements": null,
"instanceType": "",
"kernelId": "",
"keyName": "",
"latestVersion": 1,
"licenseSpecifications": [],
"maintenanceOptions": null,
"metadataOptions": null,
"monitoring": null,
"name": "lt-da69eeb",
"namePrefix": "",
"networkInterfaces": [],
"placement": null,
"privateDnsNameOptions": null,
"ramDiskId": "",
"securityGroupNames": [],
"tagSpecifications": [],
"userData": "",
"vpcSecurityGroupIds": []
}
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}This is what happens with LaunchTemplate. |
|
The issue seems to be fixed in #2379 - looks like we are duplicating the diff in the response to the engine in pulumi-terraform-bridge/pkg/tfbridge/provider.go Line 1159 in cf59a25 |
|
Removed from #2379 as out of scope, will revisit later. |
VenelinMartinov
added a commit
that referenced
this issue
Sep 16, 2024
This adds tests for the detailed diff output of - List blocks - Set blocks - MaxItemsOne list blocks for adding, removing, changing the property and adding, removing, changing nested properties in the block. A few issues here: - #2234 affects quite a few of the cases - #2399 diffs are displayed on nested properties which are not specified when a block is removed - #2400 diffs are displayed on the `__defaults` property when the number of elements in the block changes.
|
It turns out #2294 is not quite enough here. This fixes lists and sets but we seem to be mishandling maps in MakeTerraformResult: pulumi-terraform-bridge/pkg/tfbridge/schema.go Line 1011 in 6b95c60 See test in 51dd086#diff-5ecbec35cb8a03b89c5be410138532ea2da5904bfd52fdcb604d3b75414a6044R978 Nil and empty maps are both represented as an empty map by MakeTerraformResult. Should be fixable. |
|
Actually it turns out the nullness of maps is lost by PlanResourceChange here: both prop and cfg there contain |
|
A possible fix here is to walk the the pre-processed This would happen as a post-processing step of the detailed diff calculation. Alternatively, we can also include the pre-processed I'm going to leave that for later. |
|
this is all sidestepped if we only return the bottom-most property which has a diff, instead of all the properties in the tree - this works so much better. |
VenelinMartinov
added a commit
that referenced
this issue
Oct 1, 2024
…ge (#2405) This PR adds a new algorithm for calculating the detailed diff which acts on the pulumi property values for the SDKv2 bridge, comparing the planned state returned by `Diff` to the prior state. This is flagged under the existing `DiffEqualDecisionOverride` feature flag. The results look very promising so far - all the detailed diff integration tests pass and the issues previously reported are almost all fixed by this. ## Why ## The current detailed diff acts on the `InstanceDiff` structure which is internal to the plugin-sdk. This has a few shortcomings: - TF does not actually refer to this for the detailed diff, so it might point to diffs which are not present in TF. - It refers to TF attribute paths, which are tricky to translate back in some cases. - It does not compare the planned state with the prior state but compares the news vs olds - this misses properties added by TF planning. ## Implementation ## The new algorithm is under `pkg/tfbridge/detailed_diff.go` and used in `pkg/tfbridge/provider.go:Diff` only for the SDKv2 and only if the `DiffEqualDecisionOverride` is enabled. The main entrypoint is `makePulumiDetailedDiffV2` - which in turn calls `makePropDiff` on each property. That branches on the type of the property and we have a separate function responsible for calculating the detailed diff for each property type. There's a few interesting bits here: - We always walk the full tree even when comparing against a nil property and simplify the diff after in `simplifyDiff`. This is in order to get replaces correct. More on that later. - When returning the diff to the engine we only return the simplest possible diff which explains the changes. So instead of `prop: Update, prop.subprop: Add`, we only return `prop.subprop: Add`. This seems to work much better in the engine and goes around some weird behaviour in the detailed diff display (see #2234 and #2400). Moreover, the first can be inferred from the second, so there is no reason for the bridge to return the full tree if only a leaf was changed. - We can not correctly identify diffs between nil and empty collections because of how the TF SDKv2 works without additional work. This is studied in `TestNilVsEmptyListProperty` and `TestNilVsEmptyMapProperty` in `pkg/cross-tests/diff_cross_test.go`. This is probably fine for now and a full fix is not possible. We can make a partial fix for non-computed properties by inspecting the pulumi inputs, before the plan. - There's a bit of an edge case with Unknowns and Replaces - we might not have enough information to tell the user they'll get a replace because the property which causes the replaces is nested inside an unknown. There's not much to do here, except to choose which side to err on. The algorithm currently does not say there's a replace. ### On Replaces ### We do not short-circuit detailed diffs when comparing non-nil properties against nil ones. The reason for that is that a replace might be triggered by a `ForceNew` inside a nested property of a non-`ForceNew` property. We instead always walk the full tree even when comparing against a nil property. We then later do a simplification step for the detailed diff in `simplifyDiff` in order to reduce the diff to what the user expects to see. For example: This is a list of objects with two properties, one of which is `ForceNew` ``` schema = { "list_prop": { Type: List, Elem: { "prop": String "force_new_prop": StringWithForceNew } } } ``` We are diffing an unspecified list against a list with a single element ``` olds = {} news = { "list_prop": [ { "prop": "val", "force_new_prop" : "val" } ] ``` The user expects to see: ``` + list_prop ``` or because of how collections work in TF SDKv2 (see #2233) ``` + list_prop[0] ``` An element was added to the list. When calculating the detailed diff we can short-circuit the diff when comparing the two lists, as we can see they have different lengths. This would identify the correct element to be displayed to the user as the reason for the diff but would fail to identify the diff as a replace, since we never saw the `ForceNew` on the nested property `force_new_prop` of the list elements. That is why, instead of short-circuiting the diff, we walk the full tree down and compare every property against a nil if it is not specified on the other side. We then to a simplification pass over the detailed diff, which respects any replaces triggered by nested properties and bubbles these up. There is a full case study of the TF behaviour around replaces in `pkg/cross-tests/diff_cross_test.go` `TestAttributeCollectionForceNew`, `TestBlockCollectionForceNew`, `TestBlockCollectionElementForceNew`. ## Testing ## Unit tests for the detailed diff algorithm in `pkg/tfbridge/detailed_diff_test.go` - this tries to cover all meaningful permutations of schemas: - `TestBasicDetailedDiff` tests all the meaningful pairs between nil values, empty values, non-empty values and computed for each TF type. - `TestDetailedDiffObject`, `TestDetailedDiffList`, `TestDetailedDiffMap`, `TestDetailedDiffSet` covers the cases not covered above for object and collection types. - `TestDetailedDiffTFForceNewPlain`, `TestDetailedDiffTFForceNewAttributeCollection`, `TestDetailedDiffTFForceNewBlockCollection`, `TestDetailedDiffTFForceNewElemBlockCollection`, `TestDetailedDiffTFForceNewObject` cover `ForceNew` behaviour in all TF types. - `TestDetailedDiffPulumiSchemaOverride` covers pulumi schema overrides Integration tests in `pkg/tests/schema_pulumi_test.go`, mostly `TestDetailedDiffPlainTypes` and `TestUnknownBlocks`. Note that most of the edge cases and issues previously discovered here are resolved by this PR. ## Follow-up Work ## Not done but will follow-up in separate PRs: - Non-trivial set diffing - sets are currently diffed the same as lists, which has all the previous issues with set diffs. #2200 - Non-trivial list diffing - we can do something like #2295 here. Note that we still need to investigate how this interacts with ForceNew and how TF preserves or does not preserve list element identity. We likely need to respect that in order not to have confusing unexplained replaces caused by list element changes. ## Related Issues ## fixes: - fixes #2294 - fixes #2296 - fixes #1504 - fixes #1895 - fixes #2141 - fixes #2235 - fixes #2325 - fixes #2400 - fixes #2234 - fixes #2427 does not fix: - #2399 - we must either fix the saved state to not contain redundant nils or fix the display logic in the engine to ignore these. - #2233 - This works the same as TF and seems to be a limitation of the SDKv2.
|
This issue has been addressed in PR #2405 and shipped in release v3.92.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What happened?
Diffs when a non-empty collection is added/removed seem to be duplicated in the detailed diff output.
Example
https://github.com/pulumi/pulumi-terraform-bridge/pull/2159/files
list addedfor an exampleOutput of
pulumi about.
Additional context
No response
Contributing
Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).
The text was updated successfully, but these errors were encountered: