fix: persist build time spi options (#34157)

closes: #33902

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

common/src/main/java/org/keycloak/common/Profile.java

@@ -140,9 +140,6 @@ public class Profile {
             this(label, type, version, null, dependencies);
         }
 
-        /**
-         * allowNameKey should be false for new versioned features to disallow using a legacy name, like account2
-         */
         Feature(String label, Type type, int version, BooleanSupplier isAvailable, Feature... dependencies) {
             this.label = label;
             this.type = type;
@@ -161,8 +158,7 @@ public class Profile {
         }
 
         /**
-         * Get the key that uniquely identifies this feature, may be used by users if
+         * Get the key that uniquely identifies this feature
-         * allowNameKey is true.
          * <p>
          * {@link #getVersionedKey()} should instead be shown to users where possible.
          */

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java

@@ -623,6 +623,8 @@ class KeycloakProcessor {
                 if (!QuarkusPropertiesConfigSource.isSameSource(value)) {
                     return;
                 }
+            } else if (PropertyMappers.isSpiBuildTimeProperty(name)) {
+                value = Configuration.getConfigValue(name);
             }
         } else if (mapper.isBuildTime()) {
             name = mapper.getFrom();

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/Environment.java

@@ -35,8 +35,6 @@ import io.smallrye.config.SmallRyeConfig;
 
 import org.apache.commons.lang3.SystemUtils;
 import org.keycloak.common.Profile;
-import org.keycloak.common.profile.PropertiesFileProfileConfigResolver;
-import org.keycloak.common.profile.PropertiesProfileConfigResolver;
 import org.keycloak.quarkus.runtime.cli.command.AbstractCommand;
 import org.keycloak.quarkus.runtime.configuration.PersistedConfigSource;
 
@@ -242,7 +240,7 @@ public final class Environment {
         Profile profile = Profile.getInstance();
 
         if (profile == null) {
-            profile = Profile.configure(new QuarkusProfileConfigResolver(), new PropertiesProfileConfigResolver(QuarkusProfileConfigResolver::getConfig), new PropertiesFileProfileConfigResolver());
+            profile = Profile.configure(new QuarkusProfileConfigResolver());
         }
 
         return profile;

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMappers.java

@@ -38,6 +38,7 @@ import static org.keycloak.quarkus.runtime.configuration.KeycloakConfigSourcePro
 
 public final class PropertyMappers {
     
+    public static final String KC_SPI_PREFIX = "kc.spi";
     public static String VALUE_MASK = "*******";
     private static MappersConfig MAPPERS;
     private static final Logger log = Logger.getLogger(PropertyMappers.class);
@@ -94,8 +95,8 @@ public final class PropertyMappers {
                 && !"kc.config-file".equals(name);
     }
 
-    private static boolean isSpiBuildTimeProperty(String name) {
+    public static boolean isSpiBuildTimeProperty(String name) {
-        return name.startsWith("kc.spi") && (name.endsWith("provider") || name.endsWith("enabled"));
+        return name.startsWith(KC_SPI_PREFIX) && (name.endsWith("-provider") || name.endsWith("-enabled") || name.endsWith("-provider-default"));
     }
 
     private static boolean isFeaturesBuildTimeProperty(String name) {

quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartAutoBuildDistTest.java

@@ -26,8 +26,11 @@ import org.junit.jupiter.api.TestMethodOrder;
 import org.keycloak.it.junit5.extension.CLIResult;
 import org.keycloak.it.junit5.extension.DistributionTest;
 import org.keycloak.it.junit5.extension.RawDistOnly;
+import org.keycloak.it.junit5.extension.TestProvider;
 import org.keycloak.it.utils.KeycloakDistribution;
 
+import com.acme.provider.legacy.jpa.user.CustomUserProvider;
+
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand.OPTIMIZED_BUILD_OPTION_LONG;
@@ -129,4 +132,20 @@ public class StartAutoBuildDistTest {
         assertFalse(cliResult.getOutput().contains("Updating the configuration and installing your custom providers, if any. Please wait."));
         cliResult.assertStartedDevMode();
     }
+    
+    @Test
+    @TestProvider(CustomUserProvider.class)
+    @Order(10)
+    void testSpiAutoBuild(KeycloakDistribution dist) {
+        CLIResult cliResult = dist.run("start-dev", "--spi-user-provider=custom_jpa", "--spi-user-jpa-enabled=false");
+        cliResult.assertMessage("Updating the configuration");
+        cliResult.assertStartedDevMode();
+        dist.stop();
+        
+        // we should persist the spi provider and know not to rebuild
+        cliResult = dist.run("start-dev", "--spi-user-provider=custom_jpa", "--spi-user-jpa-enabled=false");
+        cliResult.assertNoMessage("Updating the configuration");
+        cliResult.assertStartedDevMode();
+    }
+    
 }

quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java

@@ -77,6 +77,17 @@ public class StartCommandDistTest {
         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");
     }
     
+    @WithEnvVars({"KC_SPI_EVENTS_LISTENER_JBOSS_LOGGING_ENABLED", "false"})
+    @Test
+    @RawDistOnly(reason = "Containers are immutable")
+    void noErrorSpiBuildtimeNotChanged(KeycloakDistribution dist) {
+        CLIResult result = dist.run("build");
+        result.assertBuild();
+
+        result = dist.run("start", "--optimized", "--http-enabled=true", "--hostname-strict=false");
+        result.assertStarted();
+    }
+
     @Test
     @Launch({ "--profile=dev", "start" })
     void failUsingDevProfile(LaunchResult result) {

testsuite/integration-arquillian/servers/auth-server/quarkus/src/main/content/conf/keycloak.conf

@@ -41,6 +41,3 @@ spi-connections-http-client-default-reuse-connections=false
 
 # set known protocol ports for basicsamltest
 spi-login-protocol-saml-known-protocols=http=8180,https=8543
-
-# expose something on management interface to turn it on
-health-enabled=true

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/ProfileAssume.java

@@ -58,4 +58,8 @@ public class ProfileAssume {
     public static void setTestContext(TestContext testContext) {
         TEST_CONTEXT = testContext;
     }
+    
+    public static Set<Profile.Feature> getDisabledFeatures() {
+        return DISABLED_FEATURES;
+    }
 }

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/AbstractQuarkusDeployableContainer.java

@@ -36,9 +36,9 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import javax.net.ssl.HostnameVerifier;
@@ -61,7 +61,10 @@ import org.jboss.logging.Logger;
 import org.jboss.shrinkwrap.api.Archive;
 import org.jboss.shrinkwrap.api.exporter.ZipExporter;
 import org.jboss.shrinkwrap.descriptor.api.Descriptor;
+import org.keycloak.common.Profile;
+import org.keycloak.common.Profile.Feature.Type;
 import org.keycloak.common.crypto.FipsMode;
+import org.keycloak.testsuite.ProfileAssume;
 import org.keycloak.testsuite.arquillian.SuiteContext;
 import org.keycloak.testsuite.model.StoreProvider;
 import org.keycloak.utils.StringUtil;
@@ -77,6 +80,7 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
 
     protected KeycloakQuarkusConfiguration configuration;
     protected List<String> additionalBuildArgs = Collections.emptyList();
+    protected Map<String, List<String>> spis = new HashMap<String, List<String>>();
 
     @Override
     public Class<KeycloakQuarkusConfiguration> getConfigurationClass() {
@@ -165,6 +169,9 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         commands.add("--http-port=" + configuration.getBindHttpPort());
         commands.add("--https-port=" + configuration.getBindHttpsPort());
         
+        commands.add("--http-relative-path=/auth");
+        commands.add("--health-enabled=true"); // expose something to management interface to turn it on
+
         if (suiteContext.get().isAuthServerMigrationEnabled()) {
             commands.add("--hostname-strict=false");
             commands.add("--hostname-strict-https=false");
@@ -185,7 +192,6 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         commands = configureArgs(commands);
 
         final StoreProvider storeProvider = StoreProvider.getCurrentProvider();
-        final Supplier<Boolean> shouldSetUpDb = () -> !restart.get() && !storeProvider.equals(StoreProvider.DEFAULT);
         final String cacheMode = System.getProperty("auth.server.quarkus.cluster.config", "local");
 
         if ("local".equals(cacheMode)) {
@@ -203,18 +209,15 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
 
         log.debugf("FIPS Mode: %s", configuration.getFipsMode());
 
-        // only run build during first execution of the server (if the DB is specified), restarts or when running cluster tests
+        if (configuration.getFipsMode() != FipsMode.DISABLED) {
-        if (restart.get() || "ha".equals(cacheMode) || shouldSetUpDb.get() || configuration.getFipsMode() != FipsMode.DISABLED) {
+            addFipsOptions(commands);
-            prepareCommandsForRebuilding(commands);
-
-            if (configuration.getFipsMode() != FipsMode.DISABLED) {
-                addFipsOptions(commands);
-            }
         }
 
         addStorageOptions(storeProvider, commands);
         addFeaturesOption(commands);
         
+        spis.values().forEach(commands::addAll);
+
         var features = getDefaultFeatures();
         if (features.contains("clusterless") || features.contains("multi-site")) {
             commands.add("--cache-remote-host=127.0.0.1");
@@ -229,26 +232,31 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         return commands;
     }
 
-    /**
-     * When enabling automatic rebuilding of the image, the `--optimized` argument must be removed,
-     * and all original build time parameters must be added.
-     */
-    private static void prepareCommandsForRebuilding(List<String> commands) {
-        commands.removeIf("--optimized"::equals);
-        commands.add("--http-relative-path=/auth");
-        commands.add("--health-enabled=true"); // expose something to management interface to turn it on
-    }
-
     protected void addFeaturesOption(List<String> commands) {
-        String enabledFeatures = configuration.getEnabledFeatures();
+        String enabledFeatures = Optional.ofNullable(configuration.getEnabledFeatures()).orElse("");
-        String disabledFeatures = configuration.getDisabledFeatures();
+        String disabledFeatures = Optional.ofNullable(configuration.getDisabledFeatures()).orElse("");
         
-        if (StringUtil.isBlank(enabledFeatures) && StringUtil.isBlank(disabledFeatures)) {
+        var disabled = ProfileAssume.getDisabledFeatures();
-            return;
+        // TODO: this is not ideal, we're trying to infer what should be enabled / disabled from what was captured
-        }
+        // as the disabled features. This at least does not understand the profile and may not age well.
-
+        // We should consider a direct mechanism - that is part of the persisted configuration - for toggling each
-        if (commands.stream().anyMatch(List.of("import", "export")::contains)) {
+        // feature
-            return;
+        if (disabled != null) {
+            enabledFeatures = "";
+            disabledFeatures = "";
+            for (Profile.Feature f : Profile.Feature.values()) {
+                if (disabled.contains(f)) {
+                    if (f.getType() == Type.DEFAULT) {
+                        disabledFeatures = f.getUnversionedKey() + (disabledFeatures.isEmpty() ? "" : ("," + disabledFeatures));
+                    }
+                } else {
+                    if (f.getType() != Type.DEFAULT) {
+                        enabledFeatures = f.getVersionedKey() + (enabledFeatures.isEmpty() ? "" : ("," + enabledFeatures));
+                    }
+                }
+            }
+        } else if (configuration.getFipsMode() != FipsMode.DISABLED) {
+            enabledFeatures = "fips" + (enabledFeatures.isEmpty() ? "" : ("," + enabledFeatures));
         }
 
         if (!StringUtil.isBlank(enabledFeatures)) {
@@ -258,9 +266,6 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         if (!StringUtil.isBlank(disabledFeatures)) {
             appendOrAddCommand(commands, "--features-disabled=", disabledFeatures);
         }
-
-        // enabling or disabling features requires rebuilding the image
-        prepareCommandsForRebuilding(commands);
     }
 
     private void appendOrAddCommand(List<String> commands, String command, String addition) {
@@ -307,6 +312,7 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
 
     public void resetConfiguration() {
         additionalBuildArgs = Collections.emptyList();
+        this.spis.clear();
     }
 
     protected void waitForReadiness() throws Exception {
@@ -316,11 +322,12 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         URL contextRoot = new URL(getBaseUrl(suiteContext) + "/auth/realms/master/");
         HttpURLConnection connection;
         long startTime = System.currentTimeMillis();
+        Exception e = null;
 
         while (true) {
             if (System.currentTimeMillis() - startTime > getStartTimeout()) {
                 stop();
-                throw new IllegalStateException("Timeout [" + getStartTimeout() + "] while waiting for Quarkus server");
+                throw new IllegalStateException("Timeout [" + getStartTimeout() + "] while waiting for Quarkus server", e);
             }
 
             checkLiveness();
@@ -346,6 +353,7 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
 
                 connection.disconnect();
             } catch (Exception ignore) {
+                e = ignore;
             }
         }
 
@@ -411,7 +419,6 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
     }
 
     private void addFipsOptions(List<String> commands) {
-        commands.add("--features=fips");
         commands.add("--fips-mode=" + configuration.getFipsMode().toString());
 
         log.debugf("Keystore file: %s, truststore file: %s",
@@ -444,4 +451,12 @@ public abstract class AbstractQuarkusDeployableContainer implements DeployableCo
         }
         return Arrays.stream(features.split(",")).collect(Collectors.toSet());
     }
+
+    public void setSpiConfig(String spi, List<String> args) {
+        this.spis.put(spi, args);
+    }
+    
+    public void removeSpiConfig(String spi) {
+        this.spis.remove(spi);
+    }
 }

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusEmbeddedDeployableContainer.java

@@ -2,6 +2,7 @@ package org.keycloak.testsuite.arquillian.containers;
 
 import java.util.List;
 import org.jboss.arquillian.container.spi.client.container.LifecycleException;
+import org.jboss.logging.Logger;
 import org.keycloak.Keycloak;
 import org.keycloak.common.Version;
 
@@ -10,6 +11,8 @@ import org.keycloak.common.Version;
  */
 public class KeycloakQuarkusEmbeddedDeployableContainer extends AbstractQuarkusDeployableContainer {
 
+    private static final Logger log = Logger.getLogger(KeycloakQuarkusEmbeddedDeployableContainer.class);
+    
     private static final String KEYCLOAK_VERSION = Version.VERSION;
 
     private Keycloak keycloak;
@@ -17,7 +20,9 @@ public class KeycloakQuarkusEmbeddedDeployableContainer extends AbstractQuarkusD
     @Override
     public void start() throws LifecycleException {
         try {
-            keycloak = configure().start(getArgs());
+            List<String> args = getArgs();
+            log.debugf("Quarkus process arguments: %s", args);
+            keycloak = configure().start(args);
             waitForReadiness();
         } catch (Exception e) {
             throw new RuntimeException(e);

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/containers/KeycloakQuarkusServerDeployableContainer.java

@@ -95,6 +95,7 @@ public class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep
         if (args != null) {
             commands.addAll(Arrays.asList(args));
         }
+        log.debugf("Non-server process arguments: %s", commands);
         ProcessBuilder pb = new ProcessBuilder(commands);
         Process p = pb.directory(wrkDir).inheritIO().start();
         try {
@@ -182,9 +183,6 @@ public class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep
         List<String> commands = new ArrayList<>(args);
 
         commands.add(0, getCommand());
-        commands.add("--optimized");
-
-        log.debugf("Quarkus parameters: %s", commands);
 
         return commands;
     }
@@ -192,6 +190,7 @@ public class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep
     private ProcessBuilder getProcessBuilder() {
         Map<String, String> env = new HashMap<>();
         String[] processCommands = getArgs(env).toArray(new String[0]);
+        log.debugf("Quarkus process arguments: %s", Arrays.asList(processCommands));
         ProcessBuilder pb = new ProcessBuilder(processCommands);
         pb.environment().putAll(env);
 
@@ -301,7 +300,11 @@ public class KeycloakQuarkusServerDeployableContainer extends AbstractQuarkusDep
                     }
                 }
             } catch (IOException e) {
-                throw new RuntimeException(e);
+                if ("Stream closed".equals(e.getMessage())) {
+                    System.out.println("Log has ended");
+                } else {
+                    throw new RuntimeException(e);
+                }
             }
         }
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/client/KeycloakTestingClient.java

@@ -81,9 +81,9 @@ public class KeycloakTestingClient implements AutoCloseable {
     }
 
     public void enableFeature(Profile.Feature feature) {
-        Set<Profile.Feature> enabledFeatures = testing().enableFeature(feature.toString());
+        Set<Profile.Feature> disabledFeatures = testing().enableFeature(feature.toString());
-        Assert.assertFalse(enabledFeatures.contains(feature));
+        Assert.assertFalse(disabledFeatures.contains(feature));
-        ProfileAssume.updateDisabledFeatures(enabledFeatures);
+        ProfileAssume.updateDisabledFeatures(disabledFeatures);
     }
 
     public void disableFeature(Profile.Feature feature) {

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SpiProvidersSwitchingUtils.java

@@ -8,7 +8,6 @@ import org.keycloak.testsuite.arquillian.annotation.SetDefaultProvider;
 import org.keycloak.testsuite.arquillian.containers.AbstractQuarkusDeployableContainer;
 import org.keycloak.utils.StringUtil;
 
-import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -82,12 +81,12 @@ public class SpiProvidersSwitchingUtils {
                         }
                     }
                 }
-                getQuarkusContainer(container).setAdditionalBuildArgs(args);
+                getQuarkusContainer(container).setSpiConfig(spiName, args);
             }
 
             @Override
             public void removeProviderConfig(Container container, String spiName) {
-                getQuarkusContainer(container).setAdditionalBuildArgs(Collections.emptyList());
+                getQuarkusContainer(container).removeSpiConfig(spiName);
             }
 
             private AbstractQuarkusDeployableContainer getQuarkusContainer(Container container) {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/ClientSearchTest.java

@@ -22,7 +22,9 @@ import org.jboss.arquillian.container.test.api.ContainerController;
 import org.jboss.arquillian.test.api.ArquillianResource;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.FixMethodOrder;
 import org.junit.Test;
+import org.junit.runners.MethodSorters;
 import org.keycloak.models.ClientProvider;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.testsuite.arquillian.containers.AbstractQuarkusDeployableContainer;
@@ -38,6 +40,8 @@ import static org.hamcrest.Matchers.containsInAnyOrder;
 /**
  * @author Vaclav Muzikar <vmuzikar@redhat.com>
  */
+// we need the unset test to run first
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
 public class ClientSearchTest extends AbstractClientTest {
     @ArquillianResource
     protected ContainerController controller;

testsuite/utils/src/main/java/org/keycloak/testsuite/PropertiesFileProfileConfigResolver.java

@@ -1,10 +1,14 @@
-package org.keycloak.common.profile;
+package org.keycloak.testsuite;
 
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.util.Properties;
 
+import org.keycloak.common.profile.ProfileException;
+import org.keycloak.common.profile.PropertiesProfileConfigResolver;
+
+
 public class PropertiesFileProfileConfigResolver extends PropertiesProfileConfigResolver {
 
     public PropertiesFileProfileConfigResolver() {

testsuite/utils/src/main/java/org/keycloak/testsuite/TestPlatform.java

@@ -23,7 +23,6 @@ import java.nio.file.Files;
 
 import org.jboss.logging.Logger;
 import org.keycloak.common.Profile;
-import org.keycloak.common.profile.PropertiesFileProfileConfigResolver;
 import org.keycloak.common.profile.PropertiesProfileConfigResolver;
 import org.keycloak.platform.PlatformProvider;