Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

credit_specification issues in aws_instance (T3) #5805

Merged
merged 3 commits into from
Sep 18, 2018
Merged

credit_specification issues in aws_instance (T3) #5805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ebcec59
T3 doesn't respect the credit_specification attr(fix)
saravanan30erd Sep 6, 2018
0e10d3a
set default value as unlimited for t3 instances
saravanan30erd Sep 6, 2018
d66ceee
acceptance tests to cover issues
saravanan30erd Sep 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions aws/resource_aws_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,13 @@ func resourceAwsInstance() *schema.Resource {
Type: schema.TypeString,
Optional: true,
Default: "standard",
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if strings.HasPrefix(d.Get("instance_type").(string), "t3") &&
old == "unlimited" && new == "standard" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing the unlimited to standard difference has the unfortunate side effect of preventing a successful update from unlimited to standard for T3 instance types, which is a valid change. I was able to tease this out by adding a new TestStep to TestAccAWSInstance_creditSpecificationT3_updateCpuCredits:

func TestAccAWSInstance_creditSpecificationT3_updateCpuCredits(t *testing.T) {
	var first, second, third ec2.Instance
	resName := "aws_instance.foo"
	rInt := acctest.RandInt()

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckInstanceDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccInstanceConfig_creditSpecification_standardCpuCredits_t3(rInt),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInstanceExists(resName, &first),
					resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
					resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "standard"),
				),
			},
			{
				Config: testAccInstanceConfig_creditSpecification_unlimitedCpuCredits_t3(rInt),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInstanceExists(resName, &second),
					resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
					resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "unlimited"),
				),
			},
			{
				Config: testAccInstanceConfig_creditSpecification_standardCpuCredits_t3(rInt),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInstanceExists(resName, &third),
					resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
					resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "standard"),
				),
			},
		},
	})
}

And its failure during run:

--- FAIL: TestAccAWSInstance_creditSpecificationT3_updateCpuCredits (143.75s)
    testing.go:527: Step 2 error: Check failed: Check 3/3 error: aws_instance.foo: Attribute 'credit_specification.0.cpu_credits' expected "standard", got "unlimited"

To encourage the correct behavior for this attribute, we needed to remove the Default: "standard" (since there is no singular default value for the attribute now) and have the DiffSuppressFunc only concern itself with scenarios where new is empty (e.g. it is not in the Terraform configuration):

"cpu_credits": {
	Type:     schema.TypeString,
	Optional: true,
	DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
		// Only work with existing instances
		if d.Id() == "" {
			return false
		}
		// Only work with missing configurations
		if new != "" {
			return false
		}
		// Only work when already set in Terraform state
		if old == "" {
			return false
		}
		return true
	},
},

This does however cause a change in behavior of the resource with regards to removing the credit_specification configuration for existing instances. Previously, we were "resetting" the credits back to standard for T2 instance types when the configuration was removed. The new behavior is leaving the credits as-is when removing the configuration since the Update function no longer sees the difference. I added a note in the documentation, but given how its more important that we support T3 overall versus that odd scenario (debatable whether we should actually support it), I'm opting to change this until we can potentially revisit in the future.

return true
}
return false
},
},
},
},
Expand Down Expand Up @@ -1741,15 +1748,22 @@ func buildAwsInstanceOpts(
InstanceType: aws.String(instanceType),
}

// Set default cpu_credits as Unlimited for T3 instance type
if strings.HasPrefix(instanceType, "t3") {
opts.CreditSpecification = &ec2.CreditSpecificationRequest{
CpuCredits: aws.String("unlimited"),
}
}

if v, ok := d.GetOk("credit_specification"); ok {
// Only T2 instances support T2 Unlimited
if strings.HasPrefix(instanceType, "t2") {
// Only T2 and T3 are burstable performance instance types and supports Unlimited
if strings.HasPrefix(instanceType, "t2") || strings.HasPrefix(instanceType, "t3") {
cs := v.([]interface{})[0].(map[string]interface{})
opts.CreditSpecification = &ec2.CreditSpecificationRequest{
CpuCredits: aws.String(cs["cpu_credits"].(string)),
}
} else {
log.Print("[WARN] credit_specification is defined but instance type is not T2. Ignoring...")
log.Print("[WARN] credit_specification is defined but instance type is not T2/T3. Ignoring...")
}
}

Expand Down
168 changes: 168 additions & 0 deletions aws/resource_aws_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1602,6 +1602,99 @@ func TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable(t *testin
})
}

func TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for adding all the tests! 💯

var instance ec2.Instance
resName := "aws_instance.foo"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccInstanceConfig_creditSpecification_unspecified_t3(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resName, &instance),
resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "unlimited"),
),
},
},
})
}

func TestAccAWSInstance_creditSpecificationT3_standardCpuCredits(t *testing.T) {
var instance ec2.Instance
resName := "aws_instance.foo"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccInstanceConfig_creditSpecification_standardCpuCredits_t3(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resName, &instance),
resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "standard"),
),
},
},
})
}

func TestAccAWSInstance_creditSpecificationT3_unlimitedCpuCredits(t *testing.T) {
var instance ec2.Instance
resName := "aws_instance.foo"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccInstanceConfig_creditSpecification_unlimitedCpuCredits_t3(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resName, &instance),
resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "unlimited"),
),
},
},
})
}

func TestAccAWSInstance_creditSpecificationT3_updateCpuCredits(t *testing.T) {
var before ec2.Instance
var after ec2.Instance
resName := "aws_instance.foo"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccInstanceConfig_creditSpecification_standardCpuCredits_t3(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resName, &before),
resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "standard"),
),
},
{
Config: testAccInstanceConfig_creditSpecification_unlimitedCpuCredits_t3(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resName, &after),
resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
resource.TestCheckResourceAttr(resName, "credit_specification.0.cpu_credits", "unlimited"),
),
},
},
})
}

func TestAccAWSInstance_UserData_EmptyStringToUnspecified(t *testing.T) {
var instance ec2.Instance
rInt := acctest.RandInt()
Expand Down Expand Up @@ -3283,6 +3376,29 @@ resource "aws_instance" "foo" {
`, rInt)
}

func testAccInstanceConfig_creditSpecification_unspecified_t3(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
cidr_block = "172.16.0.0/16"
tags {
Name = "tf-acctest-%d"
}
}

resource "aws_subnet" "my_subnet" {
vpc_id = "${aws_vpc.my_vpc.id}"
cidr_block = "172.16.20.0/24"
availability_zone = "us-west-2a"
}

resource "aws_instance" "foo" {
ami = "ami-51537029" # us-west-2
instance_type = "t3.micro"
subnet_id = "${aws_subnet.my_subnet.id}"
}
`, rInt)
}

func testAccInstanceConfig_creditSpecification_standardCpuCredits(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
Expand All @@ -3309,6 +3425,32 @@ resource "aws_instance" "foo" {
`, rInt)
}

func testAccInstanceConfig_creditSpecification_standardCpuCredits_t3(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
cidr_block = "172.16.0.0/16"
tags {
Name = "tf-acctest-%d"
}
}

resource "aws_subnet" "my_subnet" {
vpc_id = "${aws_vpc.my_vpc.id}"
cidr_block = "172.16.20.0/24"
availability_zone = "us-west-2a"
}

resource "aws_instance" "foo" {
ami = "ami-51537029" # us-west-2
instance_type = "t3.micro"
subnet_id = "${aws_subnet.my_subnet.id}"
credit_specification {
cpu_credits = "standard"
}
}
`, rInt)
}

func testAccInstanceConfig_creditSpecification_unlimitedCpuCredits(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
Expand All @@ -3335,6 +3477,32 @@ resource "aws_instance" "foo" {
`, rInt)
}

func testAccInstanceConfig_creditSpecification_unlimitedCpuCredits_t3(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
cidr_block = "172.16.0.0/16"
tags {
Name = "tf-acctest-%d"
}
}

resource "aws_subnet" "my_subnet" {
vpc_id = "${aws_vpc.my_vpc.id}"
cidr_block = "172.16.20.0/24"
availability_zone = "us-west-2a"
}

resource "aws_instance" "foo" {
ami = "ami-51537029" # us-west-2
instance_type = "t3.micro"
subnet_id = "${aws_subnet.my_subnet.id}"
credit_specification {
cpu_credits = "unlimited"
}
}
`, rInt)
}

func testAccInstanceConfig_creditSpecification_isNotAppliedToNonBurstable(rInt int) string {
return fmt.Sprintf(`
resource "aws_vpc" "my_vpc" {
Expand Down