From 078177de4ea26e860c272b1ef33d19712d1fdbd6 Mon Sep 17 00:00:00 2001 From: Wesley Pettit Date: Fri, 1 Mar 2019 15:37:31 -0800 Subject: [PATCH] Implemented tagging of container instances created by 'ecs-cli up' --- ecs-cli/modules/cli/cluster/cluster_app.go | 64 ++++++++-- .../modules/cli/cluster/cluster_app_test.go | 111 +++++++++++++++++- .../modules/cli/cluster/userdata/user_data.go | 21 ++-- .../cli/cluster/userdata/user_data_test.go | 41 ++++++- ecs-cli/modules/clients/aws/ecs/client.go | 8 ++ .../modules/clients/aws/ecs/mock/client.go | 13 ++ 6 files changed, 228 insertions(+), 30 deletions(-) diff --git a/ecs-cli/modules/cli/cluster/cluster_app.go b/ecs-cli/modules/cli/cluster/cluster_app.go index f84a46b96..824cb1332 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app.go +++ b/ecs-cli/modules/cli/cluster/cluster_app.go @@ -42,11 +42,17 @@ import ( ) // user data builder can be easily mocked in tests -var newUserDataBuilder func(string) userdata.UserDataBuilder = userdata.NewBuilder +var newUserDataBuilder func(string, bool) userdata.UserDataBuilder = userdata.NewBuilder // displayTitle flag is used to print the title for the fields const displayTitle = true +// Values returned by the ECS Settings API +const ( + ecsSettingEnabled = "enabled" + ecsSettingDisabled = "disabled" +) + const ( ParameterKeyAsgMaxSize = "AsgMaxSize" ParameterKeyVPCAzs = "VpcAvailabilityZones" @@ -237,8 +243,26 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * deleteStack = true } + tags := make([]*ecs.Tag, 0) + if tagVal := context.String(flags.ResourceTagsFlag); tagVal != "" { + tags, err = utils.ParseTags(tagVal, tags) + if err != nil { + return err + } + } + + var containerInstanceTaggingSupported bool + + if len(tags) > 0 { + // determine if container instance tagging is supported + containerInstanceTaggingSupported, err = canEnableContainerInstanceTagging(awsClients.ECSClient) + if err != nil { + return err + } + } + // Populate cfn params - cfnParams, err := cliFlagsToCfnStackParams(context, commandConfig.Cluster, launchType) + cfnParams, err := cliFlagsToCfnStackParams(context, commandConfig.Cluster, launchType, containerInstanceTaggingSupported) if err != nil { return err } @@ -287,14 +311,6 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * return fmt.Errorf("You have selected subnets. Please specify a VPC with the '--%s' flag", flags.VpcIdFlag) } - tags := make([]*ecs.Tag, 0) - if tagVal := context.String(flags.ResourceTagsFlag); tagVal != "" { - tags, err = utils.ParseTags(tagVal, tags) - if err != nil { - return err - } - } - if launchType == config.LaunchTypeEC2 { architecture, err := determineArchitecture(cfnParams) if err != nil { @@ -348,6 +364,30 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * return cfnClient.WaitUntilCreateComplete(stackName) } +func canEnableContainerInstanceTagging(client ecsclient.ECSClient) (bool, error) { + output, err := client.ListAccountSettings(&ecs.ListAccountSettingsInput{ + EffectiveSettings: aws.Bool(true), + Name: aws.String(ecs.SettingNameContainerInstanceLongArnFormat), + }) + if err != nil { + return false, err + } + + // This should never evaluate to true, unless there is a problem with API + // This if block ensures that the CLI does not panic in that case + if len(output.Settings) < 1 { + return false, fmt.Errorf("Received unexpected response from ECS Settings API: %s", output) + } + + if aws.StringValue(output.Settings[0].Value) == ecsSettingEnabled { + logrus.Warnf("Enabling container instance tagging because %s is enabled for your identity, %s. If this is not your account default setting, your instances will fail to join your cluster. You can use the PutAccountSettingDefault API to change your account default.", ecs.SettingNameContainerInstanceLongArnFormat, aws.StringValue(output.Settings[0].PrincipalArn)) + return true, nil + } + + logrus.Warnf("Disabling container instance tagging because %s is not enabled for your identity, %s. You can use the PutAccountSettingDefault API to change your account default.", ecs.SettingNameContainerInstanceLongArnFormat, aws.StringValue(output.Settings[0].PrincipalArn)) + return false, nil +} + func determineArchitecture(cfnParams *cloudformation.CfnStackParams) (string, error) { architecture := amimetadata.ArchitectureTypeX86 @@ -567,7 +607,7 @@ func deleteClusterPrompt(reader *bufio.Reader) error { } // cliFlagsToCfnStackParams converts values set for CLI flags to cloudformation stack parameters. -func cliFlagsToCfnStackParams(context *cli.Context, cluster, launchType string) (*cloudformation.CfnStackParams, error) { +func cliFlagsToCfnStackParams(context *cli.Context, cluster, launchType string, tagContainerInstances bool) (*cloudformation.CfnStackParams, error) { cfnParams := cloudformation.NewCfnStackParams(requiredParameters) for cliFlag, cfnParamKeyName := range flagNamesToStackParameterKeys { cfnParamKeyValue := context.String(cliFlag) @@ -577,7 +617,7 @@ func cliFlagsToCfnStackParams(context *cli.Context, cluster, launchType string) } if launchType == config.LaunchTypeEC2 { - builder := newUserDataBuilder(cluster) + builder := newUserDataBuilder(cluster, tagContainerInstances) // handle extra user data, which is a string slice flag if userDataFiles := context.StringSlice(flags.UserDataFlag); len(userDataFiles) > 0 { for _, file := range userDataFiles { diff --git a/ecs-cli/modules/cli/cluster/cluster_app_test.go b/ecs-cli/modules/cli/cluster/cluster_app_test.go index c4fc4e6ed..26b85e2a2 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app_test.go +++ b/ecs-cli/modules/cli/cluster/cluster_app_test.go @@ -82,8 +82,9 @@ func newMockReadWriter() *mockReadWriter { } type mockUserDataBuilder struct { - userdata string - files []string + userdata string + files []string + enableTagging bool } func (b *mockUserDataBuilder) AddFile(fileName string) error { @@ -221,7 +222,8 @@ func TestClusterUpWithUserData(t *testing.T) { userdataMock := &mockUserDataBuilder{ userdata: mockedUserData, } - newUserDataBuilder = func(clusterName string) userdata.UserDataBuilder { + newUserDataBuilder = func(clusterName string, enableTagging bool) userdata.UserDataBuilder { + userdataMock.enableTagging = enableTagging return userdataMock } @@ -240,6 +242,7 @@ func TestClusterUpWithUserData(t *testing.T) { param, err := cfnParams.GetParameter(ParameterKeyUserData) assert.NoError(t, err, "Expected User Data parameter to be set") assert.Equal(t, mockedUserData, aws.StringValue(param.ParameterValue), "Expected user data to match") + assert.False(t, userdataMock.enableTagging, "Expected container instance tagging to be disabled") }).Return("", nil), mockCloudformation.EXPECT().WaitUntilCreateComplete(stackName).Return(nil), ) @@ -626,7 +629,7 @@ func TestCliFlagsToCfnStackParams(t *testing.T) { flagSet.String(flags.KeypairNameFlag, "default", "") context := cli.NewContext(nil, flagSet, nil) - params, err := cliFlagsToCfnStackParams(context, clusterName, config.LaunchTypeEC2) + params, err := cliFlagsToCfnStackParams(context, clusterName, config.LaunchTypeEC2, false) assert.NoError(t, err, "Unexpected error from call to cliFlagsToCfnStackParams") _, err = params.GetParameter(ParameterKeyAsgMaxSize) @@ -635,7 +638,7 @@ func TestCliFlagsToCfnStackParams(t *testing.T) { flagSet.String(flags.AsgMaxSizeFlag, "2", "") context = cli.NewContext(nil, flagSet, nil) - params, err = cliFlagsToCfnStackParams(context, clusterName, config.LaunchTypeEC2) + params, err = cliFlagsToCfnStackParams(context, clusterName, config.LaunchTypeEC2, false) assert.NoError(t, err, "Unexpected error from call to cliFlagsToCfnStackParams") _, err = params.GetParameter(ParameterKeyAsgMaxSize) assert.NoError(t, err, "Unexpected error getting parameter ParameterKeyAsgMaxSize") @@ -1030,7 +1033,17 @@ func TestClusterUpWithTags(t *testing.T) { }, } + listSettingsResponse := &ecs.ListAccountSettingsOutput{ + Settings: []*ecs.Setting{ + &ecs.Setting{ + Name: aws.String(ecs.SettingNameContainerInstanceLongArnFormat), + Value: aws.String("disabled"), + }, + }, + } + gomock.InOrder( + mockECS.EXPECT().ListAccountSettings(gomock.Any()).Return(listSettingsResponse, nil), mockECS.EXPECT().CreateCluster(clusterName, gomock.Any()).Return(clusterName, nil).Do(func(x, y interface{}) { actualTags := y.([]*ecs.Tag) assert.ElementsMatch(t, expectedECSTags, actualTags, "Expected tags to match") @@ -1064,6 +1077,94 @@ func TestClusterUpWithTags(t *testing.T) { assert.NoError(t, err, "Unexpected error bringing up cluster") } +func TestClusterUpWithTagsContainerInstanceTaggingEnabled(t *testing.T) { + defer os.Clearenv() + mockECS, mockCloudformation, mockSSM := setupTest(t) + awsClients := &AWSClients{mockECS, mockCloudformation, mockSSM} + + oldNewUserDataBuilder := newUserDataBuilder + defer func() { newUserDataBuilder = oldNewUserDataBuilder }() + userdataMock := &mockUserDataBuilder{ + userdata: mockedUserData, + } + newUserDataBuilder = func(clusterName string, enableTagging bool) userdata.UserDataBuilder { + userdataMock.enableTagging = enableTagging + return userdataMock + } + + expectedCFNTags := []*sdkCFN.Tag{ + &sdkCFN.Tag{ + Key: aws.String("madman"), + Value: aws.String("with-a-box"), + }, + &sdkCFN.Tag{ + Key: aws.String("doctor"), + Value: aws.String("11"), + }, + } + + expectedECSTags := []*ecs.Tag{ + &ecs.Tag{ + Key: aws.String("madman"), + Value: aws.String("with-a-box"), + }, + &ecs.Tag{ + Key: aws.String("doctor"), + Value: aws.String("11"), + }, + } + + listSettingsResponse := &ecs.ListAccountSettingsOutput{ + Settings: []*ecs.Setting{ + &ecs.Setting{ + Name: aws.String(ecs.SettingNameContainerInstanceLongArnFormat), + Value: aws.String("enabled"), + }, + }, + } + + gomock.InOrder( + mockECS.EXPECT().ListAccountSettings(gomock.Any()).Return(listSettingsResponse, nil), + mockECS.EXPECT().CreateCluster(clusterName, gomock.Any()).Return(clusterName, nil).Do(func(x, y interface{}) { + actualTags := y.([]*ecs.Tag) + assert.ElementsMatch(t, expectedECSTags, actualTags, "Expected tags to match") + }), + ) + gomock.InOrder( + mockSSM.EXPECT().GetRecommendedECSLinuxAMI("x86").Return(amiMetadata(amiID), nil), + ) + gomock.InOrder( + mockCloudformation.EXPECT().ValidateStackExists(stackName).Return(errors.New("error")), + mockCloudformation.EXPECT().CreateStack(gomock.Any(), stackName, true, gomock.Any(), gomock.Any()).Do(func(v, w, x, y, z interface{}) { + actualTags := z.([]*sdkCFN.Tag) + assert.ElementsMatch(t, expectedCFNTags, actualTags, "Expected tags to match") + + cfnParams := y.(*cloudformation.CfnStackParams) + param, err := cfnParams.GetParameter(ParameterKeyUserData) + assert.NoError(t, err, "Expected User Data parameter to be set") + assert.Equal(t, mockedUserData, aws.StringValue(param.ParameterValue), "Expected user data to match") + }).Return("", nil), + mockCloudformation.EXPECT().WaitUntilCreateComplete(stackName).Return(nil), + mockCloudformation.EXPECT().DescribeNetworkResources(stackName).Return(nil), + ) + globalSet := flag.NewFlagSet("ecs-cli", 0) + globalContext := cli.NewContext(nil, globalSet, nil) + + flagSet := flag.NewFlagSet("ecs-cli-up", 0) + flagSet.String(flags.ResourceTagsFlag, "madman=with-a-box,doctor=11", "") + flagSet.Bool(flags.CapabilityIAMFlag, true, "") + + context := cli.NewContext(nil, flagSet, globalContext) + rdwr := newMockReadWriter() + commandConfig, err := newCommandConfig(context, rdwr) + assert.NoError(t, err, "Unexpected error creating CommandConfig") + + err = createCluster(context, awsClients, commandConfig) + assert.NoError(t, err, "Unexpected error bringing up cluster") + + assert.True(t, userdataMock.enableTagging, "Expected tagging to be enabled in container instance user data") +} + func TestDetermineArchitecture(t *testing.T) { var testCases = []struct { in string diff --git a/ecs-cli/modules/cli/cluster/userdata/user_data.go b/ecs-cli/modules/cli/cluster/userdata/user_data.go index 32da4c355..871a08aff 100644 --- a/ecs-cli/modules/cli/cluster/userdata/user_data.go +++ b/ecs-cli/modules/cli/cluster/userdata/user_data.go @@ -33,20 +33,22 @@ type UserDataBuilder interface { // Builder implements UserDataBuilder type Builder struct { - writer *multipart.Writer - clusterName string - userdata *bytes.Buffer + writer *multipart.Writer + clusterName string + userdata *bytes.Buffer + enableTagging bool } // NewBuilder creates a Builder object for a given clusterName -func NewBuilder(clusterName string) UserDataBuilder { +func NewBuilder(clusterName string, enableTagging bool) UserDataBuilder { buf := new(bytes.Buffer) writer := multipart.NewWriter(buf) builder := &Builder{ - writer: writer, - clusterName: clusterName, - userdata: buf, + writer: writer, + clusterName: clusterName, + userdata: buf, + enableTagging: enableTagging, } return builder @@ -148,6 +150,11 @@ func (b *Builder) getClusterUserData() string { #!/bin/bash echo ECS_CLUSTER=%s >> /etc/ecs/ecs.config ` + if b.enableTagging { + joinClusterUserData += ` +echo 'ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS_FROM=ec2_instance' >> /etc/ecs/ecs.config` + } + return fmt.Sprintf(joinClusterUserData, b.clusterName) } diff --git a/ecs-cli/modules/cli/cluster/userdata/user_data_test.go b/ecs-cli/modules/cli/cluster/userdata/user_data_test.go index 27fe5547a..1e1db23bc 100644 --- a/ecs-cli/modules/cli/cluster/userdata/user_data_test.go +++ b/ecs-cli/modules/cli/cluster/userdata/user_data_test.go @@ -28,11 +28,12 @@ const ( testBoundary = "========multipart-boundary==" ) -func newBuilderInTest(buf *bytes.Buffer, writer *multipart.Writer) *Builder { +func newBuilderInTest(buf *bytes.Buffer, writer *multipart.Writer, enableTagging bool) *Builder { builder := &Builder{ - writer: writer, - clusterName: testClusterName, - userdata: buf, + writer: writer, + clusterName: testClusterName, + userdata: buf, + enableTagging: enableTagging, } return builder @@ -148,7 +149,7 @@ func TestBuildUserDataWithExtraData(t *testing.T) { writer := multipart.NewWriter(buf) // set the boundary between parts so that output is deterministic writer.SetBoundary(testBoundary) - builder := newBuilderInTest(buf, writer) + builder := newBuilderInTest(buf, writer, false) err := builder.AddFile(multipartFilePath) assert.NoError(t, err, "Unexpected error calling AddFile()") @@ -182,7 +183,35 @@ echo ECS_CLUSTER=cluster >> /etc/ecs/ecs.config writer := multipart.NewWriter(buf) // set the boundary between parts so that output is deterministic writer.SetBoundary(testBoundary) - builder := newBuilderInTest(buf, writer) + builder := newBuilderInTest(buf, writer, false) + + actual, err := builder.Build() + assert.NoError(t, err, "Unexpected error calling Build()") + expected := unixifyLineEndings(expectedUserData) + assert.Equal(t, expected, actual, "Expected resulting mime multipart archive to match") +} + +func TestBuildUserDataNoExtraDataWithTaggingEnabled(t *testing.T) { + var expectedUserData = `Content-Type: multipart/mixed; boundary="========multipart-boundary==" +MIME-Version: 1.0 + +--========multipart-boundary== +Content-Type: text/text/x-shellscript; charset="utf-8" +Mime-Version: 1.0 + + +#!/bin/bash +echo ECS_CLUSTER=cluster >> /etc/ecs/ecs.config + +echo 'ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS_FROM=ec2_instance' >> /etc/ecs/ecs.config +--========multipart-boundary==-- +` + + buf := new(bytes.Buffer) + writer := multipart.NewWriter(buf) + // set the boundary between parts so that output is deterministic + writer.SetBoundary(testBoundary) + builder := newBuilderInTest(buf, writer, true) actual, err := builder.Build() assert.NoError(t, err, "Unexpected error calling Build()") diff --git a/ecs-cli/modules/clients/aws/ecs/client.go b/ecs-cli/modules/clients/aws/ecs/client.go index 400488d28..cfb2c0c67 100644 --- a/ecs-cli/modules/clients/aws/ecs/client.go +++ b/ecs-cli/modules/clients/aws/ecs/client.go @@ -59,6 +59,9 @@ type ECSClient interface { // Container Instance related GetEC2InstanceIDs(containerInstanceArns []*string) (map[string]string, error) + + // Settings related + ListAccountSettings(input *ecs.ListAccountSettingsInput) (*ecs.ListAccountSettingsOutput, error) } // ecsClient implements ECSClient @@ -421,3 +424,8 @@ func (c *ecsClient) IsActiveCluster(clusterName string) (bool, error) { log.WithFields(log.Fields{"cluster": clusterName, "status": status}).Debug("cluster status") return false, nil } + +// Checks if the given setting is enabled +func (c *ecsClient) ListAccountSettings(input *ecs.ListAccountSettingsInput) (*ecs.ListAccountSettingsOutput, error) { + return c.client.ListAccountSettings(input) +} diff --git a/ecs-cli/modules/clients/aws/ecs/mock/client.go b/ecs-cli/modules/clients/aws/ecs/mock/client.go index b59831c0e..6fbcf7ae6 100644 --- a/ecs-cli/modules/clients/aws/ecs/mock/client.go +++ b/ecs-cli/modules/clients/aws/ecs/mock/client.go @@ -176,6 +176,19 @@ func (mr *MockECSClientMockRecorder) IsActiveCluster(arg0 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsActiveCluster", reflect.TypeOf((*MockECSClient)(nil).IsActiveCluster), arg0) } +// ListAccountSettings mocks base method +func (m *MockECSClient) ListAccountSettings(arg0 *ecs0.ListAccountSettingsInput) (*ecs0.ListAccountSettingsOutput, error) { + ret := m.ctrl.Call(m, "ListAccountSettings", arg0) + ret0, _ := ret[0].(*ecs0.ListAccountSettingsOutput) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListAccountSettings indicates an expected call of ListAccountSettings +func (mr *MockECSClientMockRecorder) ListAccountSettings(arg0 interface{}) *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAccountSettings", reflect.TypeOf((*MockECSClient)(nil).ListAccountSettings), arg0) +} + // RegisterTaskDefinitionIfNeeded mocks base method func (m *MockECSClient) RegisterTaskDefinitionIfNeeded(arg0 *ecs0.RegisterTaskDefinitionInput, arg1 cache.Cache) (*ecs0.TaskDefinition, error) { ret := m.ctrl.Call(m, "RegisterTaskDefinitionIfNeeded", arg0, arg1)