Change instance profile if the instance is stopped #11104
Conversation
ghost
added
needs-triage
ryndaniels
removed
the
needs-triage
There was a problem hiding this comment.
I think we should optimize this a little by removing the extra API calls and prefer using SDK constants, but looks good overall, so I'm going to optimistically approve assuming the updated test passes on any additional changes to the IAM profile logic. 😄 🚀
Output from acceptance testing:
--- PASS: TestAccAWSInstance_addSecondaryInterface (121.25s)
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (110.40s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (94.60s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (125.64s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (84.26s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (74.33s)
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (246.11s)
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (106.04s)
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (74.08s)
--- PASS: TestAccAWSInstance_basic (258.75s)
--- PASS: TestAccAWSInstance_blockDevices (89.83s)
--- PASS: TestAccAWSInstance_changeInstanceType (365.43s)
--- PASS: TestAccAWSInstance_CreditSpecification_Empty_NonBurstable (299.15s)
--- PASS: TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable (217.43s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits (98.31s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits_t2Tot3Taint (359.00s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2 (177.94s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t3 (295.26s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits (85.03s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits_t2Tot3Taint (521.95s)
--- PASS: TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard (85.24s)
--- PASS: TestAccAWSInstance_CreditSpecification_UnspecifiedToEmpty_NonBurstable (114.62s)
--- PASS: TestAccAWSInstance_creditSpecification_updateCpuCredits (122.95s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_standardCpuCredits (86.40s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unlimitedCpuCredits (113.26s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited (300.85s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_updateCpuCredits (134.87s)
--- PASS: TestAccAWSInstance_disableApiTermination (109.61s)
--- PASS: TestAccAWSInstance_disappears (230.29s)
--- PASS: TestAccAWSInstance_EbsBlockDevice_KmsKeyArn (88.29s)
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (131.89s)
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (169.17s)
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (180.36s)
--- PASS: TestAccAWSInstance_GP2IopsDevice (79.60s)
--- PASS: TestAccAWSInstance_GP2WithIopsValue (134.66s)
--- PASS: TestAccAWSInstance_inDefaultVpcBySgId (83.03s)
--- PASS: TestAccAWSInstance_inDefaultVpcBySgName (112.97s)
--- PASS: TestAccAWSInstance_instanceProfileChange (281.48s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (89.97s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (77.15s)
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (10.52s)
--- PASS: TestAccAWSInstance_keyPairCheck (73.68s)
--- PASS: TestAccAWSInstance_multipleRegions (293.55s)
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (86.56s)
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (75.29s)
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (119.89s)
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (174.36s)
--- PASS: TestAccAWSInstance_placementGroup (64.55s)
--- PASS: TestAccAWSInstance_primaryNetworkInterface (87.87s)
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (77.44s)
--- PASS: TestAccAWSInstance_privateIP (74.14s)
--- PASS: TestAccAWSInstance_RootBlockDevice_KmsKeyArn (181.73s)
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (174.44s)
--- PASS: TestAccAWSInstance_rootInstanceStore (99.73s)
--- PASS: TestAccAWSInstance_sourceDestCheck (121.28s)
--- PASS: TestAccAWSInstance_tags (236.26s)
--- PASS: TestAccAWSInstance_UserData_EmptyStringToUnspecified (288.32s)
--- PASS: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (83.88s)
--- PASS: TestAccAWSInstance_userDataBase64 (232.03s)
--- PASS: TestAccAWSInstance_volumeTags (100.32s)
--- PASS: TestAccAWSInstance_volumeTagsComputed (134.06s)
--- PASS: TestAccAWSInstance_vpc (186.42s)
--- PASS: TestAccAWSInstance_withIamInstanceProfile (215.41s)
--- PASS: TestAccAWSInstanceDataSource_AzUserData (240.93s)
--- PASS: TestAccAWSInstanceDataSource_basic (241.75s)
--- PASS: TestAccAWSInstanceDataSource_blockDevices (87.17s)
--- PASS: TestAccAWSInstanceDataSource_creditSpecification (107.65s)
--- PASS: TestAccAWSInstanceDataSource_EbsBlockDevice_KmsKeyId (86.02s)
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_falseToTrue (152.66s)
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_trueToFalse (213.82s)
--- PASS: TestAccAWSInstanceDataSource_GetUserData (126.47s)
--- PASS: TestAccAWSInstanceDataSource_GetUserData_NoUserData (292.59s)
--- PASS: TestAccAWSInstanceDataSource_gp2IopsDevice (66.24s)
--- PASS: TestAccAWSInstanceDataSource_keyPair (195.61s)
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (77.10s)
--- PASS: TestAccAWSInstanceDataSource_privateIP (100.23s)
--- PASS: TestAccAWSInstanceDataSource_RootBlockDevice_KmsKeyId (122.02s)
--- PASS: TestAccAWSInstanceDataSource_rootInstanceStore (95.99s)
--- PASS: TestAccAWSInstanceDataSource_SecurityGroups (130.19s)
--- PASS: TestAccAWSInstanceDataSource_tags (238.29s)
--- PASS: TestAccAWSInstanceDataSource_VPC (197.20s)
--- PASS: TestAccAWSInstanceDataSource_VPCSecurityGroups (100.23s)
--- PASS: TestAccAWSInstancesDataSource_basic (269.25s)
--- PASS: TestAccAWSInstancesDataSource_instance_state_names (201.45s)
--- PASS: TestAccAWSInstancesDataSource_tags (233.30s)
aws/resource_aws_instance.go
Outdated
|
|
||
| // If the instance is running, we can replace the instance profile association. | ||
| // If it is stopped, the association must be removed and the new one attached separately. (GH-8262) | ||
| resp, err := conn.DescribeInstances(&ec2.DescribeInstancesInput{ |
There was a problem hiding this comment.
Potential optimization: we can save this extra API call by reading d.Get("instance_state").(string) for the instance state. 👍
aws/resource_aws_instance.go
Outdated
| instance := resp.Reservations[0].Instances[0] | ||
|
|
||
| if instance.State != nil { | ||
| if *instance.State.Name == "running" { |
There was a problem hiding this comment.
Prefer AWS Go SDK conversion functions and constants here and below 😄
| if *instance.State.Name == "running" { | |
| if aws.StringValue(instance.State.Name) == ec2.InstanceStateNameRunning { |
Does this also need to handle other instance states such as shutting-down and stopping?
There was a problem hiding this comment.
Good point - it looks like before, those cases would have fallen into the "running" behavior in the code. I suspect that shutting-down and stopping probably ought to fall into the stopped state, so I think I'll group those together and have every other state fall through to the old behavior. That way we'll keep things as consistent as possible, and if people run into issues with other state edge cases, we can address those as they come up. 👍
aws/resource_aws_instance.go
Outdated
| resp2, err := conn.DescribeIamInstanceProfileAssociations(request) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(resp2.IamInstanceProfileAssociations) > 0 { |
There was a problem hiding this comment.
This extra API call and conditional seem extraneous since we already check the number of associations on line 955 and get here by the length not being equal to 0.
aws/resource_aws_instance.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Nit: If we are not going to return context with the error, e.g.
if err != nil {
return fmt.Errorf("error disassociating instance with instance profile: %w", err)
}We should just directly return err 👍
aws/resource_aws_instance.go
Outdated
| _, err = conn.AssociateIamInstanceProfile(input) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("Error associating instance with instance profile: %s", err) |
There was a problem hiding this comment.
Go 1.13 allows us to return the wrapped error along with our context (e.g. an underlying InvalidInstanceID.NotFound error) by using the %w format verb. Going forward, we should likely switch to using that verb when returning error context with helper functions like these, so any consumers of these helper functions that want to check for specific error codes with isAWSErr() will work as expected.
| return fmt.Errorf("Error associating instance with instance profile: %s", err) | |
| return fmt.Errorf("error associating instance with instance profile: %w", err) |
This is a problem we'll want to address more globally going forward, but just noting here for additional visibility.
|
Updated test still passing after changes: 🙂 |
|
This has been released in version 2.44.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
|
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
Community Note
Closes #8262
Release note for CHANGELOG:
Output from acceptance testing: