Passing Host IPv4 address to container metadata file #1730

linkar-ec2 · 2018-12-07T19:55:27Z

Summary

Adds Host EC2 instance Public IPv4 address to container metadata file.
Customer ask: GitHub issue

Implementation details

Added EC2 api call to describe instance and retrieve public ipv4 address at Agent startup

Testing

Added unit tests for adding IP address to file. Manually tested as well.

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:

Description for the changelog

Saving Host IPv4 address in container metadata file

Licensing

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

adnxn

lgtm, minor comment about len checks for ec2 api call

adnxn · 2018-12-07T20:35:10Z

agent/ec2/ec2_client.go

+		seelog.Criticalf("Error calling DescribeInstances API: %v", err)
+		return "", err
+	}
+	if len(describeInstancesOutput.Reservations) == 0 || len(describeInstancesOutput.Reservations[0].Instances) == 0 {


Only 1 address is expected., if only 1 is expected. should we explicitly check for 1 for each? am i missing something?

Because we only ever pass in a single instance ID, I can't imagine a scenario in which we get more than 1 instance back. This check is to ensure we don't run into a nil pointer exception

agent/app/agent.go

agent/ec2/ec2_client.go

samuelkarp

Thanks for addressing my concern!

I think it would be a good idea to write a functional test for this as well.

samuelkarp · 2018-12-07T22:32:53Z

agent/app/agent.go

+	// Get instance ID from ec2 metadata client.
+	hostPublicIPv4Address, err := agent.ec2MetadataClient.PublicIPv4Address()
+	if err != nil {
+		seelog.Criticalf("Unable to retrieve Host Instance PublicIPv4 Address: %v", err)


I'd recommend having this as Error, not Critical.

Concern addressed

sharanyad

code lgtm. do we need to add the field to state file/bump version?

linkar-ec2 · 2018-12-08T01:12:12Z

@sharanyad We aren't saving the Host IP to the state file, just metadata manager. I don't believe we need to bump it.

sharanyad · 2018-12-10T23:50:52Z

...ests/testdata/taskdefinitions/container-metadata-file-validator-windows/task-definition.json

+      "name": "container-metadata-file-validator",
+      "memory": 512,
+      "cpu": 1024,
+      "healthCheck": {


Do we need health check for this test?

Other tests that run a custom image use the healthCheck (like v3-task-endpoint-validator). Since this test runs for ~15 seconds, doesn't hurt to have a healthCheck every 5 secs.

sharanyad · 2018-12-10T23:52:43Z

...ntainer-metadata-file-validator-windows/container-metadata-file-validator-windows.dockerfile

+# express or implied. See the License for the specific language governing
+# permissions and limitations under the License.
+
+FROM microsoft/windowsservercore:latest


Can we use microsoft/nanoserver for this?

Yes! Decreases the image size from 11gb to 1.1gb!

sharanyad · 2018-12-10T23:53:42Z

agent/functional_tests/tests/functionaltests_test.go

+
+	agent := RunAgent(t, agentOptions)
+	defer agent.Cleanup()
+	agent.RequireVersion(">=1.15.0")


since this test also checks for HostIPv4 field, shouldn't the agent version required be the most recent one?

Good catch. I think we should move away from requiring a version and represent these "requirements" as capabilities. For functional tests, all tests should run against the most up to date version of Agent.

sharanyad · 2018-12-10T23:55:52Z

misc/container-metadata-file-validator/container-metadata-file-validator.go

@@ -0,0 +1,225 @@
+// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


looks like this file is essentially the same as misc/container-metadata-file-validator-windows/container-metadata-file-validator-windows.go . is it possible to reuse?

There is different validation for network on linux, which is more detailed. The 2 files approach is taken from the v3-task-metadata-validator. The files are built/managed differently. In addition, the task definitions themselves are a bit different (with windows including "windows" identifiers)

sharanyad · 2018-12-13T18:59:01Z

agent/app/agent_test.go

+	ec2MetadataClient.EXPECT().PublicIPv4Address().Return(hostPublicIPv4Address, nil)
+
+	hostPublicIPv4AddressOutput := agent.getHostPublicIPv4AddressFromEC2Metadata()
+	assert.Equal(t, hostPublicIPv4Address, hostPublicIPv4AddressOutput)


nit:
this can just be
assert.Equal(t, hostPublicIPv4Address, agent.getHostPublicIPv4AddressFromEC2Metadata())

Changed, along with the test below.

sharanyad · 2018-12-13T19:10:02Z

agent/functional_tests/tests/functionaltests_test.go

+		},
+	}
+
+	agent := RunAgent(t, agentOptions)


I think we would still require the right agent version here, since functional tests can be run for different agent images.

Gotcha. Fixed it to remain consistent with other tests.

sharanyad

🚢

Related issue: aws#1575 Related PR: aws#1730 PR 1730 adds the Public IP Address of the host to the container metadata file, however the EC2 host may be configured without a public address. In this case, the EC2 metadata API returns a 404 response, and the host IP is not available to containers. Example ECS Agent Log ``` [ERROR] Unable to retrieve Host Instance PublicIPv4 Address: EC2MetadataError: failed to make EC2Metadata request caused by: <?xml version="1.0" encoding="iso-8859-1"?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> <head> <title>404 - Not Found</title> </head> <body> <h1>404 - Not Found</h1> </body> </html> ``` This commit adds an extra field to the container metadata json, `HostPrivateIPv4Address` which is available on EC2 hosts without a public address.

Related issue: #1575 Related PR: #1730 PR 1730 adds the Public IP Address of the host to the container metadata file, however the EC2 host may be configured without a public address. In this case, the EC2 metadata API returns a 404 response, and the host IP is not available to containers. Example ECS Agent Log ``` [ERROR] Unable to retrieve Host Instance PublicIPv4 Address: EC2MetadataError: failed to make EC2Metadata request caused by: <?xml version="1.0" encoding="iso-8859-1"?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> <head> <title>404 - Not Found</title> </head> <body> <h1>404 - Not Found</h1> </body> </html> ``` This commit adds an extra field to the container metadata json, `HostPrivateIPv4Address` which is available on EC2 hosts without a public address.

linkar-ec2 requested a review from a team December 7, 2018 19:55

adnxn reviewed Dec 7, 2018

View reviewed changes

samuelkarp previously requested changes Dec 7, 2018

View reviewed changes

agent/app/agent.go Outdated Show resolved Hide resolved

agent/ec2/ec2_client.go Outdated Show resolved Hide resolved

samuelkarp reviewed Dec 7, 2018

View reviewed changes

sharanyad reviewed Dec 8, 2018

View reviewed changes

linkar-ec2 force-pushed the ip-address-metadata branch from 11297d0 to 4c4a1aa Compare December 10, 2018 23:03

sharanyad reviewed Dec 10, 2018

View reviewed changes

adnxn approved these changes Dec 13, 2018

View reviewed changes

linkar-ec2 force-pushed the ip-address-metadata branch from 99c532f to b5eca96 Compare December 13, 2018 01:29

sharanyad reviewed Dec 13, 2018

View reviewed changes

linkar-ec2 force-pushed the ip-address-metadata branch from b5eca96 to bba72e6 Compare December 13, 2018 19:21

Passing Host IPv4 address to container metadata file

0f916d9

linkar-ec2 force-pushed the ip-address-metadata branch from bba72e6 to 0f916d9 Compare December 13, 2018 19:35

sharanyad approved these changes Dec 13, 2018

View reviewed changes

linkar-ec2 added bot/test and removed bot/test labels Dec 13, 2018

linkar-ec2 merged commit 5c78ea1 into aws:dev Dec 13, 2018

yumex93 added this to the 1.24.0 milestone Jan 3, 2019

adnxn mentioned this pull request Jan 7, 2019

Feature request for container meta data file to include ECS host instance IP address #1575

Closed

bencord0 mentioned this pull request Apr 23, 2019

Add Private Host IPv4 address to container metadata #2000

Closed

yhlee-aws mentioned this pull request May 8, 2019

Add Private Host IPv4 address to container metadata #2025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passing Host IPv4 address to container metadata file #1730

Passing Host IPv4 address to container metadata file #1730

linkar-ec2 commented Dec 7, 2018 •

edited

Loading

adnxn left a comment

adnxn Dec 7, 2018

linkar-ec2 Dec 7, 2018

samuelkarp left a comment

samuelkarp Dec 7, 2018

linkar-ec2 Dec 7, 2018

sharanyad left a comment

linkar-ec2 commented Dec 8, 2018

sharanyad Dec 10, 2018

linkar-ec2 Dec 13, 2018

sharanyad Dec 10, 2018

linkar-ec2 Dec 11, 2018

sharanyad Dec 10, 2018

linkar-ec2 Dec 11, 2018

sharanyad Dec 10, 2018

linkar-ec2 Dec 11, 2018

sharanyad Dec 13, 2018

linkar-ec2 Dec 13, 2018

sharanyad Dec 13, 2018

linkar-ec2 Dec 13, 2018

sharanyad left a comment

		@@ -0,0 +1,225 @@
		// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Passing Host IPv4 address to container metadata file #1730

Passing Host IPv4 address to container metadata file #1730

Conversation

linkar-ec2 commented Dec 7, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

adnxn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelkarp left a 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

linkar-ec2 commented Dec 8, 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

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

linkar-ec2 commented Dec 7, 2018 •

edited

Loading