Fix root volume lookup.

[jmcarp]

The rootDeviceNameInMapping check compares string pointers, which will never be the same. This patch compares string values instead.

[Fixes #4469]

[bflad]

This fix looks correct however I'm in the process of adding an acceptance test to cover the behavior.

We currently check mismatch behavior of the following AMI layout (us-west-2 ami-ef5b69df), which happens to pass because the first element of BlockDeviceMappings list is correct when the pointer to pointer comparisons fail:

            "BlockDeviceMappings": [
                {
                    "DeviceName": "/dev/sda",
                    "Ebs": {
                        "Encrypted": false,
                        "DeleteOnTermination": true,
                        "SnapshotId": "snap-d7cfe852",
                        "VolumeSize": 3,
                        "VolumeType": "standard"
                    }
                },
                {
                    "DeviceName": "/dev/sdb",
                    "VirtualName": "ephemeral0"
                }
            ],

The AMI noted in #4469 (us-gov-west-1 ami-e0aa3f81) uses the following BlockDeviceMappings layout, which explains the strange behavior when it went to use the first element of the list after failing the pointer comparisons:

            "BlockDeviceMappings": [
                {
                    "DeviceName": "/dev/xvdb",
                    "VirtualName": "ephemeral0"
                },
                {
                    "DeviceName": "/dev/xvda",
                    "Ebs": {
                        "Encrypted": false,
                        "DeleteOnTermination": true,
                        "SnapshotId": "snap-03ae98fbcef8877e4",
                        "VolumeSize": 8,
                        "VolumeType": "gp2"
                    }
                }
            ],

I'm trying to find comparable AMI lookups between the two separate partitions, which is proving a little difficult as the BlockDeviceMappings ordering is important and the DescribeImages filtering does not allow you to specify the position of DeviceName (at least that I can figure out).

[jmcarp]

I'm taking a look at unit tests for this fix, which might be easier than acceptance tests, especially if we want tests to run on multiple regions.

[bflad]

Fun fact: the BlockDeviceMappings ordering is different/can be cached differently in the API depending on how its called too:

$ aws ec2 describe-images --filters 'Name=description,Values=CoreOS Container Linux stable 1688.5.3 (HVM)'
{
    "Images": [
        {
            "Architecture": "x86_64",
            "CreationDate": "2018-04-03T06:39:07.000Z",
            "ImageId": "ami-e0aa3f81",
            "ImageLocation": "190570271432/CoreOS-stable-1688.5.3-hvm",
            "ImageType": "machine",
            "Public": true,
            "OwnerId": "190570271432",
            "State": "available",
            "BlockDeviceMappings": [
                {
                    "DeviceName": "/dev/xvda",
                    "Ebs": {
                        "Encrypted": false,
                        "DeleteOnTermination": true,
                        "SnapshotId": "snap-03ae98fbcef8877e4",
                        "VolumeSize": 8,
                        "VolumeType": "gp2"
                    }
                },
                {
                    "DeviceName": "/dev/xvdb",
                    "VirtualName": "ephemeral0"
                }
            ],
            "Description": "CoreOS Container Linux stable 1688.5.3 (HVM)",
            "EnaSupport": true,
            "Hypervisor": "xen",
            "Name": "CoreOS-stable-1688.5.3-hvm",
            "RootDeviceName": "/dev/xvda",
            "RootDeviceType": "ebs",
            "SriovNetSupport": "simple",
            "VirtualizationType": "hvm"
        }
    ]
}

$ aws ec2 describe-images --image-ids ami-e0aa3f81
{
    "Images": [
        {
            "Architecture": "x86_64",
            "CreationDate": "2018-04-03T06:39:07.000Z",
            "ImageId": "ami-e0aa3f81",
            "ImageLocation": "190570271432/CoreOS-stable-1688.5.3-hvm",
            "ImageType": "machine",
            "Public": true,
            "OwnerId": "190570271432",
            "State": "available",
            "BlockDeviceMappings": [
                {
                    "DeviceName": "/dev/xvdb",
                    "VirtualName": "ephemeral0"
                },
                {
                    "DeviceName": "/dev/xvda",
                    "Ebs": {
                        "Encrypted": false,
                        "DeleteOnTermination": true,
                        "SnapshotId": "snap-03ae98fbcef8877e4",
                        "VolumeSize": 8,
                        "VolumeType": "gp2"
                    }
                }
            ],
            "Description": "CoreOS Container Linux stable 1688.5.3 (HVM)",
            "EnaSupport": true,
            "Hypervisor": "xen",
            "Name": "CoreOS-stable-1688.5.3-hvm",
            "RootDeviceName": "/dev/xvda",
            "RootDeviceType": "ebs",
            "SriovNetSupport": "simple",
            "VirtualizationType": "hvm"
        }
    ]
}

[bflad]

LGTM -- thank you very much for adding the unit test. Confirmed it previously fails the new unit test and passes both existing acceptance testing and the new unit test with the code update.

Something I also noticed here that I figure is worth mentioning that the code will incorrectly point to the first element in the following scenario, but this is unrelated to this fix:

		{
			"mismatched root device with ephemeral device",
			[]*ec2.Image{{
				RootDeviceType: aws.String("ebs"),
				RootDeviceName: aws.String("/dev/sda1"),
				BlockDeviceMappings: []*ec2.BlockDeviceMapping{
					{
						DeviceName: aws.String("/dev/sdb")},
						VirtualName: aws.String("ephemeral0"),
					},
					{
						DeviceName: aws.String("/dev/sda")},
						Ebs: &ec2.EbsBlockDevice{},
					},
			}},
			"/dev/sda",
		},

[bflad]

This has been released in version 1.18.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!