Resource tagging in the registry-creds up command #718

PettitWesley · 2019-01-29T01:16:08Z

Description of changes:
Tag SM secrets and new IAM roles. (IAM is not supported by the standalone resource tagging API, so tagging can only be supported during role creation).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

PettitWesley · 2019-01-29T21:39:02Z

Testing:

$ ecs-cli registry-creds up reg-creds.yml --role-name cat-role2 --tags impala=tame,parker=kevin
INFO[0000] Processing credentials for registry my-registry.example.com...
INFO[0000] Existing credential secret found, using arn:aws:secretsmanager:us-west-2:144718711470:secret:amazon-ecs-cli-setup-my-registry.example.com-VUHL4v
INFO[0000] Creating resources for task execution role cat-role2...
INFO[0000] Created new task execution role arn:aws:iam::144718711470:role/cat-role2
INFO[0000] Created new task execution role policy arn:aws:iam::144718711470:policy/amazon-ecs-cli-setup-cat-role2-policy-20190129T005157Z
INFO[0000] Attached AWS managed policy arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy to role cat-role2
INFO[0000] Attached new policy arn:aws:iam::144718711470:policy/amazon-ecs-cli-setup-cat-role2-policy-20190129T005157Z to role cat-role2
INFO[0001] Writing registry credential output to new file /Users/wppttt/stuffnthings/bugbash/compose3/ecs-registry-creds_20190129T005157Z.yml
INFO[0001]
If your input file contains sensitive information, make sure that you delete it after use.

$ aws resourcegroupstaggingapi get-resources --tag-filters Key=impala,Values=tame --region us-west-2
{
    "ResourceTagMappingList": [
        {
            "ResourceARN": "arn:aws:secretsmanager:us-west-2:144718711470:secret:amazon-ecs-cli-setup-my-registry.example.com-VUHL4v",
            "Tags": [
                {
                    "Key": "impala",
                    "Value": "tame"
                },
                {
                    "Key": "parker",
                    "Value": "kevin"
                }
            ]
        }
    ]
}

aws iam list-role-tags --role-name cat-role2
{
    "Tags": [
        {
            "Key": "impala",
            "Value": "tame"
        },
        {
            "Key": "parker",
            "Value": "kevin"
        }
    ],
    "IsTruncated": false
}

allisaurus

Can you add test output that shows existing secrets getting new tags after running the cmd with --update-existing-secrets ?

ecs-cli/modules/cli/regcreds/regcreds_app.go

allisaurus · 2019-02-04T21:24:28Z

ecs-cli/modules/cli/regcreds/create_task_execution_role.go

