Create a custom JRE for the Docker image#993
Conversation
|
Sorry I missed this earlier -- could you resolve the conflicts? |
f45c0bb to
9f55520
Compare
@gaul : 😄 I understand missing a notification on GH |
|
Thanks for tackling this! Good direction overall — Should not be in this PR
Likely bugs / dead code
Drift / duplication
Forward compatibility
Coverage gap
Minor
Nice changes
|
gaul
left a comment
There was a problem hiding this comment.
A few inline notes — the must-fix item and the two most important correctness/validation gaps. (Minor nits and optional enhancements shared separately, not posted inline.)
| @@ -0,0 +1,22 @@ | |||
| { | |||
There was a problem hiding this comment.
Must fix — accidental commit. This is personal Peacock editor color settings, unrelated to the custom-JRE change. Please drop it from the PR. The repo doesn't track .vscode/, so consider adding .vscode/ to .gitignore to prevent it being re-added.
|
|
||
| # Create a custom Java runtime with jlink | ||
| RUN jlink \ | ||
| --add-modules $(cat /tmp/modules.txt) \ |
There was a problem hiding this comment.
Runtime modules can be missed — risk of broken outbound TLS. This list comes from static jdeps analysis (+ --bind-services), which misses dependencies loaded by name rather than referenced statically. The notable one is the EC crypto provider jdk.crypto.ec: ECDHE TLS handshakes to S3/Azure/GCS need sun.security.ec.SunEC, but it's loaded via the java.security config — so jdeps --print-module-deps won't report it and --bind-services won't pull it. The image can start fine, then fail on the first HTTPS connection to a backend.
This is compounded because no CI step runs the JRE that actually ships:
- The
Build custom JRE with jlinkstep builds a different, superset JRE (it addsjdk.compiler,jdk.zipfs,java.instrument,jdk.attach), and theContainerizejob builds the image but never runs it (push: falseon PRs). A missing runtime module won't surface. - Every test backend talks plaintext HTTP to localhost (transient/filesystem/azurite/localstack/fake-gcs), so nothing exercises real ECDHE TLS to a remote endpoint.
Suggestions: add jdk.crypto.ec (and likely jdk.crypto.cryptoki) explicitly to --add-modules; and/or add a smoke test that docker runs the built image and makes one real HTTPS backend call. Ideally the test job uses this same minimal module set so CI validates the artifact that ships.
| path: target/s3proxy | ||
| - uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: s3proxy-jar |
There was a problem hiding this comment.
Unused artifact. This s3proxy-jar is uploaded here and downloaded again in the Containerize job (around line 289), but the Docker build never uses it: .dockerignore only allows /target/s3proxy, /target/jdeps-modules.txt, and the run script into the build context, and no COPY references *-jar-with-dependencies.jar. (target/s3proxy is already that jar made executable.) Both this upload and the matching download can be removed.
|
A few minor / non-blocking suggestions to accompany the inline notes above (none are correctness blockers):
Optional (out of scope for this PR)
Thanks for tackling #401 — the multi-stage layout and the automated |
Fixes #401
Improvement : reduces the attack surfaces on the Docker image (makes our automated tools scream less often).
Custom Java runtime creation:
exec-maven-pluginexecution to runjdepsduring the package phase, outputting the required Java modules totarget/jdeps-modules.txt. This automates module dependency analysis forjlink.Dockerfileto use a multi-stage build: the first stage creates a custom JRE withjlinkusing modules listed injdeps-modules.txt, and the second stage builds the final runtime imageCI/CD workflow:
.github/workflows/ci-main.ymlto upload and download the newjdeps-modules.txtand the jar-with-dependencies as build artifacts, ensuring these are available for the Docker build step..dockerignoreto allowtarget/jdeps-modules.txtinto the Docker build context, so the custom JRE can be constructed from actual dependencies.Build artifact:
module-info.classfiles from the jar-with-dependencies, preventing conflicts and unnecessary files in the runtime.Docker build :