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

Using distroless base image for Docker #214

Merged
merged 17 commits into from
Nov 16, 2021

Conversation

ItalyPaleAle
Copy link
Contributor

Thanks for making Headscale!

I have a proposed improvement to the Docker image, switching the base image of the final container from Ubuntu to a "distroless" image, using images from https://github.com/GoogleContainerTools/distroless

If you've never heard of "distroless" images, that page contains a bunch of details on the benefits, but the TL;DR is that these images do not contain a full OS but rather just enough to run the application (ca-certificates, glibc, tzdata, and just a couple more things). By using a "distroless" image, the container is much smaller (they claim they're 2% of the size of a Debian base image) and it's safer, as there's a significantly smaller attack surface. Plus, "distroless" images are updated much less frequently so keeping the base image up-to-date (and include security fixes) is much simpler.

I've confirmed this builds and the headscale binary runs. I haven't performed full E2E tests however just yet.

PS: I've also removed the stage bufbuild/buf:1.0.0-rc6 which seemed unused

@ohdearaugustin
Copy link
Collaborator

I understand the point of moving to a distoless container image. One huge drawback with distroless in my opinion is debugging. So we probably would also need a debug image then. I will try it out these days.

@kradalby
Copy link
Collaborator

kradalby commented Nov 7, 2021

PS: I've also removed the stage bufbuild/buf:1.0.0-rc6 which seemed unused

That sounds right, I should have had removed that.

Thanks for this work, I have a couple of reasons for why its actually in the state it is.
This discussion has been brought up regarding using the scratch image earlier on Discord.

This are my opinions, the main reason we have a docker image is for the E2E/integration tests. We only publish it as a bonus to users whom prefer docker instead of running binaries themselves. Neither me or @juanfont use docker for our headscale setups and the documentation for the docker part is community driven.

There are two main reasons (to me) that Ubuntu is used:

  • We both use Ubuntu for our linux systems, making it the least work for us to achieve the things we actually want: improve headscale
  • The integration tests are more akin to running headscale as a user would on a machine, and therefor we need shell, curl, etc.

We could probably be clearer that while we publish the docker image, the main purpose is for the tests.

That said, I am up for changes for the better, but it needs to ensure our requirements for the use case is met:

  • Integrations tests pass
  • If we need more tools in the image for the tests, it must be trivial to add
  • There must not be an extra support burden (as we don't use it, we are not able to help people)

@ItalyPaleAle
Copy link
Contributor Author

Thanks for sharing your thoughts!

To start, note that I've fixed the integration tests and the CI should now be green. The issue is that the tests were expecting a shell so were invoking the headscale command as-is, expecting it to be in the $PATH. I had to change the invocations to /headscale to make it work.

I understand that you don't use Docker personally and I get why that is going to be the biggest hurdle. If that is the deal-breaker, I get it. Personally, I run everything inside Docker for both ease of management (easier to recreate everything) and for security (isolation, dependency management).

That said, distroless images are different from scratch because they contain a few more things that are needed to run the app, such as a CA trust store, time zone data, and in the case of apps like headscale that use Cgo, some core libraries.

You're right that with a distroless image you can't create a shell in the image. I'd argue this is a feature and not a bug, and it shouldn't make debugging much harder: anything that happens in the container should be replicable in any Debian/Ubuntu environment (Google's distroless images are based on Debian 11)

@kradalby
Copy link
Collaborator

kradalby commented Nov 9, 2021

Don't get me wrong, I see the advantages for a production container.

I can see the tests are working and that is good, its just that the lack of shell is a relatively large problem.

Whenever I develop headscale, it is typically on a Mac or a VM that can't represent the variety of Linuxes etc and running the tests, debugging and dropping into a "in-memory" headscale in docker is a lot simpler than me having to maintain a Debian 11 VM somewhere to have a similar environment.

I get that it is easy to setup something that looks like the container environment, but it is not the actually environment.
I am not particularly keen on trying to keep several VMs in sync and up to date either in comparison to having a docker container that starts in seconds giving me the fresh desired environment.

We could of course have two Dockerfiles, one for production headscale and one for integration testing/debugging, but I would argue that we will have another "keep in sync" issue.

We could run the integration tests against both the prod and debug dockerfiles to ensure there are no diff.

Edit:
I will think a bit more about this, I want #212 to at least go in first.

@ties
Copy link

ties commented Nov 9, 2021

@ItalyPaleAle perhaps the distro less "debug" image is a good base image? It still has a shell while otherwise being very limited. However I can't check right now if those are also available for the non-language specific base images.

@kradalby
Copy link
Collaborator

kradalby commented Nov 9, 2021

If The debug one has a shell and is viable, we could make it an argument in the Dockerfile:

ARG CONTAINER_NAME "distroless"
FROM $CONTAINER_NAME

And then we can override it in the integration tests with CONTAINER_NAME = distroless-debug.

Very sudo code, but that could be a solution.

@ohdearaugustin
Copy link
Collaborator

I'm still not in favor for this change as it will not be backwards compatible with how you have to execute the binary in the container. This will probably break a lot of peoples setups. So it will be a breaking change.

Is there someway to make headscale executable without the prefixed /?

@ItalyPaleAle
Copy link
Contributor Author

@ties

perhaps the distro less "debug" image is a good base image? It still has a shell while otherwise being very limited. However I can't check right now if those are also available for the non-language specific base images.

Yes those are available and there could be two images published: a production image (based on base-debian11) and a debug image (based on base-debian11:debug) which contain a busybox shell.

Want me to update the PR to create another Dockerfile?

@ohdearaugustin

Is there someway to make headscale executable without the prefixed /?

Looking into that

@ItalyPaleAle
Copy link
Contributor Author

@ohdearaugustin 🎉 Turns out you can, just by setting ENV PATH / in the Dockerfile 😃

The latest commits revert the / at the beginning of the command, both in the integration tests and in the docs. Now it is backwards-compatible

@ties
Copy link

ties commented Nov 9, 2021

@ohdearaugustin 🎉 Turns out you can, just by setting ENV PATH / in the Dockerfile 😃

Is that equivalent/a subset of saying WORKDIR /?

About the :debug images for the non production image: if people want to easily run commands in the container I think using this for some of the images makes sense. It makes introspection easier.

@ItalyPaleAle
Copy link
Contributor Author

Is that equivalent/a subset of saying WORKDIR /?

No, that's a different thing. Seems that ENV PATH / instructs Docker where to search binaries that don't have a full path in. Just setting WORKDIR / doesn't seem to work (and I'd say is not necessary in this case).

On the debug images, I will create one later today.

@ohdearaugustin
Copy link
Collaborator

@ItalyPaleAle That are great news. yeah totally makes sense to set / as search path. So we would also have more backwards compatibility. I like it more and more. (=

@ohdearaugustin
Copy link
Collaborator

ohdearaugustin commented Nov 12, 2021

So I tested the suggested distroless build and so far it looks good for the production image. The hack with ENV PATH / works for the production image, but if you try to build the debug image. (Just change to FROM gcr.io/distroless/base-debian11:debug). The image starting having weird issues as you overwrite also the PATH for the mounted busybox. I didn't found a solution yet for it.

I would definitely like to get it running with distroless. My suggestion would be we build and publish two images like:

headscale:x.x.x --> production image
headsacle:x.x.x-debug --> debug image

so you can easily interchange the images for debugging purposes. Therefore the issuse with the debug image needs to be resolved first. For now I'm out of ideas. Tried several things like adding the binary to /bin which works in production but not in the debug image. Or added the binary to the path like ENV PATH "$PATH:/headscale, works for production, but will give you an weird error message with the debug image: headscale: line 1: syntax error: unexpected word (expecting ")"). Totally strange. So for now I don't have any idea and my brain is already a bit dead^^ Way to late.

Furthermore I also would have already prepared the new release git workflow to publish those two images, still need to test it when the debug image is working.

So @ItalyPaleAle Thank you already for your effort. (=
Edit: Formatting
Edit2: It is probably some busybox sh that is not working as expected.

@ItalyPaleAle
Copy link
Contributor Author

@ohdearaugustin I actually run into the very same issue and I started looking into that but I couldn't figure it out and then I got busy :)

My guess is that it's because of the ENTRYPOINT which essentially is telling busybox to run the headscale binary as a shell script - hence the wrong character error.

I'm going to try doing some more debugging and see if I can make it work

@ItalyPaleAle
Copy link
Contributor Author

...and looking at this with a fresh mind helped! Hooray, it works now :) The trick was that I had to remove the ENTRYPOINT defined in the base image with ENTRYPOINT []

Now the debug image works:

  • docker run --rm headscale:debug launches the headscale binary
  • docker run --rm headscale:debug ls executes a shell command, in this case ls
  • docker run --rm -it headscale:debug sh launches the shell

@ohdearaugustin
Copy link
Collaborator

Awesome. Yeah I also came to the conclusion now that busybox executes the binary as shellscript and somewhere in the binary blob there is some ) and then it will fail. A yeah typical busybox problems...

So just found out if we just copy the binary to /bin/headscale in both images we won't need the hack with the set PATH.

We definitely need to docu that^^ So for now I have to go to bed haha Thanks (=

@ItalyPaleAle
Copy link
Contributor Author

@ohdearaugustin thanks for sharing that finding :) I've now updated the containers to put the binary in /bin and removed setting the PATH.

Also, I've written some docs for how to use the debug container.

@ohdearaugustin
Copy link
Collaborator

@ItalyPaleAle Added a PR with the promised changed for the release workflow to your fork. Tested already everything.

@ItalyPaleAle
Copy link
Contributor Author

Merged, thanks for flagging that!

Copy link
Collaborator

@ohdearaugustin ohdearaugustin left a comment

Choose a reason for hiding this comment

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

Looks good and I also tested it.

ohdearaugustin
ohdearaugustin previously approved these changes Nov 14, 2021
@kradalby
Copy link
Collaborator

I'm willing to give this a go, but there are some conflicts that needs to be resolved.

@ItalyPaleAle
Copy link
Contributor Author

Uhm the merge conflicts in the docs were really weird. Anyways, fixed them

@kradalby
Copy link
Collaborator

Uhm the merge conflicts in the docs were really weird. Anyways, fixed them

We merge a pr with stricter formatting for the docs (and everything else) in between.

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented Nov 15, 2021

I missed that you added prettier. I just ran it to fix the linting issues.

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.

4 participants