Repository navigation
fix: temp directory refinements - #48608
Conversation
008dafb to
d72e76f
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors temporary-directory handling so it is initialized during application startup instead of construction/build-time, and aligns the temp path with each application's data directory setup.
Changes:
- Moves
kc.io.tmpdirinitialization fromKeycloakApplicationconstruction intostartup(). - Updates Quarkus and RESTEasy test applications to source temp/data paths from their data-directory configuration.
- Improves gzip cache directory warning output and adjusts testsuite data-dir bootstrapping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
testsuite/utils/src/main/java/org/keycloak/testsuite/KeycloakServer.java |
Changes how the embedded testsuite server derives its data directory. |
services/src/test/java/org/keycloak/services/resteasy/ResteasyKeycloakApplication.java |
Switches the RESTEasy test app to use jboss.server.data.dir as its data dir. |
services/src/main/java/org/keycloak/services/resources/KeycloakApplication.java |
Moves temp-dir initialization to startup and removes the fallback in getTmpDirectory(). |
services/src/main/java/org/keycloak/encoding/GzipResourceEncodingProviderFactory.java |
Tweaks gzip cache directory creation and warning logging. |
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/jaxrs/QuarkusKeycloakApplication.java |
Changes Quarkus data-dir fallback behavior when kc.home.dir is missing. |
|
Let's wait for #48733 to go in first (which will fix this on main), then reasses the refinements here. |
|
Adding hold label to wait for #48733. |
closes: keycloak#48566 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Rebased and updated to aggregate the logic a little more. Basically the only scenario that won't have a tmp dir set is if things are launched via QuarkusTest. If / when we do that, we can decide what is the right way to do that. |
There was a problem hiding this comment.
I think I found one more related issue.
When kc.home.dir is not set, getDataDir() returns null. initTmpDirectory() then skips setting kc.io.tmpdir and getTmpDirectory() throws RuntimeException. This propagates through GzipResourceEncodingProviderFactory.initCacheDir(), which causes the same symptom - HTTP 500 for static resources.
See: shawkins#21
I guess we can address it in this PR (or I can create a follow-up).
That's actually expected behavior, and is what I'm referring to here #48608 (comment) In every path we currently test things, the home dir or data dir will be set. If we want to support lower level testing (I mentioned a quarkus test, but trying to unit test the gzip logic counts also), then it either needs to set a system property as you have shown, or we can push the automatic handling lower. Since this has turned out to just a cleanup issue anyway, I'll go ahead and remove the KeycloakApplication.getTmpDirectory method completely and make callers get the tmp dir from the DefaultKeycloakSessionFactory - that should make some of the lifecycle and responsibility even clearer. |
aaah, I missed your last comment, my bad. Feel free to ignore the suggestion and do it your way. I'll close the PR. Sorry for the noise. |
This only marginally helps as the callers would still have to access via KeycloakApplication.getSessionFactory() - which is not something we should do, and would still require additional setup in lower-level tests. The only way for this to not be static is to expose an instance method on KeycloakSessionFactory (which I'm reluctant to do since that is part of the spi) or create a temp dir spi. Going back to using a service loader just for the temp directory is possible as well. @Pepo48 does it seem worth it to do any further changes now, or just revisit this later if needed? |
Let's just revisit when needed. Thanks Steve, approving. |
…oak#48608) closes: keycloak#48566 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Keycloak 26.6 and 26.6.1 shipped `/opt/keycloak/data/tmp` as `root:root 0755`. Under a non-root UID, such as OpenShift's arbitrary SCC UID, the gzip resource cache couldn't initialise, and every static theme asset returned 404 for `Accept-Encoding: gzip` (keycloak/keycloak#48566). The chart worked around this by mounting a writable `emptyDir` at that path. Its comment said to drop the mount once Keycloak reached 26.7.0. - The upstream fix, keycloak/keycloak#48608, is merged and labelled `release/26.7.0`. It moves temp-dir creation from build time to runtime startup. - The Keycloak base image is pinned to `quay.io/keycloak/keycloak:26.7.4` (`base_image_keycloak` in `.mise/config.toml`). This PR removes the `gzip-cache` volume and its mount. `volumeMounts:` and `volumes:` now render only when `keycloak.db.caCert` sets up the DB CA mount, so an install without it gets no empty keys. ## Validation - `mise run //helm:check` (lint and kubeconform): pass - `helm template -s templates/keycloak/app.yaml`, without and with `keycloak.db.caCert`: without it, neither key renders; with it, only the `pg-ca` mount and volume render. - `mise run check:comment-types`: pass - **Not tested here:** a live Keycloak under an arbitrary non-root UID. CI's e2e runs Keycloak with the chart, but not under OpenShift's SCC. https://claude.ai/code/session_01LPhe4LaK9Sepj2X7PSjd8i --- _Generated by [Claude Code](https://claude.ai/code/session_01LPhe4LaK9Sepj2X7PSjd8i)_ Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
closes: #48566
- moves temp directory initialization to startup, rather than the KeycloakApplication constructor - so there is nothing happening at buildtime- already done by #48438