resource/aws_instance: Ignore change of user_data from omission to empty

[zehua]

Fixes #4954

https://github.com/terraform-providers/terraform-provider-aws/pull/4991/files#r208085096 fixed the issue when changing from empty user_data to omitting the field. But the other way is also a valid case that we should ignore, i.e., changing from omitting the user_data field to giving it an empty value.

Changes proposed in this pull request:

• Allow aws_instance resource to ignore when EC2 API returns empty string user_data, for changes in both direction: empty value in the field <==> field being omitted.

Did not run acceptance testing. But I've tested it locally.

[bflad]

@zehua can you please provide a previously failing acceptance test or configuration for this behavior so we can ensure we don't create a regression in the future? Thanks.

[zehua]

@bflad I don't think there is an acceptance test that covers this. To demonstrate the bug, use the example tf file from https://www.terraform.io/docs/providers/aws/r/instance.html

# Create a new instance of the latest Ubuntu 14.04 on an
# t2.micro node with an AWS Tag naming it "HelloWorld"
provider "aws" {
  region = "us-west-2"
}

data "aws_ami" "ubuntu" {
  most_recent = true

  filter {
    name   = "name"
    values = ["ubuntu/images/hvm-ssd/ubuntu-trusty-14.04-amd64-server-*"]
  }

  filter {
    name   = "virtualization-type"
    values = ["hvm"]
  }

  owners = ["099720109477"] # Canonical
}

resource "aws_instance" "web" {
  ami           = "${data.aws_ami.ubuntu.id}"
  instance_type = "t2.micro"

  tags {
    Name = "HelloWorld"
  }
}

init, plan and apply this to create a t2.micro instance. Then update the tf file to include an empty user_data

resource "aws_instance" "web" {
  ami           = "${data.aws_ami.ubuntu.id}"
  instance_type = "t2.micro"
  user_data = "" // empty

  tags {
    Name = "HelloWorld"
  }
}

Now run plan, expected output is no changes, but actual output is:

-/+ aws_instance.web (new resource required)
      id:                           "i-06f17cc4c902c76af" => <computed> (forces new resource)
      ami:                          "ami-e6202f99" => "ami-e6202f99"
      tags.Name:                    "HelloWorld" => "HelloWorld"
      ...
      user_data:                    "" => "da39a3ee5e6b4b0d3255bfef95601890afd80709" (forces new resource)

Running plan with a build from this branch produced the correct expected output.

[vmartin01]

I've updated #4954 where i'm running into inconsistencies across MacOS and RHEL 7.5

[zehua]

ping @bflad

[bflad]

Thanks for the extra information, @zehua -- I was able to implement acceptance testing to cover this:

func TestAccAWSInstance_UserData_EmptyStringToUnspecified(t *testing.T) {
	var instance ec2.Instance
	rInt := acctest.RandInt()
	resourceName := "aws_instance.test"

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckInstanceDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccInstanceConfig_UserData_EmptyString(rInt),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInstanceExists(resourceName, &instance),
				),
			},
			// Switching should show no difference
			{
				Config:             testAccInstanceConfig_UserData_Unspecified(rInt),
				ExpectNonEmptyPlan: false,
				PlanOnly:           true,
			},
		},
	})
}

func TestAccAWSInstance_UserData_UnspecifiedToEmptyString(t *testing.T) {
	var instance ec2.Instance
	rInt := acctest.RandInt()
	resourceName := "aws_instance.test"

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckInstanceDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccInstanceConfig_UserData_Unspecified(rInt),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInstanceExists(resourceName, &instance),
				),
			},
			// Switching should show no difference
			{
				Config:             testAccInstanceConfig_UserData_EmptyString(rInt),
				ExpectNonEmptyPlan: false,
				PlanOnly:           true,
			},
		},
	})
}

func testAccInstanceConfig_UserData_Base(rInt int) string {
	return fmt.Sprintf(`
data "aws_ami" "amzn-ami-minimal-hvm-ebs" {
  most_recent = true
  owners      = ["amazon"]

  filter {
    name = "name"
    values = ["amzn-ami-minimal-hvm-*"]
  }
  filter {
    name = "root-device-type"
    values = ["ebs"]
  }
}

resource "aws_vpc" "test" {
  cidr_block = "172.16.0.0/16"

  tags {
    Name = "tf-acctest-%d"
  }
}

resource "aws_subnet" "test" {
  vpc_id     = "${aws_vpc.test.id}"
  cidr_block = "172.16.0.0/24"

  tags {
    Name = "tf-acctest-%d"
  }
}
`, rInt, rInt)
}

func testAccInstanceConfig_UserData_Unspecified(rInt int) string {
	return testAccInstanceConfig_UserData_Base(rInt) + `
resource "aws_instance" "test" {
  ami           = "${data.aws_ami.amzn-ami-minimal-hvm-ebs.id}"
  instance_type = "t2.micro"
  subnet_id     = "${aws_subnet.test.id}"
}
`
}

func testAccInstanceConfig_UserData_EmptyString(rInt int) string {
	return testAccInstanceConfig_UserData_Base(rInt) + `
resource "aws_instance" "test" {
  ami           = "${data.aws_ami.amzn-ami-minimal-hvm-ebs.id}"
  instance_type = "t2.micro"
  subnet_id     = "${aws_subnet.test.id}"
  user_data     = ""
}
`
}

Before 66f2cbc:

    --- FAIL: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (124.83s)
            testing.go:527: Step 1 error: After applying this step, the plan was not empty:

                    DIFF:

                    DESTROY/CREATE: aws_instance.test
    ...
                      user_data:                    "" => "da39a3ee5e6b4b0d3255bfef95601890afd80709" (forces new resource)

After:

    --- PASS: TestAccAWSInstance_UserData_EmptyStringToUnspecified (125.19s)
    --- PASS: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (145.57s)

[bflad]

This has been released in version 1.33.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!