fix: detecting provider changes when running start optimized (#35845)

closes: #34665

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
This commit is contained in:
Steven Hawkins 2025-02-03 11:20:42 -05:00 committed by GitHub
parent efbeb8caa6
commit 332bf122f1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 106 additions and 61 deletions

View File

@ -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<String, Placeholder> secret : primary.getSpec().getPlaceholders().entrySet()) {

View File

@ -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;
}

View File

@ -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<String> 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<String> ignoredBuildTime = new ArrayList<>();
final List<String> ignoredRunTime = new ArrayList<>();
final Set<String> disabledBuildTime = new HashSet<>();
final Set<String> 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<PropertyMapper<?>> 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<String> ignoredBuildTime,
final List<String> ignoredRunTime) {
private static void checkRuntimeSpiOptions(IncludeOptions options, final List<String> 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<String, String, String> 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());
}

View File

@ -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);
}
}

View File

@ -85,8 +85,6 @@ public final class RawKeycloakDistribution implements KeycloakDistribution {
private Process keycloak;
private int exitCode = -1;
private final Path distPath;
private final List<String> outputStream = Collections.synchronizedList(new ArrayList<>());
private final List<String> 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));
}