ForceNew on a property of an array type doesn't cause replacement when elements of array change

[lukehoban]

The subnet_mapping property of resource_aws_lb is marked ForceNew: true. It is an array of structs. When the array changes though, it does not trigger a replacement of the Load Balancer, instead just trying to do a modification (which looks like it succeeds, but actually doesn't do anything in the Terraform provider since it doesn't expect to have to try to do this modification in place).

~ aws:elasticloadbalancingv2/loadBalancer:LoadBalancer: (modify)
      [id=arn:aws:elasticloadbalancing:us-east-1:153052954103:loadbalancer/net/pulumi-s-lb-1-7b62235d2fd3d529/7c5cf585dce3177a]
      [urn=urn:pulumi:fulltest::containers::aws:elasticloadbalancingv2/loadBalancer:LoadBalancer::pulumi-s-lb-1]
      enableDeletionProtection: false
      idleTimeout             : "60"
      internal                : false
      loadBalancerType        : "network"
      name                    : "pulumi-s-lb-1-7b62235d2fd3d529"
      subnetMapping           : [
      subnetMapping           : {
        - subnetId: "subnet-2030b044"
        + subnetId: "subnet-442faf20"
      }
      ]

	"subnet_mapping": {
				Type:     schema.TypeSet,
				Optional: true,
				ForceNew: true,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"subnet_id": {
							Type:     schema.TypeString,
							Required: true,
						},
						"allocation_id": {
							Type:     schema.TypeString,
							Optional: true,
						},
					},
				},
				Set: func(v interface{}) int {
					var buf bytes.Buffer
					m := v.(map[string]interface{})
					buf.WriteString(fmt.Sprintf("%s-", m["subnet_id"].(string)))
					if m["allocation_id"] != "" {
						buf.WriteString(fmt.Sprintf("%s-", m["allocation_id"].(string)))
					}
					return hashcode.String(buf.String())
				},
			},

[joeduffy]

@lukehoban Is this still an issue?

[lukehoban]

Yes.

import * as aws from "@pulumi/aws";
import * as awsinfra from "@pulumi/cloud-aws/infrastructure";

let network = new awsinfra.Network("net", {
    numberOfAvailabilityZones: 2,
    privateSubnets: true,
});

let lb = new aws.elasticloadbalancingv2.LoadBalancer("foo", {
    loadBalancerType: "network",
    subnetMapping: [{
        // Change this to `network.subnetIds[1]`
        subnetId: network.subnetIds[0],
    }],
});

After doing an initial pulumi update, make the change noted in the comment above and then pulumi update again:

$ pulumi up
Performing changes:
* pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:luke-local::cluster::pulumi:pulumi:Stack::cluster-luke-local]
    ~ aws:elasticloadbalancingv2/loadBalancer:LoadBalancer: (update)
        [id=arn:aws:elasticloadbalancing:us-west-2:153052954103:loadbalancer/net/foo-00fdae2/387129813e53a0b6]
        [urn=urn:pulumi:luke-local::cluster::aws:elasticloadbalancingv2/loadBalancer:LoadBalancer::foo]
        enableDeletionProtection: false
        idleTimeout             : 60
        loadBalancerType        : "network"
        name                    : "foo-00fdae2"
      ~ subnetMapping           : [
          ~ [0]: {
                  - subnetId: "subnet-dcb24aa5"
                  + subnetId: "subnet-4c0c1704"
                }
        ]
        ---outputs:---
        arn                     : "arn:aws:elasticloadbalancing:us-west-2:153052954103:loadbalancer/net/foo-00fdae2/387129813e53a0b6"
        arnSuffix               : "net/foo-00fdae2/387129813e53a0b6"
        dnsName                 : "foo-00fdae2-387129813e53a0b6.elb.us-west-2.amazonaws.com"
        id                      : "arn:aws:elasticloadbalancing:us-west-2:153052954103:loadbalancer/net/foo-00fdae2/387129813e53a0b6"
        idleTimeout             : "60"
        internal                : false
        ipAddressType           : "ipv4"
        subnetMapping           : [
            [0]: {
                subnetId    : "subnet-4c0c1704"
            }
        ]
        subnets                 : [
            [0]: "subnet-dcb24aa5"
        ]
        vpcId                   : "vpc-7e70f007"
        zoneId                  : "Z18D5FSROUN65G"
info: 1 change performed:
    ~ 1 resource updated
      18 resources unchanged
Update duration: 33.599665483s

Note that:

1. This was seen as an inplace-update, not a replacement, of the LoadBalancer
2. Although it says it's going to change the subnet the value of the subnet in the output is the old value, so no change was actually made.

[lukehoban]

Just hit this again - made a change to a nested property in an input which should have triggered replacement, but it only did an update (which didn't behave correctly at all - even though it succeeded).

This is a pretty serious (and subtle) bug.

[mmdriley]

These are the ResourceAttrDiffs we get back from Terraform.Diff:

info: I0405 15:40:01.034566   49341 provider.go:308] attribute access_logs.#, RequiresNew: false, old: , new: 
info: I0405 15:40:01.034583   49341 provider.go:308] attribute subnet_mapping.3213777275.allocation_id, RequiresNew: false, old: , new: 
info: I0405 15:40:01.034589   49341 provider.go:308] attribute subnet_mapping.3867093057.subnet_id, RequiresNew: false, old: , new: subnet-983ae8d3
info: I0405 15:40:01.034594   49341 provider.go:308] attribute subnet_mapping.3867093057.allocation_id, RequiresNew: false, old: , new: 
info: I0405 15:40:01.034601   49341 provider.go:308] attribute subnet_mapping.3213777275.subnet_id, RequiresNew: false, old: subnet-d3d764aa, new: 

[joeduffy]

I wonder if this is a bug in the way we're doing set diffing. For instance, as far as I know, we do nothing with the custom Set function defined on the subnet_mapping property. The above diff makes it look like we're merely modifying elements of the set, rather than removing and adding one.

I'm curious what @pgavlin thinks so long as he isn't too busy drikker øl.

[mmdriley]

This is a bug in some combination of the Terraform AWS provider and Terraform's set diffing logic.

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  ~ aws_lb.test
      enable_http2:                            "" => "true"
      idle_timeout:                            "" => "60"
      subnet_mapping.2493713080.allocation_id: "" => ""
      subnet_mapping.2493713080.subnet_id:     "" => "subnet-d3d764ab"
      subnet_mapping.3213777275.allocation_id: "" => ""
      subnet_mapping.3213777275.subnet_id:     "subnet-d3d764aa" => ""


Plan: 0 to add, 1 to change, 0 to destroy.

First, recall how terraform-provider-aws defines subnet_mapping:

"subnet_mapping": {
	Type:     schema.TypeSet,
	Optional: true,
	Computed: true,
	ForceNew: true,
	Elem: &schema.Resource{
		Schema: map[string]*schema.Schema{
			"subnet_id": {
				Type:     schema.TypeString,
				Required: true,
			},
			"allocation_id": {
				Type:     schema.TypeString,
				Optional: true,
			},
		},
	},
	Set: func(v interface{}) int {
		var buf bytes.Buffer
		m := v.(map[string]interface{})
		buf.WriteString(fmt.Sprintf("%s-", m["subnet_id"].(string)))
		if m["allocation_id"] != "" {
			buf.WriteString(fmt.Sprintf("%s-", m["allocation_id"].(string)))
		}
		return hashcode.String(buf.String())
	},
},

(ref)

Specifically: a ForceNew attribute of TypeSet, with Elems that are Resources. Those Resources, in turn, have attributes that are not ForceNew.

So... if we change subnet_id, the Resource containing it doesn't need to be recreated because it doesn't have ForceNew. So... have the set elements changed identity? Not as far as diffSet is concerned: https://github.com/hashicorp/terraform/blob/3837d0792a65b18f662d8855a89ae9379e0ed6ad/helper/schema/schema.go#L1119-L1127

Compare that to the logic immediately afterward when the element is a schema.Schema. There, Terraform copies ForceNew from the parent: https://github.com/hashicorp/terraform/blob/3837d0792a65b18f662d8855a89ae9379e0ed6ad/helper/schema/schema.go#L1128-L1140

[mmdriley]

There are a lot of examples of ForceNew-Set-of-Elem-of-Resource where all of the Resource's attributes are in turn set ForceNew:

• https://github.com/terraform-providers/terraform-provider-aws/blob/13f4090370b70599b2de369fc160e935d165ba80/aws/resource_aws_elastic_transcoder_preset.go#L62-L91
• https://github.com/terraform-providers/terraform-provider-aws/blob/ae06724ac95e95c7dcb5e435f54ffe1209e33ab8/aws/resource_aws_ssm_maintenance_window_task.go#L55-L74
• https://github.com/terraform-providers/terraform-provider-aws/blob/b8d4e1570fc43a2acee6b6e47f63c9db6b067fa2/aws/resource_aws_codecommit_trigger.go#L28-L68
• https://github.com/terraform-providers/terraform-provider-aws/blob/db201892e565d463fa07fb48d5c58464c3bda3dc/aws/resource_aws_ecs_task_definition.go#L111-L130

so I expect the right fix from the Terraform side is to mark the two attributes on this Resource as ForceNew.

As for us, we're going to change subnetMappings to subnets because the latter is simpler and better-supported by Terraform.

[mmdriley]

Sent hashicorp/terraform-provider-aws#4086 upstream.

[lukehoban]

As for us, we're going to change subnetMappings to subnets because the latter is simpler and better-supported by Terraform.

This is done now in pulumi/pulumi-cloud#451.

Given that - I think we are okay to close this out now. Thanks @mmdriley for tracking things down here.