+	for key, value := range tags {
+		iamTags = append(iamTags, &iam.Tag{
+			Key:   aws.String(key),
+			Value: value,


Is it valid to have a nil value for a tag? If so, can we add a case for it to TestCreateTaskExecutionRoleWithTags ?

Docs say its a required field: https://docs.aws.amazon.com/sdk-for-go/api/service/iam/#Tag

You can pass an empty string though, which my code handles correctly:

$ ecs-cli registry-creds up --role-name catsss --tags cat=,meow= reg-creds.yml INFO[0000] Processing credentials for registry my-registry.example.com... INFO[0000] Existing credential secret found, using arn:aws:secretsmanager:us-west-1:REDACTED:secret:amazon-ecs-cli-setup-my-registry.example.com-TIxAwp INFO[0000] Creating resources for task execution role catsss... INFO[0000] Created new task execution role arn:aws:iam::REDACTED:role/catsss INFO[0000] Created new task execution role policy arn:aws:iam::REDACTED:policy/amazon-ecs-cli-setup-catsss-policy-20190213T184615Z INFO[0001] Attached AWS managed policy arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy to role catsss INFO[0001] Attached new policy arn:aws:iam::REDACTED:policy/amazon-ecs-cli-setup-catsss-policy-20190213T184615Z to role catsss INFO[0001] Writing registry credential output to new file stuffnthings/bugbash/compose3/ecs-registry-creds_20190213T184615Z.yml INFO[0001] If your input file contains sensitive information, make sure that you delete it after use.

(I checked that the tags had been set in the Console)

Do we want to log info about what tags are being applied to what resources?

PettitWesley · 2019-02-05T02:59:34Z

@allisaurus

$ ecs-cli registry-creds up reg-creds.yml --role-name cat-role2 --update-existing-secrets --tags InnerSpeaker=favoritealbum --region us-west-2
INFO[0000] Processing credentials for registry my-registry.example.com...
INFO[0000] Existing credential secret found, using arn:aws:secretsmanager:us-west-2:144718711470:secret:amazon-ecs-cli-setup-my-registry.example.com-VUHL4v
INFO[0000] Creating resources for task execution role cat-role2...
INFO[0000] Using existing role cat-role2
INFO[0000] Created new task execution role policy arn:aws:iam::144718711470:policy/amazon-ecs-cli-setup-cat-role2-policy-20190205T025752Z
INFO[0000] Attached AWS managed policy arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy to role cat-role2
INFO[0000] Attached new policy arn:aws:iam::144718711470:policy/amazon-ecs-cli-setup-cat-role2-policy-20190205T025752Z to role cat-role2
INFO[0001] Writing registry credential output to new file /Users/wppttt/stuffnthings/bugbash/compose3/ecs-registry-creds_20190205T025752Z.yml
INFO[0001]
If your input file contains sensitive information, make sure that you delete it after use.

New and old tags are present on the secret:

$ aws resourcegroupstaggingapi get-resources --tag-filters Key=InnerSpeaker,Values=favoritealbum --region us-west-2
{
    "ResourceTagMappingList": [
        {
            "ResourceARN": "arn:aws:secretsmanager:us-west-2:144718711470:secret:amazon-ecs-cli-setup-my-registry.example.com-VUHL4v",
            "Tags": [
                {
                    "Key": "InnerSpeaker",
                    "Value": "favoritealbum"
                },
                {
                    "Key": "impala",
                    "Value": "tame"
                },
                {
                    "Key": "parker",
                    "Value": "kevin"
                }
            ]
        }
    ]
}

allisaurus · 2019-02-25T20:01:27Z

ecs-cli/modules/cli/regcreds/regcreds_app.go

 	"github.com/aws/amazon-ecs-cli/ecs-cli/modules/utils/regcredio"
 	"github.com/aws/aws-sdk-go/aws"
+	taggngSDK "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"


nit: should be taggingSDK

allisaurus · 2019-02-25T20:08:42Z

ecs-cli/modules/cli/regcreds/regcreds_app.go

+	}
+
+	for resource, info := range output.FailedResourcesMap {
+		return fmt.Errorf("Failed to tag resource %s; error=%s", resource, *info.ErrorMessage)


can we add test for this error case (FailedResourceMap != nil) and the API error return (line 342)?

allisaurus · 2019-02-25T20:15:49Z

ecs-cli/modules/commands/regcreds/regcreds_command.go

@@ -65,5 +65,9 @@ func regcredsUpFlags() []cli.Flag {
 			Name:  flags.OutputDirFlag,
 			Usage: "[Optional] The directory where the output file should be created. If none specified, file will be created in the current working directory.",
 		},
+		cli.StringFlag{
+			Name:  flags.ResourceTagsFlag,
+			Usage: "[Optional] Add AWS Resource tags to the Secrets Manager secrets and IAM Role. Note that tags can only be added to the IAM role at time of creation.",


Suggested edit for conciseness + consistency w/ other flags:
"The AWS Resource tags to add to the Secrets Manager secrets and new IAM Role. Existing IAM Roles cannot be tagged."

allisaurus · 2019-02-25T20:18:55Z

ecs-cli/modules/clients/aws/iam/client.go

 	createRoleRequest := iam.CreateRoleInput{
 		AssumeRolePolicyDocument: aws.String(assumeRolePolicyDoc),
 		Description:              aws.String(roleDescription),
 		RoleName:                 aws.String(roleName),
 	}
+	if len(tags) > 0 {
+		createRoleRequest.Tags = tags
+	}
 	roleResult, err := c.CreateRole(createRoleRequest)
 	// if err is b/c role already exists, OK to continue
 	if err != nil && !utils.EntityAlreadyExists(err) {


What happens if we try to apply tags to an existing role? Does it return a distinct error or succeed? If the former, wondering if we should be checking for a specific error here & allowing it to succeed (like with EntityAlreadyExists) or let it fail.

Adding here what we discussed: trying to re-create an existing role but with tags will still fail with an EntityAlreadyExists error.

I'm happy to go along with what @allisaurus agrees with but curious why this is the correct behavior?

As per offline discussion, I understand now that this is strictly on creation of a new role, so this seems fine.

allisaurus · 2019-02-27T16:33:09Z

LGTM! Though do consider the suggested flag text change.

SoManyHs

A few questions just for my own clarification but otherwise LGTM

SoManyHs · 2019-02-27T18:26:47Z

ecs-cli/modules/clients/aws/iam/client.go

 	createRoleRequest := iam.CreateRoleInput{
 		AssumeRolePolicyDocument: aws.String(assumeRolePolicyDoc),
 		Description:              aws.String(roleDescription),
 		RoleName:                 aws.String(roleName),
 	}
+	if len(tags) > 0 {
+		createRoleRequest.Tags = tags
+	}
 	roleResult, err := c.CreateRole(createRoleRequest)
 	// if err is b/c role already exists, OK to continue
 	if err != nil && !utils.EntityAlreadyExists(err) {


I'm happy to go along with what @allisaurus agrees with but curious why this is the correct behavior?

SoManyHs · 2019-02-27T18:27:59Z

ecs-cli/modules/cli/regcreds/create_task_execution_role.go

+	for key, value := range tags {
+		iamTags = append(iamTags, &iam.Tag{
+			Key:   aws.String(key),
+			Value: value,


Do we want to log info about what tags are being applied to what resources?

PettitWesley requested review from allisaurus and SoManyHs January 29, 2019 01:16

PettitWesley changed the base branch from dev to tagging January 29, 2019 23:32

PettitWesley force-pushed the tag-regcreds branch from 2bc76eb to cc92faf Compare January 30, 2019 19:22

allisaurus requested changes Feb 4, 2019

View reviewed changes

PettitWesley mentioned this pull request Feb 13, 2019

Tagging resources #670

Closed

PettitWesley force-pushed the tag-regcreds branch from 2bd004c to 65df7f1 Compare February 13, 2019 18:43

PettitWesley force-pushed the tag-regcreds branch from 65df7f1 to ce01726 Compare February 22, 2019 23:42

allisaurus requested changes Feb 25, 2019

View reviewed changes

allisaurus approved these changes Feb 27, 2019

View reviewed changes

SoManyHs approved these changes Feb 27, 2019

View reviewed changes

PettitWesley added 2 commits February 28, 2019 15:50

Implemented resource tagging in the 'registy-creds up' command

1f9b745

Unit tests for tagging in 'registry-creds up'

6aec326

PettitWesley force-pushed the tag-regcreds branch from 4287af8 to 6aec326 Compare February 28, 2019 23:54

PettitWesley merged commit 0f138cc into aws:tagging Mar 1, 2019

PettitWesley deleted the tag-regcreds branch March 1, 2019 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource tagging in the registry-creds up command #718

Resource tagging in the registry-creds up command #718

PettitWesley commented Jan 29, 2019

PettitWesley commented Jan 29, 2019

allisaurus left a comment

allisaurus Feb 4, 2019

PettitWesley Feb 13, 2019

SoManyHs Feb 27, 2019

PettitWesley commented Feb 5, 2019

allisaurus Feb 25, 2019

allisaurus Feb 25, 2019

allisaurus Feb 25, 2019

allisaurus Feb 25, 2019

PettitWesley Feb 27, 2019

SoManyHs Feb 27, 2019

SoManyHs Feb 27, 2019

allisaurus commented Feb 27, 2019

SoManyHs left a comment

SoManyHs Feb 27, 2019

SoManyHs Feb 27, 2019

Resource tagging in the registry-creds up command #718

Resource tagging in the registry-creds up command #718

Conversation

PettitWesley commented Jan 29, 2019

PettitWesley commented Jan 29, 2019

allisaurus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PettitWesley commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisaurus commented Feb 27, 2019

SoManyHs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment