Skip to content

Commit

Permalink
improve: matcher for config maps (#2121)
Browse files Browse the repository at this point in the history 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
  • Loading branch information
@csviri
csviri authored Nov 14, 2023
1 parent 4cc151e commit d763fcb
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 36 deletions.
34 changes: 17 additions & 17 deletions ...peratorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,26 +73,26 @@ public Result<R> match(R actualResource, P primary, Context<P> 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 <R> resource
* @return results of matching
*/
public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
R actualResource,
boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality,
boolean specEquality, Context<P> context) {
boolean valuesEquality, Context<P> context) {
return match(desired, actualResource, considerLabelsAndAnnotations,
labelsAndAnnotationsEquality, specEquality, context, EMPTY_ARRAY);
labelsAndAnnotationsEquality, valuesEquality, context, EMPTY_ARRAY);
}

/**
Expand Down Expand Up @@ -171,14 +171,14 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(

public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
R actualResource,
boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean specEquality,
boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean valuesEquality,
Context<P> context,
String... ignoredPaths) {
final List<String> 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");
}
Expand All @@ -192,15 +192,15 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> 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
&& 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);
}
}

Expand Down
52 changes: 33 additions & 19 deletions ...torsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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();
Expand All @@ -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() {
Expand Down

0 comments on commit d763fcb

Please sign in to comment.