Directory.DockerBuild with ONBUILD in Dockerfile

[rare flint]

Posted this in #docker but unsure if those channels are used much so adding it here.

We're finding ONBUILD is ignored by Dagger's Directory.DockerBuild? Build an image with Docker it works, build it with Dagger it doesn't. Compare the two, the Docker-built image has the correct COPY/ENTRYPOINT/CMD instructions (all from ONBUILD) and the Dagger image doesn't. It's a very easy test - the Dagger-built image doesn't have the ONBUILD COPY content, the Docker-built image does.

Found https://github.com/dagger/dagger/issues/7530 which describes exactly our usecase. Building a base image that has ONBUILD instructions, used by other images. Is this not supported?

[rare flint]

I've searched docs and the go pkg and can't find any mention of this not being supported? If it's not supported and undocumented that's quite dangerous - fortunately this was caught before teams updated to a base image that would have failed immediately.

[stone sail]

This is surprising to me because we've used the official docker build implementation (buildkit dockerfile frontend). But recently we've been gradually refactoring out buildkit in a major way, so may that's introduced a regression? Or, maybe ONBUILD never worked in spite of our using the official implementation, and we never noticed because it's rarely used? 🤔

cc @slender cloak @vestal verge

[vestal verge]

a very quick glance doesn't show any integration tests for this: (grep -RIi ONBUILD core/integration/ yields 0 results).

I wonder if at the very least, we could detect if an ONBUILD is present, and display a warning (or even error)?

[slender cloak]

I think we should support it if an image that has ONBUILD set is passed to DockerBuild api

[rare flint]

I can provide a small repro if it'll save some time demonstrating the missing functionality

[rare flint]

Dockerfiles:

Dockerfile.base:

FROM alpine:latest

ONBUILD COPY test.txt /tmp/test.txt

Dockefile.service:

FROM onbuildbase:latest

Docker test:

docker run --rm --entrypoint sh onbuildbase -c "ls -al /tmp"
total 0
drwxrwxrwt    1 root     root             0 Jan 27 21:19 .
drwxr-xr-x    1 root     root            12 Feb 23 17:44 ..

No test.txt in base image since it's using ONBUILD.

docker run --rm --entrypoint sh onbuildservice -c "ls -al /tmp"
total 4
drwxrwxrwt    1 root     root            16 Feb 23 17:38 .
drwxr-xr-x    1 root     root            12 Feb 23 17:44 ..
-rw-r--r--    1 root     root             5 Feb 23 17:38 test.txt

test.txt in image that uses base image with ONBUILD

Dagger test:

dagger core directory with-directory --path / --source . docker-build --dockerfile Dockerfile.base publish --address ttl.sh/onbuildtestbase:1h

Update Dockerfile.service to:

FROM ttl.sh/onbuildtestbase:1h

dagger core directory with-directory --path / --source . docker-build --dockerfile Dockerfile.service with-exec --args "ls,/tmp" stdout -> empty, missing test.txt

(This is an simplified version of the issue I encountered today - we have amazon linux base images that we provide to user teams to build on top of. The base image has ONBUILD x3, and the users know to simply create specific artifacts in their pipelines and build their (very short) Dockerfiles (which use FROM <amazon-linux-base-image-with-onbuild-we-provide> and the ONBUILD picks up the necessary artifacts.)

[vestal verge]

I put together a very quick test on my side under https://github.com/alexcb/dagger/commit/23bd08c4080a633887e4aeaf7122b57b1e426f22 which also includes a panic I was expecting to see, which wasn't hit, so I'll have to take a deeper look, which will include the above reproduction steps you've included -- thanks!

[stone sail]

Sorry @rare flint I misunderstood the issue. I thought you were using an image with onbuild as part of Directory.dockerBuild. But actually you are producing an image with onbuild. Much less surprising that it doesn't work (but still a problem)

Now I'm curious what that onbuild is for, and how it's used downstream!

Fun fact, I added onbuild to the Dockerfile syntax as a primitive attempt to make build pipelines programmable and composable... The same design aspirations that eventually led to Dagger 😁

--> original bikeshedding thread, 12 years ago https://github.com/moby/moby/pull/3254#issuecomment-31032942

[rare flint]

The curious thing is then that I implemented a Dagger solution to the above problem in a matter of minutes and tested it successfully in a few more minutes... Good sign! Unfortunately not so easy to implement yet, we need to support onbuild a while longer.

We provide a base image with a lot of configuration and set-up. Developer teams use that as a base for their own Dockerfiles (which typically only have two or three other lines in them). Our image has onbuild in it, they produce jar files, onbuild copies them into the final image that we run in our environments.

[stone sail]

makes sense. Glad that you're seeing a path to a Dagger solution! But we should absolutely support onbuild & all other oci fields in the meantime!

[vestal verge]

There's a lot of yak-shaving needed for this, but I have a rough first go at implementing it.

1. I currently have some logic under the container.Directory() code, which looks to see if there's any OnBuild commands under: https://github.com/dagger/dagger/pull/11901/changes#diff-89b0b6c38f29bb6615327b6126c2479d29b032ef974017a9358c0c0721fa17f9R1863-R1880

2. If there are, it then makes a hidden dagql call to container._onbuild which then iterates under each command under: https://github.com/dagger/dagger/pull/11901/changes#diff-89b0b6c38f29bb6615327b6126c2479d29b032ef974017a9358c0c0721fa17f9R1015-R1035

3. once _onbuild returns, it clears out the ONBUILD commands, then procceeds executing whatever call was made, e.g. directory("/src").

My idea is to move all logic from step 1 into a helper func, then we will have to call that each time we interact with a container.

I had initially thought about simply doing this check for ONBUILD under container.from; however that codepath doesn't include the case where we are doing a directory.DockerBuild()

I also considered handling the ONBUILD commands at the end of the directory.DockerBuild() function, but that also didn't feel right, since there could be a case where we simply want to do a directory.DockerBuild().Publish() -- which shouldn't execute the ONBUILD.

an extra ugly detail is that the ONBUILD commands are simple strings, so we need to introduce a dependency on "github.com/dagger/dagger/internal/buildkit/frontend/dockerfile/parser" in order to parse them into a RunCommand or CopyCommand or whatever else we need to support.

[slender cloak]

Might be misunderstanding, but are you going down the path of trying to run the ONBUILD command at times other than when DockerBuild is called? If so, I don't think we need that. I think all we need is to make sure that ONBUILD propagates to published images and stays in pulled images, in order to satisfy the use case here: #1475527948588023808 message

lemme know if i'm missing something

[vestal verge]

Yeah, I was trying to tackle both cases (publishing ONBUILDs, and supporting a container.From("some-image-that-has-an-onbuild").WithExec(.....)) -- but maybe that's overkill?

I'll tackle the first step, to keep the scope a bit smaller.

From an ideal functionality point of view, is this test correct?

    t.Run("onbuild", func(ctx context.Context, t *testctx.T) {
        dir := baseDir.
            WithNewFile("foo", "old-value").
            WithNewFile("Dockerfile",
                `FROM `+golangImage+`
    WORKDIR /src
    COPY foo .
    ONBUILD RUN echo -n new-value > foo
    `)
        content, err := dir.DockerBuild().Directory("/src").File("foo").Contents(ctx)
        require.NoError(t, err)
        require.Equal(t, "new-value", content)
    })

or would we expect the contents of foo to be "old-value"?

[stone sail]

Mmm setting ONBUILD in an image should not by itself execute the on-build trigger. It configures a "callback" if the image is ever used as a base in a downstream build - in which case that downstream build would trigger the command.

• After building the base image -> ONBUILD is configured in the metadata but not executed -> file contents should be "old-value"
• Adter downstream user uses the base image for their own build -> ONBUILD trigger is executed -> downstream file contents will be "new-value"

[vestal verge]

Do we want to support this with the dagger API too? e.g. container.Fom("some-image-with-an-onbuild").WithExec(....).Publish("a-new_tag").

would the newly created image have the ONBUILD commands executed in it?

[stone sail]

That's what I originally thought MB was trying to do. I was surprised because I assumed that's already what Directory.dockerBuild() does, since it's based on buildkit's official dockerfile frontend under the hood

EDIT: ooooh sorry you mean support it in withExec. mmmm no I don't think we should support that. It's too niche a feature to justify the effort IMO. But we should support it in Dockerfile compat mode though

[vestal verge]

ah ok! that clears things up 🙂

[stone sail]

If anyone ever complains about it, we could follow the example of entrypoint compat, and make it an opt-in argument

[rare flint]

Just to be clear about this, ONBUILD COPY some_file.txt does nothing in the base image, just stores the onbuild in metadata as Solomon mentioned. There's no some_file.txt at that point, and Docker doesn't care, that instruction is for later. If I attempt to build the downstream image that uses this image as a base then it errors unable to copy. That's our use-case - copying jar files into an image that uses our base image (which has the onbuild) for many microservices.

The test has a file foo with the value old-value but the representative test is no such file exists at that point, nor does it need to.

[vestal verge]

so to be ultra clear about this, does this cover your representative test?

    t.Run("onbuild-wont-copy-file", func(ctx context.Context, t *testctx.T) {
        dir := baseDir.
            WithNewFile("Dockerfile",
                `FROM `+golangImage+`
    WORKDIR /src
    ONBUILD COPY this-doesnt-exist-and-thats-ok .
    `)
        _, err := dir.DockerBuild().WithExec([]string{"true"}).Sync(ctx)
        require.NoError(t, err)
    })

[rare flint]

First half yes. The image you build from that Dockerfile needs to be the base image of another image build, then onbuild will trigger

[vestal verge]

I'm going to be extending this test to then publish this image, and then check that it contains the ONBUILD command in the metadata.

then a following test that does a new DockerBuild() which includes a FROM ... of the above published image -- and make sure that build does contain the file (or returns an error saying file not found)

[rare flint]

That'll help yes, docker inspect shows onbuild metadata. If you publish it to ttl.sh and then use it to as the base for another build that build should run the onbuild trigger, not just store metadata

Yep, you've got it

ONBUILD RUN echo to a file should work fine, same thing ultimately as our copy - that it's executed only when used as a base image. My testing earlier suggests Dagger doesn't actually do this currently so your test should fail for now

[rare flint]

@vestal verge Can you keep me in the loop on progress here? We're torn between hacky-patching our current Dagger image building solution, or supporting two different image building solutions (other is bash-based, quite outdated but still works) and it's largely because we don't know how long we'd have to support either a hacky workaround or two solutions

[vestal verge]

Will do! I'm just starting my day, and this will be my main focus. I suspect it's just a bit of glue code that we need on the dagger side -- I'll share an update before signing off tonight.

[vestal verge]

I just marked my PR ready for review merged my PR: https://github.com/dagger/dagger/pull/11901

[vestal verge]

I tried to directly fetch the metadata (over a http request), but it was getting a bit gnarly having to unmarshal the manifest, then following it to fetch the metadata, so I decided to abandon that part of the test; however the test still validates that the ONBUILD both errors (when the file doesn't exist), and passes when it does exist. So we still have the test coverage needed.

I'm tempted to go on a side quest, and try adding dagger API calls to support adding ONBUILD, SHELL, and HEALTHCHECK commands next, which would address https://github.com/dagger/dagger/issues/9515

[stone sail]

Seems worth it, you already paid the mental overhead upfront right?

[vestal verge]

yeah, the cache is still warm.

[rare flint]

Reviewed the test, this looks exactly what we need. I await a release!