Skip to content

Conversation

jmtd
Copy link
Member

@jmtd jmtd commented Apr 29, 2025

This is an alternative approach to #577 , re-using something I landed in the RHEL10 branch.

Prior to this, the images typically use the following method to move files into place within the container: cekit calls configure.sh, which copies the files and calls chmod etc manually. Instead, use cekit's existing artifacts feature to declaratively define which files should be copied into the image. Instead of doing chmod etc., rely on the default mask (ultimately the defaults for a Containerfile COPY command).

If a file should be executable, set the execute bit on the file in Git itself. This is preserved, and maps to anyone-can-execute (resolving OPENJDK-3655).

This is a larger change, but I think results in a cleaner source code, and less deviation between ubi9 and rhel10.

Besides the test suite passing (including the new test), here is an analysis of the differences in the image:

$ diff -u \
 <(docker run --rm -i before find /opt /usr/local -type f -printf "%M %P\n" | sort -k2) \
 <(docker run --rm -i after  find /opt /usr/local -type f -printf "%M %P\n" | sort -k2) \
 | wdiff -d
[--- /dev/fd/63-]{+++ /dev/fd/62+}	2025-04-29 14:51:52.923259747 +0100
@@ -1,20 +1,20 @@
[--rwxrwxr---]
{+-rwxr-xr-x+} jboss/container/java/jvm/debug-options
[--rwxrwxr---]
{+-rwxr-xr-x+} jboss/container/java/jvm/java-default-options
[--rw-rw-r---]
{+-rw-r--r--+} jboss/container/java/proxy/parse-proxy-url.sh
[--rw-rw-r---]
{+-rw-r--r--+} jboss/container/java/proxy/proxy-options
[--rw-rw-r---]
{+-rw-r--r--+} jboss/container/java/proxy/translate-no-proxy.sh
[--rwxrwxr---]
{+-rwxr-xr-x+} jboss/container/java/run/run-java.sh
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/java/s2i/maven-overrides
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/java/s2i/maven-s2i-overrides
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/java/s2i/s2i-core-hooks
[--rw-rw-r---]
{+-rw-r--r--+} jboss/container/maven/default/jboss-settings.xml
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/maven/default/maven.sh
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/maven/s2i/maven-overrides
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/maven/s2i/maven-s2i
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/openjdk/jdk/jvm-options
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/s2i/core/s2i-core
[--rwxrwxr---]
{+-rw-r--r--+} jboss/container/util/logging/logging.sh
[--rwxrwxr---]
{+-rwxr-xr-x+} s2i/assemble
[--rwxrwxr---]
{+-rwxr-xr-x+} s2i/run
[--rwxrwxr---]
{+-rw-r--r--+} s2i/save-artifacts
[--rwxrwxr---]

Some of the shell scripts are intended to be sourced, and do not need +x; others are intended to be executed, and do. Unfortunately that's not clear from their filenames (translate-no-proxy.sh should be sourced; java-default-options should be executed). That could be cleaned up in future work.

@jmtd jmtd force-pushed the OPENJDK-3655-scriptperms-alt branch 3 times, most recently from 0567a45 to c4dee5d Compare April 29, 2025 13:37
@jmtd jmtd requested a review from jerboaa April 29, 2025 13:55
@jmtd jmtd changed the title WIP Openjdk 3655 scriptperms alt [OPENJDK-3655] set more sensible permissions on scripts Apr 29, 2025
@jmtd
Copy link
Member Author

jmtd commented Apr 29, 2025

Builder images have both failed on one test only, features/jboss.container.maven.s2i/java_s2i_inc.feature:7 Check incremental builds cache .m2, which is notoriously unreliable on GHA. I'll try to run it locally.

@jmtd jmtd removed the request for review from jerboaa April 29, 2025 15:26
@jmtd
Copy link
Member Author

jmtd commented Apr 29, 2025

Seems that test is catching an issue. Untagged for review whilst I investigate.

Edit: fixed! It was indeed a missing +x on save-artifacts. Tests work ;)

@jmtd jmtd force-pushed the OPENJDK-3655-scriptperms-alt branch from c4dee5d to fb008e0 Compare April 30, 2025 10:53
We currently move scripts into place by executing a configure.sh script,
and changing ownership and permissions etc. However, in the majority of
cases we could instead declare where scripts should be installed using
artifacts:. The advantages are: smaller sources; more declarative; more
uniform ownership (default root:root); no need to chown (set the
permissions on the script in git directly).

Also addresses OPENJDK-2814 (running user shouldn't own
/opt/jboss/container)

Signed-off-by: Jonathan Dowland <[email protected]>
@jmtd jmtd force-pushed the OPENJDK-3655-scriptperms-alt branch from fb008e0 to 20a5342 Compare April 30, 2025 15:26
@jmtd jmtd requested a review from jerboaa April 30, 2025 16:03
@jmtd jmtd merged commit a8f82bf into rh-openjdk:ubi9 May 1, 2025
4 checks passed
@jmtd jmtd deleted the OPENJDK-3655-scriptperms-alt branch May 1, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants