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

Add a Dockerfile. #143

Merged
merged 3 commits into from
Jul 3, 2018
Merged

Add a Dockerfile. #143

merged 3 commits into from
Jul 3, 2018

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Jul 1, 2018

This PR adds a simple Dockerfile.

In the case of running tests for a client with Docker, it avoids having to build the image during the execution of the tests or maintain a fork to build an image for the Docker Hub.

If this PR interests you, could you activate the automatic builds from the Docker Hub so that the image is built automatically? (this would allow to easily run the tests with a Pebble always up to date)

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

This looks great @idez, thanks! I think it would be reasonable to turn on the automatic Dockerhub builds. 👍

My Docker/container fluency is pretty low, can you provide a usage example for the README? Do you need to do anything to expose the Pebble WFE port to the host machine or other containers? Is it possible to provide command line arguments (-strict, -dnsserver, etc) at runtime or does that have to be factored into the Dockerfile?

COPY --from=builder /go/bin/pebble /usr/bin/pebble
COPY --from=builder /go/src/github.com/letsencrypt/pebble/test/ /test/

ENTRYPOINT [ "/usr/bin/pebble" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a --config /test/config.json no? Otherwise I think pebble will default to trying to read from test/config/pebble-config.json in the pwd.

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 keep the behavior of Pebble, the default config file path is test/config/pebble-config.json
I copy this file and all the content of /test/ inside the container:

COPY --from=builder /go/src/github.com/letsencrypt/pebble/test/ /test/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, and PWD == / in this case so everything works 👍

Thanks for clarifying.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be CMD rather than ENTRYPOINT. The difference: If you pass an alternate command to docker run, it gets passed as an argument to ENTRYPOINT, but it overrides CMD. So for instance if you wanted to run bash in the context of this container it would not work right due to the ENTRYPOINT.

RUN go get -u github.com/jmhodges/clock \
&& go get -u gopkg.in/square/go-jose.v2

RUN go install ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make the Builder script also run the unit tests? Is that an anti-pattern?

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 purpose of a Dockerfile is not to replace a CI.
A Dockerfile must contain only what allows to build.

@cpu
Copy link
Contributor

cpu commented Jul 1, 2018

@shred 👋 Would you be interested in taking a quick 👀 at this PR? Would it help you remove any of the Pebble Docker definitions in the ACME4J pom.xml? https://github.com/shred/acme4j/blob/b9aeb816152d98752cf9f9be15be857865dda1f7/acme4j-it/pom.xml#L120-L170

@ldez
Copy link
Contributor Author

ldez commented Jul 1, 2018

Here is a small example with a docker-compose file:

pebble:
  image: letsencrypt/pebble
  command: -dnsserver ${CUSTOM_DNS_IP}:5053
  ports:
    - 14000:14000
  environment:
    - PEBBLE_VA_NOSLEEP=1
    - PEBBLE_WFE_NONCEREJECT=30

And with a Docker command:

docker run -e "PEBBLE_VA_NOSLEEP=1"  letsencrypt/pebble -dnsserver 10.10.10.10:5053

@ldez
Copy link
Contributor Author

ldez commented Jul 2, 2018

Documentation added in 3691aa9

@shred
Copy link
Contributor

shred commented Jul 2, 2018

@cpu Thanks for the pointer. This PR looks good, and I could use an automated build of Pebble. But I prefer to keep the current way for now, because with a few modifications to the pom file, I can also use my own Pebble fork for tests.

@cpu
Copy link
Contributor

cpu commented Jul 3, 2018

This PR looks good, and I could use an automated build of Pebble. But I prefer to keep the current way for now, because with a few modifications to the pom file, I can also use my own Pebble fork for tests.

@shred Thanks for taking a look, appreciated! Totally understand the desire to keep your setup as-is. If its working don't break it :-) You were just the first person I thought of that I knew was using Pebble with Docker and wanted to see if this PR made sense to you. Thanks!

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thank you @ldez - the README update is excellent. Much appreciated!

@cpu cpu merged commit 95cc672 into letsencrypt:master Jul 3, 2018
@ldez ldez deleted the feature/dockerfile branch July 3, 2018 14:32
@cpu
Copy link
Contributor

cpu commented Jul 3, 2018

@ldez I will try to activate the automatic Dockerhub builds soon. I created a repository (https://hub.docker.com/r/letsencrypt/pebble/) but haven't been able to create an automatic build yet. I'm having a hard time getting the UI to let me create a build for a repo in the Let's Encrypt GH organization even though I'm a member.

COPY --from=builder /go/bin/pebble /usr/bin/pebble
COPY --from=builder /go/src/github.com/letsencrypt/pebble/test/ /test/

ENTRYPOINT [ "/usr/bin/pebble" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be CMD rather than ENTRYPOINT. The difference: If you pass an alternate command to docker run, it gets passed as an argument to ENTRYPOINT, but it overrides CMD. So for instance if you wanted to run bash in the context of this container it would not work right due to the ENTRYPOINT.

WORKDIR /go/src/github.com/letsencrypt/pebble
COPY . .

RUN go get -u github.com/jmhodges/clock \
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the line below can be condensed into go get ./..., since go get fetches dependencies recursively.

FROM golang:1.10-alpine as builder

RUN apk --update upgrade \
&& apk --no-cache --no-progress add git mercurial bash gcc musl-dev curl tar \
Copy link
Contributor

Choose a reason for hiding this comment

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

What are mercurial, musl-dev, and tar used for?

@felixfontein
Copy link
Contributor

@cpu Are you planning to use tags for versioning, so it is possible to access a specific version (that would be a git commit ID, I guess) of Pebble?

@cpu
Copy link
Contributor

cpu commented Jul 16, 2018

@cpu Are you planning to use tags for versioning, so it is possible to access a specific version (that would be a git commit ID, I guess) of Pebble?

@felixfontein Do you want to start an issue to sort this out? I haven't given it a lot of thought and would be interested in discussing options.

@felixfontein
Copy link
Contributor

@cpu I've created a new issue: #146

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.

None yet

5 participants