Add distroless Docker image variant#17876
Conversation
c340495 to
5dcf33a
Compare
|
Note: This should work with CI as well. |
|
I think you can avoid a bunch of complexity if adopting Docker Bake is acceptable for the project? It's effectively a config syntax in HCL that maps into the Docker CLI build command + options. That allows for easily declaring two images and their tags, then you can use the Github Actions support if you like, or If that sounds appealing to you, let me know and I can provide some examples. |
There was a problem hiding this comment.
Turns out that this Dockerfile feedback pretty much applies to the existing Dockerfile in the project too, which due to it's USER nobody is likely affected by this concern too (not an issue for distroless that sets USER with UID/GID values).
Have you tested this Dockerfile.distroless build? Won't it have ownership issues? The primary Dockerfile corrects these before switching to a non-root user:
Line 24 in c4b0da9
You'd need to do the equivalent I believe and then switch back to nonroot, but ensure you do this with the UID/GID for broader compatibility. You may want to include a comment about that distinction to avoid anyone mistakenly changing it as an "improvement".
Since you're differing in ownership, you'd also be wise to more clearly document this difference (there's related markdown docs on this project, but you should possibly also highlight this on the page descriptions at the registries the image is published to).
Your COPY + WORKDIR are probably also with the existing USER context, that minimizes the need to apply some of the corrections, but technically /bin placed files should be root:root 😅 (and presumably chmod g+w /prometheus is still relevant? better reasoning explained here)
EDIT: Regardless of USER the COPY instruction appears to default to root ownership unless using --chown. WORKDIR will use USER if it needs to create the directory. The chmod g+w can be handled with COPY --chmod=775 (requires BuildKit container driver rather than docker build driver, otherwise seems to be ignored).
For the /prometheus location, the UID/GID will be used for new anonymous/named volume mounts, but bind volume mounts will replace it entirely.
This can matter when your container process needs to write to persisted volumes, such as with /prometheus which is probably why it's advising a named data volume (or uses the implicit anonymous data volume in the primary Dockerfile), since there's no docs there mentioning the expectation for matching the containers UID/GID.
Introducing this image variant with a different USER mapping will not be compatible to change between images without going into the container and changing that volumes ownership first, which would need to be done with an intermediary container in this case. You may want to cover this in docs for migrating between image variants as something for users to be mindful of.
I personally find it better to make nonroot use optional/opt-in, since it can come with various caveats (k8s example, OpenShift is another one IIRC). More often than not those interested in nonroot can get many benefits from more explicit configuration (like dropping all capabilities) or running a container rootless (eg: Docker rootless or default for Podman run as a non-root user).
|
Thanks for the detailed feedback @polarathene! Regarding ownership issues: I've tested the distroless image and it works correctly. The Regarding running as root: I disagree with making nonroot optional/opt-in. The entire goal of this distroless variant is to increase security, not diminish it. We're not changing the default busybox image - users who need or want the traditional nobody-based image can continue using it. The distroless variant is explicitly for users who want enhanced security by default. And it would be default for Prometheus 4. Regarding migation Users explicitly choosing the distroless variant will be making a conscious decision to adopt a different security model. I expect them to pay attention to the details and read the release notes which specify ownership changes. Regarding the build system:
|
e56c98a to
8ef71e1
Compare
351beb4 to
73e6ecd
Compare
|
I believe the logic is cleaner now:
|
73e6ecd to
c333224
Compare
c333224 to
1d5c041
Compare
Introduces distroless image using UID/GID 65532 instead of nobody, and removes VOLUME declaration. Busybox image remains default with unchanged tags for backwards compatibility. Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
1d5c041 to
1105c82
Compare
|
Nice |
|
Quite a bit of activity while I was away and unavailable to chime in 😅 Seems I'm a bit too late.
Well you had it in the other image for a reason 🤷♂️ Might as well try be consistent? Perhaps I'm one of the few that aren't fond of troubleshooting projects when undocumented differences cause different/unexpected compatibility issues? Will it be documented alongside the difference in UID/GID? I would hope that that kind of information isn't going to be hidden away in a changelog only, since that'd prevent any new users adopting the images from knowing any better.
What is the drawback to being consistent by having the distroless variant use the In the event of a rootful container escape, the While for a rootless container (irrelevant to what is run in the container itself), the ID would map to a value outside the host range for users by exceeding You still get the perks of non-root user in a container with
I have a WIP example I'll share/PR once I've had time to wrap it up and polish it. You can reject that if you like but it wouldn't interfere with the concerns you've expressed that'd negatively impact anyone. I did however in the interest of minimizing noise/complexity, unify both busybox and distroless variants into a single I get the impression my proposal will be rejected by this project, but perhaps there will still be some value that you can extract from it. |
|
@roidelapluie this is blocking the v3.10 release as the CI is failing at this step https://github.com/prometheus/prometheus/actions/runs/22120978450/job/63963288131 I will look at mitigating this. Maybe will skip this image that does not exist. |
|
#18115 removes riscv64 from distroless |
Introduces distroless image using UID/GID 65532 instead of nobody, and removes VOLUME declaration. Busybox image remains default with unchanged tags for backwards compatibility.
Which issue(s) does the PR fix:
Fixes #8657
Closes #17680
Relates to #16048
Does this PR introduce a user-facing change?
For the top of the CHANGELOG and the release messages, I would add this message: