Skip to content
This repository has been archived by the owner on Sep 13, 2024. It is now read-only.

Start the ECS Agent with --userns host #73

Closed
wants to merge 2 commits into from
Closed

Start the ECS Agent with --userns host #73

wants to merge 2 commits into from

Conversation

rickard-von-essen
Copy link
Contributor

@rickard-von-essen rickard-von-essen commented Nov 17, 2016 •
edited
Loading

If you start your docker daemon with --userns-remap=<val> docker will map users inside containers to other uids on the host. Most notably this makes uid 0 inside a container != uid 0 on the host.

ECS agent needs access to the host network but you can't set --network host when you have userns-remap. By setting --userns host the ECS agent is started without userns remapping. This flag was added in Docker 1.11.

This required a update of the go-dockerclient.

@richardpen richardpen mentioned this pull request Jan 16, 2017
Closed
@rickard-von-essen
Copy link
Contributor Author

Rebased...

Some feedback would be appreciated...

@samuelkarp
Copy link
Contributor

@rickard-von-essen Apologies for the extremely delayed response.

Can you change go-dockerclient to be the most recent version instead of the first version with support for UsernsMode? That version vendors dependencies with import path rewriting, which we'd like to avoid if possible.

@rickard-von-essen
Copy link
Contributor Author

@samuelkarp no problem, I'll update this tomorrow.

@rickard-von-essen
Copy link
Contributor Author

The most recent go-dockerclient requires go 1.7 because of the use of context. Should I still go ahead with that update?

@samuelkarp
Copy link
Contributor

@rickard-von-essen Thanks. We need to keep ecs-init on Go 1.6 for now, so we'll need to still use golang.org/x/net/context instead of context. Can you pick the most recent version that still uses golang.org/x/net/context?

@rickard-von-essen
Copy link
Contributor Author

Can you pick the most recent version that still uses golang.org/x/net/context

Yes, brb

@rickard-von-essen
Copy link
Contributor Author

Last version that doesn't depend on context seems to be fsouza/go-dockerclient@c342fd4. I'll update to that.

rickard-von-essen
rickard-von-essen commented Feb 15, 2017
View reviewed changes
ecs-init/docker/docker.go Outdated
@@ -94,7 +96,7 @@ func (c *Client) IsAgentImageLoaded() (bool, error) {

// LoadImage loads an io.Reader into Docker
func (c *Client) LoadImage(image io.Reader) error {
return c.docker.LoadImage(godocker.LoadImageOptions{image})
return c.docker.LoadImage(godocker.LoadImageOptions{image, context.Background()})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put context.Background() here or do you want context.TODO() instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

context.TODO() seems more appropriate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@rickard-von-essen rickard-von-essen force-pushed the userns-mode-host branch 2 times, most recently from a070c48 to 5e50777 Compare February 16, 2017 09:26
samuelkarp
samuelkarp reviewed Feb 21, 2017
View reviewed changes
Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Code looks pretty good, just one minor change and we need to test this across different Docker versions and configurations before merging.

ecs-init/docker/docker_test.go Outdated
@@ -94,7 +96,7 @@ func TestLoadImage(t *testing.T) {

mockDocker := NewMockdockerclient(mockCtrl)

mockDocker.EXPECT().LoadImage(godocker.LoadImageOptions{nil})
mockDocker.EXPECT().LoadImage(godocker.LoadImageOptions{nil, context.Background()})
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this should expect either context.TODO() or gomock.Any() instead of context.Background().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@rickard-von-essen
Updated go-dockerclient to the last that works with Go 1.6
69da83b 
This add support for UsernsMode but doesn't use "context" introduced in
Go 1.7 .

github.com/fsouza/go-dockerclient => c342fd4c3d69bdc9767083cc61d11756e7323b64
@rickard-von-essen
Support docker running with userns-remp
8d70d2f
@samuelkarp
Copy link
Contributor

Code looks good, we just need to test this now.

@CorbyAsCode
Copy link

When is this scheduled to be released as an RPM? It looks like docker-1.12.6-1.17.amzn1.x86_64 and ecs-init-1.14.0-2.amzn1.x86_64 are the latest.

@samuelkarp
Copy link
Contributor

Just to give an update here: I tested this with Docker 1.12.6 on Amazon Linux and saw that it was working correctly (inspecting showed me "UsernsMode": "host"). We still need to test this against older versions of Docker (back to 1.6.0, since that's the oldest supported in our spec file right now).

@rickard-von-essen rickard-von-essen mentioned this pull request Apr 11, 2017
Open
@nmeyerhans
Copy link
Contributor

Merged. Thank you @rickard-von-essen!

@nmeyerhans nmeyerhans closed this May 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samuelkarp samuelkarp samuelkarp 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.
4 participants
@rickard-von-essen @samuelkarp @CorbyAsCode @nmeyerhans