ECS Agent tagging support #1618

haikuoliu · 2018-10-12T17:08:49Z

Summary

ECS local tags support

Implementation details

Introduce ECS_CONTAINER_INSTANCE_TAGS env var to allow customer to configure tags on the instance.
Introduce ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS to get the tags of the ec2 instance that Agent is running on using DescribeTags API. We can specify either none or ec2_instance for this env var, the reason that we don't use a boolean is because we may introduce other ways to propagate the tags (like ask backend to propagate tags instead of doing it on Agent side).
ECS model changes for tags, introduce a new API ListTagsForResourceInput for testing purpose.
ECS client API changes to allow tags passed from RCI call.
Introduce EC2 client API, DescribeECSTagsForInstance is used for RCI call, and CreateTags for func test.
Add InstanceID API to EC2 metadata client.
Functional test and manual test against Gamma endpoint.
Vendor packages update.
Joel reviewed my README.

TODO:
Make func test pass once the ListTagsForResource API is in prod.

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

Description for the changelog

Licensing

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

haikuoliu · 2018-10-15T03:29:43Z

Func tests are expected to fail as the ListTagsForResource API is not in prod yet.

petderek

Neat!

petderek · 2018-10-15T18:21:10Z

agent/config/config.go

+
+	// When ContainerInstancePropagateTagsEC2InstanceType is specified, agent will
+	// make DescribeTags API call to get tags remotely.
+	ContainerInstancePropagateTagsEC2InstanceType


Does this get stored in the state file? if so, we should test that the values of these don't change. (IE, we don't accidentally reorder them in the future)

This will not be stored in the state file, every time Agent starts it will read the config file and populate this value again.

petderek · 2018-10-15T18:22:06Z

agent/config/config.go

@@ -406,6 +416,9 @@ func environmentConfig() (Config, error) {
 	var errs []error
 	instanceAttributes, errs = parseInstanceAttributes(errs)

+	var containerInstanceTags map[string]string
+	containerInstanceTags, errs = parseContainerInstanceTags(errs)


This might be a silly question, but why doesn't := work?

Yeah that will work, changed to the simpler format.

agent/ec2/ec2_client.go

petderek · 2018-10-15T18:29:35Z

agent/config/config_test.go

@@ -592,6 +601,49 @@ func TestUserDataConfig(t *testing.T) {
 	}
 }



Add test cases for the key:value pairs?

I think I've already added here. LMK if you want me to include more test cases.

petderek · 2018-10-15T18:31:12Z

agent/config/config_test.go

