From 4b177ea62b506d95bc6acb513bb1306f9cf18e2c Mon Sep 17 00:00:00 2001 From: Peter Zaoral Date: Tue, 24 Sep 2024 14:57:34 +0200 Subject: [PATCH] Make Keycloak fail with an error when the persisted build options differs from those provided * PropertyException is now thrown instead of a warning * Operator guides clarification around health and metrics options Closes: #32717 Signed-off-by: Peter Zaoral --- .../topics/changes/changes-26_0_0.adoc | 6 ++++- .../guides/operator/customizing-keycloak.adoc | 2 ++ .../keycloak/quarkus/runtime/cli/Picocli.java | 8 +++---- .../it/cli/dist/StartCommandDistTest.java | 8 +++---- ...cloakQuarkusServerDeployableContainer.java | 24 ++++++++++++++++--- 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc index 2d598be5e85c..02fe373849bb 100644 --- a/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc @@ -302,7 +302,7 @@ The following event types are now deprecated and will be removed in a future ver = `--import-realm` option can import the master realm -When running a `start` or `start-dev` command with the `--import-realm` option before the master realm exists, it will be imported if it exists in the import material. The previous behavior was that the master realm was created first, then its import skipped. +When running a `start` or `start-dev` command with the `--import-realm` option before the master realm exists, it will be imported if it exists in the import material. The previous behavior was that the master realm was created first, then its import skipped. = BouncyCastle FIPS updated @@ -325,3 +325,7 @@ Keycloak JS now utilizes the Web Crypto API to calculate the SHA-256 digests nee - `createRegisterUrl()` Make sure to update your code to `await` these methods. + += Stricter startup behavior for build-time options + +When the provided build-time options differ at startup from the values persisted in the server image during the last optimized {project_name} build, {project_name} will now fail to start. Previously, a warning message was displayed in such cases. diff --git a/docs/guides/operator/customizing-keycloak.adoc b/docs/guides/operator/customizing-keycloak.adoc index 3f12b45b0469..23af84c660ee 100644 --- a/docs/guides/operator/customizing-keycloak.adoc +++ b/docs/guides/operator/customizing-keycloak.adoc @@ -22,6 +22,8 @@ To avoid this delay, you can provide a custom image with the augmentation built- With a custom image, you can also specify the Keycloak _build-time_ configurations and extensions during the build of the container. +WARNING: When using optimized custom image, `health-enabled` and `metrics-enabled` options need to be explicitly set in the Containerfile. + For instructions on how to build such an image, see <@links.server id="containers"/>. === Providing a custom {project_name} image diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java index bdd5f995d4a0..1b85d21c81cb 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java @@ -375,7 +375,7 @@ public static void validateConfig(List cliArgs, AbstractCommand abstract if (options.includeRuntime) { disabledMappers.addAll(PropertyMappers.getDisabledRuntimeMappers().values()); } - + checkSpiOptions(options, ignoredBuildTime, ignoredRunTime); for (OptionCategory category : abstractCommand.getOptionCategories()) { @@ -430,7 +430,7 @@ public static void validateConfig(List cliArgs, AbstractCommand abstract Logger logger = Logger.getLogger(Picocli.class); // logger can't be instantiated in a class field if (!ignoredBuildTime.isEmpty()) { - logger.warn(format("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n", + throw new PropertyException(format("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n", String.join(", ", ignoredBuildTime))); } else if (!ignoredRunTime.isEmpty()) { logger.warn(format("The following run time options were found, but will be ignored during build time: %s\n", @@ -460,7 +460,7 @@ private static void checkSpiOptions(IncludeOptions options, final List i continue; } boolean buildTimeOption = key.endsWith("-provider") || key.endsWith("-provider-default") || key.endsWith("-enabled"); - + ConfigValue configValue = Configuration.getConfigValue(key); String configValueStr = configValue.getValue(); @@ -468,7 +468,7 @@ private static void checkSpiOptions(IncludeOptions options, final List i if (configValueStr == null || configValue.getConfigSourceOrdinal() < 300) { continue; } - + if (!options.includeBuildTime) { if (buildTimeOption) { String currentValue = getRawPersistedProperty(key).orElse(null); diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java index f2ea4435be42..c8f5830763fa 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java @@ -69,12 +69,12 @@ void warnSpiRuntimeAtBuildtime(LaunchResult result) { @Test @RawDistOnly(reason = "Containers are immutable") - void warnSpiBuildtimeAtRuntime(KeycloakDistribution dist) { + void errorSpiBuildtimeAtRuntime(KeycloakDistribution dist) { CLIResult result = dist.run("build"); result.assertBuild(); result = dist.run("start", "--optimized", "--http-enabled=true", "--hostname-strict=false", "--spi-events-listener-jboss-logging-enabled=false"); - result.assertMessage("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.spi-events-listener-jboss-logging-enabled"); + result.assertError("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.spi-events-listener-jboss-logging-enabled"); } @Test @@ -211,12 +211,12 @@ void testStartAfterStartDev(KeycloakDistribution dist) { @Test @RawDistOnly(reason = "Containers are immutable") - void testWarningWhenOverridingNonCliBuildOptionsDuringStart(KeycloakDistribution dist) { + void testErrorWhenOverridingNonCliBuildOptionsDuringStart(KeycloakDistribution dist) { CLIResult cliResult = dist.run("build", "--features=preview"); cliResult.assertBuild(); dist.setEnvVar("KC_DB", "postgres"); cliResult = dist.run("start", "--optimized", "--hostname=localhost", "--http-enabled=true"); - cliResult.assertMessage("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.db"); + cliResult.assertError("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.db"); } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java index b44dad127a3d..2e507ed3fcbc 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java @@ -51,7 +51,22 @@ public void start() throws LifecycleException { logProcessor = new LogProcessor(new BufferedReader(new InputStreamReader(container.getInputStream()))); stdoutForwarderThread = new Thread(logProcessor); stdoutForwarderThread.start(); - waitForReadiness(); + + try { + waitForReadiness(); + } catch (Exception e) { + if (logProcessor.containsBuildTimeOptionsError()) { + log.warn("The build time options have values that differ from what is persisted. Restarting container..."); + container.destroy(); + container = startContainer(); + logProcessor = new LogProcessor(new BufferedReader(new InputStreamReader(container.getInputStream()))); + stdoutForwarderThread = new Thread(logProcessor); + stdoutForwarderThread.start(); + waitForReadiness(); + } else { + throw e; + } + } } catch (Exception e) { throw new RuntimeException(e); } @@ -285,8 +300,7 @@ public void run() { loggedLines.remove(0); } } - } - catch (IOException e) { + } catch (IOException e) { throw new RuntimeException(e); } } @@ -294,5 +308,9 @@ public void run() { public String getBufferedLog() { return String.join("\n", loggedLines); } + + public boolean containsBuildTimeOptionsError() { + return loggedLines.stream().anyMatch(line -> line.contains("The following build time options have values that differ from what is persisted")); + } } }