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 index 98198bf39a..c71da5d5ad 100644 --- 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 @@ -73,26 +73,26 @@ public Result match(R actualResource, P primary, Context

context) { * @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. - * @param specEquality if {@code false}, the algorithm checks if the properties in the desired - * resource spec are same as in the actual resource spec. The reason is that admission - * controllers and default Kubernetes controllers might add default values to some - * properties which are not set in the desired resources' spec and comparing it with simple - * equality check would mean that such resource will not match (while conceptually should). - * However, there is an issue with this for example if desired spec contains a list of - * values and a value is removed, this still will match the actual state from previous - * reconciliation. Setting this parameter to {@code true}, will match the resources only if - * all properties and values are equal. This could be implemented also by overriding equals - * method of spec, should be done as an optimization - this implementation does not require - * that. + * @param valuesEquality if {@code false}, the algorithm checks if the properties in the desired + * resource spec (or other non metadata value) are same as in the actual resource spec. The + * reason is that admission controllers and default Kubernetes controllers might add + * default values to some properties which are not set in the desired resources' spec and + * comparing it with simple equality check would mean that such resource will not match + * (while conceptually should). However, there is an issue with this for example if desired + * spec contains a list of values and a value is removed, this still will match the actual + * state from previous reconciliation. Setting this parameter to {@code true}, will match + * the resources only if all properties and values are equal. This could be implemented + * also by overriding equals method of spec, should be done as an optimization - this + * implementation does not require that. * @param resource * @return results of matching */ public static Result match(R desired, R actualResource, boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality, - boolean specEquality, Context

context) { + boolean valuesEquality, Context

context) { return match(desired, actualResource, considerLabelsAndAnnotations, - labelsAndAnnotationsEquality, specEquality, context, EMPTY_ARRAY); + labelsAndAnnotationsEquality, valuesEquality, context, EMPTY_ARRAY); } /** @@ -171,14 +171,14 @@ public static Result match( public static Result match(R desired, R actualResource, - boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean specEquality, + boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean valuesEquality, Context

context, String... ignoredPaths) { final List ignoreList = ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths) : Collections.emptyList(); - if (specEquality && !ignoreList.isEmpty()) { + if (valuesEquality && !ignoreList.isEmpty()) { throw new IllegalArgumentException( "Equality should be false in case of ignore list provided"); } @@ -192,7 +192,7 @@ public static Result match(R d for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) { var node = wholeDiffJsonPatch.get(i); if (nodeIsChildOf(node, List.of(SPEC))) { - matched = match(specEquality, node, ignoreList); + matched = match(valuesEquality, node, ignoreList); } else if (nodeIsChildOf(node, List.of(METADATA))) { // conditionally consider labels and annotations if (considerMetadata @@ -200,7 +200,7 @@ && nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) { matched = match(labelsAndAnnotationsEquality, node, Collections.emptyList()); } } else if (!nodeIsChildOf(node, IGNORED_FIELDS)) { - matched = match(true, node, ignoreList); + matched = match(valuesEquality, node, ignoreList); } } 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 index b1ff214f3b..a2eea9279c 100644 --- 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 @@ -1,13 +1,12 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.Map; import java.util.Optional; import org.junit.jupiter.api.BeforeAll; 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.*; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; import io.fabric8.kubernetes.api.model.apps.DeploymentStatusBuilder; @@ -17,6 +16,7 @@ import io.javaoperatorsdk.operator.processing.dependent.Matcher; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.updatermatcher.GenericResourceUpdaterMatcher; +import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -55,9 +55,8 @@ void matchesAdditiveOnlyChanges() { @Test void matchesWithStrongSpecEquality() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true, true, - true) + assertThat(match(dependentResource, actual, null, context, true, true, + true) .matched()) .withFailMessage("Adding values should fail matching when strong equality is required") .isFalse(); @@ -93,8 +92,7 @@ void ignoreStatus() { void doesNotMatchChangedValuesWhenNoIgnoredPathsAreProvided() { actual = createDeployment(); actual.getSpec().setReplicas(2); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true).matched()) + assertThat(match(dependentResource, actual, null, context, true).matched()) .withFailMessage( "Should not have matched because values have changed and no ignored path is provided") .isFalse(); @@ -104,8 +102,7 @@ void doesNotMatchChangedValuesWhenNoIgnoredPathsAreProvided() { void doesNotAttemptToMatchIgnoredPaths() { actual = createDeployment(); actual.getSpec().setReplicas(2); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, false, "/spec/replicas").matched()) + assertThat(match(dependentResource, actual, null, context, false, "/spec/replicas").matched()) .withFailMessage("Should not have compared ignored paths") .isTrue(); } @@ -114,8 +111,7 @@ void doesNotAttemptToMatchIgnoredPaths() { void ignoresWholeSubPath() { actual = createDeployment(); actual.getSpec().getTemplate().getMetadata().getLabels().put("additional-key", "val"); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, false, "/spec/template").matched()) + assertThat(match(dependentResource, actual, null, context, false, "/spec/template").matched()) .withFailMessage("Should match when only changes impact ignored sub-paths") .isTrue(); } @@ -127,18 +123,15 @@ void matchesMetadata() { .addToAnnotations("test", "value") .endMetadata() .build(); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, false).matched()) + assertThat(match(dependentResource, actual, null, context, false).matched()) .withFailMessage("Annotations shouldn't matter when metadata is not considered") .isTrue(); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true, true, true).matched()) + assertThat(match(dependentResource, actual, null, context, true, true, true).matched()) .withFailMessage("Annotations should matter when metadata is considered") .isFalse(); - assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true, false).matched()) + assertThat(match(dependentResource, actual, null, context, true, false).matched()) .withFailMessage( "Should match when strong equality is not considered and only additive changes are made") .isTrue(); @@ -155,7 +148,28 @@ void checkServiceAccount() { .build(); final var matcher = GenericResourceUpdaterMatcher.updaterMatcherFor(ServiceAccount.class); - assertThat(matcher.matches(actual, desired, context)).isFalse(); + assertThat(matcher.matches(actual, desired, context)).isTrue(); + } + + @Test + void matchConfigMap() { + var desired = createConfigMap(); + var actual = createConfigMap(); + actual.getData().put("key2", "val2"); + + var match = GenericKubernetesResourceMatcher.match(desired, actual, true, + true, false, context); + assertThat(match.matched()).isTrue(); + } + + ConfigMap createConfigMap() { + return new ConfigMapBuilder() + .withMetadata(new ObjectMetaBuilder() + .withName("tes1") + .withNamespace("default") + .build()) + .withData(Map.of("key1", "val1")) + .build(); } Deployment createDeployment() {