acme-dns icon indicating copy to clipboard operation
acme-dns copied to clipboard

small Dockerfile improvements

Open schue30 opened this issue 6 years ago • 14 comments

This PR improves the following points in the Dockerfile:

  • build exact git tag instead of the current master
  • update of the current golang build environment from 1.9.2 to 1.11.4
  • use scratch instead of an alpine image
  • run container as user 1000 (instead of root)
  • change listening ports from system ports 53, 80, 443 to 5353, 8080, 8443

WARNING - this PR contains breaking changes in the Dockerfile The container that is started from the image that is built from this Dockerfile is running as user 1000 instead of root (0). As a non root user cannot (per default) listen on system ports (<1024), I had to change them to ones that are higher than 1024. In this case 5353, 8080. Updates from older versions should not be a problem, as long as the user 1000 or the group 0 has read permissions on the config file, write permissions on the sqlite db file and the listening ports are higher than 1024.

schue30 avatar Dec 25 '18 21:12 schue30

Coverage Status

Coverage remained the same at 93.831% when pulling 6ada9dd5b92747148d3d196641f3cd66ef3be880 on schue30:master into e1f1d6af34f82b3b75021883a02f72db9d9d41b5 on joohoi:master.

coveralls avatar Dec 25 '18 21:12 coveralls

Thanks for the PR! This looks like a good update, I'm just slightly hesitant to merge it straight away because of the backwards breaking changes. I'll test it throughly in the near future to be able to figure out a good way to inform the current users about the changes they need to make.

joohoi avatar Jan 20 '19 16:01 joohoi

FYI, this PR should also include changes to docker-compose.yml (at a minimum I believe the port mappings need to be updated).

Ajedi32 avatar Jan 21 '19 18:01 Ajedi32

Also, this fixes #79

Ajedi32 avatar Jan 21 '19 18:01 Ajedi32

FYI, this PR should also include changes to docker-compose.yml (at a minimum I believe the port mappings need to be updated).

Yes, for sure. I will add the change for the docker-compose.yml.

schue30 avatar Jan 23 '19 14:01 schue30

Ok, this PR looks good. I had to pick the Go base image line from this PR for the work done in #147 so there's a merge conflict for that line, I'm sorry.

In addition to the merge conflict resolution, I think we should update the README to point to the new port configuration in: https://github.com/joohoi/acme-dns#using-docker

joohoi avatar Feb 04 '19 07:02 joohoi

Ping @schue30 are you interested to continue working on this PR or can someone else continue from here on? I would love to hear your opinion especially about the volume change, I'm not a Docker expert by any means myself.

joohoi avatar Feb 22 '19 14:02 joohoi

Pong @joohoi

schue30 avatar Mar 04 '19 21:03 schue30

Thanks for all the work you've put into this @schue30 and @tcely ! Looks robust. I'm still wondering how to notify the current Docker users of acme-dns about the breaking changes. What's the usual process?

joohoi avatar Mar 19 '19 18:03 joohoi

Regarding communication, normally I'd suggest a major version bump as a way of denoting that the changes could break stuff. In this case though it doesn't seem like there are any changes to acme-dns itself, just to the docker container. So I guess you could just bump the docker container version, but then it'd be out of sync with the acme-dns version. Hmm...

Ajedi32 avatar Mar 19 '19 20:03 Ajedi32

Thanks for all the work you've put into this @schue30 and @tcely ! Looks robust. I'm still wondering how to notify the current Docker users of acme-dns about the breaking changes. What's the usual process?

I'm not sure we can do any better than a note at the top of the README.md file.

tcely avatar Mar 19 '19 20:03 tcely

I see that breaking changes stop this PR from being merged. There are ways to accomplish nearly the same without breaking changes.

  1. File permissions problem: Start container as root with a new entrypoint script. The new entrypoint script ensures correct file permissions on every startup and then drops root with e.g. gosu. This is common practice in several Docker images.
  2. Port problem: use old ports (80, etc) and add container capability NET_BIND_SERVICE so non-root processes can open lower ports in this container. (As explained in Docker security documentation)

// edit: fixed broken link.

zokradonh avatar Sep 29 '19 15:09 zokradonh

Hey folks, i've just implemented my self-hosted acme-dns instance into production using the modified version of the Dockerfile: https://github.com/joohoi/acme-dns/commit/43c4f1bef8a695ffa7686730dbff9bfa44fd9189

I don't want to create a PR yet as i see you are trying to solve similar docker image improvements here. My proposal is to use a multi stage docker build using the scratch as a final image. This has following features:

  • resulting image contains only the acme-dns binary
  • much more secure and decreased attack surface by using the scratch image
  • backward compatible with the old image

This solves all the previous implementation discussions that should be rather solved by outside of the project:

  • USER/uid discussion - you don't need to solve this for scratch images
  • EXPOSE/VOLUME - those are exposed/mapped overriden at the container runtime and are implementation-specific. In the Dockerfile they have practically only informational purpose.
  • config shouldn't be included in the docker image as the default one is not possible to use for the real implementation either

I think this can significantly your whole workflow.

sbocinec avatar Nov 08 '19 18:11 sbocinec

Root in scratch image is fine for my level of paranoia.

Would make things much more simple.

zokradonh avatar Nov 08 '19 20:11 zokradonh