Make Keycloak fail with an error when the persisted build options dif…

…fers from those provided * PropertyException is now thrown instead of a warning * Operator guides clarification around health and metrics options Closes: keycloak#32717 Signed-off-by: Peter Zaoral <[email protected]>
Pepo48 · Sep 30, 2024 · 4b177ea · 4b177ea
1 parent 8d314a6
commit 4b177ea
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 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
@@ -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
@@ -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
@@ -375,7 +375,7 @@ public static void validateConfig(List<String> 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<String> 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,15 +460,15 @@ private static void checkSpiOptions(IncludeOptions options, final List<String> i
                 continue;
             }
             boolean buildTimeOption = key.endsWith("-provider") || key.endsWith("-provider-default") || key.endsWith("-enabled");
-            
+
             ConfigValue configValue = Configuration.getConfigValue(key);
             String configValueStr = configValue.getValue();
 
             // don't consider missing or anything below standard env properties
             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
@@ -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/...rg/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java b/...rg/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,14 +300,17 @@ public void run() {
                         loggedLines.remove(0);
                     }
                 }
-            }
-            catch (IOException e) {
+            } catch (IOException e) {
                 throw new RuntimeException(e);
             }
         }
 
         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"));
+        }
     }
 }