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

Jump to bottom

Add Private Host IPv4 address to container metadata #2000

Closed
wants to merge 1 commit into from
Closed

Add Private Host IPv4 address to container metadata #2000

wants to merge 1 commit into from

Conversation

bencord0
Copy link

Summary

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.

Implementation details

This commit mostly follows the implementation from #1730 but adds an extra field for the private IP. If the address is not available in the ec2 metadata, then the field is omitted in the container metadata json.

Testing

I've added tests for this field. Mostly they are copies of the existing public IP tests, with sed replacements.

make test-in-docker

I have also tested this with a running ECS instance based on Amazon Linux 2.

# on development machine
make docker
docker tag amazon/amazon-ecs-agent:make amazon/amazon-ecs-agent:latest
docker save amazon/amazon-ecs-agent:latest | xz > ecs-agent.tar.xz

scp ecs-agent.tar.xz <ecs host>:/tmp/ecs-agent.tar.xz

# on ECS instance
docker load < /tmp/ecs-agent.tar.xz
systemctl restart ecs

New tests cover the changes: yes

Description for the changelog

Feature - Add Host EC2 instance Private IPv4 address to 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.

@bencord0 bencord0 force-pushed the privateip_in_container_metadata branch from 3a3ce18 to e2918c2 Compare April 23, 2019 14:41
@bencord0 bencord0 changed the base branch from master to dev April 23, 2019 14:41
@bencord0 bencord0 force-pushed the privateip_in_container_metadata branch from e2918c2 to 8cc3d10 Compare April 23, 2019 15:27
@adnxn adnxn added the bot/test label Apr 25, 2019
@yhlee-aws
Copy link
Contributor

@bencord0 Could you please rebase from dev and update this PR? some test related fixes were made after this PR was open, and that should fix the failing windows functional test.

adnxn
adnxn reviewed Apr 26, 2019
View reviewed changes
Copy link
Contributor

@adnxn adnxn left a comment

Choose a reason for hiding this comment

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

changes looks fine to me. were the *_mock.go changes generated with make gogenerate?

thanks for the thorough testing with a dev build of the agent 🙏

agent/containermetadata/manager_test.go Outdated
_, _, _, _, done := managerSetup(t)
defer done()
newManager := &metadataManager{}
newManager.SetHostPrivateIPv4Address(hostPublicIPv4Address)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think you meant to put hostPrivateIPv4Address. looks like the assert statement just works cause the values are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

fenxiong
fenxiong approved these changes Apr 26, 2019
View reviewed changes
Copy link
Contributor

@fenxiong fenxiong left a comment

Choose a reason for hiding this comment

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

lgtm

agent/containermetadata/manager_test.go Outdated
_, _, _, _, done := managerSetup(t)
defer done()
newManager := &metadataManager{}
newManager.SetHostPrivateIPv4Address(hostPublicIPv4Address)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Add Private Host IPv4 address to container metadata
58da9df 
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.
@bencord0 bencord0 force-pushed the privateip_in_container_metadata branch from 8cc3d10 to 58da9df Compare April 29, 2019 08:31
@bencord0
Copy link
Author

@yunhee-l I've now rebased ontop of latest dev.

@adnxn I had trouble running the make gogenerate script, the mock stubs were copied and sed in-place. I don't know if rerunning go generate has different output.

Also, typo in manager_test.go is fixed.

@yhlee-aws
Copy link
Contributor

This requires another rebase on dev branch to push. I'm looking into pulling this PR myself, will update.

@yhlee-aws yhlee-aws mentioned this pull request May 8, 2019
Merged
@yhlee-aws
Copy link
Contributor

#2025 opened, pulling from this PR. Closing this in favor of the new PR.

@yhlee-aws yhlee-aws closed this May 8, 2019
@fenxiong fenxiong mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adnxn adnxn adnxn left review comments

@fenxiong fenxiong fenxiong approved these changes

@yhlee-aws yhlee-aws yhlee-aws approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bencord0 @yhlee-aws @adnxn @fenxiong @amazon-ecs-bot