fix: Fix infinite resource updates due empty EnvVars

Signed-off-by: Antonio Fernandez Alhambra <[email protected]>

Author: afalhambra-hivemq

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceRequirementsSanitizer.java renamed to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/PodTemplateSpecSanitizer.java

@@ -1,33 +1,34 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
-import java.util.List;
-import java.util.Map;
-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;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
 
 /**
- * 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.
  *
  * <p>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.
  *
  * <p>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<String, Object> actualMap,
       final PodTemplateSpec actualTemplate,
       final PodTemplateSpec desiredTemplate) {
@@ -37,31 +38,37 @@ 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<String, Object> actualMap,
       final List<Container> actualContainers,
       final List<Container> desiredContainers,
       final String containerPath) {
     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 +128,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 +145,53 @@ private static void sanitizeQuantities(
                       }
                     }));
   }
+
+  @SuppressWarnings("unchecked")
+  private static void sanitizeEnvVars(
+      final Map<String, Object> actualMap,
+      final List<EnvVar> actualEnvVars,
+      final List<EnvVar> 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<String, String>) envVar)
+                                      .get("name")
+                                      .equals(actualEnvVarName))
+                          // add the actual EnvVar value with an empty string to prevent a
+                          // resource update
+                          .forEach(envVar -> ((Map<String, String>) envVar).put("value", ""));
+                    }));
+  }
+
+  private static boolean hasEnvVarNoEmptyValue(
+      final String envVarName, final List<EnvVar> envVars) {
+    return envVars.stream()
+        .anyMatch(
+            envVar ->
+                Objects.equals(envVarName, envVar.getName())
+                    && envVar.getValue() != null
+                    && !envVar.getValue().isEmpty());
+  }
 }

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<String, Object> 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());