Create standalone docker file & allow it to be used for retrieving credentials manually #72
Conversation
Previous dockerfile (renamed to build.Dockerfile) allowed building the project within dockerfile. However, the generated image could not be uploaded to DockerHub. The new dockerfile generates a minimal alpine image which can be used directly. Also the produced binary is statically linked, so it can be copied into other docker images without issues.
Dockerfile
Outdated
|
|
||
| CMD make | ||
| FROM alpine:3.6 |
There was a problem hiding this comment.
Is there any particular reason you need a full Alpine container instead of a scratch container? The executable is statically-linked, so the only thing that should be necessary at that point is a CA certificate bundle rather than the rest of the Linux userland.
There was a problem hiding this comment.
Internally we used this container to run scripts within our CI so we found it useful to have basic UNIX utilities. Alpine overhead is around 4 MB which seemed reasonable to me. Also I am not sure how to just add the CA certificate bundle. If you have some pointers, I shall be happy to change this.
There was a problem hiding this comment.
I think my preference would be for a scratch container so that we don't need to worry about updating the base layer. You can see an example that builds a scratch container with a CA certificate bundle here.
There was a problem hiding this comment.
OK I shall update the Dockerfile to use scratch base.
| var user, token string | ||
| user, token, err = helper.Get(server) | ||
| if err == nil { | ||
| fmt.Printf("docker login -u %s -p %s %s\n", user, token, server) |
There was a problem hiding this comment.
I'm a little leery of doing this. Docker has already changed the arguments that it accepts for docker login; it used to be that --email was required, but now providing --email fails. You can see the challenge that we had with the AWS CLI and adapting it here: aws/aws-cli#1926.
There was a problem hiding this comment.
I know; the latest version of docker is recommending to pass the arguments over stdin using --password-stdin flag. It is very annoying that the CLI is not kept backwards compatible.
My primary use case was running eval $(docker run --rm -e AWS_REGION=us-east-1 dolbylabs/amazon-ecr-credential-helper) in our normal EC2 instances on startup to be able to login to ECR. Since we pull our images only at startup, this was sufficient.
The only other alternative is to vendor the moby/client and call RegistryLogin() function directly. I can make this change if you no not mind the extra vendor dependencies. I can still use the resulting image by bind mounting docker socket.
There was a problem hiding this comment.
I don't think that using moby/client/RegistryLogin() with the Docker socket will actually cause the ~/.docker/config.json file to be written since the Docker socket controls the Docker daemon and not the client.
I think your use-case makes sense, but I'm still concerned as docker login does change (and the change you mentioned I didn't even know about until you said it here!). Maybe instead of just calling it eval, we can encode the expected Docker CLI version in the name of the command somehow? Something like eval-1.11 so that it produces a docker login command without --email?
There was a problem hiding this comment.
I can change eval to eval-1.11 so it is more explicit. I can also add eval-17.06 with following code:
fmt.Printf("echo %s | docker login --password-stdin -u %s %s", token, user, server)
It will help to silence warnings from docker.
There was a problem hiding this comment.
The email flag for docker login has been optional since docker v1.4. So I do not think we need to worry about the email flag. I can add --password-stdin flag to eval command to handle the new way and add -e flag without options to output email.
There was a problem hiding this comment.
I don't know offhand, but I'm pretty sure email was still required as of Docker 1.9. I think I would prefer separate commands that correspond to the first version where the Docker CLI expected the invocation.
|
Apologies for the delay. Can you confirm that your contribution is under the terms of the Apache 2.0 license? |
|
I hereby confirm that all my contributions to this repository are under the terms of the Apache 2.0 license. |
Older docker versions require -e flag to be present and the latest docker version warns if password is not provided over stdin. This change allows any of these behaviours to be specified using the same command line flags.
There was a problem hiding this comment.
Thanks for sending this (and apologies for the delay in responding!). I think the code looks good, but I'd like @rpnguyen's comments as to whether we'd like to enable the docker login command line experience through the credential helper.
| } | ||
| var echoPassword, passwordOpt string | ||
| if passwordStdin { | ||
| echoPassword = fmt.Sprintf("echo %s|", token) |
There was a problem hiding this comment.
It might be better to do this as <<< %s so that echo's command line entry in /proc doesn't have the token.
There was a problem hiding this comment.
Fish does not support herestrings. So I am hesitant in making this change.
|
How about landing the standalone docker first, then workout the |
This patch changes the Dockerfile so a proper docker image can be created. The binary within generated docker image is statically linked so it can be used in other Linux based Dockerfiles.
I also added ability to emulate behavior of
aws ecr get-loginso this docker image can be used to pull from ECR when only Docker is available (e.g. CoreOS)