diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizer.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizer.java similarity index 57% rename from operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizer.java rename to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizer.java index 7193085b63..962059961e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizer.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizer.java @@ -2,32 +2,34 @@ import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import io.fabric8.kubernetes.api.model.Container; +import io.fabric8.kubernetes.api.model.EnvVar; import io.fabric8.kubernetes.api.model.GenericKubernetesResource; import io.fabric8.kubernetes.api.model.PodTemplateSpec; import io.fabric8.kubernetes.api.model.Quantity; import io.fabric8.kubernetes.api.model.ResourceRequirements; /** - * Sanitizes the {@link ResourceRequirements} in the containers of a pair of {@link PodTemplateSpec} - * instances. + * Sanitizes the {@link ResourceRequirements} and the {@link EnvVar} in the containers of a pair of + * {@link PodTemplateSpec} instances. * *

When the sanitizer finds a mismatch in the structure of the given templates, before it gets to - * the nested resource limits and requests, it returns early without fixing the actual map. This is - * an optimization because the given templates will anyway differ at this point. This means we do - * not have to attempt to sanitize the resources for these use cases, since there will anyway be an - * update of the K8s resource. + * the nested fields, it returns early without fixing the actual map. This is an optimization + * because the given templates will anyway differ at this point. This means we do not have to + * attempt to sanitize the fields for these use cases, since there will anyway be an update of the + * K8s resource. * *

The algorithm traverses the whole template structure because we need the actual and desired - * {@link Quantity} instances to compare their numerical amount. Using the {@link + * {@link Quantity} and {@link EnvVar} instances. Using the {@link * GenericKubernetesResource#get(Map, Object...)} shortcut would need to create new instances just * for the sanitization check. */ -class ResourceRequirementsSanitizer { +class PodTemplateSpecSanitizer { - static void sanitizeResourceRequirements( + static void sanitizePodTemplateSpec( final Map actualMap, final PodTemplateSpec actualTemplate, final PodTemplateSpec desiredTemplate) { @@ -37,19 +39,19 @@ static void sanitizeResourceRequirements( if (actualTemplate.getSpec() == null || desiredTemplate.getSpec() == null) { return; } - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actualTemplate.getSpec().getInitContainers(), desiredTemplate.getSpec().getInitContainers(), "initContainers"); - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actualTemplate.getSpec().getContainers(), desiredTemplate.getSpec().getContainers(), "containers"); } - private static void sanitizeResourceRequirements( + private static void sanitizePodTemplateSpec( final Map actualMap, final List actualContainers, final List desiredContainers, @@ -57,11 +59,17 @@ private static void sanitizeResourceRequirements( int containers = desiredContainers.size(); if (containers == actualContainers.size()) { for (int containerIndex = 0; containerIndex < containers; containerIndex++) { - var desiredContainer = desiredContainers.get(containerIndex); - var actualContainer = actualContainers.get(containerIndex); + final var desiredContainer = desiredContainers.get(containerIndex); + final var actualContainer = actualContainers.get(containerIndex); if (!desiredContainer.getName().equals(actualContainer.getName())) { return; } + sanitizeEnvVars( + actualMap, + actualContainer.getEnv(), + desiredContainer.getEnv(), + containerPath, + containerIndex); sanitizeResourceRequirements( actualMap, actualContainer.getResources(), @@ -121,7 +129,7 @@ private static void sanitizeQuantities( m -> actualResource.forEach( (key, actualQuantity) -> { - var desiredQuantity = desiredResource.get(key); + final var desiredQuantity = desiredResource.get(key); if (desiredQuantity == null) { return; } @@ -138,4 +146,53 @@ private static void sanitizeQuantities( } })); } + + @SuppressWarnings("unchecked") + private static void sanitizeEnvVars( + final Map actualMap, + final List actualEnvVars, + final List desiredEnvVars, + final String containerPath, + final int containerIndex) { + if (desiredEnvVars.isEmpty() || actualEnvVars.isEmpty()) { + return; + } + Optional.ofNullable( + GenericKubernetesResource.get( + actualMap, "spec", "template", "spec", containerPath, containerIndex, "env")) + .map(List.class::cast) + .ifPresent( + envVars -> + actualEnvVars.forEach( + actualEnvVar -> { + final var actualEnvVarName = actualEnvVar.getName(); + final var actualEnvVarValue = actualEnvVar.getValue(); + // check if the actual EnvVar value string is not null or the desired EnvVar + // already contains the same EnvVar name with a non empty EnvVar value + final var isDesiredEnvVarEmpty = + hasEnvVarNoEmptyValue(actualEnvVarName, desiredEnvVars); + if (actualEnvVarValue != null || isDesiredEnvVarEmpty) { + return; + } + envVars.stream() + .filter( + envVar -> + ((Map) envVar) + .get("name") + .equals(actualEnvVarName)) + // add the actual EnvVar value with an empty string to prevent a + // resource update + .forEach(envVar -> ((Map) envVar).put("value", "")); + })); + } + + private static boolean hasEnvVarNoEmptyValue( + final String envVarName, final List envVars) { + return envVars.stream() + .anyMatch( + envVar -> + Objects.equals(envVarName, envVar.getName()) + && envVar.getValue() != null + && !envVar.getValue().isEmpty()); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 3c051acfb4..5da1269c74 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -31,7 +31,7 @@ import com.github.difflib.DiffUtils; import com.github.difflib.UnifiedDiffUtils; -import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements; +import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.PodTemplateSpecSanitizer.sanitizePodTemplateSpec; /** * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA. @@ -203,22 +203,22 @@ private void sanitizeState(R actual, R desired, Map actualMap) { } } } - sanitizeResourceRequirements(actualMap, actualSpec.getTemplate(), desiredSpec.getTemplate()); + sanitizePodTemplateSpec(actualMap, actualSpec.getTemplate(), desiredSpec.getTemplate()); } else if (actual instanceof Deployment actualDeployment && desired instanceof Deployment desiredDeployment) { - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actualDeployment.getSpec().getTemplate(), desiredDeployment.getSpec().getTemplate()); } else if (actual instanceof ReplicaSet actualReplicaSet && desired instanceof ReplicaSet desiredReplicaSet) { - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actualReplicaSet.getSpec().getTemplate(), desiredReplicaSet.getSpec().getTemplate()); } else if (actual instanceof DaemonSet actualDaemonSet && desired instanceof DaemonSet desiredDaemonSet) { - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actualDaemonSet.getSpec().getTemplate(), desiredDaemonSet.getSpec().getTemplate()); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizerTest.java similarity index 53% rename from operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizerTest.java rename to operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizerTest.java index 79f3640883..091a1a666c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizerTest.java @@ -1,10 +1,14 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.List; import java.util.Map; +import org.assertj.core.api.ListAssert; import org.assertj.core.api.MapAssert; import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.EnvVar; +import io.fabric8.kubernetes.api.model.EnvVarBuilder; import io.fabric8.kubernetes.api.model.GenericKubernetesResource; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; @@ -15,17 +19,17 @@ import io.fabric8.kubernetes.client.utils.KubernetesSerialization; import io.javaoperatorsdk.operator.MockKubernetesClient; -import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements; +import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.PodTemplateSpecSanitizer.sanitizePodTemplateSpec; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verifyNoInteractions; /** - * Tests the {@link ResourceRequirementsSanitizer} with combinations of matching and mismatching K8s - * resources, using a mix of containers and init containers, as well as resource requests and - * limits. + * Tests the {@link PodTemplateSpecSanitizer} with combinations of matching and mismatching K8s + * resources, using a mix of containers and init containers, as well as resource requests and limits + * along with environment variables. */ -class ResourceRequirementsSanitizerTest { +class PodTemplateSpecSanitizerTest { private final Map actualMap = mock(); @@ -33,26 +37,26 @@ class ResourceRequirementsSanitizerTest { private final KubernetesSerialization serialization = client.getKubernetesSerialization(); @Test - void testSanitizeResourceRequirements_whenTemplateIsNull_doNothing() { + void testSanitizePodTemplateSpec_whenTemplateIsNull_doNothing() { final var template = new PodTemplateSpecBuilder().build(); - sanitizeResourceRequirements(actualMap, null, template); - sanitizeResourceRequirements(actualMap, template, null); + sanitizePodTemplateSpec(actualMap, null, template); + sanitizePodTemplateSpec(actualMap, template, null); verifyNoInteractions(actualMap); } @Test - void testSanitizeResourceRequirements_whenTemplateSpecIsNull_doNothing() { + void testSanitizePodTemplateSpec_whenTemplateSpecIsNull_doNothing() { final var template = new PodTemplateSpecBuilder().withSpec(null).build(); final var templateWithSpec = new PodTemplateSpecBuilder().withNewSpec().endSpec().build(); - sanitizeResourceRequirements(actualMap, template, templateWithSpec); - sanitizeResourceRequirements(actualMap, templateWithSpec, template); + sanitizePodTemplateSpec(actualMap, template, templateWithSpec); + sanitizePodTemplateSpec(actualMap, templateWithSpec, template); verifyNoInteractions(actualMap); } @Test - void testSanitizeResourceRequirements_whenContainerSizeMismatch_doNothing() { + void testSanitizePodTemplateSpec_whenContainerSizeMismatch_doNothing() { final var template = new PodTemplateSpecBuilder() .withNewSpec() @@ -73,13 +77,13 @@ void testSanitizeResourceRequirements_whenContainerSizeMismatch_doNothing() { .endSpec() .build(); - sanitizeResourceRequirements(actualMap, template, templateWithTwoContainers); - sanitizeResourceRequirements(actualMap, templateWithTwoContainers, template); + sanitizePodTemplateSpec(actualMap, template, templateWithTwoContainers); + sanitizePodTemplateSpec(actualMap, templateWithTwoContainers, template); verifyNoInteractions(actualMap); } @Test - void testSanitizeResourceRequirements_whenContainerNameMismatch_doNothing() { + void testSanitizePodTemplateSpec_whenContainerNameMismatch_doNothing() { final var template = new PodTemplateSpecBuilder() .withNewSpec() @@ -97,13 +101,13 @@ void testSanitizeResourceRequirements_whenContainerNameMismatch_doNothing() { .endSpec() .build(); - sanitizeResourceRequirements(actualMap, template, templateWithNewContainerName); - sanitizeResourceRequirements(actualMap, templateWithNewContainerName, template); + sanitizePodTemplateSpec(actualMap, template, templateWithNewContainerName); + sanitizePodTemplateSpec(actualMap, templateWithNewContainerName, template); verifyNoInteractions(actualMap); } @Test - void testSanitizeResourceRequirements_whenResourceIsNull_doNothing() { + void testSanitizePodTemplateSpec_whenResourceIsNull_doNothing() { final var template = new PodTemplateSpecBuilder() .withNewSpec() @@ -123,8 +127,8 @@ void testSanitizeResourceRequirements_whenResourceIsNull_doNothing() { .endSpec() .build(); - sanitizeResourceRequirements(actualMap, template, templateWithResource); - sanitizeResourceRequirements(actualMap, templateWithResource, template); + sanitizePodTemplateSpec(actualMap, template, templateWithResource); + sanitizePodTemplateSpec(actualMap, templateWithResource, template); verifyNoInteractions(actualMap); } @@ -155,7 +159,7 @@ void testSanitizeResourceRequirements_whenResourceKeyMismatch_doNothing() { } @Test - void testSanitizeResourceRequirements_whenResourcesHaveSameAmountAndFormat_doNothing() { + void testSanitizePodTemplateSpec_whenResourcesHaveSameAmountAndFormat_doNothing() { final var actualMap = sanitizeRequestsAndLimits( ContainerType.CONTAINER, @@ -168,7 +172,7 @@ void testSanitizeResourceRequirements_whenResourcesHaveSameAmountAndFormat_doNot } @Test - void testSanitizeResourceRequirements_whenResourcesHaveNumericalAmountMismatch_doNothing() { + void testSanitizePodTemplateSpec_whenResourcesHaveNumericalAmountMismatch_doNothing() { final var actualMap = sanitizeRequestsAndLimits( ContainerType.INIT_CONTAINER, @@ -200,17 +204,139 @@ void testSanitizeResourceRequirements_whenResourcesHaveNumericalAmountMismatch_d assertContainerResources(actualMap, "limits").hasSize(1).containsEntry("cpu", "4000m"); } - @SuppressWarnings("unchecked") + @Test + void testSanitizePodTemplateSpec_whenEnvVarsIsEmpty_doNothing() { + final var template = + new PodTemplateSpecBuilder() + .withNewSpec() + .addNewContainer() + .withName("test") + .endContainer() + .endSpec() + .build(); + final var templateWithEnvVars = + new PodTemplateSpecBuilder() + .withNewSpec() + .addNewContainer() + .withName("test") + .withEnv(List.of(new EnvVarBuilder().withName("FOO").withValue("foobar").build())) + .endContainer() + .endSpec() + .build(); + + sanitizePodTemplateSpec(actualMap, template, templateWithEnvVars); + sanitizePodTemplateSpec(actualMap, templateWithEnvVars, template); + verifyNoInteractions(actualMap); + } + + @Test + void testSanitizePodTemplateSpec_whenActualEnvVarValueIsNotEmpty_doNothing() { + final var actualMap = + sanitizeEnvVars( + ContainerType.CONTAINER, + List.of( + new EnvVarBuilder().withName("FOO").withValue("foo").build(), + new EnvVarBuilder().withName("BAR").withValue("bar").build()), + List.of( + new EnvVarBuilder().withName("FOO").withValue("bar").build(), + new EnvVarBuilder().withName("BAR").withValue("foo").build())); + assertContainerEnvVars(actualMap) + .hasSize(2) + .containsExactly( + Map.of("name", "FOO", "value", "foo"), Map.of("name", "BAR", "value", "bar")); + } + + @Test + void testSanitizePodTemplateSpec_whenActualAndDesiredEnvVarsAreDifferent_doNothing() { + final var actualMap = + sanitizeEnvVars( + ContainerType.INIT_CONTAINER, + List.of(new EnvVarBuilder().withName("FOO").withValue("foo").build()), + List.of(new EnvVarBuilder().withName("BAR").withValue("bar").build())); + assertInitContainerEnvVars(actualMap) + .hasSize(1) + .containsExactly(Map.of("name", "FOO", "value", "foo")); + } + + @Test + void testSanitizePodTemplateSpec_whenActualEnvVarIsEmpty_doNothing() { + final var actualMap = + sanitizeEnvVars( + ContainerType.INIT_CONTAINER, + List.of( + new EnvVarBuilder().withName("FOO").withValue("").build(), + new EnvVarBuilder().withName("BAR").withValue("").build()), + List.of( + new EnvVarBuilder().withName("FOO").withValue("foo").build(), + new EnvVarBuilder().withName("BAR").withValue("").build())); + assertInitContainerEnvVars(actualMap) + .hasSize(2) + .containsExactly(Map.of("name", "FOO", "value", ""), Map.of("name", "BAR", "value", "")); + } + + @Test + void testSanitizePodTemplateSpec_whenActualEnvVarIsNull_doNothing() { + final var actualMap = + sanitizeEnvVars( + ContainerType.CONTAINER, + List.of( + new EnvVarBuilder().withName("FOO").withValue(null).build(), + new EnvVarBuilder().withName("BAR").withValue(null).build()), + List.of( + new EnvVarBuilder().withName("FOO").withValue("foo").build(), + new EnvVarBuilder().withName("BAR").withValue(" ").build())); + assertContainerEnvVars(actualMap) + .hasSize(2) + .containsExactly(Map.of("name", "FOO"), Map.of("name", "BAR")); + } + + @Test + void + testSanitizePodTemplateSpec_whenActualEnvVarIsNull_withDesiredEnvVarEmpty_thenSanitizeActualMap() { + final var actualMap = + sanitizeEnvVars( + ContainerType.CONTAINER, + List.of( + new EnvVarBuilder().withName("FOO").withValue(null).build(), + new EnvVarBuilder().withName("BAR").withValue(null).build()), + List.of( + new EnvVarBuilder().withName("FOO").withValue("").build(), + new EnvVarBuilder().withName("BAR").withValue("").build())); + assertContainerEnvVars(actualMap) + .hasSize(2) + .containsExactly(Map.of("name", "FOO", "value", ""), Map.of("name", "BAR", "value", "")); + } + private Map sanitizeRequestsAndLimits( final ContainerType type, final Map actualRequests, final Map desiredRequests, final Map actualLimits, final Map desiredLimits) { - final var actual = createStatefulSet(type, actualRequests, actualLimits); - final var desired = createStatefulSet(type, desiredRequests, desiredLimits); + return sanitize( + type, actualRequests, desiredRequests, actualLimits, desiredLimits, List.of(), List.of()); + } + + private Map sanitizeEnvVars( + final ContainerType type, + final List actualEnvVars, + final List desiredEnvVars) { + return sanitize(type, Map.of(), Map.of(), Map.of(), Map.of(), actualEnvVars, desiredEnvVars); + } + + @SuppressWarnings("unchecked") + private Map sanitize( + final ContainerType type, + final Map actualRequests, + final Map desiredRequests, + final Map actualLimits, + final Map desiredLimits, + final List actualEnvVars, + final List desiredEnvVars) { + final var actual = createStatefulSet(type, actualRequests, actualLimits, actualEnvVars); + final var desired = createStatefulSet(type, desiredRequests, desiredLimits, desiredEnvVars); final var actualMap = serialization.convertValue(actual, Map.class); - sanitizeResourceRequirements( + sanitizePodTemplateSpec( actualMap, actual.getSpec().getTemplate(), desired.getSpec().getTemplate()); return actualMap; } @@ -223,7 +349,8 @@ private enum ContainerType { private static StatefulSet createStatefulSet( final ContainerType type, final Map requests, - final Map limits) { + final Map limits, + final List envVars) { var builder = new StatefulSetBuilder().withNewSpec().withNewTemplate().withNewSpec(); if (type == ContainerType.CONTAINER) { builder = @@ -234,6 +361,7 @@ private static StatefulSet createStatefulSet( .withRequests(requests) .withLimits(limits) .endResources() + .withEnv(envVars) .endContainer(); } else { builder = @@ -244,6 +372,7 @@ private static StatefulSet createStatefulSet( .withRequests(requests) .withLimits(limits) .endResources() + .withEnv(envVars) .endInitContainer(); } return builder.endSpec().endTemplate().endSpec().build(); @@ -262,4 +391,18 @@ private static MapAssert assertInitContainerResources( GenericKubernetesResource.>get( actualMap, "spec", "template", "spec", "initContainers", 0, "resources", resourceName)); } + + private static ListAssert> assertContainerEnvVars( + final Map actualMap) { + return assertThat( + GenericKubernetesResource.>>get( + actualMap, "spec", "template", "spec", "containers", 0, "env")); + } + + private static ListAssert> assertInitContainerEnvVars( + final Map actualMap) { + return assertThat( + GenericKubernetesResource.>>get( + actualMap, "spec", "template", "spec", "initContainers", 0, "env")); + } } diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/statefulset.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/statefulset.yaml index f40fbeb607..bb8a2df04b 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/statefulset.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/statefulset.yaml @@ -21,6 +21,13 @@ spec: ports: - containerPort: 80 name: web + env: + - name: APP1_HOST_NAME + value: "" + - name: APP2_HOST_NAME + value: "localhost" + - name: APP3_HOST_NAME + value: " " volumeMounts: - name: www mountPath: /usr/share/nginx/html