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

Custom native container runtime options are overwritten #17223

Closed
fbuetler opened this issue May 14, 2021 · 4 comments · Fixed by #21998
Closed

Custom native container runtime options are overwritten #17223

fbuetler opened this issue May 14, 2021 · 4 comments · Fixed by #21998
Labels
area/native-image kind/bug Something isn't working
Milestone

Comments

@fbuetler
Copy link

fbuetler commented May 14, 2021

Describe the bug

When building a quarkus application as a native image with version 1.12.1 and above it fails due to a change in the container runtime option order. This change happened in PR 15288.

Up to and with 1.12.0 custom native container runtime options were placed after default options e.g.

<quarkus.native.container-runtime-options>-uroot:root</quarkus.native.container-runtime-options>

resulted in docker run --env LANG=C --user 1000:1000 -uroot:root --rm [..] . This results in using root in the docker as later arguments overwrite previous ones.
(Note: this bugs occurs on Linux and therefore the --user 1000:1000 is added with 1000:1000 being the current system user)

After 1.12.1 (actually it was introduced in 1.13.0 and backported to 1.12.1) custom options are placed before default options. E.g. the same native container runtime options as above resulted in docker run --env LANG=C -uroot:root --user 1000:1000 --rm [..] with 1000 being used as the user in the container.

Expected behavior

Custom native container runtime options to overwrite the ones given by the system (default options).

Actual behavior

Custom native container runtime options do not have precedence.

Proposed change

NativeImageBuildLocalContainerRunner.java change to:

    @Override
    protected List<String> getContainerRuntimeBuildArgs() {
        List<String> containerRuntimeArgs = new ArrayList<>(); // changed line
        String volumeOutputPath = outputPath;
        if (SystemUtils.IS_OS_WINDOWS) {
            volumeOutputPath = FileUtil.translateToVolumePath(volumeOutputPath);
        } else if (SystemUtils.IS_OS_LINUX) {
            String uid = getLinuxID("-ur");
            String gid = getLinuxID("-gr");
            if (uid != null && gid != null && !uid.isEmpty() && !gid.isEmpty()) {
                Collections.addAll(containerRuntimeArgs, "--user", uid + ":" + gid);
                if (containerRuntime == NativeConfig.ContainerRuntime.PODMAN) {
                    // Needed to avoid AccessDeniedExceptions
                    containerRuntimeArgs.add("--userns=keep-id");
                }
            }
        }
        Collections.addAll(containerRuntimeArgs, super.getContainerRuntimeBuildArgs()); // added line

        Collections.addAll(containerRuntimeArgs, "--rm", "-v",
                volumeOutputPath + ":" + NativeImageBuildStep.CONTAINER_BUILD_VOLUME_PATH + ":z");
        return containerRuntimeArgs;
    }

To reestablish the same logic as it was in 1.12.0:

...
        if (SystemUtils.IS_OS_LINUX) {
            String uid = getLinuxID("-ur");
            String gid = getLinuxID("-gr");
            if (uid != null && gid != null && !uid.isEmpty() && !gid.isEmpty()) {
                Collections.addAll(nativeImage, "--user", uid + ":" + gid);
                if (containerRuntime == ContainerRuntime.PODMAN) {
                    // Needed to avoid AccessDeniedExceptions
                    nativeImage.add("--userns=keep-id");
                }
            }
        }
        nativeConfig.containerRuntimeOptions.ifPresent(nativeImage::addAll);
...

To Reproduce

Steps to reproduce the behavior:

  1. Build native image with 1.12.0, observe correct native container runtime options order
  2. Build native image with 1.13.0 (or even 1.12.1), observe flipped order.

Configuration

<quarkus.package.type>native</quarkus.package.type>
<quarkus.native.container-runtime-options>-uroot:root</quarkus.native.container-runtime-options>

Environment

Output of uname -a or ver

➜ uname -a           
Linux Colter 5.8.0-50-generic #56~20.04.1-Ubuntu SMP Mon Apr 12 21:46:35 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

➜ java --version                                                                   
openjdk 11.0.11 2021-04-20
OpenJDK Runtime Environment (build 11.0.11+9-Ubuntu-0ubuntu2.20.04)
OpenJDK 64-Bit Server VM (build 11.0.11+9-Ubuntu-0ubuntu2.20.04, mixed mode, sharing)

GraalVM version (if different from Java)

The one configured by quarkus.

Quarkus version or git rev

As explained above.

Build tool (ie. output of mvnw --version or gradlew --version)

➜ mvn --version
Apache Maven 3.6.3
Maven home: /usr/share/maven
Java version: 11.0.11, vendor: Ubuntu, runtime: /usr/lib/jvm/java-11-openjdk-amd64
Default locale: en_GB, platform encoding: UTF-8
OS name: "linux", version: "5.8.0-50-generic", arch: "amd64", family: "unix"

Additional context

I'd volunteer to fix this bug.

@fbuetler fbuetler added the kind/bug Something isn't working label May 14, 2021
@quarkus-bot quarkus-bot bot added the env/windows Impacts Windows machines label May 14, 2021
@fbuetler
Copy link
Author

I disagree with the label env/windows set by the quarkus-bot. It should be env/linux (if this labels exists) or rather env/graalvm-java11 .

@daviddavidgit
Copy link

This still seems to be an issue!
In my opinion, the changes of @fbuetler will fix the problem.
And I agree with @fbuetler, this is not a environment in windows problem, but rather a more general env/graalvm.

@Karm
Copy link
Member

Karm commented Dec 7, 2021

@zakkak Do you have an opinion on the order of additional arguments passed to the container build? Order matters. Context: Change in #15288

@zakkak zakkak added area/native-image and removed env/windows Impacts Windows machines labels Dec 7, 2021
zakkak added a commit to zakkak/quarkus that referenced this issue Dec 7, 2021
Forces `quarkus.native.container-runtime-options` to come after the base
arguments added by Quarkus to allow users to override the latter,
e.g. to override the --user option.

Closes: quarkusio#17223
@zakkak
Copy link
Contributor

zakkak commented Dec 7, 2021

@Karm thanks for bringing this to my attention. I think @fbuetler 's suggestion makes sense, but I think we should better ensure this behaviour by keeping the --user etc. flags in NativeImageBuildContainerRunner#baseContainerRuntimeArgs.

Please see #21998 for my proposal.

@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Dec 13, 2021
@gsmet gsmet modified the milestones: 2.7 - main, 2.6.0.Final Dec 13, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Dec 13, 2021
Forces `quarkus.native.container-runtime-options` to come after the base
arguments added by Quarkus to allow users to override the latter,
e.g. to override the --user option.

Closes: quarkusio#17223
(cherry picked from commit 9dcaafa)
@gsmet gsmet modified the milestones: 2.6.0.Final, 2.2.4.Final Dec 14, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Dec 14, 2021
Forces `quarkus.native.container-runtime-options` to come after the base
arguments added by Quarkus to allow users to override the latter,
e.g. to override the --user option.

Closes: quarkusio#17223
(cherry picked from commit 9dcaafa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/native-image kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants