diff --git a/docs/documentation/upgrading/topics/changes/changes-26_6_0.adoc b/docs/documentation/upgrading/topics/changes/changes-26_6_0.adoc index 26ddb9b5641..076d6700d80 100644 --- a/docs/documentation/upgrading/topics/changes/changes-26_6_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-26_6_0.adoc @@ -25,16 +25,6 @@ Configuring + defers validation to the client's standard 'Valid redirect URIs'. Administrators must ensure that all configured URIs for the fields listed above use https. Clients attempting to update or register with http in these fields will fail validation when using the 'secure-client-uris' executor. -=== New CLI option overrides Quarkus transaction timeout property - -A new CLI option `transaction-default-timeout` has been introduced to configure the default transaction timeout in {project_name}. -This new option will override any value set in the `quarkus.transaction-manager.default-transaction-timeout` Quarkus property. - -While using Quarkus properties directly is not the supported way to configure {project_name}, some users may have used `quarkus.transaction-manager.default-transaction-timeout` to customize the transaction timeout. -If you are using this Quarkus property, the new `transaction-default-timeout` CLI option will take precedence and override your configuration when upgrading to this version. - -To maintain your desired transaction timeout configuration, migrate to using the `transaction-default-timeout` CLI option and remove the `quarkus.transaction-manager.default-transaction-timeout` property from your configuration. - === The Identity Provider issuer should be unique for the JWT authorization grant and client assertions The JWT authorization grant and the client assertion authentication use the `issuer` claim passed in the assertion to locate the Identity Provider defined in {project_name}, and that provider validates the claims and signature in the assertion. Since this version, the `issuer` configuration option should be unique to only identify one provider. The grant operation or the assertion client authentication fail if more providers are returned for a single issuer. In the same way, the Identity Providers, when configured to allow the JWT authorization grant or client assertions, are validated to have a unique issuer defined in its configuration. Any update or add operation will fail if the issuer collides with another provider. @@ -77,6 +67,27 @@ For more information about brokering, see link:{developerguide_link}#_identity_b Notable changes may include internal behavior changes that prevent common misconfigurations, bugs that are fixed, or changes to simplify running {project_name}. It also lists significant changes to internal APIs. +=== Usage of second-class options + +In prior releases the introduction of a first-class option with a default would cause the second-class option (typically spi or quarkus properties) to no longer be used at all. + +For example, `spi-truststore--file--hostname-verification-policy` was marked as deprecated after the introduction of `tls-hostname-verifier`. However since +`tls-hostname-verifier` included a default, direct usage of `spi-truststore--file--hostname-verification-policy` was no longer considered. + +To ensure that the further introduction of new first-class options does not immediately break existing configurations, the logic for determining the value of a second-class option +will now consider the direct usage of the second-class value ahead of a default, or derived default, from the first-class option. + +Please review the server startup to see if there are any warnings about the continued usage of second-class options, and update them accordingly. + +=== New option for transaction timeout + +A new option `transaction-default-timeout` has been introduced to configure the default transaction timeout in {project_name}. + +While using Quarkus properties directly is not the supported way to configure {project_name}, some users may have used `quarkus.transaction-manager.default-transaction-timeout` to customize the transaction timeout. +If you are using this Quarkus property, the new `transaction-default-timeout` CLI option when set will take precedence and override your configuration when upgrading to this version. + +To maintain your desired transaction timeout configuration, migrate to using the `transaction-default-timeout` CLI option and remove the `quarkus.transaction-manager.default-transaction-timeout` property from your configuration. + === Endpoints are opened while Keycloak is initializing By default, {project_name} now opens its HTTP(S) and Management ports while initialization is still in progress. diff --git a/docs/guides/server/configuration.adoc b/docs/guides/server/configuration.adoc index 1e408888af2..240244dc8b8 100644 --- a/docs/guides/server/configuration.adoc +++ b/docs/guides/server/configuration.adoc @@ -43,6 +43,9 @@ Based on the priority of application, the value that is used at startup is `cliV If `--db-url=cliValue` had not been used, the applied value would be `KC_DB_URL=envVarValue`. If the value were not applied by either the command line or an environment variable, `db-url=confFileValue` would be used. If none of the previous values were applied, the value `kc.db-url=keystoreValue` would be used due to the lowest priority among the available configuration sources. +{project_name} configuration options often map to lower-level Quarkus or spi options. We refer to these as first-class and second-class configuration respectively. +There are startup warnings to indicate if you are using a second-class option. For example if you try to directly use `quarkus.http.port`, you will be warned to use `kc.http-port` instead. + == Formats for configuration The configuration uses a _unified-per-source_ format, which simplifies translation of a key/value pair from one configuration source to another. Note that these formats apply to spi options as well. @@ -144,8 +147,6 @@ You can use only a https://github.com/keycloak/keycloak/blob/main/quarkus/runtim You can also store Quarkus properties in a Java KeyStore. -Note that some Quarkus properties are already mapped in the {project_name} configuration, such as `quarkus.http.port` and similar essential properties. If the property is used by {project_Name}, defining that property key in `quarkus.properties` has no effect. The {project_Name} configuration value takes precedence over the Quarkus property value. - === Using special characters in values {project_name} depends upon Quarkus and MicroProfile for processing configuration values. Be aware that value expressions are supported. For example, `$\{some_key}` evaluates to the value of `some_key`. 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 d0a4b3247da..208a965caec 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 @@ -268,14 +268,13 @@ public class Picocli { // first validate the advertised property names // - this allows for efficient resolution of wildcard values and checking spi options Configuration.getPropertyNames().forEach(name -> { - if (!name.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX)) { - return; // there are canonical mappings to kc. values - no need to consider alternative forms - } - if (!options.includeRuntime) { - checkRuntimeSpiOptions(name, ignoredRunTime); - } - if (PropertyMappers.isMaybeSpiBuildTimeProperty(name)) { - ambiguousSpi.add(name); + if (name.startsWith(PropertyMappers.KC_SPI_PREFIX)) { + if (!options.includeRuntime) { + checkRuntimeSpiOptions(name, ignoredRunTime); + } + if (PropertyMappers.isMaybeSpiBuildTimeProperty(name)) { + ambiguousSpi.add(name); + } } PropertyMapper mapper = PropertyMappers.getMapper(name); if (mapper == null) { @@ -288,8 +287,10 @@ public class Picocli { secondClassOptions.put(name, forKey.getFrom()); } } - if (!mapper.hasWildcard()) { - return; // non-wildcard options will be validated in the next pass + if (!mapper.hasWildcard() // non-wildcard options will be validated in the next pass + // validate only canonical mappings to kc. values - no need to consider alternative forms + || !name.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX)) { + return; } validateProperty(abstractCommand, options, ignoredRunTime, disabledBuildTime, disabledRunTime, deprecatedInUse, missingOption, disabledMappers.contains(mapper), forKey, unnecessary); @@ -335,7 +336,11 @@ public class Picocli { warn("The following SPI options are using the legacy format and are not being treated as build time options. Please use the new format with the appropriate -- separators to resolve this ambiguity: " + String.join("\n", ambiguousSpi)); } secondClassOptions.forEach((key, firstClass) -> { - warn("Please use the first-class option `%s` instead of `%s`".formatted(firstClass, key)); + if (Configuration.getConfigValue(firstClass).getConfigSourceName() != null) { + warn("With the first-class option `%s` set, you should remove the usage of `%s`".formatted(firstClass, key)); + } else { + warn("Please use the first-class option `%s` instead of `%s`".formatted(firstClass, key)); + } }); if (!unnecessary.isEmpty()) { info("The following options were specified, but are typically not relevant for this command: " + String.join("\n", unnecessary)); @@ -444,9 +449,6 @@ public class Picocli { } private static void checkRuntimeSpiOptions(String key, final List ignoredRunTime) { - if (!key.startsWith(PropertyMappers.KC_SPI_PREFIX)) { - return; - } boolean buildTimeOption = PropertyMappers.isSpiBuildTimeProperty(key); if (!buildTimeOption) { diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java index ca7e602ed58..8b10cdd249e 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java @@ -104,7 +104,7 @@ public final class Build extends AbstractCommand { if (Start.NAME.equals(cmd) || Build.NAME.equals(cmd)) { executionError(spec.commandLine(), Messages.devProfileNotAllowedError(cmd)); } - } else if (Configuration.getConfigValue(DB).getConfigSourceOrdinal() == 0) { + } else if (Configuration.isDefault(Configuration.getConfigValue(DB))) { picocli.warn("Usage of the default value for the db option in the production profile is deprecated. Please explicitly set the db instead."); } } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java index 0ae82da3873..ceccb3a7af7 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java @@ -22,6 +22,7 @@ import java.util.Optional; import java.util.Properties; import org.keycloak.config.Option; +import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper; import org.keycloak.utils.StringUtil; import io.quarkus.runtime.configuration.ConfigUtils; @@ -50,9 +51,16 @@ public final class Configuration { return getOptionalBooleanValue(NS_KEYCLOAK_PREFIX + option.getKey()).orElse(false); } + /** + * Return true if the value is not derived (meaning that the config source name is set) + * and the config source is something that a user can change the values in + */ public static boolean isUserModifiable(ConfigValue configValue) { - // for now we won't validate these as it's not expected for the user to specify options from a source below our properties - return configValue.getConfigSourceName() != null && configValue.getConfigSourceOrdinal() >= KeycloakPropertiesConfigSource.PROPERTIES_FILE_ORDINAL; + return configValue.getConfigSourceName() != null && !isDefault(configValue); + } + + public static boolean isDefault(ConfigValue configValue) { + return configValue.getConfigSourceOrdinal() <= PropertyMapper.DEFAULT_VALUE_ORDINAL; } public static boolean isSet(Option option) { diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java index 8525a50a094..9ebf63fbb54 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java @@ -61,6 +61,7 @@ import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvi public class PropertyMapper { + public static final int DEFAULT_VALUE_ORDINAL = 251; // keycloak defaults are logically higher than classpath entries protected final Option option; private final String to; private Function enabled; @@ -117,12 +118,18 @@ public class PropertyMapper { *

