fix: clean up IAM roles on env stack failures by efekarakus · Pull Request #2107 · aws/copilot-cli

efekarakus · 2021-03-25T23:54:03Z

We now wait for a failed stack to be deleted first before attempting to re-create it.
Best-effort attempt to delete IAM roles on env delete this removes intermittent role deletion errors in the CLI.
If a user deletes an environment, and then re-creates it, env init now ensures the previous dangling IAM roles are deleted.
Similarly, if the env stack creation fails, we delete any retained IAM roles created part of the stack.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

kohidave · 2021-03-25T23:56:24Z

Heroic.

bvtujo

What David said. This is amazing, and should help with a lot of our frustrating error cases. I combed through and couldn't find anything confusing; so I just have a naming nit.

bvtujo · 2021-03-29T17:30:32Z

+	serviceLinkedRoleCreator
+}
+
+type stackExister interface {


nit: Is there a way we can reframe this? The name is a little confusing, and while it's clear that it's not a stackCreator it's not immediately clear what the interface does from the exister verb.

Suggested change

type stackExister interface {

type stackExistChecker interface

iamhopaul123 · 2021-03-29T17:47:22Z

+// Exists returns true if the environment stack exists, false otherwise.
+// If an error occurs for another reason than ErrStackNotFound, then returns the error.


Maybe

Suggested change

// Exists returns true if the environment stack exists, false otherwise.

// If an error occurs for another reason than ErrStackNotFound, then returns the error.

// Exists returns true if the CloudFormation stack exists, false otherwise.

// If an error occurs for another reason than ErrStackNotFound, then returns the error.

as aws.cloudformation pkg should be agnostic about Copilot terminology?

Yup sorry, I had this method originally in the describe pkg and while moving it here forgot to update the comment :D fixed!

iamhopaul123 · 2021-03-29T19:09:26Z

 	if status.requiresCleanup() {
 		// If the stack exists, but failed to create, we'll clean it up and then re-create it.
-		if err := c.Delete(stack.Name); err != nil {
+		if err := c.DeleteAndWait(stack.Name); err != nil {


iamhopaul123 · 2021-03-29T19:35:34Z

 		}
+		// The stack failed to create due to an unexpect reason.
+		// Delete the retained roles created part of the stack.
+		o.tryDeletingEnvRoles(o.appName, o.name)


If we delete the role before deleting the stack (since it failed to be deleted), will this make users not authorized to manage the env stack unless they manually create the env manager role again (if env upgrade happened before)?

PH's comment makes me wonder whether it is guaranteed that if deployAndRenderEnvironment fails, then the env stack will be rolled back and delete 🤔

Very good point! I think most of the time it will just be deleted but sometimes the rollback could fail (see ROLLBACK_FAILED in here). It is just since we always try deleting the roles for a clean state when we create the environment so maybe it is ok to skip deleting the roles if the creation fails?

I don't think I am fully following, so I'll try my best to clarify but please let me know if I'm not answering properly:

If we delete the role before deleting the stack (since it failed to be deleted), will this make users not authorized to manage the env stack unless they manually create the env manager role again

We don't pass a RoleARN for CloudFormation to assume when we initially create the stack. So on failure, it is safe to delete the roles that were retained since they were not used yet to mutate the cloudformation stack.

whether it is guaranteed that if deployAndRenderEnvironment fails, then the env stack will be rolled back and delete

On failure, deployAndRenderEnvironment will indeed rollback back the stack but won't delete it. Instead, the second time that the customer runs copilot env init then on Create we will check if the stack requires deletion and if so first delete it and then re-create it:

copilot-cli/internal/pkg/aws/cloudformation/cloudformation.go

Lines 48 to 65 in 1284849

func (c *CloudFormation) Create(stack *Stack) (changeSetID string, err error) {

descr, err := c.Describe(stack.Name)

if err != nil {

var stackNotFound *ErrStackNotFound

if !errors.As(err, &stackNotFound) {

return "", err

}

// If the stack does not exist, create it.

return c.create(stack)

}

status := StackStatus(aws.StringValue(descr.StackStatus))

if status.requiresCleanup() {

// If the stack exists, but failed to create, we'll clean it up and then re-create it.

if err := c.Delete(stack.Name); err != nil {

return "", fmt.Errorf("clean up previously failed stack %s: %w", stack.Name, err)

}

return c.create(stack)

}

it is ok to skip deleting the roles if the creation fails

This would be true, if only the stack got deleted if the creation fails. Instead it goes into a ROLLBACK_COMPLETE state I believe. So if the user just re-runs copilot env init and we don't clean up the IAM roles on failed create, then we will:

first check if the stack exists ✅, so skip

The stack tries to re-create the IAM but fails because the CFNExecRole already exists ❌

Does this help?

Ah yes. Sorry I was stupid I wasn't aware it is creating an environment so env upgrade can't happen before.

Im totally ok with the current implementation but

The stack tries to re-create the IAM but fails because the CFNExecRole already exists ❌

env init will try to delete the existing IAM roles before deploying the stack right?

Yesss thanks for the explanation! It makes sense. I have no problem with the current implementation either but same question as Penghao's just to make sure my understanding aligns with the implementation.

env init will try to delete the existing IAM roles before deploying the stack right?

Yes that's right! We delete the roles iff:

There is no environment cloudformation stack

But there are IAM roles

This scenario means that env delete previously failed to delete the EnvManagerRole and it should be cleaned up before trying to re-create it. I updated the comment in the cleanUpDanglingRoles method, hopefully that explains a bit better this scenario.

Lou1415926

Awesome! Nothing much to add except for a tiny nit on comment and a quesiton.

Lou1415926 · 2021-03-29T22:01:22Z

 		}
+		// The stack failed to create due to an unexpect reason.
+		// Delete the retained roles created part of the stack.
+		o.tryDeletingEnvRoles(o.appName, o.name)


PH's comment makes me wonder whether it is guaranteed that if deployAndRenderEnvironment fails, then the env stack will be rolled back and delete 🤔

Fixes aws#2100 1. We now wait for a failed stack to be deleted first before attempting to re-create it. 2. Best-effort attempt to delete IAM roles on `env delete` this removes intermittent role deletion errors in the CLI. 3. If a user deletes an environment, and then re-creates it, `env init` now ensures the previous dangling IAM roles are deleted. 4. Similarly, if the env stack creation fails, we delete any retained IAM roles created part of the stack. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

efekarakus added 8 commits March 24, 2021 16:10

fix: wait until deletion succeeds before re-creating a failed stack

a0da035

chore: best-effort delete env IAM roles on "env delete"

a2d8cc2

refactor: move Metadata method from describe to cfn pkg

b2b7f21

refactor: remove sdk cfn pkg infavor of internal in describe pkg

bb4f28c

chore: add Exists method to cfn pkg to detect if stack is found

f50f4d8

chore(iam): add ListRoleTags method

217ba89

chore: accept either a role ARN or name as input to DeleteRole

4a96386

fix: delete any dangling IAM roles before re-creating env stack

319794c

efekarakus requested review from Lou1415926, bvtujo, huanjani and iamhopaul123 March 25, 2021 23:54

efekarakus requested a review from a team as a code owner March 25, 2021 23:54

bvtujo approved these changes Mar 29, 2021

View reviewed changes

iamhopaul123 reviewed Mar 29, 2021

View reviewed changes

Lou1415926 reviewed Mar 29, 2021

View reviewed changes

efekarakus added 3 commits March 30, 2021 09:24

refactor: rename stackExister interface to stackExistChecker

de6bbdd

refactor: update comment of StackResources method

fa2497b

refactor: add additional comments to cleanUpDanglingRoles

f5a4072

Lou1415926 approved these changes Mar 31, 2021

View reviewed changes

Merge branch 'mainline' into issues/fix-2100

f123588

mergify Bot merged commit f75d750 into aws:mainline Mar 31, 2021

	type stackExister interface {
	type stackExistChecker interface

		// Exists returns true if the environment stack exists, false otherwise.
		// If an error occurs for another reason than ErrStackNotFound, then returns the error.

	func (c CloudFormation) Create(stack Stack) (changeSetID string, err error) {
	descr, err := c.Describe(stack.Name)
	if err != nil {
	var stackNotFound *ErrStackNotFound
	if !errors.As(err, &stackNotFound) {
	return "", err
	}
	// If the stack does not exist, create it.
	return c.create(stack)
	}
	status := StackStatus(aws.StringValue(descr.StackStatus))
	if status.requiresCleanup() {
	// If the stack exists, but failed to create, we'll clean it up and then re-create it.
	if err := c.Delete(stack.Name); err != nil {
	return "", fmt.Errorf("clean up previously failed stack %s: %w", stack.Name, err)
	}
	return c.create(stack)
	}

Conversation

efekarakus commented Mar 25, 2021

Uh oh!

kohidave commented Mar 25, 2021

Uh oh!

bvtujo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iamhopaul123 Mar 29, 2021 •

edited

Loading