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.

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

Run docker as buildkite agent with userns-remap #341

Merged
merged 8 commits into from
Sep 20, 2017

Conversation

lox
Copy link
Contributor

@lox lox commented Sep 8, 2017

This uses the --userns-remap feature in recent docker daemons to remap the uid of root in docker containers to the buildkite-agent users on the host.

@lox lox requested a review from toolmantim September 8, 2017 07:21
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

I don't know enough to really approve this. I'll let you merge as you need, or get someone else to put some eyeballs on it.

@lox lox force-pushed the run-docker-as-buildkite-agent branch from 039285c to 908a1ea Compare September 13, 2017 03:53
@lox lox requested a review from vektah September 13, 2017 05:27
@lox
Copy link
Contributor Author

lox commented Sep 13, 2017

I'm going to merge this after I put out a point release. I'm thinking of doing it as 2.3.0 @toolmantim, feels?

@toolmantim
Copy link
Contributor

The release plan sounds good, testing aside!

@lox
Copy link
Contributor Author

lox commented Sep 13, 2017

Possibly an rc1 so we can test if before it hits 2.3.0.

@vektah
Copy link
Contributor

vektah commented Sep 13, 2017

What happens to other user ids in the container? eg nobody writing doctrine cache files out to a volume mount? I think will land in following user ids so there will be stuff owned by 501+$uid in that directory, so you probably still need the permission fixing scripts. but I'm not sure how mapping that to a range of 1 works. maybe it just flat out breaks?

👍 for an RC, this is a whole lot of change.

The list of breaks from #32:

  • Using --privileged mode flag on docker run
  • sharing PID or NET namespaces with the host (--pid=host or --net=host)
  • sharing a network namespace with an existing container (--net=container:other)
  • sharing an IPC namespace with an existing container (--ipc=container:other)
  • A --readonly container filesystem (this is a Linux kernel restriction against remounting with modified flags of a currently mounted filesystem when inside a user namespace)
  • external (volume or graph) drivers which are unaware/incapable of using daemon user mappings

@lox
Copy link
Contributor Author

lox commented Sep 13, 2017

Yeah, that is a pretty epic list huh.

@lox
Copy link
Contributor Author

lox commented Sep 13, 2017

I might try and make this opt-in.

@toolmantim
Copy link
Contributor

Maybe a v3.0 milestone feature?

@lox lox force-pushed the run-docker-as-buildkite-agent branch from 0886af6 to 22e5929 Compare September 15, 2017 06:38
@@ -237,6 +237,14 @@ Parameters:
- false
Default: "true"

EnableDockerUserNamespaceRemap:
Type: String
Description: Enables experimental feature to run docker as buildkite-agent
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we let this feature be turned on using an ENV var?

If we do add it as another option on the stack setup screen, might need to make it clear that it's not "buildkite-agent" the binary we're taking about here in the description?

If it's a "this could be buggy, so you better know what you're doing" then perhaps we just say:

Enables experimental Docker userns-remap support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that it needs to have the system docker daemon started with the setting

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah of course! 🙃

@lox
Copy link
Contributor Author

lox commented Sep 15, 2017

If this is an optional setting, are you ok with a 2.3.0 release @toolmantim?

@lox lox merged commit be5cafe into master Sep 20, 2017
@lox lox deleted the run-docker-as-buildkite-agent branch September 20, 2017 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants