feat: use processors for matching as well since usually both are related (#1911)

metacosm · web-flow · commit b15605222ad0 · 2023-05-24T11:27:31.000+02:00
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java
@@ -2,14 +2,10 @@
 
 import java.util.Objects;
 
-import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
-import io.fabric8.kubernetes.api.model.Secret;
-import io.fabric8.zjsonpatch.JsonDiff;
-import io.javaoperatorsdk.operator.ReconcilerUtils;
-import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors.GenericResourceUpdatePreProcessor;
 
 public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends HasMetadata>
     implements Matcher<R, P> {
@@ -22,7 +18,7 @@ private GenericKubernetesResourceMatcher(KubernetesDependentResource<R, P> depen
 
   @SuppressWarnings({"unchecked", "rawtypes"})
   static <R extends HasMetadata, P extends HasMetadata> Matcher<R, P> matcherFor(
-      Class<R> resourceType, KubernetesDependentResource<R, P> dependentResource) {
+      KubernetesDependentResource<R, P> dependentResource) {
     return new GenericKubernetesResourceMatcher(dependentResource);
   }
 
@@ -61,6 +57,7 @@ public static <R extends HasMetadata> Result<R> match(R desired, R actualResourc
    * @return results of matching
    * @param <R> resource
    */
+  @SuppressWarnings("unchecked")
   public static <R extends HasMetadata> Result<R> match(R desired, R actualResource,
       boolean considerMetadata, boolean equality) {
     if (considerMetadata) {
@@ -74,36 +71,10 @@ public static <R extends HasMetadata> Result<R> match(R desired, R actualResourc
       }
     }
 
-    if (desired instanceof ConfigMap) {
-      return Result.computed(
-          ResourceComparators.compareConfigMapData((ConfigMap) desired, (ConfigMap) actualResource),
-          desired);
-    } else if (desired instanceof Secret) {
-      return Result.computed(
-          ResourceComparators.compareSecretData((Secret) desired, (Secret) actualResource),
-          desired);
-    } else {
-      final var objectMapper = ConfigurationServiceProvider.instance().getObjectMapper();
-
-      // reflection will be replaced by this:
-      // https://github.com/fabric8io/kubernetes-client/issues/3816
-      var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired));
-      var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource));
-      var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode);
-      // In case of equality is set to true, no diffs are allowed, so we return early if diffs exist
-      // On contrary (if equality is false), "add" is allowed for cases when for some
-      // resources Kubernetes fills-in values into spec.
-      if (equality && diffJsonPatch.size() > 0) {
-        return Result.computed(false, desired);
-      }
-      for (int i = 0; i < diffJsonPatch.size(); i++) {
-        String operation = diffJsonPatch.get(i).get("op").asText();
-        if (!operation.equals("add")) {
-          return Result.computed(false, desired);
-        }
-      }
-      return Result.computed(true, desired);
-    }
+    final ResourceUpdatePreProcessor<R> processor =
+        GenericResourceUpdatePreProcessor.processorFor((Class<R>) desired.getClass());
+    final var matched = processor.matches(actualResource, desired, equality);
+    return Result.computed(matched, desired);
   }
 
   /**
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
@@ -50,7 +50,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   public KubernetesDependentResource(Class<R> resourceType) {
     super(resourceType);
     matcher = this instanceof Matcher ? (Matcher<R, P>) this
-        : GenericKubernetesResourceMatcher.matcherFor(resourceType, this);
+        : GenericKubernetesResourceMatcher.matcherFor(this);
 
     processor = this instanceof ResourceUpdatePreProcessor
         ? (ResourceUpdatePreProcessor<R>) this
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceUpdatePreProcessor.java
@@ -6,4 +6,6 @@
 public interface ResourceUpdatePreProcessor<R extends HasMetadata> {
 
   R replaceSpecOnActual(R actual, R desired, Context<?> context);
+
+  boolean matches(R actual, R desired, boolean equality);
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ClusterRoleBindingResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ClusterRoleBindingResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding;
 
 public class ClusterRoleBindingResourceUpdatePreProcessor
@@ -10,4 +12,10 @@ protected void updateClonedActual(ClusterRoleBinding actual, ClusterRoleBinding
     actual.setRoleRef(desired.getRoleRef());
     actual.setSubjects(desired.getSubjects());
   }
+
+  @Override
+  public boolean matches(ClusterRoleBinding actual, ClusterRoleBinding desired, boolean equality) {
+    return Objects.equals(actual.getRoleRef(), desired.getRoleRef()) &&
+        Objects.equals(actual.getSubjects(), desired.getSubjects());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ClusterRoleResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ClusterRoleResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
 
 public class ClusterRoleResourceUpdatePreProcessor
@@ -10,4 +12,10 @@ protected void updateClonedActual(ClusterRole actual, ClusterRole desired) {
     actual.setAggregationRule(desired.getAggregationRule());
     actual.setRules(desired.getRules());
   }
+
+  @Override
+  public boolean matches(ClusterRole actual, ClusterRole desired, boolean equality) {
+    return Objects.equals(actual.getRules(), desired.getRules()) &&
+        Objects.equals(actual.getAggregationRule(), desired.getAggregationRule());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ConfigMapResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ConfigMapResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.ConfigMap;
 
 public class ConfigMapResourceUpdatePreProcessor
@@ -11,4 +13,11 @@ protected void updateClonedActual(ConfigMap actual, ConfigMap desired) {
     actual.setBinaryData((desired.getBinaryData()));
     actual.setImmutable(desired.getImmutable());
   }
+
+  @Override
+  public boolean matches(ConfigMap actual, ConfigMap desired, boolean equality) {
+    return Objects.equals(actual.getImmutable(), desired.getImmutable()) &&
+        Objects.equals(actual.getData(), desired.getData()) &&
+        Objects.equals(actual.getBinaryData(), desired.getBinaryData());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/GenericResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/GenericResourceUpdatePreProcessor.java
@@ -11,6 +11,7 @@
 import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding;
 import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
+import io.fabric8.zjsonpatch.JsonDiff;
 import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
@@ -50,4 +51,29 @@ protected void updateClonedActual(R actual, R desired) {
     var desiredSpec = ReconcilerUtils.getSpec(desired);
     ReconcilerUtils.setSpec(actual, desiredSpec);
   }
+
+  @Override
+  public boolean matches(R actual, R desired, boolean equality) {
+    final var objectMapper = ConfigurationServiceProvider.instance().getObjectMapper();
+
+    // reflection will be replaced by this:
+    // https://github.com/fabric8io/kubernetes-client/issues/3816
+    var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired));
+    var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actual));
+    var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode);
+    // In case of equality is set to true, no diffs are allowed, so we return early if diffs exist
+    // On contrary (if equality is false), "add" is allowed for cases when for some
+    // resources Kubernetes fills-in values into spec.
+    final var diffNumber = diffJsonPatch.size();
+    if (equality && diffNumber > 0) {
+      return false;
+    }
+    for (int i = 0; i < diffNumber; i++) {
+      String operation = diffJsonPatch.get(i).get("op").asText();
+      if (!operation.equals("add")) {
+        return false;
+      }
+    }
+    return true;
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/RoleBindingResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/RoleBindingResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
 
 public class RoleBindingResourceUpdatePreProcessor
@@ -10,4 +12,10 @@ protected void updateClonedActual(RoleBinding actual, RoleBinding desired) {
     actual.setRoleRef(desired.getRoleRef());
     actual.setSubjects(desired.getSubjects());
   }
+
+  @Override
+  public boolean matches(RoleBinding actual, RoleBinding desired, boolean equality) {
+    return Objects.equals(actual.getRoleRef(), desired.getRoleRef()) &&
+        Objects.equals(actual.getSubjects(), desired.getSubjects());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/RoleResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/RoleResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.rbac.Role;
 
 public class RoleResourceUpdatePreProcessor extends GenericResourceUpdatePreProcessor<Role> {
@@ -8,4 +10,9 @@ public class RoleResourceUpdatePreProcessor extends GenericResourceUpdatePreProc
   protected void updateClonedActual(Role actual, Role desired) {
     actual.setRules(desired.getRules());
   }
+
+  @Override
+  public boolean matches(Role actual, Role desired, boolean equality) {
+    return Objects.equals(actual.getRules(), desired.getRules());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/SecretResourceUpdatePreProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/SecretResourceUpdatePreProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.Secret;
 
 public class SecretResourceUpdatePreProcessor extends GenericResourceUpdatePreProcessor<Secret> {
@@ -11,4 +13,12 @@ protected void updateClonedActual(Secret actual, Secret desired) {
     actual.setImmutable(desired.getImmutable());
     actual.setType(desired.getType());
   }
+
+  @Override
+  public boolean matches(Secret actual, Secret desired, boolean equality) {
+    return Objects.equals(actual.getImmutable(), desired.getImmutable()) &&
+        Objects.equals(actual.getType(), desired.getType()) &&
+        Objects.equals(actual.getData(), desired.getData()) &&
+        Objects.equals(actual.getStringData(), desired.getStringData());
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ServiceAccountResourceUpdateProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/processors/ServiceAccountResourceUpdateProcessor.java
@@ -1,5 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.ServiceAccount;
 
 public class ServiceAccountResourceUpdateProcessor
@@ -11,4 +13,12 @@ protected void updateClonedActual(ServiceAccount actual, ServiceAccount desired)
     actual.setImagePullSecrets(desired.getImagePullSecrets());
     actual.setSecrets(desired.getSecrets());
   }
+
+  @Override
+  public boolean matches(ServiceAccount actual, ServiceAccount desired, boolean equality) {
+    return Objects.equals(actual.getAutomountServiceAccountToken(),
+        desired.getAutomountServiceAccountToken()) &&
+        Objects.equals(actual.getImagePullSecrets(), desired.getImagePullSecrets()) &&
+        Objects.equals(actual.getSecrets(), desired.getSecrets());
+  }
 }
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java
@@ -6,6 +6,8 @@
 import org.junit.jupiter.api.Test;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.ServiceAccount;
+import io.fabric8.kubernetes.api.model.ServiceAccountBuilder;
 import io.fabric8.kubernetes.api.model.apps.Deployment;
 import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
 import io.javaoperatorsdk.operator.ReconcilerUtils;
@@ -32,8 +34,7 @@ void checksIfDesiredValuesAreTheSame() {
     var actual = createDeployment();
     final var desired = createDeployment();
     final var dependentResource = new TestDependentResource(desired);
-    final var matcher =
-        GenericKubernetesResourceMatcher.matcherFor(Deployment.class, dependentResource);
+    final var matcher = GenericKubernetesResourceMatcher.matcherFor(dependentResource);
     assertThat(matcher.match(actual, null, context).matched()).isTrue();
     assertThat(matcher.match(actual, null, context).computedDesired().isPresent()).isTrue();
     assertThat(matcher.match(actual, null, context).computedDesired().get()).isEqualTo(desired);
@@ -75,6 +76,19 @@ void checksIfDesiredValuesAreTheSame() {
         .isFalse();
   }
 
+  @Test
+  void checkServiceAccount() {
+    final var serviceAccountDR = new ServiceAccountDR();
+
+    final var desired = serviceAccountDR.desired(null, context);
+    var actual = new ServiceAccountBuilder(desired)
+        .addNewImagePullSecret("imagePullSecret3")
+        .build();
+
+    final var matcher = GenericKubernetesResourceMatcher.matcherFor(serviceAccountDR);
+    assertThat(matcher.match(actual, null, context).matched()).isFalse();
+  }
+
   Deployment createDeployment() {
     return ReconcilerUtils.loadYaml(
         Deployment.class, GenericKubernetesResourceMatcherTest.class, "nginx-deployment.yaml");
@@ -88,6 +102,24 @@ HasMetadata createPrimary(String caseName) {
         .build();
   }
 
+  private static class ServiceAccountDR
+      extends KubernetesDependentResource<ServiceAccount, HasMetadata> {
+
+    public ServiceAccountDR() {
+      super(ServiceAccount.class);
+    }
+
+    @Override
+    protected ServiceAccount desired(HasMetadata primary, Context<HasMetadata> context) {
+      return new ServiceAccountBuilder()
+          .withNewMetadata().withName("foo").endMetadata()
+          .withAutomountServiceAccountToken()
+          .addNewImagePullSecret("imagePullSecret1")
+          .addNewImagePullSecret("imagePullSecret2")
+          .build();
+    }
+  }
+
   private class TestDependentResource extends KubernetesDependentResource<Deployment, HasMetadata> {
 
     private final Deployment desired;

Original file line number	Diff line number	Diff line change
`@@ -6,4 +6,6 @@`
`6`	`6`	`public interface ResourceUpdatePreProcessor<R extends HasMetadata> {`
`7`	`7`
`8`	`8`	`R replaceSpecOnActual(R actual, R desired, Context<?> context);`
	`9`	`+`
	`10`	`+ boolean matches(R actual, R desired, boolean equality);`
`9`	`11`	`}`