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.

Sign up for GitHub

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

Resource tagging in the registry-creds up command #718

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Resource tagging in the registry-creds up command #718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f9b745
Implemented resource tagging in the 'registy-creds up' command
PettitWesley Feb 28, 2019
6aec326
Unit tests for tagging in 'registry-creds up'
PettitWesley Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions ecs-cli/modules/cli/regcreds/create_task_execution_role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ type executionRoleParams struct {
CredEntries map[string]regcredio.CredsOutputEntry
RoleName string
Region string
Tags map[string]*string
}

// returns the time of IAM policy creation so that other resources (i.e., output file) can be dated to match
func createTaskExecutionRole(params executionRoleParams, iamClient iamClient.Client, kmsClient kmsClient.Client) (*time.Time, error) {
log.Infof("Creating resources for task execution role %s...", params.RoleName)

// create role
roleName, err := createOrFindRole(params.RoleName, iamClient)
roleName, err := createOrFindRole(params.RoleName, iamClient, convertToIAMTags(params.Tags))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -88,8 +89,8 @@ func createRegistryCredentialsPolicy(roleName, policyDoc string, createTime time
return policyResult.Policy, nil
}

func createOrFindRole(roleName string, client iamClient.Client) (string, error) {
roleResult, err := client.CreateOrFindRole(roleName, roleDescriptionString, assumeRolePolicyDocString)
func createOrFindRole(roleName string, client iamClient.Client, tags []*iam.Tag) (string, error) {
roleResult, err := client.CreateOrFindRole(roleName, roleDescriptionString, assumeRolePolicyDocString, tags)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -119,3 +120,15 @@ func attachRolePolicies(secretPolicyARN, roleName, region string, client iamClie

return nil
}

func convertToIAMTags(tags map[string]*string) []*iam.Tag {
var iamTags []*iam.Tag
for key, value := range tags {
iamTags = append(iamTags, &iam.Tag{
Key: aws.String(key),
Value: value,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

})
}

return iamTags
}
76 changes: 71 additions & 5 deletions ecs-cli/modules/cli/regcreds/create_task_execution_role_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestCreateTaskExecutionRole(t *testing.T) {

mocks := setupTestController(t)
gomock.InOrder(
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, nil).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(&iam.CreateRoleOutput{Role: &iam.Role{Arn: testRoleArn}}, nil),
)
gomock.InOrder(
Expand Down Expand Up @@ -76,7 +76,7 @@ func TestCreateTaskExecutionRole_NoKMSKey(t *testing.T) {

mocks := setupTestController(t)
gomock.InOrder(
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, nil).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(&iam.CreateRoleOutput{Role: &iam.Role{Arn: testRoleArn}}, nil),
)
gomock.InOrder(
Expand Down Expand Up @@ -110,7 +110,7 @@ func TestCreateTaskExecutionRole_RoleExists(t *testing.T) {
mocks := setupTestController(t)
gomock.InOrder(
// CreateOrFindRole should return nil if given role already exists
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString).Return("", nil),
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, nil).Return("", nil),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(nil, roleExistsError),
)
gomock.InOrder(
Expand Down Expand Up @@ -140,7 +140,7 @@ func TestCreateTaskExecutionRole_ErrorOnCreateRoleFails(t *testing.T) {

mocks := setupTestController(t)
gomock.InOrder(
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString).Return("", errors.New("something went wrong")),
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, nil).Return("", errors.New("something went wrong")),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(nil, errors.New("something went wrong")),
)

Expand All @@ -166,7 +166,7 @@ func TestCreateTaskExecutionRole_ErrorOnCreatePolicyFails(t *testing.T) {

mocks := setupTestController(t)
gomock.InOrder(
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, nil).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(&iam.CreateRoleOutput{Role: &iam.Role{Arn: testRoleArn}}, nil),
)
gomock.InOrder(
Expand All @@ -182,3 +182,69 @@ func TestCreateTaskExecutionRole_ErrorOnCreatePolicyFails(t *testing.T) {
_, err := createTaskExecutionRole(testParams, mocks.MockIAM, mocks.MockKMS)
assert.Error(t, err, "Expected error when CreatePolicy fails")
}

func TestCreateTaskExecutionRoleWithTags(t *testing.T) {
testRegistry := "myreg.test.io"
testRegCredARN := "arn:aws:secret/some-test-arn"
testRegKMSKey := "arn:aws:kms:key/67yt-756yth"

testCreds := map[string]regcredio.CredsOutputEntry{
testRegistry: regcredio.BuildOutputEntry(testRegCredARN, testRegKMSKey, []string{"test"}),
}

testRoleName := "myNginxProjectRole"

testPolicyArn := aws.String("arn:aws:iam::policy/" + testRoleName + "-policy")
testRoleArn := aws.String("arn:aws:iam::role/" + testRoleName)

testParams := executionRoleParams{
CredEntries: testCreds,
RoleName: testRoleName,
Region: "us-west-2",
Tags: map[string]*string{
"Hey": aws.String("Jude"),
"Come": aws.String("Together"),
"Hello": aws.String("Goodbye"),
"Abbey": aws.String("Road"),
},
}

expectedTags := []*iam.Tag{
&iam.Tag{
Key: aws.String("Hey"),
Value: aws.String("Jude"),
},
&iam.Tag{
Key: aws.String("Come"),
Value: aws.String("Together"),
},
&iam.Tag{
Key: aws.String("Hello"),
Value: aws.String("Goodbye"),
},
&iam.Tag{
Key: aws.String("Abbey"),
Value: aws.String("Road"),
},
}

mocks := setupTestController(t)
gomock.InOrder(
mocks.MockIAM.EXPECT().CreateOrFindRole(testRoleName, roleDescriptionString, assumeRolePolicyDocString, gomock.Any()).Do(func(w, x, y, z interface{}) {
tags := z.([]*iam.Tag)
assert.ElementsMatch(t, tags, expectedTags, "Expected Tags to match")
}).Return(*testRoleArn, nil),
mocks.MockIAM.EXPECT().CreateRole(gomock.Any()).Return(&iam.CreateRoleOutput{Role: &iam.Role{Arn: testRoleArn}}, nil),
)
gomock.InOrder(
// If KMSKeyID present, first thing to happen should be verifying its ARN
mocks.MockKMS.EXPECT().GetValidKeyARN(testRegKMSKey).Return(testRegKMSKey, nil),
mocks.MockIAM.EXPECT().CreatePolicy(gomock.Any()).Return(&iam.CreatePolicyOutput{Policy: &iam.Policy{Arn: testPolicyArn}}, nil),
mocks.MockIAM.EXPECT().AttachRolePolicy(getExecutionRolePolicyARN("us-west-2"), testRoleName).Return(nil, nil),
mocks.MockIAM.EXPECT().AttachRolePolicy(*testPolicyArn, testRoleName).Return(nil, nil),
)

policyCreateTime, err := createTaskExecutionRole(testParams, mocks.MockIAM, mocks.MockKMS)
assert.NoError(t, err, "Unexpected error when creating task execution role")
assert.NotNil(t, policyCreateTime, "Expected policy create time to be non-nil")
}
43 changes: 43 additions & 0 deletions ecs-cli/modules/cli/regcreds/regcreds_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ import (
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/iam"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/kms"
secretsClient "github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/secretsmanager"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/tagging"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/commands/flags"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/config"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/utils"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/utils/regcredio"
"github.com/aws/aws-sdk-go/aws"
taggingSDK "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
"github.com/aws/aws-sdk-go/service/secretsmanager"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
Expand Down Expand Up @@ -85,6 +88,14 @@ func Up(c *cli.Context) {
log.Fatal("Error executing 'up': ", err)
}

var tags map[string]*string
if tagVal := c.String(flags.ResourceTagsFlag); tagVal != "" {
tags, err = utils.GetTagsMap(tagVal)
if err != nil {
log.Fatal("Error executing 'up': ", err)
}
}

var policyCreateTime *time.Time
if !skipRole {
region := commandConfig.Session.Config.Region
Expand All @@ -93,6 +104,7 @@ func Up(c *cli.Context) {
CredEntries: credentialOutput,
RoleName: roleName,
Region: *region,
Tags: tags,
}

policyCreateTime, err = createTaskExecutionRole(roleParams, iamClient, kmsClient)
Expand All @@ -103,6 +115,14 @@ func Up(c *cli.Context) {
log.Info("Skipping role creation.")
}

if len(tags) > 0 {
taggingClient := tagging.NewTaggingClient(commandConfig)
err = tagRegistryCredentials(credentialOutput, tags, taggingClient)
if err != nil {
log.Fatal("Failed to tag resources: ", err)
}
}

// produce output file
if !skipOutput {
regcredio.GenerateCredsOutput(credentialOutput, roleName, outputDir, policyCreateTime)
Expand Down Expand Up @@ -306,3 +326,26 @@ func validateOutputOptions(outputDir string, skipOutput bool) error {
}
return nil
}

func tagRegistryCredentials(creds map[string]regcredio.CredsOutputEntry, tags map[string]*string, taggingClient tagging.Client) error {
var arns []*string

for _, credInfo := range creds {
arns = append(arns, aws.String(credInfo.CredentialARN))
}

input := &taggingSDK.TagResourcesInput{
ResourceARNList: arns,
Tags: tags,
}
output, err := taggingClient.TagResources(input)
if err != nil {
return err
}

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

Choose a reason for hiding this comment

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

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

}

return nil
}
92 changes: 92 additions & 0 deletions ecs-cli/modules/cli/regcreds/regcreds_app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ import (
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/iam/mock"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/kms/mock"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/secretsmanager/mock"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/clients/aws/tagging/mock"
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/utils/regcredio"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/kms"
taggingSDK "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
secretsmanager "github.com/aws/aws-sdk-go/service/secretsmanager"
"github.com/golang/mock/gomock"
"github.com/pkg/errors"
Expand Down Expand Up @@ -216,6 +218,96 @@ func TestGetOrCreateRegistryCredentials_ErrorOnUpdate(t *testing.T) {
assert.Error(t, err)
}

func TestTagRegistryCredentials(t *testing.T) {
creds := map[string]regcredio.CredsOutputEntry{
"the-who-registry.com": regcredio.CredsOutputEntry{
CredentialARN: "arn:aws:secretsmanager:eu-west-1:111111111111:secret:path/whoareyou-1978",
},
}

tags := map[string]*string{
"Baba": aws.String("O'riley"),
"Eminence": aws.String("Front"),
"My": aws.String("Generation"),
}

ctrl := gomock.NewController(t)

mockTagging := mock_tagging.NewMockClient(ctrl)

gomock.InOrder(
mockTagging.EXPECT().TagResources(gomock.Any()).Do(func(x interface{}) {
input := x.(*taggingSDK.TagResourcesInput)
assert.Equal(t, tags, input.Tags, "Expected tags to match")
}).Return(&taggingSDK.TagResourcesOutput{}, nil),
)

err := tagRegistryCredentials(creds, tags, mockTagging)
assert.NoError(t, err, "Unexpected error calling tagRegistryCredentials")
}

func TestTagRegistryCredentialsError(t *testing.T) {
creds := map[string]regcredio.CredsOutputEntry{
"the-who-registry.com": regcredio.CredsOutputEntry{
CredentialARN: "arn:aws:secretsmanager:eu-west-1:111111111111:secret:path/whoareyou-1978",
},
}

tags := map[string]*string{
"Baba": aws.String("O'riley"),
"Eminence": aws.String("Front"),
"My": aws.String("Generation"),
}

ctrl := gomock.NewController(t)

mockTagging := mock_tagging.NewMockClient(ctrl)

gomock.InOrder(
mockTagging.EXPECT().TagResources(gomock.Any()).Do(func(x interface{}) {
input := x.(*taggingSDK.TagResourcesInput)
assert.Equal(t, tags, input.Tags, "Expected tags to match")
}).Return(nil, fmt.Errorf("Some API error")),
)

err := tagRegistryCredentials(creds, tags, mockTagging)
assert.Error(t, err, "Expected error calling tagRegistryCredentials")
}

func TestTagRegistryCredentialsFailedResources(t *testing.T) {
creds := map[string]regcredio.CredsOutputEntry{
"the-who-registry.com": regcredio.CredsOutputEntry{
CredentialARN: "arn:aws:secretsmanager:eu-west-1:111111111111:secret:path/whoareyou-1978",
},
}

tags := map[string]*string{
"Baba": aws.String("O'riley"),
"Eminence": aws.String("Front"),
"My": aws.String("Generation"),
}

ctrl := gomock.NewController(t)

mockTagging := mock_tagging.NewMockClient(ctrl)

gomock.InOrder(
mockTagging.EXPECT().TagResources(gomock.Any()).Do(func(x interface{}) {
input := x.(*taggingSDK.TagResourcesInput)
assert.Equal(t, tags, input.Tags, "Expected tags to match")
}).Return(&taggingSDK.TagResourcesOutput{
FailedResourcesMap: map[string]*taggingSDK.FailureInfo{
"arn:aws:secretsmanager:eu-west-1:111111111111:secret:path/whoareyou-1978": &taggingSDK.FailureInfo{
ErrorMessage: aws.String("Auth Error: who are you"),
},
},
}, nil),
)

err := tagRegistryCredentials(creds, tags, mockTagging)
assert.Error(t, err, "Expected error calling tagRegistryCredentials")
}

func TestValidateCredsInput_ErrorEmptyCreds(t *testing.T) {
emptyCredMap := make(map[string]regcredio.RegistryCredEntry)
emptyCredsInput := regcredio.ECSRegCredsInput{
Expand Down
7 changes: 5 additions & 2 deletions ecs-cli/modules/clients/aws/iam/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type Client interface {
AttachRolePolicy(policyArn, roleName string) (*iam.AttachRolePolicyOutput, error)
CreateRole(iam.CreateRoleInput) (*iam.CreateRoleOutput, error)
CreatePolicy(iam.CreatePolicyInput) (*iam.CreatePolicyOutput, error)
CreateOrFindRole(string, string, string) (string, error)
CreateOrFindRole(string, string, string, []*iam.Tag) (string, error)
}

type iamClient struct {
Expand Down Expand Up @@ -81,12 +81,15 @@ func (c *iamClient) CreatePolicy(input iam.CreatePolicyInput) (*iam.CreatePolicy
}

// CreateOrFindRole returns a new role ARN or an empty string if role already exists
func (c *iamClient) CreateOrFindRole(roleName, roleDescription, assumeRolePolicyDoc string) (string, error) {
func (c *iamClient) CreateOrFindRole(roleName, roleDescription, assumeRolePolicyDoc string, tags []*iam.Tag) (string, error) {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down
8 changes: 4 additions & 4 deletions ecs-cli/modules/clients/aws/iam/mock/client.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading