Enable buildx support in docker container image extension

[edeandrea]

This was spawned from the conversation in #25467 - a way to pass additional arguments to the container-image-docker build process.

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

[edeandrea]

Do you see anything wrong with the way I added this to DockerConfig? Basically just allow what the config says to "pass through" to the executable. Or would you see that as a potential security hole (i.e. injection)?

/**
  * Additional command line arguments to be passed to the {@link #executableName}.
 */
@ConfigItem
public Optional<List<String>> additionalCommandArgs;

I thought about just adding a platform config item specifically, but that option ONLY exists if you're using docker buildx build.

Happy to have a conversation and/or change things up based on discussion.

[geoand]

What is buildx?

If we can avoid having catch-all configuration, I would prefer it.

[edeandrea]

What is buildx?

https://docs.docker.com/buildx/working-with-buildx

Its used to build multi-arch images by emulating the arch outside of the arch of the system actually building the image.

[geoand]

I'll have to take a look and get back to you

[edeandrea]

See https://github.com/quarkusio/quarkus-super-heroes/blob/main/.github/workflows/build-push-container-images.yml#L81-L89 as well. With this change I wouldn't have to use the buildx github action. I could just use mvn package (like I was doing before building multi-arch images).

[edeandrea]

I'll have to take a look and get back to you

Sounds good. Happy to chat about it more and what you think the best approach would be. I agree having a "catch-all" that blindly appends things to the command line isn't the best idea.

We could instead use a platform config attribute, which if configured could reconfigure executableName to be docker buildx instead of just docker (and of course document it). That's another idea.

[geoand]

See https://github.com/quarkusio/quarkus-super-heroes/blob/main/.github/workflows/build-push-container-images.yml#L81-L89 as well. With this change I wouldn't have to use the buildx github action. I could just use mvn package (like I was doing before building multi-arch images).

How does buildx come into play in this case? Is it automatically added because platform was specified?

[geoand]

We could instead use a platform config attribute, which if configured could reconfigure executableName to be docker buildx instead of just docker (and of course document it). That's another idea.

I don't think this is correct, because the executable is still docker isn't it? IIUC, what needs to be done is to add buildx if platform is specified, no?

[edeandrea]

Buildx is a plugin to the docker command, so you would run the command docker buildx --platform .... There's also a GitHub action which sets it up (see https://github.com/quarkusio/quarkus-super-heroes/blob/ebfcf21d7d880595358d475f4dfc0fad51cfeb31/.github/workflows/build-push-container-images.yml#L76 and https://github.com/docker/setup-buildx-action). It's included as well in the docker cli I think (I just set up a new Mac machine and when I brew install docker it was there.l The install parameter in the action essentially creates a symlink so that docker -> docker buildx.

So in that use case where you are using the GitHub action with install=true, setting the executableName to docker buildx would actually not be correct.

Maybe the thing to do is to introduce the new platform parameter and if it is set, then check what the exefutableName is. If it's anything other than docker then fail the build and notify the user that they have conflicting settings that they need to fix. This way we aren't making assumptions on what the user is trying to do.

I'm not sure we could (or should) test that they have docker symlinked to docker buildx. The only way to really know would be to run the docker command and parse the output of it in some way.

[geoand]

I think the simplest and most reasonable thing to do is add a platform parameter to our Docker configuration and then alter the code that handles it to also add buildx when platform is present

[edeandrea]

I think the simplest and most reasonable thing to do is add a platform parameter to our Docker configuration and then alter the code that handles it to also add buildx when platform is present

I agree. If we do this, though, and the user in the workflow sets the action to install=true then this will cause the docker command to fail. The user will have to go back and fix their workflow by removing install=true, which could have repercussions in other steps in their workflow.

I don't think that is the majority use case though. If you simply run docker build and pass the --platform flag, the build will run and the flag will be ignored, therefore successfully producing a container image that isn't of the arch you expected. I think this is the more important use case to guard against.

I also think we should take it 1 step further. If the --platform parameter is set, then we should check what the executableName is. If it's anything other than docker (maybe they're using podman) then we should fail the build and notify the user that they have conflicting settings that they need to fix.

And of course, all of this will be documented in the documentation :)

[edeandrea]

@geoand are there tests anywhere in the codebase that test all these config options and make sure the correct docker build command is run?

[geoand]

Nope, but you can certainly add some

[edeandrea]

Something I just thought of. There are a bunch of options that buildx supports (https://docs.docker.com/engine/reference/commandline/buildx_build/#options). Some of these are just carry-over from the generic docker build, but others (like the output) are specific to buildx.

Instead of a single platform property (i.e. quarkus.docker.platform), what about a set of nested properties specific for buildx (quarkus.docker.buildx.*). I don't think we need to support every option right now. For now, maybe just platform and output?

Output is important because docker buildx can only load created images into docker images if doing a single-arch build.

WDYT @geoand ?

[geoand]

I like the idea

[edeandrea]

@geoand take a look at what I pushed. I made a few other changes as well around tagging/pushing.

When building multi-arch images in a single step, those images are not loaded into docker images (for some reason buildx doesn't support that). Instead buildx wants to tag/push those images directly while also creating the manifest lists at the same time.

I used the combination of configuring any of the new quarkus.docker.buildx.* properties along with the existing quarkus.container-image.push property and did the necessary configuration for buildx to be able to tag & push in a single step.

If not using any of the buildx properties then things work the way they always did.

I also wasn't sure if I needed to add @ConfigDocSection and @ConfigItem to DockerConfig.buildx. It seems to work without those, but I wasn't sure.

[geoand]

Once everything is done, we'll definitely need the commits in better shape :)

[edeandrea]

I'm done coding. On the PR you can look on the top at the file browser. It combines the commits.

I tried to rebase to a single commit but something got messed up when I merged main back in.

[geoand]

We'll need the commits squashed / renamed, as we require semantic commits in Quarkus and a repository that builds properly at any commit.

My request has nothing to do with viewing the changes

[edeandrea]

I'll take care of it on Monday.

[edeandrea]

@geoand I'm not sure what happened here. I went to try to rebase all the commits into a single commit and also rebase main on top but things got messed up, so instead I opened #25589. I'm closing this one in favor of that one.