* In preference order we are looking for: * 

-     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ] ---> [ {@link #to} ]
-     * (explicit)     (derived)           (fallback)         (fallback)
+     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ]
+     * (explicit)      (derived)           (fallback)
      *
*

* - * 2. Transform found value + * 2. Use to + *

+ * If we are looking for the `to` value, and no value was found from step 1 or it is effectively a default value, + * then use the `to` value if it was set by the user. + *

+ * + * 3. Transform found value *

* If we found a value for the attribute name, it needs to be transformed via {@link #transformValue} method. How to transform it? *

    @@ -130,6 +137,10 @@ public class PropertyMapper { *
  • If the value contains an expression, expand it using SmallRye logic *
  • Finally the returned {@link ConfigValue} is made to match what was requested - with the name, value, rawValue, and ordinal set appropriately. *
+ * + * 4. Use to + *

+ * If no value is found or mapped, return the `to` value. */ ConfigValue getConfigValue(String name, ConfigSourceInterceptorContext context) { String from = getFrom(); @@ -138,6 +149,7 @@ public class PropertyMapper { // we don't want the NestedPropertyMappingInterceptor to restart the chain here, so we force a proceed // this ensures that mapFrom transformers, and regular transformers are applied exclusively - not chained ConfigValue config = convertValue(NestedPropertyMappingInterceptor.proceed(context, from)); + Optional directValue = null; boolean parentValue = false; if (mapFrom != null && (config == null || config.getValue() == null)) { @@ -148,12 +160,21 @@ public class PropertyMapper { parentValue = true; } + // instead of using a default or mapping from a default, check if the value is already set and use that instead + // a warning should be emitted about this in picocli + if (!name.equals(from) && (config == null || config.getValue() == null || (parentValue && Configuration.isDefault(config)))) { + directValue = Optional.ofNullable(context.proceed(name)); + if (directValue.filter(c -> c.getValue() != null && Configuration.isUserModifiable(c)).isPresent()) { + return directValue.get(); + } + } + if (config != null && config.getValue() != null) { config = transformValue(name, config, context, parentValue); } else { String defaultValue = this.option.getDefaultValue().map(Option::getDefaultValueString).orElse(null); config = transformValue(name, new ConfigValueBuilder().withName(name) - .withValue(defaultValue).withRawValue(defaultValue).build(), + .withValue(defaultValue).withRawValue(defaultValue).withConfigSourceOrdinal(DEFAULT_VALUE_ORDINAL).build(), context, false); } @@ -162,6 +183,9 @@ public class PropertyMapper { } // now try any defaults from quarkus + if (directValue != null) { + return directValue.orElse(null); + } return context.proceed(name); } diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java index d5f33bef380..1cd15636606 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java @@ -756,7 +756,26 @@ public class PicocliTest extends AbstractConfigurationTest { public void derivedPropertyUsage() { NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--hostname=localhost", "--spi-hostname-v2-hostname=second-class"); assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); - assertThat(nonRunningPicocli.getOutString(), containsString("Please use the first-class option `kc.hostname` instead of `kc.spi-hostname-v2-hostname`")); + assertThat(nonRunningPicocli.getOutString(), containsString("With the first-class option `kc.hostname` set, you should remove the usage of `kc.spi-hostname-v2-hostname`")); + } + + @Test + public void quarkusPropertyInQuarkusPropertiesWarning() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), + containsString("Please use the first-class option `kc.http-port` instead of `quarkus.http.port`")); + } + + @Test + public void quarkusPropertyInQuarkusPropertiesWarningWhenKcOptionSet() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + // When kc.http-port is explicitly set, should still warn about quarkus.http.port + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port=7070"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), + containsString("With the first-class option `kc.http-port` set, you should remove the usage of `quarkus.http.port`")); } @Test diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java index 41e803a0f1f..7a12c60fe2b 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java @@ -713,6 +713,25 @@ public class ConfigurationTest extends AbstractConfigurationTest { assertEquals("200k", config.getConfigValue("quarkus.http.limits.max-header-size").getValue()); } + @Test + public void testQuarkusPropertyTakesPrecedenceOverDefault() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + ConfigArgsConfigSource.setCliArgs(""); + SmallRyeConfig config = createConfig(); + assertEquals("9090", config.getConfigValue("quarkus.http.port").getValue()); + // the kc.http-port still returns the default since no kc.* option was explicitly set + assertEquals("8080", config.getConfigValue("kc.http-port").getValue()); + } + + @Test + public void testKcPropertyTakesPrecedenceOverQuarkusProperty() { + // Explicitly setting kc.http-port should override the quarkus.properties value (9090) + ConfigArgsConfigSource.setCliArgs("--http-port=7070"); + SmallRyeConfig config = createConfig(); + assertEquals("7070", config.getConfigValue("quarkus.http.port").getValue()); + assertEquals("7070", config.getConfigValue("kc.http-port").getValue()); + } + @Test public void testPostgresTLSOptions() { doDatabaseTlsOptionTest("postgres", diff --git a/quarkus/runtime/src/test/resources/conf/quarkus.properties b/quarkus/runtime/src/test/resources/conf/quarkus.properties index 7baeeca7e37..e193987f676 100644 --- a/quarkus/runtime/src/test/resources/conf/quarkus.properties +++ b/quarkus/runtime/src/test/resources/conf/quarkus.properties @@ -2,9 +2,6 @@ # Logging configuration. INFO is the default level for most of the categories quarkus.log.min-level=TRACE -quarkus.log.level = INFO -quarkus.log.category."org.jboss.resteasy.resteasy_jaxrs.i18n".level=WARN -quarkus.log.category."org.infinispan.transaction.lookup.JBossStandaloneJTAManagerLookup".level=WARN # For test nested properties quarkus.datasource.foo = jdbc:h2:file:${kc.home.dir:${kc.db.url.path:~}}/data/keycloakdb