Repository navigation
fix: reviewed hidden options for what can be removed - #48020
Conversation
|
Actually I'll tease this apart into separate prs. edit: done This pr just changes hidden to synthetic and does not expand what options are exposed. |
04553af to
f108a5f
Compare
|
@mabartos should be ready for a review. This refined the synthetic concept so that it's still possible to do a get of the from / kc key and get the derived value when possible so that the existing usage of the logging and header related options could remain. The mapping logic still won't look for the direct setting of the value, so there's no concern about it being set in an unexpected way. |
There was a problem hiding this comment.
Pull request overview
This PR follows up on #48003 by reducing reliance on hidden CLI/config options, converting several of them into synthetic (internal/derived) options, and adjusting configuration/property-mapping behavior and tests accordingly.
Changes:
- Remove or convert a set of previously hidden options (DB/telemetry/tracing/proxy/logging/transaction related) to synthetic/internal-only options.
- Update property-mapper duplicate-handling to allow ca
10000
nonical mappers for
kc.*keys when synthetic mappers also exist. - Update unit/integration tests to reflect the new exposed/hidden/synthetic option behavior and resulting external (Quarkus) config values.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/db/DbTest.java | Adjusts DB test to assert on the Quarkus property key (PG target server type) rather than a removed Keycloak option. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/TelemetryConfigurationTest.java | Updates telemetry defaults test expectations after removing telemetry-enabled. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java | Updates proxy mapping assertions and DB property/connect-timeout related expectations after hidden option cleanup. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java | Removes precedence tests for hidden telemetry/tracing header “parent” options and updates expectations for synthetic behavior. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/TransactionPropertyMappers.java | Removes dependency on a dedicated hidden lock-timeout option and maps from transaction-setup-timeout instead. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/TelemetryPropertyMappers.java | Switches quarkus.otel.enabled mapping to be driven by feature state rather than telemetry-enabled. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/ProxyPropertyMappers.java | Consolidates multiple proxy sub-mappings behind a synthetic option derived from proxy-headers. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMappers.java | Changes duplicate-mapper handling to prefer non-synthetic canonical mappers for kc.* keys. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java | Tightens hidden/description semantics and enforces descriptions for non-hidden options at build time. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/ImportPropertyMappers.java | Converts import-mode placeholder option from hidden to synthetic. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/ExportPropertyMappers.java | Converts export-mode placeholder option from hidden to synthetic. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/DatabasePropertyMappers.java | Removes Oracle TLS transport mapping option and instead amends default Oracle URLs to use tcps: when TLS is enabled. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/ShortErrorMessageHandler.java | Adjusts disabled-option messaging so disabled hidden options are treated as unknown options. |
| quarkus/config-api/src/main/java/org/keycloak/config/database/Database.java | Refactors Oracle default URL composition and introduces a shared Oracle URL prefix constant. |
| quarkus/config-api/src/main/java/org/keycloak/config/TransactionOptions.java | Removes the hidden transaction-db-lock-timeout option. |
| quarkus/config-api/src/main/java/org/keycloak/config/TracingOptions.java | Marks tracing-headers as synthetic. |
| quarkus/config-api/src/main/java/org/keycloak/config/TelemetryOptions.java | Marks telemetry-logs-headers / telemetry-metrics-headers as synthetic and removes telemetry-enabled. |
| quarkus/config-api/src/main/java/org/keycloak/config/ProxyOptions.java | Removes several proxy options that were effectively intermediates. |
| quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java | Marks log “enabled” intermediates as synthetic. |
| quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java | Removes hidden DB options no longer required after mapping refactors. |
| @@ -112,7 +112,7 @@ public enum TracingCompression { | |||
|
|
|||
| public static final Option<List<String>> TRACING_HEADERS = OptionBuilder.listOptionBuilder("tracing-headers", String.class) | |||
| .category(OptionCategory.TRACING) | |||
| .hidden() | |||
| .synthetic() | |||
| .description("Hidden option for OpenTelemetry headers that will be part of the exporter request. Values in format 'key1=val1,key2=val2'. Overrides the 'tracing-header-<header>' options.") | |||
There was a problem hiding this comment.
TRACING_HEADERS is now marked synthetic(). Synthetic options are not read from their own config key (see PropertyMapper#getConfigValue), so this option can no longer “override the 'tracing-header-
| .description("Hidden option for OpenTelemetry headers that will be part of the exporter request. Values in format 'key1=val1,key2=val2'. Overrides the 'tracing-header-<header>' options.") | |
| .description("Hidden synthetic option aggregating OpenTelemetry exporter headers from the 'tracing-header-<header>' options. Values use the format 'key1=val1,key2=val2'.") |
closes: keycloak#48003 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: keycloak#48003 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
Addressed the review comments. |
This isn't true yet for synthetic wildcards - you cannot get the value using the from ( ). I can either try to expand the logic in this PR, or capture a follow up and in general think about speading up the matching routine as our number of wildcards has continued to grow. |
| fromOption(TransactionOptions.TRANSACTION_SETUP_TIMEOUT) | ||
| .mapFrom(TransactionOptions.TRANSACTION_SETUP_TIMEOUT) |
There was a problem hiding this comment.
@shawkins Does it work correctly? If I call the Configuration.getConf(TRANSACTION_SETUP_TIMEOUT), how does it know which property mapper should be used? Isn't it required to create a synthetic version of the option here? fromOption(TransactionOptions.TRANSACTION_SETUP_TIMEOUT)
There was a problem hiding this comment.
@mabartos In the pr that introduced synthetic options, this was added as implicit behavior. If the from and mapFrom as the same, the PropertyMapper builder will implicitly create a synthetic option.
The intention with the initial changes was to make so most of the time an explicit synthetic option was not needed.
The changes in this pr relax that, but not completely: #48020 (comment)
* fix: replacing hidden options with synthetic with possible closes: keycloak#48003 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * fix: refining synthetic logic and removing more hidden options closes: keycloak#48003 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * addressing review comments Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
also converted db-connect-timeout to an option for all datasourcesand exposed more management optionscloses: #48003
The only remaining hidden options once this and the related prs are merged will be those that we either explicitly rely on as an intermediate value, or possible need (db-dialect, several header options).