From 332bf122f1af16b4187af5ecc3129bd63e504335 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Mon, 3 Feb 2025 11:20:42 -0500 Subject: [PATCH] fix: detecting provider changes when running start optimized (#35845) closes: #34665 Signed-off-by: Steve Hawkins --- ...ycloakRealmImportJobDependentResource.java | 2 - .../integration/RealmImportTest.java | 11 +- .../keycloak/quarkus/runtime/cli/Picocli.java | 105 +++++++++--------- .../quarkus/runtime/cli/PicocliTest.java | 45 ++++++++ .../it/utils/RawKeycloakDistribution.java | 4 +- 5 files changed, 106 insertions(+), 61 deletions(-) diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakRealmImportJobDependentResource.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakRealmImportJobDependentResource.java index af9bf6abf08..33cb7b81259 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakRealmImportJobDependentResource.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakRealmImportJobDependentResource.java @@ -93,8 +93,6 @@ public class KeycloakRealmImportJobDependentResource extends KubernetesDependent // The Job should not connect to the cache envvars.add(new EnvVarBuilder().withName(cacheEnvVarName).withValue("local").build()); - // The Job doesn't need health to be enabled - envvars.add(new EnvVarBuilder().withName(healthEnvVarName).withValue("false").build()); if (replacePlaceholders) { for (Map.Entry secret : primary.getSpec().getPlaceholders().entrySet()) { diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/RealmImportTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/RealmImportTest.java index 0d54cae6115..475f9a4762a 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/RealmImportTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/RealmImportTest.java @@ -37,7 +37,6 @@ import org.keycloak.operator.Config; import org.keycloak.operator.controllers.KeycloakServiceDependentResource; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; import org.keycloak.operator.crds.v2alpha1.realmimport.KeycloakRealmImport; -import org.keycloak.operator.crds.v2alpha1.realmimport.KeycloakRealmImportBuilder; import org.keycloak.operator.crds.v2alpha1.realmimport.Placeholder; import org.keycloak.operator.testsuite.utils.CRAssert; import org.keycloak.operator.testsuite.utils.K8sUtils; @@ -75,7 +74,7 @@ public class RealmImportTest extends BaseOperatorTest { deleteDB(); deployDB(); } - + @Override public void afterEach(QuarkusTestMethodContext context) { super.afterEach(context); @@ -140,10 +139,10 @@ public class RealmImportTest extends BaseOperatorTest { "MY_SMTP_SERVER", new Placeholder(new SecretKeySelectorBuilder().withName("keycloak-smtp-secret").withKey("SMTP_SERVER").build()))); return realmImport; }); - + // Assert var envvars = assertWorkingRealmImport(kc); - + assertThat(envvars.stream().filter(e -> e.getName().equals("MY_SMTP_PORT")).findAny().get().getValueFrom().getSecretKeyRef().getKey()).isEqualTo("SMTP_PORT"); assertThat(envvars.stream().filter(e -> e.getName().equals("MY_SMTP_SERVER")).findAny().get().getValueFrom().getSecretKeyRef().getKey()).isEqualTo("SMTP_SERVER"); } @@ -180,7 +179,7 @@ public class RealmImportTest extends BaseOperatorTest { assertThat(container).isNotNull(); var envvars = container.getEnv(); assertThat(envvars.stream().filter(e -> e.getName().equals(getKeycloakOptionEnvVarName("cache"))).findAny().get().getValue()).isEqualTo("local"); - assertThat(envvars.stream().filter(e -> e.getName().equals(getKeycloakOptionEnvVarName("health-enabled"))).findAny().get().getValue()).isEqualTo("false"); + assertThat(envvars.stream().filter(e -> e.getName().equals(getKeycloakOptionEnvVarName("health-enabled"))).findAny().isEmpty()); assertThat(job.getSpec().getTemplate().getSpec().getImagePullSecrets().size()).isEqualTo(1); assertThat(job.getSpec().getTemplate().getSpec().getImagePullSecrets().get(0).getName()).isEqualTo("my-empty-secret"); @@ -199,7 +198,7 @@ public class RealmImportTest extends BaseOperatorTest { }); assertThat(getJobArgs()).contains("build"); - + return envvars; } 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 cdced90f9c2..91bb53a8778 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 @@ -27,7 +27,6 @@ import static org.keycloak.quarkus.runtime.cli.OptionRenderer.decorateDuplicitOp import static org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand.OPTIMIZED_BUILD_OPTION_LONG; import static org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource.parseConfigArgs; import static org.keycloak.quarkus.runtime.configuration.Configuration.OPTION_PART_SEPARATOR; -import static org.keycloak.quarkus.runtime.configuration.Configuration.getRawPersistedProperty; import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX; import static org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers.maskValue; import static picocli.CommandLine.Model.UsageMessageSpec.SECTION_KEY_COMMAND_LIST; @@ -76,6 +75,7 @@ import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers; import io.quarkus.bootstrap.runner.QuarkusEntryPoint; import io.quarkus.runtime.LaunchMode; import io.smallrye.config.ConfigValue; +import io.smallrye.mutiny.tuples.Functions.TriConsumer; import picocli.CommandLine; import picocli.CommandLine.DuplicateOptionAnnotationsException; import picocli.CommandLine.Help.Ansi; @@ -91,7 +91,7 @@ import picocli.CommandLine.ParseResult; public class Picocli { - private static final String KC_PROVIDER_FILE_PREFIX = "kc.provider.file."; + static final String KC_PROVIDER_FILE_PREFIX = "kc.provider.file."; public static final String ARG_PREFIX = "--"; public static final String ARG_SHORT_PREFIX = "-"; public static final String NO_PARAM_LABEL = "none"; @@ -350,12 +350,26 @@ public class Picocli { return; } + final List ignoredBuildTime = new ArrayList<>(); + + if (!options.includeBuildTime) { + // check for provider changes, or overrides of existing persisted options + // we have to ignore things like the profile properties because the commands set them at runtime + checkChangesInBuildOptions((key, oldValue, newValue) -> { + if (key.startsWith(KC_PROVIDER_FILE_PREFIX)) { + throw new PropertyException("A provider JAR was updated since the last build, please rebuild for this to be fully utilized."); + } else if (newValue != null && !isIgnoredPersistedOption(key) + && isUserModifiable(Configuration.getConfigValue(key))) { + ignoredBuildTime.add(key); + } + }); + } + final boolean disabledMappersInterceptorEnabled = DisabledMappersInterceptor.isEnabled(); // return to the state before the disable try { PropertyMappingInterceptor.disable(); // we don't want the mapped / transformed properties, we want what the user effectively supplied DisabledMappersInterceptor.disable(); // we want all properties, even disabled ones - final List ignoredBuildTime = new ArrayList<>(); final List ignoredRunTime = new ArrayList<>(); final Set disabledBuildTime = new HashSet<>(); final Set disabledRunTime = new HashSet<>(); @@ -368,10 +382,10 @@ public class Picocli { } if (options.includeRuntime) { disabledMappers.addAll(PropertyMappers.getDisabledRuntimeMappers().values()); + } else { + checkRuntimeSpiOptions(options, ignoredRunTime); } - checkSpiOptions(options, ignoredBuildTime, ignoredRunTime); - for (OptionCategory category : abstractCommand.getOptionCategories()) { List> mappers = new ArrayList<>(disabledMappers); Optional.ofNullable(PropertyMappers.getRuntimeMappers().get(category)).ifPresent(mappers::addAll); @@ -401,20 +415,13 @@ public class Picocli { return; } - if (mapper.isBuildTime() && !options.includeBuildTime) { - String currentValue = getRawPersistedProperty(mapper.getFrom()).orElse(null); - if (!Objects.equals(configValueStr, currentValue)) { - ignoredBuildTime.add(mapper.getFrom()); - return; - } - } if (mapper.isRunTime() && !options.includeRuntime) { if (configValueStr != null) { ignoredRunTime.add(mapper.getFrom()); } return; } - + if (configValueStr == null) { if (mapper.isRequired()) { handleRequired(missingOption, mapper); @@ -461,31 +468,21 @@ public class Picocli { return configValue.getConfigSourceOrdinal() >= KeycloakPropertiesConfigSource.PROPERTIES_FILE_ORDINAL; } - private static void checkSpiOptions(IncludeOptions options, final List ignoredBuildTime, - final List ignoredRunTime) { + private static void checkRuntimeSpiOptions(IncludeOptions options, final List ignoredRunTime) { for (String key : Configuration.getConfig().getPropertyNames()) { if (!key.startsWith(PropertyMappers.KC_SPI_PREFIX)) { continue; } boolean buildTimeOption = PropertyMappers.isSpiBuildTimeProperty(key); - ConfigValue configValue = Configuration.getConfigValue(key); - String configValueStr = configValue.getValue(); + if (!buildTimeOption) { + 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); - if (!configValueStr.equals(currentValue)) { - ignoredBuildTime.add(key); - } + // don't consider missing or anything below standard env properties + if (configValueStr != null && isUserModifiable(configValue)) { + ignoredRunTime.add(key); } - } else if (!options.includeRuntime && !buildTimeOption) { - ignoredRunTime.add(key); } } } @@ -671,7 +668,7 @@ public class Picocli { if (!result.includeBuildTime && !result.includeRuntime) { return result; - } else if (result.includeRuntime && !result.includeBuildTime && !ShowConfig.NAME.equals(commandName)) { + } else if (result.includeRuntime && !result.includeBuildTime) { result.includeBuildTime = isRebuilt() || !cliArgs.contains(OPTIMIZED_BUILD_OPTION_LONG); } else if (result.includeBuildTime && !result.includeRuntime) { result.includeRuntime = isRebuildCheck(); @@ -880,23 +877,7 @@ public class Picocli { private static void checkChangesInBuildOptionsDuringAutoBuild(PrintWriter out) { StringBuilder options = new StringBuilder(); - var current = getNonPersistedBuildTimeOptions(); - var persisted = Configuration.getRawPersistedProperties(); - - // TODO: order is not well defined here - - current.forEach((key, value) -> { - String persistedValue = persisted.get(key); - if (!value.equals(persistedValue)) { - optionChanged(options, (String)key, persistedValue, (String)value); - } - }); - - persisted.forEach((key, value) -> { - if (current.get(key) == null) { - optionChanged(options, key, value, null); - } - }); + checkChangesInBuildOptions((key, oldValue, newValue) -> optionChanged(options, key, oldValue, newValue)); if (options.isEmpty()) { return; @@ -912,11 +893,30 @@ public class Picocli { ); } + private static void checkChangesInBuildOptions(TriConsumer valueChanged) { + var current = getNonPersistedBuildTimeOptions(); + var persisted = Configuration.getRawPersistedProperties(); + + // TODO: order is not well defined here + + current.forEach((key, value) -> { + String persistedValue = persisted.get(key); + if (!value.equals(persistedValue)) { + valueChanged.accept((String)key, persistedValue, (String)value); + } + }); + + persisted.forEach((key, value) -> { + if (current.get(key) == null) { + valueChanged.accept(key, value, null); + } + }); + } + private static void optionChanged(StringBuilder options, String key, String oldValue, String newValue) { // the assumption here is that no build time options need mask handling boolean isIgnored = !key.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX) - || key.startsWith(KC_PROVIDER_FILE_PREFIX) || key.equals(Configuration.KC_OPTIMIZED) - || key.equals(org.keycloak.common.util.Environment.PROFILE); + || key.startsWith(KC_PROVIDER_FILE_PREFIX) || isIgnoredPersistedOption(key); if (!isIgnored) { key = key.substring(3); options.append("\n\t- ").append(key).append("=") @@ -926,6 +926,11 @@ public class Picocli { } } + private static boolean isIgnoredPersistedOption(String key) { + return key.equals(Configuration.KC_OPTIMIZED) || key.equals(org.keycloak.common.util.Environment.PROFILE) + || key.equals(LaunchMode.current().getProfileKey()); + } + public void start() { KeycloakMain.start(errorHandler, getErrWriter()); } 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 f9b051f1855..a3325459b8e 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 @@ -296,6 +296,14 @@ public class PicocliTest extends AbstractConfigurationTest { assertThat(nonRunningPicocli.getErrString(), containsString("The '--optimized' flag was used for first ever server start.")); } + @Test + public void optimizedExport() { + build("build", "--db=dev-file"); + + NonRunningPicocli nonRunningPicocli = pseudoLaunch("export", "--optimized", "--dir=data"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + } + @Test public void testReaugFromProdToDev() { build("build", "--db=dev-file"); @@ -461,4 +469,41 @@ public class PicocliTest extends AbstractConfigurationTest { assertThat(nonRunningPicocli.getErrString(), containsString( "Invalid value for option '--log-syslog-max-length': value wrong not in correct format (regular expression): [0-9]+[BbKkMmGgTtPpEeZzYy]?")); } + + @Test + public void providerChanged() { + build("build", "--db=dev-file"); + + addPersistedConfigValues(Map.of(Picocli.KC_PROVIDER_FILE_PREFIX + "fake", "value")); + + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start", "--optimized"); + assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); + assertTrue(nonRunningPicocli.getErrString().contains("A provider JAR was updated since the last build, please rebuild for this to be fully utilized.")); + } + + @Test + public void buildOptionChangedWithOptimized() { + build("build", "--db=dev-file"); + + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start", "--optimized", "--db=dev-mem"); + assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); + assertTrue(nonRunningPicocli.getErrString().contains("Build time option: '--db' not usable with pre-built image and --optimized")); + } + + @Test + public void buildOptionWithOptimized() { + build("build", "--metrics-enabled=true", "--db=dev-file"); + + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start", "--optimized", "--http-enabled=true", "--hostname=keycloak"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + } + + @Test + public void buildDBWithOptimized() { + build("build", "--db=mariadb"); + + NonRunningPicocli nonRunningPicocli = pseudoLaunch("import", "--optimized", "--dir=./", "--override=false"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + } + } diff --git a/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java b/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java index a79e3a1acd5..512d7662630 100644 --- a/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java +++ b/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java @@ -85,8 +85,6 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { private Process keycloak; private int exitCode = -1; private final Path distPath; - private final List outputStream = Collections.synchronizedList(new ArrayList<>()); - private final List errorStream = Collections.synchronizedList(new ArrayList<>()); private boolean manualStop; private String relativePath; private int httpPort; @@ -115,7 +113,7 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { this.distPath = prepareDistribution(); this.outputConsumer = outputConsumer; } - + public CLIResult kcadm(String... arguments) throws IOException { return kcadm(Arrays.asList(arguments)); }