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.

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

r/iam_role: Delete inline policies when force_detach_policies = true #2388

Merged
merged 4 commits into from
Jan 3, 2018

Conversation

atsushi-ishibashi
Copy link
Contributor

@atsushi-ishibashi atsushi-ishibashi commented Nov 21, 2017

Fix: #2279

@atsushi-ishibashi atsushi-ishibashi changed the title [WIP]r/iam_role: Delete inline policies when force_detach_policies = true r/iam_role: Delete inline policies when force_detach_policies = true Nov 21, 2017
@atsushi-ishibashi
Copy link
Contributor Author

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRole_force_detach_policies'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRole_force_detach_policies -timeout 120m
=== RUN   TestAccAWSIAMRole_force_detach_policies
--- PASS: TestAccAWSIAMRole_force_detach_policies (40.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.050s

@atsushi-ishibashi
Copy link
Contributor Author

the current force_detach_policies use ListAttachedRolePolicies to find policies which will be deatached.
But ListRolePolicies says

Lists the names of the inline policies that are embedded in the specified IAM role.
An IAM role can also have managed policies attached to it. To list the managed policies that are attached to a role, use ListAttachedRolePolicies.

So when inlined policies are attached to role, you got error DeleteConflict: Cannot delete entity, must detach all policies first. even though force_detach_policies=true

In this PR, delete inline policies when force_detach_policies=true. I wonder if force_detach_policies is appropriate because it will delete inline policies...

@aleybovich
Copy link

@atsushi-ishibashi force_detach_policies is definitely appropriate - the idea of that flag is to be able to destroy a role with policies attached, no matter whether they are inline or separate.

@Ninir Ninir added the bug Addresses a defect in current functionality. label Nov 21, 2017

func testAccAWSIAMRoleConfig_force_detach_policies(rName string) string {
return fmt.Sprintf(`
resource "aws_iam_role_policy" "test" {
Copy link
Member

Choose a reason for hiding this comment

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

Technically we're not really testing the new functionality here IMO, because the policy will get destroyed first due to its relationship and force_destroy won't get chance to ever take action.
I think we'd need to remove those policies from state as part of the test in order to verify it actually works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right👍
But I couldn't have an idea to realize it... Could you give me advice?

Copy link
Member

@radeksimko radeksimko Dec 5, 2017

Choose a reason for hiding this comment

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

So I think the easier way is to add policies outside of the config, instead of removing them from state. We could add testAccAddAWSIAMRolePolicy("aws_iam_role.test") to your new test:

		Steps: []resource.TestStep{
			{
				Config: testAccAWSIAMRoleConfig_force_detach_policies(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSRoleExists("aws_iam_role.test", &conf),
					testAccAddAWSIAMRolePolicy("aws_iam_role.test"),
				),
			},
		},

which would just call the API to add a policy outside of any Terraform CRUD.

Testing framework will then attempt to destroy the role and exercise this new functionality.

@@ -285,6 +285,25 @@ func resourceAwsIamRoleDelete(d *schema.ResourceData, meta interface{}) error {
}
}
}
// For inline policies
inlinePoliciesResp, err := iamconn.ListRolePolicies(&iam.ListRolePoliciesInput{
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we going to miss some policies here potentially due to pagination? How do you feel about using ListRolePoliciesPages instead?

@radeksimko radeksimko added waiting-response Maintainers are waiting on response from community or contributor. bug Addresses a defect in current functionality. and removed bug Addresses a defect in current functionality. labels Dec 2, 2017
@atsushi-ishibashi
Copy link
Contributor Author

atsushi-ishibashi commented Dec 28, 2017

@radeksimko Ok👍

TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRole_force_detach_policies -timeout 120m
=== RUN   TestAccAWSIAMRole_force_detach_policies
--- PASS: TestAccAWSIAMRole_force_detach_policies (56.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	56.439s

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 3, 2018
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@radeksimko radeksimko merged commit 0dedb8d into hashicorp:master Jan 3, 2018
@bflad bflad added this to the v1.7.0 milestone Jan 12, 2018
@bflad
Copy link
Contributor

bflad commented Jan 12, 2018

This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 8, 2020

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 ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_iam_role.force_detach_policies no longer works
5 participants