Skip to content

ONBUILD instructions must be added prior to skipUnusedStages #332

@mzihlmann

Description

@mzihlmann

Actual behavior
As part of the cache-lookahead implementation I realized that ONBUILD instructions can give us some headaches. ONBUILD instructions are defined in the baseimage, potentially upstream, so we must fetch the manifest of these images beforehand. As remote fetch is expensive, the current implementation avoids uneccessary fetches by first dropping all unreferenced stages. This kind of blocked me as I can't move adding ONBUILD instructions prior to stage-dropping as we would lose out on that optimization. But it turns out that our current optimization is actually bogus, it is not possible to drop stages before adding ONBUILD instructions.

ONBUILD instructions are coming from the base-stage but get executed inside the context of the build-stage. This is problematic as they can add new dependencies to the stage via COPY --from. Initially I assumed that whilst they might add COPY --from it would likely be illegal to reference anything but remote images in ONBUILD instructions originating from remote images, but that is not the case, an upstream image can reference local stages 🤯.

This means if we do use skipUnusedStages, which is the default now, there are some edge-cases in which ONBUILD instructions can throw us a wrench. I will add some example cases to the integration tests later.

In a way, this is good news to me working on cache-lookahead, as I can proceed moving the transformation earlier. But fixing this edge-case bug will have some performance impact on everyone, so I'm not happy about that.

Expected behavior
It is a bit contrived, but I tested with buildkit and it can even add dependencies to local stages with a COPY instruction in a remote image.

FROM alpine
ONBUILD COPY --from=base /blubb /blubb
docker build . --tag onbuild --push
FROM alpine AS base
RUN touch /blubb

FROM onbuild
RUN ls -lah /blubb

will indeed copy the file from base stage

They even reference this specific edge-case in the spec, so it's not something that happens by accident. I think it is a really weird decision to support this edge-case though. I honestly don't see the value in it and supporting it will deteriorate performance for everyone as now optimizations can only kick in once all baseimages are resolved 😕.
https://docs.docker.com/reference/dockerfile/#copy-or-mount-from-stage-image-or-context

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions