From b0b9c1a76eab7c2cf6966c32044713dc0ec2aada Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Fri, 14 Jul 2023 06:42:03 -0400 Subject: [PATCH] Switches the merging logic to using the override as the basis (#21415) Ensures additionalProperties are the lowest precedence Also removes support for setting the image pull secrets via the unsupported podtemplate Closes #10503 --- .../controllers/KeycloakDeployment.java | 402 ++++++------------ .../controllers/KeycloakDistConfigurator.java | 4 +- .../integration/KeycloakDeploymentTest.java | 3 +- .../integration/RealmImportTest.java | 11 +- .../testsuite/unit/PodTemplateTest.java | 42 ++ 5 files changed, 177 insertions(+), 285 deletions(-) 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 9ac59eda31..7cbabe557f 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java @@ -16,19 +16,20 @@ */ package org.keycloak.operator.controllers; -import io.fabric8.kubernetes.api.model.Container; import io.fabric8.kubernetes.api.model.ContainerState; import io.fabric8.kubernetes.api.model.ContainerStateWaiting; import io.fabric8.kubernetes.api.model.EnvVar; import io.fabric8.kubernetes.api.model.EnvVarBuilder; import io.fabric8.kubernetes.api.model.EnvVarSourceBuilder; -import io.fabric8.kubernetes.api.model.HTTPGetActionBuilder; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.PodSpec; +import io.fabric8.kubernetes.api.model.PodSpecFluent.ContainersNested; import io.fabric8.kubernetes.api.model.PodStatus; import io.fabric8.kubernetes.api.model.PodTemplateSpec; -import io.fabric8.kubernetes.api.model.ResourceRequirements; +import io.fabric8.kubernetes.api.model.PodTemplateSpecFluent.SpecNested; import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.api.model.apps.StatefulSetBuilder; +import io.fabric8.kubernetes.api.model.apps.StatefulSetSpecFluent.TemplateNested; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.readiness.Readiness; import io.fabric8.kubernetes.client.utils.Serialization; @@ -39,20 +40,23 @@ import org.keycloak.operator.Config; import org.keycloak.operator.Constants; import org.keycloak.operator.Utils; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; +import org.keycloak.operator.crds.v2alpha1.deployment.KeycloakSpec; import org.keycloak.operator.crds.v2alpha1.deployment.KeycloakStatusAggregator; import org.keycloak.operator.crds.v2alpha1.deployment.ValueOrSecret; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.UnsupportedSpec; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Base64; -import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.keycloak.operator.crds.v2alpha1.CRDUtils.isTlsConfigured; @@ -77,8 +81,10 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu this.adminSecretName = adminSecretName; this.existingDeployment = existingDeployment; this.baseDeployment = createBaseDeployment(); - this.distConfigurator = configureDist(); - mergePodTemplate(this.baseDeployment.getSpec().getTemplate()); + this.distConfigurator = new KeycloakDistConfigurator(keycloakCR, baseDeployment, client); + this.distConfigurator.configureDistOptions(); + // after the distConfiguration, we can add the remaining default / additionalConfig + addRemainingEnvVars(); } @Override @@ -104,38 +110,31 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu } public void validatePodTemplate(KeycloakStatusAggregator status) { - if (keycloakCR.getSpec() == null || - keycloakCR.getSpec().getUnsupported() == null || - keycloakCR.getSpec().getUnsupported().getPodTemplate() == null) { + var spec = getPodTemplateSpec(); + if (spec.isEmpty()) { return; } - var overlayTemplate = this.keycloakCR.getSpec().getUnsupported().getPodTemplate(); + var overlayTemplate = spec.orElseThrow(); - if (overlayTemplate.getMetadata() != null && - overlayTemplate.getMetadata().getName() != null) { - status.addWarningMessage("The name of the podTemplate cannot be modified"); + if (overlayTemplate.getMetadata() != null) { + if (overlayTemplate.getMetadata().getName() != null) { + status.addWarningMessage("The name of the podTemplate cannot be modified"); + } + if (overlayTemplate.getMetadata().getNamespace() != null) { + status.addWarningMessage("The namespace of the podTemplate cannot be modified"); + } } - if (overlayTemplate.getMetadata() != null && - overlayTemplate.getMetadata().getNamespace() != null) { - status.addWarningMessage("The namespace of the podTemplate cannot be modified"); - } - - if (overlayTemplate.getSpec() != null && - overlayTemplate.getSpec().getContainers() != null && - overlayTemplate.getSpec().getContainers().size() > 0 && - overlayTemplate.getSpec().getContainers().get(0) != null && - overlayTemplate.getSpec().getContainers().get(0).getName() != null) { - status.addWarningMessage("The name of the keycloak container cannot be modified"); - } - - if (overlayTemplate.getSpec() != null && - overlayTemplate.getSpec().getContainers() != null && - overlayTemplate.getSpec().getContainers().size() > 0 && - overlayTemplate.getSpec().getContainers().get(0) != null && - overlayTemplate.getSpec().getContainers().get(0).getImage() != null) { - status.addWarningMessage("The image of the keycloak container cannot be modified using podTemplate"); - } + Optional.ofNullable(overlayTemplate.getSpec()).map(PodSpec::getContainers).flatMap(l -> l.stream().findFirst()) + .ifPresent(container -> { + if (container.getName() != null) { + status.addWarningMessage("The name of the keycloak container cannot be modified"); + } + if (container.getImage() != null) { + status.addWarningMessage( + "The image of the keycloak container cannot be modified using podTemplate"); + } + }); if (overlayTemplate.getSpec() != null && CollectionUtil.isNotEmpty(overlayTemplate.getSpec().getImagePullSecrets())) { @@ -143,247 +142,71 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu } } - private void mergeMaps(Map map1, Map map2, Consumer> consumer) { - var map = new HashMap(); - Optional.ofNullable(map1).ifPresent(e -> map.putAll(e)); - Optional.ofNullable(map2).ifPresent(e -> map.putAll(e)); - consumer.accept(map); - } - - private void mergeLists(List list1, List list2, Consumer> consumer) { - var list = new ArrayList(); - Optional.ofNullable(list1).ifPresent(e -> list.addAll(e)); - Optional.ofNullable(list2).ifPresent(e -> list.addAll(e)); - consumer.accept(list); - } - - private void mergeField(T value, Consumer consumer) { - if (value != null && (!(value instanceof List) || ((List) value).size() > 0)) { - consumer.accept(value); - } - } - - private void mergePodTemplate(PodTemplateSpec baseTemplate) { - if (keycloakCR.getSpec() == null || - keycloakCR.getSpec().getUnsupported() == null || - keycloakCR.getSpec().getUnsupported().getPodTemplate() == null) { - return; - } - - var overlayTemplate = keycloakCR.getSpec().getUnsupported().getPodTemplate(); - - mergeMaps( - Optional.ofNullable(baseTemplate.getMetadata()).map(m -> m.getLabels()).orElse(null), - Optional.ofNullable(overlayTemplate.getMetadata()).map(m -> m.getLabels()).orElse(null), - labels -> baseTemplate.getMetadata().setLabels(labels)); - - mergeMaps( - Optional.ofNullable(baseTemplate.getMetadata()).map(m -> m.getAnnotations()).orElse(null), - Optional.ofNullable(overlayTemplate.getMetadata()).map(m -> m.getAnnotations()).orElse(null), - annotations -> baseTemplate.getMetadata().setAnnotations(annotations)); - - var baseSpec = baseTemplate.getSpec(); - var overlaySpec = overlayTemplate.getSpec(); - - var containers = new ArrayList(); - var overlayContainers = - (overlaySpec == null || overlaySpec.getContainers() == null) ? - new ArrayList() : overlaySpec.getContainers(); - if (overlayContainers.size() >= 1) { - var keycloakBaseContainer = baseSpec.getContainers().get(0); - var keycloakOverlayContainer = overlayContainers.get(0); - mergeField(keycloakOverlayContainer.getCommand(), v -> keycloakBaseContainer.setCommand(v)); - mergeField(keycloakOverlayContainer.getReadinessProbe(), v -> keycloakBaseContainer.setReadinessProbe(v)); - mergeField(keycloakOverlayContainer.getLivenessProbe(), v -> keycloakBaseContainer.setLivenessProbe(v)); - mergeField(keycloakOverlayContainer.getStartupProbe(), v -> keycloakBaseContainer.setStartupProbe(v)); - mergeField(keycloakOverlayContainer.getArgs(), v -> keycloakBaseContainer.setArgs(v)); - mergeField(keycloakOverlayContainer.getImagePullPolicy(), v -> keycloakBaseContainer.setImagePullPolicy(v)); - mergeField(keycloakOverlayContainer.getLifecycle(), v -> keycloakBaseContainer.setLifecycle(v)); - mergeField(keycloakOverlayContainer.getSecurityContext(), v -> keycloakBaseContainer.setSecurityContext(v)); - mergeField(keycloakOverlayContainer.getWorkingDir(), v -> keycloakBaseContainer.setWorkingDir(v)); - - var resources = new ResourceRequirements(); - mergeMaps( - Optional.ofNullable(keycloakBaseContainer.getResources()).map(r -> r.getRequests()).orElse(null), - Optional.ofNullable(keycloakOverlayContainer.getResources()).map(r -> r.getRequests()).orElse(null), - requests -> resources.setRequests(requests)); - mergeMaps( - Optional.ofNullable(keycloakBaseContainer.getResources()).map(l -> l.getLimits()).orElse(null), - Optional.ofNullable(keycloakOverlayContainer.getResources()).map(l -> l.getLimits()).orElse(null), - limits -> resources.setLimits(limits)); - keycloakBaseContainer.setResources(resources); - - mergeLists( - keycloakBaseContainer.getPorts(), - keycloakOverlayContainer.getPorts(), - p -> keycloakBaseContainer.setPorts(p)); - mergeLists( - keycloakBaseContainer.getEnvFrom(), - keycloakOverlayContainer.getEnvFrom(), - e -> keycloakBaseContainer.setEnvFrom(e)); - mergeLists( - keycloakBaseContainer.getEnv(), - keycloakOverlayContainer.getEnv(), - e -> keycloakBaseContainer.setEnv(e)); - mergeLists( - keycloakBaseContainer.getVolumeMounts(), - keycloakOverlayContainer.getVolumeMounts(), - vm -> keycloakBaseContainer.setVolumeMounts(vm)); - mergeLists( - keycloakBaseContainer.getVolumeDevices(), - keycloakOverlayContainer.getVolumeDevices(), - vd -> keycloakBaseContainer.setVolumeDevices(vd)); - - containers.add(keycloakBaseContainer); - - // Skip keycloak container and add the rest - for (int i = 1; i < overlayContainers.size(); i++) { - containers.add(overlayContainers.get(i)); - } - - baseSpec.setContainers(containers); - } - - if (overlaySpec != null) { - mergeField(overlaySpec.getActiveDeadlineSeconds(), ads -> baseSpec.setActiveDeadlineSeconds(ads)); - mergeField(overlaySpec.getAffinity(), a -> baseSpec.setAffinity(a)); - mergeField(overlaySpec.getAutomountServiceAccountToken(), a -> baseSpec.setAutomountServiceAccountToken(a)); - mergeField(overlaySpec.getDnsConfig(), dc -> baseSpec.setDnsConfig(dc)); - mergeField(overlaySpec.getDnsPolicy(), dp -> baseSpec.setDnsPolicy(dp)); - mergeField(overlaySpec.getEnableServiceLinks(), esl -> baseSpec.setEnableServiceLinks(esl)); - mergeField(overlaySpec.getHostIPC(), h -> baseSpec.setHostIPC(h)); - mergeField(overlaySpec.getHostname(), h -> baseSpec.setHostname(h)); - mergeField(overlaySpec.getHostNetwork(), h -> baseSpec.setHostNetwork(h)); - mergeField(overlaySpec.getHostPID(), h -> baseSpec.setHostPID(h)); - mergeField(overlaySpec.getNodeName(), n -> baseSpec.setNodeName(n)); - mergeField(overlaySpec.getNodeSelector(), ns -> baseSpec.setNodeSelector(ns)); - mergeField(overlaySpec.getPreemptionPolicy(), pp -> baseSpec.setPreemptionPolicy(pp)); - mergeField(overlaySpec.getPriority(), p -> baseSpec.setPriority(p)); - mergeField(overlaySpec.getPriorityClassName(), pcn -> baseSpec.setPriorityClassName(pcn)); - mergeField(overlaySpec.getRestartPolicy(), rp -> baseSpec.setRestartPolicy(rp)); - mergeField(overlaySpec.getRuntimeClassName(), rcn -> baseSpec.setRuntimeClassName(rcn)); - mergeField(overlaySpec.getSchedulerName(), sn -> baseSpec.setSchedulerName(sn)); - mergeField(overlaySpec.getSecurityContext(), sc -> baseSpec.setSecurityContext(sc)); - mergeField(overlaySpec.getServiceAccount(), sa -> baseSpec.setServiceAccount(sa)); - mergeField(overlaySpec.getServiceAccountName(), san -> baseSpec.setServiceAccountName(san)); - mergeField(overlaySpec.getSetHostnameAsFQDN(), h -> baseSpec.setSetHostnameAsFQDN(h)); - mergeField(overlaySpec.getShareProcessNamespace(), spn -> baseSpec.setShareProcessNamespace(spn)); - mergeField(overlaySpec.getSubdomain(), s -> baseSpec.setSubdomain(s)); - mergeField(overlaySpec.getTerminationGracePeriodSeconds(), t -> baseSpec.setTerminationGracePeriodSeconds(t)); - - mergeLists( - baseSpec.getImagePullSecrets(), - overlaySpec.getImagePullSecrets(), - ips -> baseSpec.setImagePullSecrets(ips)); - mergeLists( - baseSpec.getHostAliases(), - overlaySpec.getHostAliases(), - ha -> baseSpec.setHostAliases(ha)); - mergeLists( - baseSpec.getEphemeralContainers(), - overlaySpec.getEphemeralContainers(), - ec -> baseSpec.setEphemeralContainers(ec)); - mergeLists( - baseSpec.getInitContainers(), - overlaySpec.getInitContainers(), - ic -> baseSpec.setInitContainers(ic)); - mergeLists( - baseSpec.getReadinessGates(), - overlaySpec.getReadinessGates(), - rg -> baseSpec.setReadinessGates(rg)); - mergeLists( - baseSpec.getTolerations(), - overlaySpec.getTolerations(), - t -> baseSpec.setTolerations(t)); - mergeLists( - baseSpec.getTopologySpreadConstraints(), - overlaySpec.getTopologySpreadConstraints(), - tpc -> baseSpec.setTopologySpreadConstraints(tpc)); - - mergeLists( - baseSpec.getVolumes(), - overlaySpec.getVolumes(), - v -> baseSpec.setVolumes(v)); - - mergeMaps( - baseSpec.getOverhead(), - overlaySpec.getOverhead(), - o -> baseSpec.setOverhead(o)); - } + private Optional getPodTemplateSpec() { + return Optional.ofNullable(keycloakCR.getSpec()).map(KeycloakSpec::getUnsupported).map(UnsupportedSpec::getPodTemplate); } private StatefulSet createBaseDeployment() { - StatefulSet baseDeployment = new StatefulSetBuilder() - .withNewMetadata() - .endMetadata() - .withNewSpec() - .withNewSelector() - .addToMatchLabels("app", "") - .endSelector() - .withNewTemplate() - .withNewMetadata() - .addToLabels("app", "") - .endMetadata() - .withNewSpec() - .withRestartPolicy("Always") - .withTerminationGracePeriodSeconds(30L) - .withDnsPolicy("ClusterFirst") - .addNewContainer() - .withName("keycloak") - .withArgs("start") - .addNewPort() - .withName(Constants.KEYCLOAK_HTTPS_PORT_NAME) - .withContainerPort(Constants.KEYCLOAK_HTTPS_PORT) - .withProtocol(Constants.KEYCLOAK_SERVICE_PROTOCOL) - .endPort() - .addNewPort() - .withName(Constants.KEYCLOAK_HTTP_PORT_NAME) - .withContainerPort(Constants.KEYCLOAK_HTTP_PORT) - .withProtocol(Constants.KEYCLOAK_SERVICE_PROTOCOL) - .endPort() - .withNewReadinessProbe() - .withInitialDelaySeconds(20) - .withPeriodSeconds(2) - .withFailureThreshold(250) - .endReadinessProbe() - .withNewLivenessProbe() - .withInitialDelaySeconds(20) - .withPeriodSeconds(2) - .withFailureThreshold(150) - .endLivenessProbe() - .endContainer() - .endSpec() - .endTemplate() - .endSpec() - .build(); - - baseDeployment.getMetadata().setName(getName()); - baseDeployment.getMetadata().setNamespace(getNamespace()); - baseDeployment.getSpec().getSelector().setMatchLabels(getInstanceLabels()); - baseDeployment.getSpec().setReplicas(keycloakCR.getSpec().getInstances()); - Map labels = getInstanceLabels(); if (operatorConfig.keycloak().podLabels() != null) { labels.putAll(operatorConfig.keycloak().podLabels()); } - baseDeployment.getSpec().getTemplate().getMetadata().setLabels(labels); - Container container = baseDeployment.getSpec().getTemplate().getSpec().getContainers().get(0); + /* Create a builder for the statefulset, note that the pod template spec is used as the basis + * over that some values are forced, others will let the template override, others merge + */ + + StatefulSetBuilder baseDeploymentBuilder = new StatefulSetBuilder() + .withNewMetadata() + .withName(getName()) + .withNamespace(getNamespace()) + .endMetadata() + .withNewSpec() + .withNewSelector() + .withMatchLabels(getInstanceLabels()) + .endSelector() + .withNewTemplateLike(getPodTemplateSpec().orElseGet(PodTemplateSpec::new)) + .editOrNewMetadata().addToLabels(labels).endMetadata() + .editOrNewSpec().withImagePullSecrets(keycloakCR.getSpec().getImagePullSecrets()).endSpec() + .endTemplate() + .withReplicas(keycloakCR.getSpec().getInstances()) + .endSpec(); + + var specBuilder = baseDeploymentBuilder.editSpec().editTemplate().editOrNewSpec(); + + if (!specBuilder.hasRestartPolicy()) { + specBuilder.withRestartPolicy("Always"); + } + if (!specBuilder.hasTerminationGracePeriodSeconds()) { + specBuilder.withTerminationGracePeriodSeconds(30L); + } + if (!specBuilder.hasDnsPolicy()) { + specBuilder.withDnsPolicy("ClusterFirst"); + } + + // there isn't currently an editOrNewFirstContainer, so we need to do this manually + ContainersNested>>> containerBuilder = null; + if (specBuilder.buildContainers().isEmpty()) { + containerBuilder = specBuilder.addNewContainer(); + } else { + containerBuilder = specBuilder.editFirstContainer(); + } + + containerBuilder.withName("keycloak"); + var customImage = Optional.ofNullable(keycloakCR.getSpec().getImage()); - container.setImage(customImage.orElse(operatorConfig.keycloak().image())); + containerBuilder.withImage(customImage.orElse(operatorConfig.keycloak().image())); + if (!containerBuilder.hasImagePullPolicy()) { + containerBuilder.withImagePullPolicy(operatorConfig.keycloak().imagePullPolicy()); + } + if (Optional.ofNullable(containerBuilder.getArgs()).orElse(List.of()).isEmpty()) { + containerBuilder.withArgs("start"); + } if (customImage.isPresent()) { - container.getArgs().add("--optimized"); + containerBuilder.addToArgs("--optimized"); } - if (CollectionUtil.isNotEmpty(keycloakCR.getSpec().getImagePullSecrets())) { - baseDeployment.getSpec().getTemplate().getSpec().setImagePullSecrets(keycloakCR.getSpec().getImagePullSecrets()); - } - - container.setImagePullPolicy(operatorConfig.keycloak().imagePullPolicy()); - - container.setEnv(getEnvVars()); - // probes var tlsConfigured = isTlsConfigured(keycloakCR); var protocol = !tlsConfigured ? "HTTP" : "HTTPS"; @@ -394,28 +217,59 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu .map(path -> !path.endsWith("/") ? path + "/" : path) .orElse("/"); - container.getReadinessProbe().setHttpGet( - new HTTPGetActionBuilder() + if (!containerBuilder.hasReadinessProbe()) { + containerBuilder.withNewReadinessProbe() + .withInitialDelaySeconds(20) + .withPeriodSeconds(2) + .withFailureThreshold(250) + .withNewHttpGet() .withScheme(protocol) .withNewPort(kcPort) .withPath(kcRelativePath + "health/ready") - .build() - ); - container.getLivenessProbe().setHttpGet( - new HTTPGetActionBuilder() + .endHttpGet() + .endReadinessProbe(); + } + if (!containerBuilder.hasLivenessProbe()) { + containerBuilder.withNewLivenessProbe() + .withInitialDelaySeconds(20) + .withPeriodSeconds(2) + .withFailureThreshold(150) + .withNewHttpGet() .withScheme(protocol) .withNewPort(kcPort) .withPath(kcRelativePath + "health/live") - .build() - ); + .endHttpGet() + .endLivenessProbe(); + } + + // add in ports - there's no merging being done here + StatefulSet baseDeployment = containerBuilder + .addNewPort() + .withName(Constants.KEYCLOAK_HTTPS_PORT_NAME) + .withContainerPort(Constants.KEYCLOAK_HTTPS_PORT) + .withProtocol(Constants.KEYCLOAK_SERVICE_PROTOCOL) + .endPort() + .addNewPort() + .withName(Constants.KEYCLOAK_HTTP_PORT_NAME) + .withContainerPort(Constants.KEYCLOAK_HTTP_PORT) + .withProtocol(Constants.KEYCLOAK_SERVICE_PROTOCOL) + .endPort() + .endContainer().endSpec().endTemplate().endSpec().build(); return baseDeployment; } - private KeycloakDistConfigurator configureDist() { - final KeycloakDistConfigurator config = new KeycloakDistConfigurator(keycloakCR, baseDeployment, client); - config.configureDistOptions(); - return config; + private void addRemainingEnvVars() { + // add in the remaining envVars, but only if they are not already set + var envVars = getEnvVars(); + var env = baseDeployment.getSpec().getTemplate().getSpec().getContainers().get(0).getEnv(); + if (env != null && !env.isEmpty()) { + // this is also a final rationalization of whatever was added first by any previous manipulation wins + envVars = new ArrayList<>(Stream.concat(env.stream(), envVars.stream()) + .collect(Collectors.toMap(EnvVar::getName, Function.identity(), (e1, e2) -> e1, LinkedHashMap::new)) + .values()); + } + baseDeployment.getSpec().getTemplate().getSpec().getContainers().get(0).setEnv(envVars); } private List getEnvVars() { diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java index 5ab9a2fb07..d72b62996c 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java @@ -149,8 +149,8 @@ public class KeycloakDistConfigurator { .withMountPath(Constants.CERTIFICATES_FOLDER) .build(); - deployment.getSpec().getTemplate().getSpec().getVolumes().add(volume); - kcContainer.getVolumeMounts().add(volumeMount); + deployment.getSpec().getTemplate().getSpec().getVolumes().add(0, volume); + kcContainer.getVolumeMounts().add(0, volumeMount); } public void configureDatabase() { diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java index bb4f0ff035..5bb206fa1f 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java @@ -121,10 +121,11 @@ public class KeycloakDeploymentTest extends BaseOperatorTest { var c = k8sclient.apps().statefulSets().inNamespace(namespace).withName(deploymentName).get() .getSpec().getTemplate().getSpec().getContainers().get(0); assertThat(c.getImage()).isEqualTo("quay.io/keycloak/non-existing-keycloak"); + // additionalOptions should not override the first-class assertThat(c.getEnv().stream() .anyMatch(e -> e.getName().equals(KeycloakDistConfigurator.getKeycloakOptionEnvVarName(dbConf.getName())) && dbConf.getValue().equals(e.getValue()))) - .isTrue(); + .isFalse(); }); } catch (Exception e) { 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 fee18cf7a7..4299213d94 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 @@ -18,9 +18,9 @@ package org.keycloak.operator.testsuite.integration; import io.fabric8.kubernetes.api.model.LocalObjectReferenceBuilder; -import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; import io.quarkus.logging.Log; import io.quarkus.test.junit.QuarkusTest; + import org.awaitility.Awaitility; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -30,8 +30,8 @@ import org.keycloak.operator.testsuite.utils.CRAssert; import org.keycloak.operator.testsuite.utils.K8sUtils; import org.keycloak.operator.controllers.KeycloakService; import org.keycloak.operator.crds.v2alpha1.realmimport.KeycloakRealmImport; -import org.keycloak.operator.crds.v2alpha1.deployment.spec.UnsupportedSpec; +import java.util.Arrays; import java.util.stream.Collectors; import static java.util.concurrent.TimeUnit.MINUTES; @@ -82,12 +82,7 @@ public class RealmImportTest extends BaseOperatorTest { public void testWorkingRealmImport() { // Arrange var kc = getDefaultKeycloakDeployment(); - var podTemplate = new PodTemplateSpecBuilder() - .withNewSpec() - .withImagePullSecrets(new LocalObjectReferenceBuilder().withName("my-empty-secret").build()) - .endSpec() - .build(); - kc.getSpec().setUnsupported(new UnsupportedSpec(podTemplate)); + kc.getSpec().setImagePullSecrets(Arrays.asList(new LocalObjectReferenceBuilder().withName("my-empty-secret").build())); deployKeycloak(k8sclient, kc, false); // Act diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/unit/PodTemplateTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/unit/PodTemplateTest.java index 1de1f3a73c..bff4c00963 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/unit/PodTemplateTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/unit/PodTemplateTest.java @@ -18,6 +18,7 @@ package org.keycloak.operator.testsuite.unit; import io.fabric8.kubernetes.api.model.Container; +import io.fabric8.kubernetes.api.model.EnvVar; import io.fabric8.kubernetes.api.model.IntOrString; import io.fabric8.kubernetes.api.model.PodTemplateSpec; import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; @@ -25,6 +26,8 @@ import io.fabric8.kubernetes.api.model.ProbeBuilder; import io.fabric8.kubernetes.api.model.apps.StatefulSet; import io.fabric8.kubernetes.api.model.apps.StatefulSetBuilder; import io.quarkus.test.junit.QuarkusTest; + +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.keycloak.operator.Config; import org.keycloak.operator.controllers.KeycloakDeployment; @@ -40,6 +43,7 @@ import java.util.Collections; import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; +import java.util.stream.Collectors; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -274,6 +278,44 @@ public class PodTemplateTest { assertEquals(value, envVar.getValue()); } + @Test + public void testEnvVarConflict() { + // Arrange + var additionalPodTemplate = new PodTemplateSpecBuilder() + .withNewSpec() + .addNewContainer() + .addNewEnv() + .withName("KC_CACHE_STACK") + .withValue("template_stack") + .endEnv() + .addNewEnv() + .withName("KC_DB_URL_HOST") + .withValue("template_host") + .endEnv() + .endContainer() + .endSpec() + .build(); + + // Act + var podTemplate = getDeployment(additionalPodTemplate, null, + s -> s.addNewAdditionalOption("cache.stack", "additional_stack") + .addNewAdditionalOption("http.port", "additional_port").withNewDatabaseSpec().withHost("spec-host") + .endDatabaseSpec()) + .getSpec().getTemplate(); + + // Assert + var envVar = podTemplate.getSpec().getContainers().get(0).getEnv(); + var envVarMap = envVar.stream().collect(Collectors.toMap(EnvVar::getName, Function.identity(), (e1, e2) -> { + Assertions.fail("duplicate env" + e1.getName()); + return e1; + })); + // template spec takes the most priority for envs - only fields called out in the KeycloakDeployment warning are overriden by the rest of the spec + assertThat(envVarMap.get("KC_CACHE_STACK").getValue()).isEqualTo("template_stack"); + assertThat(envVarMap.get("KC_DB_URL_HOST").getValue()).isEqualTo("template_host"); + // the main spec takes priority over the additional options + assertThat(envVarMap.get("KC_HTTP_PORT").getValue()).isEqualTo("8080"); + } + @Test public void testAnnotationsAreNotMerged() { // Arrange