From 0afc4a8af7a9b0e90cd9d8d7d0e2cf4e6224b974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Fri, 14 Oct 2022 15:42:09 +0200 Subject: [PATCH] Refactor `KeycloakDeploymentConfig` (#14880) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martin Bartoš --- .../controllers/KeycloakDeployment.java | 14 +- ...fig.java => KeycloakDistConfigurator.java} | 125 +++++++++++------- ...java => KeycloakDistConfiguratorTest.java} | 30 +++-- ...ialization-keycloak-cr-with-empty-list.yml | 10 ++ 4 files changed, 118 insertions(+), 61 deletions(-) rename operator/src/main/java/org/keycloak/operator/controllers/{KeycloakDeploymentConfig.java => KeycloakDistConfigurator.java} (75%) rename operator/src/test/java/org/keycloak/operator/testsuite/unit/{KeycloakDeploymentConfigTest.java => KeycloakDistConfiguratorTest.java} (78%) create mode 100644 operator/src/test/resources/test-serialization-keycloak-cr-with-empty-list.yml diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java index 6021eddef0..f210ae8b68 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java @@ -49,7 +49,7 @@ import static io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericB public class KeycloakDeployment extends OperatorManagedResource implements StatusUpdater { private final Config operatorConfig; - private final KeycloakDeploymentConfig deploymentConfig; + private final KeycloakDistConfigurator distConfigurator; private final Keycloak keycloakCR; private final StatefulSet existingDeployment; @@ -75,7 +75,8 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu } this.baseDeployment = createBaseDeployment(); - this.deploymentConfig = createDeploymentConfig(); + this.distConfigurator = configureDist(); + mergePodTemplate(this.baseDeployment.getSpec().getTemplate()); } @Override @@ -394,10 +395,9 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu return baseDeployment; } - private KeycloakDeploymentConfig createDeploymentConfig() { - final KeycloakDeploymentConfig config = new KeycloakDeploymentConfig(keycloakCR, baseDeployment, client); - config.configureProperties(); - mergePodTemplate(baseDeployment.getSpec().getTemplate()); + private KeycloakDistConfigurator configureDist() { + final KeycloakDistConfigurator config = new KeycloakDistConfigurator(keycloakCR, baseDeployment, client); + config.configureDistOptions(); return config; } @@ -485,7 +485,7 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu status.addRollingUpdateMessage("Rolling out deployment update"); } - deploymentConfig.validateProperties(status); + distConfigurator.validateOptions(status); } public Set getConfigSecretsNames() { diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentConfig.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java similarity index 75% rename from operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentConfig.java rename to operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java index 18a6ee2353..2ada21d9c0 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentConfig.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java @@ -17,34 +17,44 @@ package org.keycloak.operator.controllers; +import io.fabric8.kubernetes.api.model.EnvVar; import io.fabric8.kubernetes.api.model.EnvVarBuilder; import io.fabric8.kubernetes.api.model.ExecActionBuilder; import io.fabric8.kubernetes.api.model.VolumeBuilder; import io.fabric8.kubernetes.api.model.VolumeMountBuilder; import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.client.KubernetesClient; +import io.quarkus.logging.Log; import org.keycloak.common.util.CollectionUtil; import org.keycloak.operator.Constants; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; import org.keycloak.operator.crds.v2alpha1.deployment.KeycloakStatusBuilder; import org.keycloak.operator.crds.v2alpha1.deployment.ValueOrSecret; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.FeatureSpec; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.TransactionsSpec; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Base64; import java.util.Collection; +import java.util.HashSet; import java.util.List; +import java.util.Objects; +import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; +import static org.keycloak.operator.controllers.KeycloakDeployment.getEnvVarName; + /** * Configuration for the KeycloakDeployment */ -public class KeycloakDeploymentConfig { +public class KeycloakDistConfigurator { private final Keycloak keycloakCR; private final StatefulSet deployment; private final KubernetesClient client; - public KeycloakDeploymentConfig(Keycloak keycloakCR, StatefulSet deployment, KubernetesClient client) { + public KeycloakDistConfigurator(Keycloak keycloakCR, StatefulSet deployment, KubernetesClient client) { this.keycloakCR = keycloakCR; this.deployment = deployment; this.client = client; @@ -53,18 +63,12 @@ public class KeycloakDeploymentConfig { /** * Specify first-class citizens fields which should not be added as general server configuration property */ - private final static List FIRST_CLASS_FIELDS = List.of( - "hostname", - "tlsSecret", - "features", - "features-disabled", - "transaction-xa-enabled" - ); + private final Set firstClassConfigOptions = new HashSet<>(); /** * Configure configuration properties for the KeycloakDeployment */ - protected void configureProperties() { + public void configureDistOptions() { configureHostname(); configureTLS(); configureFeatures(); @@ -76,7 +80,7 @@ public class KeycloakDeploymentConfig { * * @param status Keycloak Status builder */ - protected void validateProperties(KeycloakStatusBuilder status) { + public void validateOptions(KeycloakStatusBuilder status) { assumeFirstClassCitizens(status); } @@ -189,42 +193,14 @@ public class KeycloakDeploymentConfig { } public void configureFeatures() { - var featureSpec = keycloakCR.getSpec().getFeatureSpec(); - if (featureSpec == null) return; - - var kcContainer = deployment.getSpec().getTemplate().getSpec().getContainers().get(0); - var envVars = kcContainer.getEnv(); - var enabledFeatures = featureSpec.getEnabledFeatures(); - var disabledFeatures = featureSpec.getDisabledFeatures(); - - if (CollectionUtil.isNotEmpty(enabledFeatures)) { - envVars.add(new EnvVarBuilder() - .withName("KC_FEATURES") - .withValue(CollectionUtil.join(enabledFeatures, ",")) - .build()); - } - - if (CollectionUtil.isNotEmpty(disabledFeatures)) { - envVars.add(new EnvVarBuilder() - .withName("KC_FEATURES_DISABLED") - .withValue(CollectionUtil.join(disabledFeatures, ",")) - .build()); - } + optionMapper(keycloakCR.getSpec().getFeatureSpec()) + .mapOptionFromCollection("features", FeatureSpec::getEnabledFeatures) + .mapOptionFromCollection("features-disabled", FeatureSpec::getDisabledFeatures); } public void configureTransactions() { - var transactionsSpec = keycloakCR.getSpec().getTransactionsSpec(); - if (transactionsSpec == null) return; - - var kcContainer = deployment.getSpec().getTemplate().getSpec().getContainers().get(0); - var envVars = kcContainer.getEnv(); - - if (transactionsSpec.isXaEnabled() != null) { - envVars.add(new EnvVarBuilder() - .withName("KC_TRANSACTION_XA_ENABLED") - .withValue(String.valueOf(transactionsSpec.isXaEnabled())) - .build()); - } + optionMapper(keycloakCR.getSpec().getTransactionsSpec()) + .mapOption("transaction-xa-enabled", TransactionsSpec::isXaEnabled); } /* ---------- END of configuration of first-class citizen fields ---------- */ @@ -280,10 +256,69 @@ public class KeycloakDeploymentConfig { .map(ValueOrSecret::getName) .collect(Collectors.toSet()); - final var sameItems = CollectionUtil.intersection(serverConfigNames, FIRST_CLASS_FIELDS); + final var sameItems = CollectionUtil.intersection(serverConfigNames, firstClassConfigOptions); if (CollectionUtil.isNotEmpty(sameItems)) { status.addWarningMessage("You need to specify these fields as the first-class citizen of the CR: " + CollectionUtil.join(sameItems, ",")); } } + + private OptionMapper optionMapper(T optionSpec) { + return new OptionMapper<>(optionSpec); + } + + private class OptionMapper { + private final T categorySpec; + private final List envVars; + + public OptionMapper(T optionSpec) { + this.categorySpec = optionSpec; + + var kcContainer = deployment.getSpec().getTemplate().getSpec().getContainers().get(0); + var envVars = kcContainer.getEnv(); + if (envVars == null) { + envVars = new ArrayList<>(); + kcContainer.setEnv(envVars); + } + this.envVars = envVars; + } + + public OptionMapper mapOption(String optionName, Function optionValueSupplier) { + firstClassConfigOptions.add(optionName); + + if (categorySpec == null) { + Log.debugf("No category spec provided for %s", optionName); + return this; + } + + R value = optionValueSupplier.apply(categorySpec); + String valueStr = String.valueOf(value); + + if (value == null || valueStr.trim().isEmpty()) { + Log.debugf("No value provided for %s", optionName); + return this; + } + + EnvVar envVar = new EnvVarBuilder() + .withName(getEnvVarName(optionName)) + .withValue(valueStr) + .build(); + + envVars.add(envVar); + + return this; + } + + public OptionMapper mapOption(String optionName, R optionValue) { + return mapOption(optionName, s -> optionValue); + } + + protected > OptionMapper mapOptionFromCollection(String optionName, Function optionValueSupplier) { + return mapOption(optionName, s -> { + var value = optionValueSupplier.apply(s); + if (value == null) return null; + return value.stream().filter(Objects::nonNull).map(String::valueOf).collect(Collectors.joining(",")); + }); + } + } } diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDeploymentConfigTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java similarity index 78% rename from operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDeploymentConfigTest.java rename to operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java index e354aeec01..5d9576f3ee 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDeploymentConfigTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java @@ -25,7 +25,7 @@ import io.quarkus.test.junit.QuarkusTest; import org.junit.jupiter.api.Test; import org.keycloak.common.util.CollectionUtil; import org.keycloak.common.util.ObjectUtil; -import org.keycloak.operator.controllers.KeycloakDeploymentConfig; +import org.keycloak.operator.controllers.KeycloakDistConfigurator; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; import org.keycloak.operator.testsuite.utils.K8sUtils; @@ -36,39 +36,51 @@ import java.util.function.Consumer; import static org.assertj.core.api.Assertions.assertThat; @QuarkusTest -public class KeycloakDeploymentConfigTest { +public class KeycloakDistConfiguratorTest { @Test public void enabledFeatures() { - testFirstClassCitizenEnvVars("KC_FEATURES", KeycloakDeploymentConfig::configureFeatures, "docker", "authorization"); + testFirstClassCitizenEnvVars("KC_FEATURES", KeycloakDistConfigurator::configureFeatures, "docker", "authorization"); } @Test public void disabledFeatures() { - testFirstClassCitizenEnvVars("KC_FEATURES_DISABLED", KeycloakDeploymentConfig::configureFeatures, "admin", "step-up-authentication"); + testFirstClassCitizenEnvVars("KC_FEATURES_DISABLED", KeycloakDistConfigurator::configureFeatures, "admin", "step-up-authentication"); } @Test public void transactions() { - testFirstClassCitizenEnvVars("KC_TRANSACTION_XA_ENABLED", KeycloakDeploymentConfig::configureTransactions, "false"); + testFirstClassCitizenEnvVars("KC_TRANSACTION_XA_ENABLED", KeycloakDistConfigurator::configureTransactions, "false"); + } + + @Test + public void testEmptyLists() { + final Keycloak keycloak = K8sUtils.getResourceFromFile("test-serialization-keycloak-cr-with-empty-list.yml", Keycloak.class); + final StatefulSet deployment = getBasicKcDeployment(); + final KeycloakDistConfigurator distConfig = new KeycloakDistConfigurator(keycloak, deployment, null); + + final List envVars = deployment.getSpec().getTemplate().getSpec().getContainers().get(0).getEnv(); + distConfig.configureFeatures(); + assertEnvVarNotPresent(envVars, "KC_FEATURES"); + assertEnvVarNotPresent(envVars, "KC_FEATURES_DISABLED"); } /* UTILS */ - private void testFirstClassCitizenEnvVars(String varName, Consumer config, String... expectedValues) { + private void testFirstClassCitizenEnvVars(String varName, Consumer config, String... expectedValues) { testFirstClassCitizenEnvVars("/test-serialization-keycloak-cr.yml", varName, config, expectedValues); } - private void testFirstClassCitizenEnvVars(String crName, String varName, Consumer config, String... expectedValues) { + private void testFirstClassCitizenEnvVars(String crName, String varName, Consumer config, String... expectedValues) { final Keycloak keycloak = K8sUtils.getResourceFromFile(crName, Keycloak.class); final StatefulSet deployment = getBasicKcDeployment(); - final KeycloakDeploymentConfig deploymentConfig = new KeycloakDeploymentConfig(keycloak, deployment, null); + final KeycloakDistConfigurator distConfig = new KeycloakDistConfigurator(keycloak, deployment, null); final Container container = deployment.getSpec().getTemplate().getSpec().getContainers().get(0); assertThat(container).isNotNull(); assertEnvVarNotPresent(container.getEnv(), varName); - config.accept(deploymentConfig); + config.accept(distConfig); assertContainerEnvVar(container.getEnv(), varName, expectedValues); } diff --git a/operator/src/test/resources/test-serialization-keycloak-cr-with-empty-list.yml b/operator/src/test/resources/test-serialization-keycloak-cr-with-empty-list.yml new file mode 100644 index 0000000000..f0488adb92 --- /dev/null +++ b/operator/src/test/resources/test-serialization-keycloak-cr-with-empty-list.yml @@ -0,0 +1,10 @@ +apiVersion: k8s.keycloak.org/v2alpha1 +kind: Keycloak +metadata: + name: test-serialization-kc +spec: + features: + enabled: + - + hostname: my-hostname + tlsSecret: my-tls-secret \ No newline at end of file