Add blackhole imds route for awsvpc network mode #968

aaithal · 2017-09-06T21:58:09Z

Summary

Add AWSVPCBlockInstanceMetdata config (ECS_AWSVPC_BLOCK_IMDS) for blocking access to instance metdata for awsvpc tasks

Implementation details

5e8001c agent/app: register task-eni-block-instance-metadata capability when AWSVPCBlockInstanceMetdata config is enabled
63734d3 agent/ecscni,agent/engine: wire in AWSVPCBlockInstanceMetdata config to CNI config
5f0e4ed agent/config: add AWSVPCBlockInstanceMetdata config for blocking access to instance metdata for awsvpc tasks

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

New tests cover the changes: Yes

Description for the changelog

None

Licensing

This contribution is under the terms of the Apache 2.0 License: Yes

…ss to instance metdata for awsvpc tasks

…to CNI config

…AWSVPCBlockInstanceMetdata config is enabled

nmeyerhans

I think there should be a commit to CHANGELOG.md as well.

aaithal · 2017-09-07T18:22:36Z

@nmeyerhans I decided not to do a changelog entry as I think this should be covered under 0e782a5. Do you think this warrants an entry by itself?

richardpen · 2017-09-07T18:31:47Z

agent/app/agent_capability.go

@@ -124,6 +126,14 @@ func (agent *ecsAgent) capabilities() []*ecs.Attribute {
 		capabilities = append(capabilities, taskENIVersionAttribute)
 	}

+	if agent.cfg.AWSVPCBlockInstanceMetdata {


You may want to move this above inside the check for TaskENIEnabled since this relies on the awsvpc network mode and cni plugins.

I thought about and ended up not doing it as it's essentially a nop to do this if Task ENI is not enabled. I'll change it if you insist.

Move this above makes the logic more clear to me, let's do it, thanks.

richardpen · 2017-09-07T18:34:20Z

README.md

@@ -177,6 +177,7 @@ configure them as something other than the defaults.
 | `ECS_INSTANCE_ATTRIBUTES` | `{"stack": "prod"}` | These attributes take effect only during initial registration. After the agent has joined an ECS cluster, use the PutAttributes API action to add additional attributes. For more information, see [Amazon ECS Container Agent Configuration](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-agent-config.html) in the Amazon ECS Developer Guide.| `{}` | `{}` |
 | `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | `false` |
 | `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | `/amazon-ecs-cni-plugins` |
+| `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to Instance Metdata for Tasks started with `awsvpc` network mode | `false` | `false`|


Do you we need to add a link to the instance metadata here, I'm not sure if all the customers know Instance Metadata?

Sure, I can do that.

richardpen · 2017-09-07T18:35:28Z

agent/config/config_test.go

@@ -380,3 +380,11 @@ func TestImageCleanupMinimumNumImagesToDeletePerCycle(t *testing.T) {
 		t.Errorf("Wrong value for NumImagesToDeletePerCycle: %v", cfg.NumImagesToDeletePerCycle)
 	}
 }
+
+func TestAWSVPCBlockInstanceMetadata(t *testing.T) {


Please add a simple comment here?

Isn't the name itself descriptive?

Only if I know that this test was in the config package, it will have different meaning in engine package, where the test may be testing the process to block the metadata. Ignore my comments if we are supposed to have the context when reading the code.

…ty if 'task-eni' is not set

nmeyerhans

LGTM. Though it'd make me really happy to see more detailed commit messages. https://chris.beams.io/posts/git-commit/ has some examples of good commit messages and explains why they matter.

aaithal · 2017-09-07T19:18:03Z

@nmeyerhans that'd make me happy as well. I thought the changes here were small enough and didn't need additional context. Let me know if you had any particular commit(s) in mind and I can add some context.

nmeyerhans · 2017-09-07T20:26:19Z

One thing that'd help the commit messages here is to ensure that the "subject" (first line of the message) is no longer than about 50 characters. When displaying a commit log, Github truncates at 70 characters, so there's a little room to be flexible here, but most of the commits in this PR don't even fit within the 70 character limit. Any context or details that don't fit within ~50 characters should be expanded upon in the "body" of the commit message.

Note for others who might be reading: Do as I say, not as I do. I've certainly pushed some poor commit messages in the past. More recently, though, we've had internal conversations about improving our commit logs, and I'm trying to drive that effort forward...

aaithal added 2 commits September 6, 2017 14:40

agent/config: add AWSVPCBlockInstanceMetdata config for blocking acce…

5f0e4ed

…ss to instance metdata for awsvpc tasks

agent/ecscni,agent/engine: wire in AWSVPCBlockInstanceMetdata config …

63734d3

…to CNI config

aaithal requested review from nmeyerhans, samuelkarp, vsiddharth and richardpen September 6, 2017 21:58

aaithal force-pushed the addBlackholeIMDSRoute branch from 1274e8f to d6a614f Compare September 6, 2017 23:32

aaithal added 2 commits September 7, 2017 08:25

agent/app: register task-eni-block-instance-metadata capability when …

654e8bf

…AWSVPCBlockInstanceMetdata config is enabled

README: add documentation for ECS_AWSVPC_BLOCK_IMDS

820d1bd

aaithal force-pushed the addBlackholeIMDSRoute branch from d6a614f to 820d1bd Compare September 7, 2017 15:26

samuelkarp approved these changes Sep 7, 2017

View reviewed changes

nmeyerhans reviewed Sep 7, 2017

View reviewed changes

richardpen reviewed Sep 7, 2017

View reviewed changes

agent/app: Don't register 'task-eni-block-instance-metadata' capabili…

7b85734

…ty if 'task-eni' is not set

nmeyerhans approved these changes Sep 7, 2017

View reviewed changes

richardpen approved these changes Sep 7, 2017

View reviewed changes

aaithal merged commit 5cc2efa into aws:dev Sep 7, 2017

aaithal mentioned this pull request Oct 24, 2017

Fix flaky test TestGetTaskByArn and TestMetadataFileUpdatedAgentRestart #1030

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add blackhole imds route for awsvpc network mode #968

Add blackhole imds route for awsvpc network mode #968

aaithal commented Sep 6, 2017 •

edited

Loading

nmeyerhans left a comment

aaithal commented Sep 7, 2017

richardpen Sep 7, 2017

aaithal Sep 7, 2017

richardpen Sep 7, 2017

richardpen Sep 7, 2017

aaithal Sep 7, 2017

richardpen Sep 7, 2017

aaithal Sep 7, 2017

richardpen Sep 7, 2017

nmeyerhans left a comment

aaithal commented Sep 7, 2017 •

edited

Loading

nmeyerhans commented Sep 7, 2017

Add blackhole imds route for awsvpc network mode #968

Add blackhole imds route for awsvpc network mode #968

Conversation

aaithal commented Sep 6, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

nmeyerhans left a comment

Choose a reason for hiding this comment

aaithal commented Sep 7, 2017

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

nmeyerhans left a comment

Choose a reason for hiding this comment

aaithal commented Sep 7, 2017 • edited Loading

nmeyerhans commented Sep 7, 2017

aaithal commented Sep 6, 2017 •

edited

Loading

aaithal commented Sep 7, 2017 •

edited

Loading