credit_specification issues in aws_instance (T3) #5805
Conversation
bflad
added
bug
There was a problem hiding this comment.
You were very close, @saravanan30erd! After fixing up the note below (with slight behavior change) and adding some additional testing in 607c7bb. 🚀
58 tests passed (all tests)
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (11.26s)
--- PASS: TestAccAWSInstance_importInEc2Classic (86.47s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (89.50s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (90.54s)
--- PASS: TestAccAWSInstance_vpc (93.76s)
--- PASS: TestAccAWSInstance_rootInstanceStore (100.24s)
--- PASS: TestAccAWSInstance_userDataBase64 (105.30s)
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (107.70s)
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgName (110.44s)
--- PASS: TestAccAWSInstance_basic (120.12s)
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (164.42s)
--- PASS: TestAccAWSInstance_GP2IopsDevice (164.32s)
--- PASS: TestAccAWSInstance_GP2WithIopsValue (167.87s)
--- PASS: TestAccAWSInstance_placementGroup (168.34s)
--- PASS: TestAccAWSInstance_volumeTags (77.83s)
--- PASS: TestAccAWSInstance_blockDevices (178.25s)
--- PASS: TestAccAWSInstance_privateIP (79.10s)
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (78.60s)
--- PASS: TestAccAWSInstance_volumeTagsComputed (98.39s)
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgId (199.93s)
--- PASS: TestAccAWSInstance_tags (119.11s)
--- PASS: TestAccAWSInstance_keyPairCheck (89.47s)
--- PASS: TestAccAWSInstance_importBasic (213.66s)
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (205.98s)
--- PASS: TestAccAWSInstance_disableApiTermination (231.99s)
--- PASS: TestAccAWSInstance_multipleRegions (241.48s)
--- PASS: TestAccAWSInstance_primaryNetworkInterface (81.90s)
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (82.11s)
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (93.72s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (88.82s)
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (191.69s)
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (72.79s)
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (69.74s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (92.75s)
--- PASS: TestAccAWSInstance_addSecondaryInterface (139.23s)
--- PASS: TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard (81.16s)
--- PASS: TestAccAWSInstance_sourceDestCheck (328.82s)
--- PASS: TestAccAWSInstance_creditSpecification_updateCpuCredits (81.14s)
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (182.79s)
--- PASS: TestAccAWSInstance_instanceProfileChange (253.48s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unlimitedCpuCredits (82.28s)
--- PASS: TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable (92.09s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_updateCpuCredits (87.06s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (198.83s)
--- PASS: TestAccAWSInstance_withIamInstanceProfile (302.16s)
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (197.64s)
--- PASS: TestAccAWSInstance_UserData_EmptyStringToUnspecified (87.76s)
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (184.79s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (211.62s)
--- PASS: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (87.70s)
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (251.47s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits (196.43s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits (198.87s)
--- PASS: TestAccAWSInstance_changeInstanceType (326.83s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited (272.01s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_standardCpuCredits (291.39s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits_t2Tot3Taint (346.78s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits_t2Tot3Taint (378.31s)
| @@ -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" { | |||
There was a problem hiding this comment.
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.
| @@ -1602,6 +1602,99 @@ func TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable(t *testin | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited(t *testing.T) { | |||
There was a problem hiding this comment.
Thank you so much for adding all the tests! 💯
|
This has been released in version 1.37.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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! |
ghost
locked and limited conversation to collaborators
Fixes #5651
Fixes #5654
Fixes #5769
Fixes #5719
Fixes #5853
Changes proposed in this pull request:
It doesn't respect the credit_specification attribute for T3 instanceslooks like
credit_specificationwill be applied only for t2 instances(during creation) as other type will not support this, included t3 with t2 now.Even if I didn't change anything or I changed an unrelated resource, it would still update the credit specification on every applysince we set the default value as
standard, it will make diffunlimited => standardon every apply,so added the diffsupress func.Note: I tried to remove default but facing diff
standard => " "for all other instance types as AWS setting standard by default, so don't want to affect other instance types.Output from acceptance testing: