From d5d6390b1c3a4ebf938870475a5769f7a2f1a557 Mon Sep 17 00:00:00 2001 From: Peter Zaoral Date: Mon, 30 Sep 2024 19:28:23 +0200 Subject: [PATCH] Make Keycloak fail with an error when the persisted build options differs from those provided (#33241) * 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 2d598be5e8..02fe373849 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 3f12b45b04..23af84c660 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 bdd5f995d4..1b85d21c81 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 class Picocli { if (options.includeRuntime) { disabledMappers.addAll(PropertyMappers.getDisabledRuntimeMappers().values()); } - + checkSpiOptions(options, ignoredBuildTime, ignoredRunTime); for (OptionCategory category : abstractCommand.getOptionCategories()) { @@ -430,7 +430,7 @@ public class Picocli { 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 @@ public class Picocli { 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 @@ public class Picocli { 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 f2ea4435be..c8f5830763 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 @@ public class StartCommandDistTest { @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 @@ public class StartCommandDistTest { @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 b44dad127a..2e507ed3fc 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 class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep 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 class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep loggedLines.remove(0); } } - } - catch (IOException e) { + } catch (IOException e) { throw new RuntimeException(e); } } @@ -294,5 +308,9 @@ public class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep 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")); + } } }