+
+	for _, tc := range testcases {
+		// Test the parse function only.
+		t.Run(tc.name, func(t *testing.T) {


I'd probably merge these two t.Run functions together and just make sure the "message" includes a note that the parsing didn't work as expected.

petderek · 2018-10-15T18:32:30Z

agent/utils/utils.go

@@ -208,3 +210,20 @@ func IsAWSErrorCodeEqual(err error, code string) bool {
 	awsErr, ok := err.(awserr.Error)
 	return ok && awsErr.Code() == code
 }
+
+// MapToTags converts a map to a slice of tags.
+func MapToTags(tagsMap map[string]string) []*ecs.Tag {


FFTI: I'd consider moving this and the other tagging-specific functions (mergeTags) into a tagging-specific file / package.

yumex93 · 2018-10-16T06:31:09Z

agent/api/ecsclient/client_test.go

+func extractTagsMapFromRegisterContainerInstanceInput(req *ecs.RegisterContainerInstanceInput) map[string]string {
+	tagsMap := make(map[string]string, len(req.Tags))
+	for i := range req.Tags {
+		tagsMap[*req.Tags[i].Key] = aws.StringValue(req.Tags[i].Value)


Why key is using pointer dereference while value is using aws.StringValue here?

yumex93 · 2018-10-16T06:37:15Z

agent/ec2/mocks/ec2_mocks.go

 	ec2metadata "github.com/aws/aws-sdk-go/aws/ec2metadata"
+	ec20 "github.com/aws/aws-sdk-go/service/ec2"


This ec20 is a little bit weird as a name. Maybe change '/agent/ec2' to agent_ec2 and keep '/service/ec2' as ec2.

I don't know, this is something auto-generated...

its the worst!

sharanyad · 2018-10-16T14:29:43Z

README.md

@@ -131,6 +131,8 @@ additional details on each available environment variable.
 | `ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
 | `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated integer values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |
 | `ECS_SHARED_VOLUME_MATCH_FULL_CONFIG` | `true` | When `true`, ECS Agent will compare name, driver options, and labels to make sure volumes are identical. When `false`, Agent will short circuit shared volume comparison if the names match. This is the default Docker behavior. If a volume is shared across instances, this should be set to `false`. | `false` | `false`|
+| `ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS` | `"ec2_instance"` | If `"ec2_instance"` is specified, tags of the ec2 instance will be registered to ECS, make sure you allow `ec2:DescribeTags` action in your instance profile. | `"none"` | `"none"` |


Name ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS can be a bit confusing and it sounds like a bool var.

how about something like -
ECS_INSTANCE_REGISTER_TAG_TYPES or ECS_INSTANCE_OBTAIN_TAG_TYPES?

Confirmed with Akram, decided to go with ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS_FROM.

sharanyad · 2018-10-16T15:30:36Z

agent/config/parse.go

+}
+
+func parseContainerInstancePropagateTags() ContainerInstancePropagateTagsType {
+	containerInstancePropagateTagsString := os.Getenv("ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS")


I feel like ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS should be a list of strings rather than one, if we want to support multiple tag types in the future and not break backwards compatibility.

I think customer only has one option - either ask Agent to propagate the tags or ask backend to propagate the tags, customer will get the same tags either way, so supporting both of them seems to do duplicate work.

The reason why I brought this up is because we have ContainerInstancePropagateTagsType as an enum -- implying there can be other values too?
Do we know what they could be or if multiple values can be used?

If ec2-instance is going to be the only value, we could just have a string as opposed to an enum.

Yes, it can be other values, but even if there are other possible values, customer can only use one of them. For example, if we have backend as one of the values in the future, customer only needs ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS to be either ec2_instance or backend, because we will get the same set of tags when we ask backend to propagate or we let Agent propagate tags.

haikuoliu · 2018-10-18T06:39:21Z

@yumex93 @sharanyad I've refactored accordingly.

Also introduced PutAccountSetting API used in functional test, this is required for tags related API.

haikuoliu · 2018-10-23T05:24:19Z

Joel reviewed my README. Rebase the dev branch, and added changelog.

sharanyad · 2018-10-23T17:11:09Z

CHANGELOG.md

@@ -1,8 +1,11 @@
 # Changelog

+## 1.22.0-dev
+* Feature - Introduce two environment variables to support ECS tagging [#1618](https://github.com/aws/amazon-ecs-agent/pull/1618)


Can we also specify what they are?

sharanyad · 2018-10-23T17:23:27Z

agent/ecs_client/model/api/api-2.json

@@ -310,6 +325,20 @@
        {"shape":"ServiceNotFoundException"}
      ]
    },
+    "PutAccountSetting":{


is this part of tagging?

Yes, used in func test.

sharanyad · 2018-10-23T17:29:31Z

agent/app/agent_test.go

+	assert.Equal(t, localValue, aws.StringValue(mergedTags[2].Value))
+}
+
+func TestGetContainerInstanceTagsFromEC2API(t *testing.T) {


Please add negative test case as well

sharanyad · 2018-10-23T17:33:54Z

agent/functional_tests/tests/functionaltests_test.go

+				localKey, localValue, commonKey, commonKeyLocalValue),
+		},
+	}
+	agent := RunAgent(t, agentOptions)


this test will require specific agent version.

sharanyad · 2018-10-23T17:35:08Z

agent/functional_tests/tests/functionaltests_test.go

+func TestContainerInstanceTags(t *testing.T) {
+	// We need long container instance ARN for tagging APIs, PutAccountSettingInput
+	// will enable long container instance ARN.
+	putAccountSettingInput := ecsapi.PutAccountSettingInput{


Will customers have to manually enable this?

yes, it will be described in doc

agent/ec2/ec2_client.go

sharanyad

please add a comment in the functional test to change the required agent version during staging

petderek · 2018-10-23T23:28:26Z

agent/config/config_test.go

@@ -88,6 +88,7 @@ func TestEnvironmentConfig(t *testing.T) {
 	defer setTestEnv("ECS_NUM_IMAGES_DELETE_PER_CYCLE", "2")()
 	defer setTestEnv("ECS_IMAGE_PULL_BEHAVIOR", "always")()
 	defer setTestEnv("ECS_INSTANCE_ATTRIBUTES", "{\"my_attribute\": \"testing\"}")()
+	defer setTestEnv("ECS_CONTAINER_INSTANCE_TAGS", "{\"my_tag\": \"testing\"}")()


nit: Use string literals instead of escaping:

defer setTestEnv("ECS_CONTAINER_INSTANCE_TAGS", `{"my_tag": "testing"}`)()

agent/ec2/ec2_client.go

petderek · 2018-10-23T23:50:22Z

agent/ec2/mocks/ec2_mocks.go

 	ec2metadata "github.com/aws/aws-sdk-go/aws/ec2metadata"
+	ec20 "github.com/aws/aws-sdk-go/service/ec2"


its the worst!

Introduce ECS_CONTAINER_INSTANCE_TAGS environment variable and ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS to allow Agent retrieve tags locally and remotely.

haikuoliu added this to the 1.22.0 milestone Oct 12, 2018

haikuoliu force-pushed the tags branch from 8f54fc6 to b75237f Compare October 15, 2018 03:02

haikuoliu changed the title ~~ECS local tags support~~ ECS Agent tagging support Oct 15, 2018

haikuoliu force-pushed the tags branch from b75237f to ed4f271 Compare October 15, 2018 03:26

haikuoliu requested review from brycecarman and a team and removed request for brycecarman October 15, 2018 03:28

petderek reviewed Oct 15, 2018

View reviewed changes

yumex93 reviewed Oct 16, 2018

View reviewed changes

sharanyad reviewed Oct 16, 2018

View reviewed changes

haikuoliu force-pushed the tags branch from 213c903 to 640b025 Compare October 23, 2018 05:21

haikuoliu added a commit to haikuoliu/amazon-ecs-agent that referenced this pull request Oct 23, 2018

changelog for aws#1618

640b025

haikuoliu added the bot/test label Oct 23, 2018

sharanyad reviewed Oct 23, 2018

View reviewed changes

sharanyad approved these changes Oct 23, 2018

View reviewed changes

haikuoliu force-pushed the tags branch from c0c692b to e5bd40c Compare October 23, 2018 21:31

petderek approved these changes Oct 23, 2018

View reviewed changes

haikuoliu force-pushed the tags branch from e5bd40c to 0dd592f Compare October 24, 2018 00:37

haikuoliu added bot/test and removed bot/test labels Oct 25, 2018

haikuoliu force-pushed the tags branch from 0dd592f to af5ba9f Compare October 25, 2018 07:07

haikuoliu closed this Oct 25, 2018

haikuoliu force-pushed the tags branch from 8f7e122 to 0bdac27 Compare October 25, 2018 18:40

haikuoliu reopened this Oct 25, 2018

ECS tagging support

875216a

Introduce ECS_CONTAINER_INSTANCE_TAGS environment variable and ECS_CONTAINER_INSTANCE_PROPAGATE_TAGS to allow Agent retrieve tags locally and remotely.

Call DeleteAccountSetting for TestContainerInstanceTags

13245a4

haikuoliu force-pushed the tags branch from 8f7e122 to 13245a4 Compare October 25, 2018 21:36

haikuoliu merged commit a2a7d40 into aws:dev Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECS Agent tagging support #1618

ECS Agent tagging support #1618

haikuoliu commented Oct 12, 2018 •

edited

Loading

haikuoliu commented Oct 15, 2018

petderek left a comment

petderek Oct 15, 2018

haikuoliu Oct 16, 2018

petderek Oct 15, 2018

haikuoliu Oct 16, 2018

petderek Oct 15, 2018

haikuoliu Oct 16, 2018

petderek Oct 15, 2018

petderek Oct 15, 2018

yumex93 Oct 16, 2018

yumex93 Oct 16, 2018

haikuoliu Oct 16, 2018

petderek Oct 23, 2018

sharanyad Oct 16, 2018

haikuoliu Oct 16, 2018

sharanyad Oct 16, 2018

haikuoliu Oct 16, 2018

sharanyad Oct 17, 2018

haikuoliu Oct 17, 2018

haikuoliu commented Oct 18, 2018

haikuoliu commented Oct 23, 2018

sharanyad Oct 23, 2018

sharanyad Oct 23, 2018

haikuoliu Oct 23, 2018

sharanyad Oct 23, 2018

sharanyad Oct 23, 2018

sharanyad Oct 23, 2018

haikuoliu Oct 23, 2018

sharanyad left a comment

petderek Oct 23, 2018

petderek Oct 23, 2018

		@@ -592,6 +601,49 @@ func TestUserDataConfig(t *testing.T) {
		}
		}

		ec2metadata "github.com/aws/aws-sdk-go/aws/ec2metadata"
		ec20 "github.com/aws/aws-sdk-go/service/ec2"

ECS Agent tagging support #1618

ECS Agent tagging support #1618

Conversation

haikuoliu commented Oct 12, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

haikuoliu commented Oct 15, 2018

petderek 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

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

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

Choose a reason for hiding this comment

haikuoliu commented Oct 18, 2018

haikuoliu commented Oct 23, 2018

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

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haikuoliu commented Oct 12, 2018 •

edited

Loading