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

Build a docker container #90

Merged
merged 1 commit into from Jun 6, 2019

Conversation

4 participants
@jbergstroem
Copy link
Contributor

commented Apr 29, 2019

The build phase makes the assumption that any capnproto-files are pre-processed before building in-docker. I personally prefer "add-on-change" for protos over add-on-compile since that may lead to a dirty build.

Chose to add this as a PR even though there's already an open one. Feel free to close if we should focus on one PR with a slightly different direction instead.

Finally, my comment in the previous PR (#89) set out to clone inside the container which likely is a safer approach for release builds, but copying tree allows us to do "developer"/in-transit builds.

@jbergstroem jbergstroem force-pushed the jbergstroem:feature/dockerfile branch 2 times, most recently from 758c868 to 1deec64 Apr 29, 2019

@nickvollmar
Copy link
Collaborator

left a comment

This is pretty much ready to go! I've requested a few changes.

Thank you for contributing!

Show resolved Hide resolved Dockerfile Outdated
Show resolved Hide resolved Dockerfile Outdated
Dockerfile Outdated
WORKDIR /go/src/github.com/cloudflare/cloudflared/
COPY . .

RUN apk add --no-cache gcc musl-dev git upx ca-certificates

This comment has been minimized.

Copy link
@nickvollmar

nickvollmar May 8, 2019

Collaborator

Should we run apk update && apk add? I usually see that done in Dockerfiles, to pick up changes to the registry between the base image build and the Dockerfile build.

This comment has been minimized.

Copy link
@jbergstroem

jbergstroem May 9, 2019

Author Contributor

Upstream doesn't seem to do it and in general I'm not a fan of doing so because it means we're sliding the updates between builds (read: non-reproducable). This (not running update) should mean we layer our container properly.

Aside: I usually make an exception if the parent container has security issues left unfixed, but alpine is a bit different here as well.

This comment has been minimized.

Copy link
@jbergstroem

jbergstroem May 9, 2019

Author Contributor

Since this is somewhat subjective I'll leave it for you to decide what you prefer.

@jbergstroem jbergstroem force-pushed the jbergstroem:feature/dockerfile branch from 1deec64 to 7e1f85b May 9, 2019

@cblecker

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@jbergstroem If you use the debian-based golang image as the builder, you can just use the make cloudflared rule and not include all the extra golang flags in the Dockerfile. This makes the docker build more consistent with what would be built on a local system, and doesn't make a difference to the end image as you discard the builder anyways.

Additionally, if you use gcr.io/distroless/base for the final image then you have the smallest available attack surface, while still having full glibc libraries.

Example:

FROM golang:1.12 as builder
WORKDIR /go/src/github.com/cloudflare/cloudflared/
COPY . .
RUN apt-get update && apt-get install -y --no-install-recommends upx
RUN make cloudflared && upx --no-progress cloudflared

###

FROM gcr.io/distroless/base
COPY --from=builder /go/src/github.com/cloudflare/cloudflared/cloudflared /
ENTRYPOINT ["/cloudflared"]

Results in:

$ docker images cloudflared
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
cloudflared         latest              5597453d2a6e        5 minutes ago       32.1MB
$ docker run --rm cloudflared version
cloudflared version 2019.5.0-3-gbabcd9f (built 2019-05-30-0554 UTC)
@jbergstroem

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@jbergstroem If you use the debian-based golang image as the builder

I can change; up to upstream wrt what they prefer really.

@nickvollmar

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

Very interesting! I had not heard of distroless until now. Thank you for the suggestion, @cblecker. I agree that reducing the attack surface area is valuable.

Surprisingly, the Distroless image size seems to be a bit bigger than the Alpine image.

$ docker images --filter=reference='cloudflared:*'
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
cloudflared         distroless          1728712c0b9e        2 minutes ago       32.1MB
cloudflared         alpine              6689ad34f5a6        7 minutes ago       21.3MB

My first reflex was that I'd mixed up the tags, but I double- and triple-checked that this wasn't the case...

My next assumption was that the Alpine executable was considerably smaller. But no, it's actually a bit larger than the one built by Debian for Distroless (15.4M golang:1.12-alpine, 15.2M golang:1.12).

It seems like there's something to be investigated/tweaked here. If Distroless is really... distroless, then my instinct tells me it should be smaller than any distro-based image...

For the sake of publishing a Docker image, my reflex is to merge this PR as-is and investigate Distroless later.

@nickvollmar nickvollmar self-requested a review May 31, 2019

@cblecker

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@nickvollmar the difference is in the libc libraries. Alpine has Busybox + APK + muslc, while distroless is the full glibc library.

More details: https://medium.com/@dwdraju/distroless-is-for-security-if-not-for-size-6eac789f695f

The other distinct advantage is because it’s the full glibc library, you can use the debian based image for the build step with make, so you keep the build commands identical and in a single place

@jbergstroem

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

The other distinct advantage is because it’s the full glibc library, you can use the debian based image for the build step with make, so you keep the build commands identical and in a single place

My main argument for switching would be performance (glibc tends to be faster over the board), but I don't know how critical this is. Size would be not perhaps equal but similar since I built it statically and would switch to linking dynamically (so, we'd almost be comparing libc's). We could also just build static and move to FROM SCRATCH. Either way, let me know what you prefer and I'll update.

@cblecker

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

The other thing I noticed that could make the cloudflared image smaller is stripping the debugging information from the release binary (adding -s -w to the ldflags). But that's probably best for a separate PR

edit: I created #102 for this

@jbergstroem jbergstroem force-pushed the jbergstroem:feature/dockerfile branch from 7e1f85b to 082db4a Jun 2, 2019

@jbergstroem

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

Switched to debian as base and rebased on top of latest master.

The other thing I noticed that could make the cloudflared image smaller is stripping the debugging information from the release binary (adding -s -w to the ldflags). But that's probably best for a separate PR

Yes; happy to open another PR that would add a -debug docker tag and slim the regular release as a result if people think it would be used. I don't mind the few extra megs since containers are compressed and downloaded as such over the wire. edit: never mind, already done in #102.

Show resolved Hide resolved Dockerfile Outdated

@jbergstroem jbergstroem force-pushed the jbergstroem:feature/dockerfile branch from 082db4a to 68549db Jun 2, 2019

@jbergstroem

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@cyakimov brought up (offline) that base from distroless also adds openssl which would be unecessary. With that said I think we can stay with this for convenience reasons (since the alternative would be going back to static linking again).

Edit: this (static) is how k8s-land seems to build their images; use gcr.io/distroless/static and use CGO_ENABLED=0 while building.

@nickvollmar
Copy link
Collaborator

left a comment

Sorry for the noise –– a couple more items I thought of. Thanks for this amazing work.

Show resolved Hide resolved Dockerfile Outdated
@nickvollmar

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Oh, oops, I'm bad at Github. I meant to include the comment https://github.com/cloudflare/cloudflared/pull/90/files#r290885688 in that last message.

Build a docker container
Use debian/distroless (glibc) as a base which in general gives
slightly higher performance than alpine (musl) while trading
off container size (about 10mb).

The build phase makes the assumption that any capnproto-files
are pre-processed before building in-docker.

@jbergstroem jbergstroem force-pushed the jbergstroem:feature/dockerfile branch from 68549db to b35d850 Jun 5, 2019

@nickvollmar

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Thanks!

@sssilver Could you merge this when you get a chance?

@jbergstroem

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Since there's not enough comments in this PR I took the opportunity of summarizing the size differences between uncompressed distroless container flavors (-strip incorporates changes from #102):

$ docker images --filter=reference='cloudflared:*'
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
cloudflared         static-strip        a0ef335ac719        17 seconds ago      8.92MB
cloudflared         static              793ad1c667a0        2 minutes ago       14.6MB
cloudflared         base                561100fb7ba6        4 minutes ago       32.1MB

@sssilver sssilver merged commit d3f9aa2 into cloudflare:master Jun 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.