Present clearer diff fo single element changes to sets

[lukehoban]

Currently, when a single item in a set has a diff, we show every item of the set having changed places.

For example - in the diff below (from pulumi/pulumi-aws#920) - there is one element actually changing (IamInstanceProfile's value is changing from an id to an ARN) - but all other elements are getting mixed up:

Previewing update (cameron):
  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:cameron::beanstalk-env-order-bug::pulumi:pulumi:Stack::beanstalk-env-order-bug-cameron]
    ~ aws:elasticbeanstalk/environment:Environment: (update)
        [id=e-mvzjv6gg2f]
        [urn=urn:pulumi:cameron::beanstalk-env-order-bug::aws:elasticbeanstalk/environment:Environment::beanstalk-env-order-bug-cameron]
        [provider=urn:pulumi:cameron::beanstalk-env-order-bug::pulumi:providers:aws::default_1_27_0::6d629690-746c-4946-b638-718881ccf3f6]
      ~ settings: [
          ~ [0]: {
                  ~ name     : "VPCId" => "VPCId"
                  ~ namespace: "aws:ec2:vpc" => "aws:ec2:vpc"
                  - resource : ""
                  ~ value    : "vpc-0a79acb18cf59054f" => "vpc-0a79acb18cf59054f"
                }
          ~ [1]: {
                  ~ name     : "MaxSize" => "Subnets"
                  ~ namespace: "aws:autoscaling:asg" => "aws:ec2:vpc"
                  ~ value    : "1" => "subnet-037fba84fc1fd4664"
                }
          ~ [2]: {
                  ~ name     : "setting1" => "IamInstanceProfile"
                  ~ namespace: "aws:elasticbeanstalk:application:environment" => "aws:autoscaling:launchconfiguration"
                  ~ value    : "value1" => "arn:aws:iam::052848974346:instance-profile/beanstalk-env-order-bug-cameron-617dfaf"
                }
          ~ [3]: {
                  ~ name     : "MinSize" => "InstanceType"
                  ~ namespace: "aws:autoscaling:asg" => "aws:autoscaling:launchconfiguration"
                  ~ value    : "1" => "t3.medium"
                }
          ~ [4]: {
                  ~ name     : "IamInstanceProfile" => "MinSize"
                  ~ namespace: "aws:autoscaling:launchconfiguration" => "aws:autoscaling:asg"
                  ~ value    : "beanstalk-env-order-bug-cameron-617dfaf" => "1"
                }
          ~ [5]: {
                  ~ name     : "setting2" => "MaxSize"
                  ~ namespace: "aws:elasticbeanstalk:application:environment" => "aws:autoscaling:asg"
                  ~ value    : "value2" => "1"
                }
          ~ [6]: {
                  ~ name     : "Subnets" => "setting1"
                  ~ namespace: "aws:ec2:vpc" => "aws:elasticbeanstalk:application:environment"
                  ~ value    : "subnet-037fba84fc1fd4664" => "value1"
                }
          ~ [7]: {
                  ~ name     : "InstanceType" => "setting2"
                  ~ namespace: "aws:autoscaling:launchconfiguration" => "aws:elasticbeanstalk:application:environment"
                  ~ value    : "t3.medium" => "value2"
                }
          ~ [8]: {
                  ~ name     : "setting3" => "setting3"
                  ~ namespace: "aws:elasticbeanstalk:application:environment" => "aws:elasticbeanstalk:application:environment"
                  ~ value    : "value3" => "value3"
                }
        ]

[t0yv0]

Another user affected: pulumi/pulumi-aws#2856

Also, there is another related issue here #1756 that goes beyond just presentation, currently the user is not able to use ignoreChanges to do-what-I-mean over such set element changes.

[t0yv0]

Another repro of this with some debug details on why it's happening. #1894 - I think this is exactly the problem in pulumi/pulumi-aws#2095

[t0yv0]

Thinking about possible solutions here; one option would be to extend the CLI with special support in the detailedDiff to express "element added" vs "element removed" vs "element updated" changes irrespective of numeric index in a set.

If we do not change the CLI interface, I am thinking we could still make progress if we drop the assumption that the numeric indices [0..3] mean anything related to the program positions.

This would work as follows. Suppose we are changing from set S1: set<T> to set S2: set<T> with set element identity function f: T -> K. The diff between S1 and S2 will have:

type diff = array<changes<T>>
type change<T> = added(T) | removed(T) | changed(T, T)

The invariant is that changes only happen at the same identity:

changed x y ==> f(x) == f(y)

Then we can sort changes by f. This gives an canonical order and allocates integers to every change - these integers no longer line up with element positions in the source program before and after the change, but since Pulumi prefers sorting by f already this "almost lines up", the advantage being that there is no mixup happening if the identity function f is correctly specified: changing an element identity will have one removed entry and one added entry, and a changed entry will always correspond to an element that preserves its identity.

Unfortunately I still don't have it worked out how to naturally interpret ignoreChanges in the setting #1756 - and a complete proposal should have an answer for that. The key difficulty is that ignoring changes may be affecting element identity.

[t0yv0]

There is a subtlety that @corymhall points out that indicates my mental model of the set semantics above is not accurate. The .Set hash customizer acts not as an element key but indeed as identity. The following program is considered a no-change since all the former and new elements map to the same value Set("A") == Set("B") == Set("C") == 1:

func TestSetChanging(t *testing.T) {
	skipUnlessLinux(t)
	resource := &schema.Resource{
		Schema: map[string]*schema.Schema{
			"set": {
				Type:     schema.TypeSet,
				Optional: true,
				Set: func(i interface{}) int {
					return 1
				},
				Elem: &schema.Schema{
					Type:     schema.TypeString,
					Optional: true,
				},
			},
		},
	}
	runDiffCheck(t, diffTestCase{
		Resource: resource,
		Config1: map[string]any{
			"set": []any{"A"},
		},
		Config2: map[string]any{
			"set": []any{"B", "C"},
		},
	})
}

Therefore it would seem change(T, T) diffs should never even arise in TF, because an element is either being added or removed but never changed in place.

There is a slight complication I recall, possibly tangential here but listing for completeness: TF treats sets of objects in a bit map-like fashion but it seems not quite tied to the Set function, coming from https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfshim/sdk-v2/internal/tf/plans/objchange/objchange.go#L430 where objects inside a set are matched by non-computed attr-sets to propagate computed attrs in the planning phase.

[mikhailshilkov]

Another case reported by a customer:

... specifically around EC2 security group ingress. We have an array of security group rules specified (of type pulumi.Input<pulumi.Input<inputs.ec2.SecurityGroupIngress>[]>). I noticed that when we delete one rule, the diff is pretty confusing because the array is unordered. It is difficult to see what was actually deleted in the diff because it rearranges every single rule in the update.

[t0yv0]

@mjeffryes @iwahbe per a conversation with @lukehoban I'm convinced we need a gRPC protocol change to accommodate this and other TF impedance mismatch, there's a high barrier to these though so we'll need to take some time to iterate on a proposal and some prototyping.

[pulumi-bot]

This issue has been addressed in PR #2451 and shipped in release v3.94.0.