Repository navigation
Warn when running in a container without being PID 1 - #48479
Conversation
| @@ -161,6 +161,11 @@ if [ "$PRINT_ENV" = "true" ]; then | |||
| echo "Using JAVA_RUN_OPTS: $JAVA_RUN_OPTS" | |||
| fi | |||
|
|
|||
| # Warn if running in a container without being PID 1, as signals may not be forwarded correctly and graceful shutdown may not work | |||
| if [ "$KC_RUN_IN_CONTAINER" = "true" ] && [ "$$" != "1" ]; then | |||
| echo "WARNING: Keycloak is running inside a container, but is not PID 1. Graceful shutdown may not work. Use 'exec' in your entrypoint script to ensure signals are forwarded correctly. See https://www.keycloak.org/server/containers for more details." | |||
There was a problem hiding this comment.
The only additional consideration here is that this warning shouldn't matter for non-server mode commands (import, export, boostrap-admin). Is it worth accounting for this?
There was a problem hiding this comment.
Thanks for taking a look! Yeah I did not consider that. Will modify to exclude those scenarios
0797b9f to
110d400
Compare
| @@ -68,6 +68,7 @@ do | |||
| case "$1" in | |||
| -D*) SERVER_OPTS="$SERVER_OPTS ${OPT}";; | |||
| *) CONFIG_ARGS="$CONFIG_ARGS ${OPT}" | |||
| [ -z "$KC_CMD" ] && KC_CMD="$1" | |||
There was a problem hiding this comment.
Unfortunately this assumption won't generally hold. You are allowed to put arguments like -v or -cf ahead of command.
We could refine this to be the first argument without a leading -, or to avoid any additional arugment parsing in kc.sh we could instead set the initiating pid as an environment variable or system property and let the java logic emit the warning.
There was a problem hiding this comment.
@shawkins I think I've addressed this concern if you could take another look?
There was a problem hiding this comment.
Let me kick off the ci run. I'll try to think of any additional corner cases.
110d400 to
d1b6524
Compare
|
@EspenRoth after considering that @Pepo48 should kc.bat be doing something similar, or are we not worried about users who would be rolling their own windows images? |
6621b72 to
5f97b59
Compare
|
@shawkins short answer: no, I think no adjustments are needed as far as Windows world of things. Unless I missed something completely, there's no official support for Windows containers. I wouldn't be even surprised if I was the only one who actually tried to use Windows containers in connection with Keycloak (back in the QE days, just for testing purposes. I've never seen any upstream issue/discussion either. Strictly technically speaking:
If someone would go further despite of that then on docker environment (windowsservercore 1803+) it's a non-existent problem - On Kubernetes, the situation is slightly different from what I found - only the entrypoint process gets the signal, not children. So if someone ran |
|
Thanks for the input @Pepo48, it's safe to say then that we aren't worried about kc.bat. |
There was a problem hiding this comment.
I noticed a typo. Otherwise LGTM
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BruteForceTest#testNoFailureResetForPermanentLockout |
6276f8e to
c55a80b
Compare
There was a problem hiding this comment.
We should consider also adding a container specific tests that verifies the warning is not present there.
c55a80b to
6aa7277
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostSigTransientTestKeycloak CI - Adapter IT Strict Cookies |
6aa7277 to
de2406f
Compare
de2406f to
b7d3120
Compare
When KC_RUN_IN_CONTAINER=true but the process is not PID 1, graceful shutdown may fail silently because signals are not forwarded correctly. This adds a warning at startup to alert users to use exec in their entrypoint scripts. Closes keycloak#48059 Signed-off-by: Espen Roth <eroth1622@gmail.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
b7d3120 to
51dd0fa
Compare
Head branch was pushed to by a user without write access
|
Thank you for everyone involved in this, and @EspenRoth for getting the ball rolling. Keycloak will be safer to use with this. |
* Warn when running in a container without being PID 1 When KC_RUN_IN_CONTAINER=true but the process is not PID 1, graceful shutdown may fail silently because signals are not forwarded correctly. This adds a warning at startup to alert users to use exec in their entrypoint scripts. Closes keycloak#48059 Signed-off-by: Espen Roth <eroth1622@gmail.com> * moving to java logic to avoid any command detection in the script Signed-off-by: Steve Hawkins <shawkins@redhat.com> * Update quarkus/dist/src/main/content/bin/kc.sh Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> --------- Signed-off-by: Espen Roth <eroth1622@gmail.com> Signed-off-by: Steve Hawkins <shawkins@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> Co-authored-by: Steve Hawkins <shawkins@redhat.com> Co-authored-by: Peter Zaoral <pepo48@gmail.com>
Summary
When
KC_RUN_IN_CONTAINER=truebut the Keycloak process is not PID 1, graceful shutdown may fail silently because signals such asSIGTERMare not forwarded correctly. This can lead to cache inconsistencies or data loss.This change adds a startup warning in
kc.shto alert users to useexecin their entrypoint scripts, with a link to the existing container documentation that already describes the correct approach. The warning is only emitted for server commands (start,start-dev) and is skipped for non-server commands such asimport,export, andbootstrap-admin.Changes
quarkus/dist/src/main/content/bin/kc.sh— added PID 1 check with warning message printed at startup when running in a container without being PID 1; command detection refined to identify the first non-flag argument so that flags like-vor-cfpreceding the subcommand are handled correctlyquarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/JavaOptsScriptTest.java— added three tests covering the presence and absence of the warningTest plan
testContainerNonPid1Warning— asserts warning appears whenKC_RUN_IN_CONTAINER=trueand command isstarttestNoContainerNonPid1Warning— asserts warning is absent when env var is not settestContainerNonPid1WarningAbsentForNonServerCommands— asserts warning is absent for non-server commands (e.g.export)JavaOptsScriptTesttests passspotless:checkpassesNotes
Claude Code was used to assist in writing the code, tests, and PR description for this contribution. All changes have been reviewed and are fully understood by the author.
Closes #48059