Add import to aws_iam_role_policy_attachment

[mindfulmonk]

Fixes #544
Based on @jzbruno hashicorp/terraform#12101

Changes proposed in this pull request:

• Add import functionality to role_policy_attachment

Output from acceptance testing:

make testacc TESTARGS='-run=TestAccAwsIamRolePolicyAttachmentImport'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAwsIamRolePolicyAttachmentImport -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsIamRolePolicyAttachmentImport
--- PASS: TestAccAwsIamRolePolicyAttachmentImport (39.39s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	39.450s

The test is not very robust, I had to have an empty "test-role" in my AWS account to get it to pass. Would appreciate some help on that front.

[oioki]

Hello @bflad!
What is blocking from merging this pull request?

[robkinyon-roivant]

Bump. Could this be merged?

[bflad]

Initial review below with suggestions for simplification and some explanation why I needed to mark this as a breaking change. Please reach out with any questions or if you do not have time to implement the feedback. Thanks!

[bflad]

While its unlikely that operators are currently using the ID of this resource due to its format, we do not currently have a precedent for changing the format of resource IDs and changing them is considered a major breaking change: https://www.terraform.io/docs/extend/best-practices/versioning.html#example-major-number-increments

We can support import without changing this. 👍

[bflad]

We strongly prefer to have resources only import resources 1:1 currently -- otherwise this can cause dangerous behavior if the operator is not completely vigilant after import. See also: #5631

[bflad]

I should also note this behavior of multiple imports can be very problematic if the resources are implemented across separate modules. It will force the operator to terraform state rm the other attachments repeatedly for each module (and likely unexpectedly).

[bflad]

Instead of implementing a "complex" import function (importing all attached policies), which can be dangerous, we should instead support importing the resource 1:1.

This can be implemented via (which expects a format ROLENAME_POLICYARN):

		Importer: &schema.ResourceImporter{
			State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
				idParts := strings.Split(d.Id(), "_")
				if len(idParts) != 2 || idParts[0] == "" || idParts[1] == "" {
					return nil, fmt.Errorf("unexpected format of ID (%q), expected ROLENAME_POLICYARN", d.Id())
				}
				roleName := idParts[0]
				policyARN := idParts[1]
				d.Set("policy_arn", policyARN)
				d.Set("role_name", roleName)
				d.SetId(resource.PrefixedUniqueId(fmt.Sprintf("%s-", roleName)))
				return []*schema.ResourceData{d}, nil
			},
		},

And tested via:

// new function
func testAccAWSIAMRolePolicyAttachmentImportStateIdFunc(resourceName string) resource.ImportStateIdFunc {
	return func(s *terraform.State) (string, error) {
		rs, ok := s.RootModule().Resources[resourceName]
		if !ok {
			return "", fmt.Errorf("not found: %s", resourceName)
		}

		return fmt.Sprintf("%s_%s", rs.Primary.Attributes["role_name"], rs.Primary.Attributes["policy_arn"]), nil
	}
}

// As a new TestStep in TestAccAWSRolePolicyAttachment_basic
{
	ResourceName:      "aws_iam_role_policy_attachment.test-attach",
	ImportState:       true,
	ImportStateIdFunc: testAccAWSIAMRolePolicyAttachmentImportStateIdFunc("aws_iam_role_policy_attachment.test-attach"),
	ImportStateVerify: true,
},

[jzbruno]

Thanks for the review. I will take a look at this tonight.

[jzbruno]

@bflad Got this working. Thanks for the review and suggestions. I don't think ImportStateIdFunc existed when I created the PR over a year ago.

I have a couple questions.

• This is no longer my original fork and PR. Should I create a new fork and PR? Suggestions on the best way to handle this?
• Both roles and policies allow an arbitrary number of _ in their names and ARNs can have /. How about // for the delimiter of the import ID?

Names of users, groups, roles, policies, instance profiles, and server certificates must be alphanumeric, including the following common characters: plus (+), equal (=), comma (,), period (.), at (@), underscore (_), and hyphen (-).

Limitations on IAM Entities and Objects - AWS Identity and Access Management

[jzbruno]

@bflad Ok. I was wrong about getting this to work. The issue here is that the create method generates a unique ID for the attachment but that ID does not contain the policy ARN.

When ImportStateVerify is true the created state is compared to the imported state. Using the ImportStateIdFunc won’t help because:

1. … if we lookup the policy ARN from the state when the new and old states are compared they won’t match. Created = <role_name>-<timestamp>, Imported = <role_name>-<policy_arn>
2. … if we lookup the timestamp and return an ID that matches, the import function does not have the correct policy ARN.

Options:

1. Change the attachment create function to generate an ID containing both the role and policy information.
2. Change the import function to behave different for test cases.
3. Set ImportStateVerify to false.

I have chosen 3 as the option with the least impact. See the new PR #5910.

Please let me know if you have further suggestions.

[bflad]

Superseded by #5910, which has been merged and will release with version 1.37.0 of the AWS provider, likely tomorrow. 👍

[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!