improve: matcher always considers metadata (#2273)

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java

@@ -58,7 +58,7 @@ static <R extends HasMetadata, P extends HasMetadata> Matcher<R, P> matcherFor(
   @Override
   public Result<R> match(R actualResource, P primary, Context<P> context) {
     var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, false, false, false, context);
+    return match(desired, actualResource, false, false, context);
   }
 
   /**
@@ -67,9 +67,6 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
    *
    * @param desired the desired resource
    * @param actualResource the actual resource
-   * @param considerLabelsAndAnnotations {@code true} if labels and annotations will be checked for
-   *        equality, {@code false} otherwise (meaning that metadata changes will be ignored for
-   *        matching purposes)
    * @param labelsAndAnnotationsEquality if true labels and annotation match exactly in the actual
    *        and desired state if false, additional elements are allowed in actual annotations.
    *        Considered only if considerLabelsAndAnnotations is true.
@@ -89,9 +86,9 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
    */
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
       R actualResource,
-      boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality,
+      boolean labelsAndAnnotationsEquality,
       boolean valuesEquality, Context<P> context) {
-    return match(desired, actualResource, considerLabelsAndAnnotations,
+    return match(desired, actualResource,
         labelsAndAnnotationsEquality, valuesEquality, context, EMPTY_ARRAY);
   }
 
@@ -101,9 +98,6 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    *
    * @param desired the desired resource
    * @param actualResource the actual resource
-   * @param considerLabelsAndAnnotations {@code true} if labels and annotations will be checked for
-   *        equality, {@code false} otherwise (meaning that metadata changes will be ignored for
-   *        matching purposes)
    * @param labelsAndAnnotationsEquality if true labels and annotation match exactly in the actual
    *        and desired state if false, additional elements are allowed in actual annotations.
    *        Considered only if considerLabelsAndAnnotations is true.
@@ -116,9 +110,9 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    */
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
       R actualResource,
-      boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality,
+      boolean labelsAndAnnotationsEquality,
       Context<P> context, String... ignorePaths) {
-    return match(desired, actualResource, considerLabelsAndAnnotations,
+    return match(desired, actualResource,
         labelsAndAnnotationsEquality, false, context, ignorePaths);
   }
 
@@ -133,9 +127,6 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    *        matches the desired state or not
    * @param primary the primary resource from which we want to compute the desired state
    * @param context the {@link Context} instance within which this method is called
-   * @param considerLabelsAndAnnotations {@code true} to consider the metadata of the actual
-   *        resource when determining if it matches the desired state, {@code false} if matching
-   *        should occur only considering the spec of the resources
    * @param labelsAndAnnotationsEquality if true labels and annotation match exactly in the actual
    *        and desired state if false, additional elements are allowed in actual annotations.
    *        Considered only if considerLabelsAndAnnotations is true.
@@ -150,28 +141,28 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    */
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
       KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
-      Context<P> context, boolean considerLabelsAndAnnotations,
+      Context<P> context,
       boolean labelsAndAnnotationsEquality,
       String... ignorePaths) {
     final var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, considerLabelsAndAnnotations,
+    return match(desired, actualResource,
         labelsAndAnnotationsEquality, context,
         ignorePaths);
   }
 
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
       KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
-      Context<P> context, boolean considerLabelsAndAnnotations,
+      Context<P> context,
+      boolean specEquality,
       boolean labelsAndAnnotationsEquality,
-      boolean specEquality) {
+      String... ignorePaths) {
     final var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, considerLabelsAndAnnotations,
-        labelsAndAnnotationsEquality, specEquality, context);
+    return match(desired, actualResource,
+        labelsAndAnnotationsEquality, specEquality, context, ignorePaths);
   }
 
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
-      R actualResource,
-      boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean valuesEquality,
+      R actualResource, boolean labelsAndAnnotationsEquality, boolean valuesEquality,
       Context<P> context,
       String... ignoredPaths) {
     final List<String> ignoreList =
@@ -195,8 +186,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
         matched = match(valuesEquality, node, ignoreList);
       } else if (nodeIsChildOf(node, List.of(METADATA))) {
         // conditionally consider labels and annotations
-        if (considerMetadata
-            && nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) {
+        if (nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) {
           matched = match(labelsAndAnnotationsEquality, node, Collections.emptyList());
         }
       } else if (!nodeIsChildOf(node, IGNORED_FIELDS)) {
@@ -227,20 +217,4 @@ static String getPath(JsonNode n) {
     return n.get(PATH).asText();
   }
 
-  @Deprecated(forRemoval = true)
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
-      KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
-      Context<P> context, boolean considerLabelsAndAnnotations, boolean specEquality) {
-    final var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, considerLabelsAndAnnotations, specEquality, context);
-  }
-
-  @Deprecated(forRemoval = true)
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
-      KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
-      Context<P> context, boolean considerLabelsAndAnnotations, String... ignorePaths) {
-    final var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, considerLabelsAndAnnotations, true, context, ignorePaths);
-  }
-
 }

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/updatermatcher/GenericResourceUpdaterMatcher.java

@@ -43,7 +43,7 @@ public R updateResource(R actual, R desired, Context<?> context) {
 
   @Override
   public boolean matches(R actual, R desired, Context<?> context) {
-    return GenericKubernetesResourceMatcher.match(desired, actual, true,
+    return GenericKubernetesResourceMatcher.match(desired, actual,
         false, false, context).matched();
   }
 

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java

@@ -55,8 +55,7 @@ void matchesAdditiveOnlyChanges() {
   @Test
   void matchesWithStrongSpecEquality() {
     actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val");
-    assertThat(match(dependentResource, actual, null, context, true, true,
-        true)
+    assertThat(match(desired, actual, true, true, context)
         .matched())
         .withFailMessage("Adding values should fail matching when strong equality is required")
         .isFalse();
@@ -127,11 +126,11 @@ void matchesMetadata() {
         .withFailMessage("Annotations shouldn't matter when metadata is not considered")
         .isTrue();
 
-    assertThat(match(dependentResource, actual, null, context, true, true, true).matched())
+    assertThat(match(desired, actual, true, true, context).matched())
         .withFailMessage("Annotations should matter when metadata is considered")
         .isFalse();
 
-    assertThat(match(dependentResource, actual, null, context, true, false).matched())
+    assertThat(match(desired, actual, false, false, context).matched())
         .withFailMessage(
             "Should match when strong equality is not considered and only additive changes are made")
         .isTrue();
@@ -157,7 +156,7 @@ void matchConfigMap() {
     var actual = createConfigMap();
     actual.getData().put("key2", "val2");
 
-    var match = GenericKubernetesResourceMatcher.match(desired, actual, true,
+    var match = GenericKubernetesResourceMatcher.match(desired, actual,
         true, false, context);
     assertThat(match.matched()).isTrue();
   }

Diff for: operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/ssalegacymatcher/ServiceDependentResource.java

@@ -39,8 +39,10 @@ protected Service desired(SSALegacyMatcherCustomResource primary,
   @Override
   public Result<Service> match(Service actualResource, SSALegacyMatcherCustomResource primary,
       Context<SSALegacyMatcherCustomResource> context) {
+    var desired = desired(primary, context);
+
     return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context,
-        true, false, false);
+        false, false);
   }
 
   // override just to check the exec count