From 35547d3391e4d84d5559cdc26f3c3b004b39df62 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 2 May 2023 14:23:12 +0200 Subject: [PATCH 01/17] feat: ignore list support for kubernetes dependent matchers --- .../dependent/DesiredEqualsMatcher.java | 19 --- .../GenericKubernetesResourceMatcher.java | 131 +++++++++++++++--- .../GenericKubernetesResourceMatcherTest.java | 17 ++- 3 files changed, 126 insertions(+), 41 deletions(-) delete mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java deleted file mode 100644 index 459d7951d6..0000000000 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java +++ /dev/null @@ -1,19 +0,0 @@ -package io.javaoperatorsdk.operator.processing.dependent; - -import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.api.reconciler.Context; - -public class DesiredEqualsMatcher implements Matcher { - - private final AbstractDependentResource abstractDependentResource; - - public DesiredEqualsMatcher(AbstractDependentResource abstractDependentResource) { - this.abstractDependentResource = abstractDependentResource; - } - - @Override - public Result match(R actualResource, P primary, Context

context) { - var desired = abstractDependentResource.desired(primary, context); - return Result.computed(actualResource.equals(desired), desired); - } -} 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 6a16d21b44..abeead1747 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 @@ -1,16 +1,17 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; -import java.util.Objects; +import java.util.*; 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 com.fasterxml.jackson.databind.JsonNode; + public class GenericKubernetesResourceMatcher implements Matcher { @@ -63,17 +64,50 @@ public static Result match(R desired, R actualResourc */ public static Result match(R desired, R actualResource, boolean considerMetadata, boolean equality) { + return match(desired, actualResource, considerMetadata, equality, Collections.emptyList()); + } + + public static Result match(R desired, R actualResource, + boolean considerMetadata, String... ignoreList) { + return match(desired, actualResource, considerMetadata, false, Arrays.asList(ignoreList)); + } + + + private static Result match(R desired, R actualResource, + boolean considerMetadata, boolean equality, List ignoreList) { + if (equality && !ignoreList.isEmpty()) { + throw new IllegalArgumentException( + "Equality should be false in case of ignore list provided"); + } + + final var objectMapper = ConfigurationServiceProvider.instance().getObjectMapper(); + + var desiredNode = objectMapper.valueToTree(desired); + var actualNode = objectMapper.valueToTree(actualResource); + var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode); + + var considerIgnoreList = !equality && !ignoreList.isEmpty(); + if (considerMetadata) { - final var desiredMetadata = desired.getMetadata(); - final var actualMetadata = actualResource.getMetadata(); - final var matched = - Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) && - Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels()); - if (!matched) { - return Result.computed(false, desired); + if (equality) { + final var desiredMetadata = desired.getMetadata(); + final var actualMetadata = actualResource.getMetadata(); + + final var matched = + Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) && + Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels()); + if (!matched) { + return Result.computed(false, desired); + } + } else { + var metadataJSonDiffs = getDiffsWithPathSuffix(wholeDiffJsonPatch, + "/metadata/labels", + "/metadata/annotations"); + if (!allDiffsAreAddOps(metadataJSonDiffs)) { + return Result.computed(false, desired); + } } } - if (desired instanceof ConfigMap) { return Result.computed( ResourceComparators.compareConfigMapData((ConfigMap) desired, (ConfigMap) actualResource), @@ -83,22 +117,21 @@ public static Result match(R desired, R actualResourc 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); + var specDiffJsonPatch = getDiffsWithPathSuffix(wholeDiffJsonPatch, "/spec"); // 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) { + if (equality && !specDiffJsonPatch.isEmpty()) { 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")) { + if (considerIgnoreList) { + if (!allDiffsOnIgnoreList(specDiffJsonPatch, ignoreList)) { + return Result.computed(false, desired); + } + } else { + if (!allDiffsAreAddOps(specDiffJsonPatch)) { return Result.computed(false, desired); } } @@ -106,6 +139,38 @@ public static Result match(R desired, R actualResourc } } + private static boolean allDiffsAreAddOps(List metadataJSonDiffs) { + if (metadataJSonDiffs.isEmpty()) { + return true; + } + return metadataJSonDiffs.stream().allMatch(n -> "add".equals(n.get("op").asText())); + } + + private static boolean allDiffsOnIgnoreList(List metadataJSonDiffs, + List ignoreList) { + if (metadataJSonDiffs.isEmpty()) { + return true; + } + return metadataJSonDiffs.stream().allMatch(n -> { + var path = n.get("path").asText(); + return ignoreList.stream().anyMatch(path::startsWith); + }); + } + + private static List getDiffsWithPathSuffix(JsonNode diffJsonPatch, + String... ignorePaths) { + var res = new ArrayList(); + var prefixList = Arrays.asList(ignorePaths); + for (int i = 0; i < diffJsonPatch.size(); i++) { + var node = diffJsonPatch.get(i); + String path = diffJsonPatch.get(i).get("path").asText(); + if (prefixList.stream().anyMatch(path::startsWith)) { + res.add(node); + } + } + return res; + } + /** * Determines whether the specified actual resource matches the desired state defined by the * specified {@link KubernetesDependentResource} based on the observed state of the associated @@ -133,6 +198,34 @@ public static Result match( return match(desired, actualResource, considerMetadata, strongEquality); } + /** + * Determines whether the specified actual resource matches the desired state defined by the + * specified {@link KubernetesDependentResource} based on the observed state of the associated + * specified primary resource. + * + * @param dependentResource the {@link KubernetesDependentResource} implementation used to compute + * the desired state associated with the specified primary resource + * @param actualResource the observed dependent resource for which we want to determine whether it + * 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 considerMetadata {@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 + * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object + * @param the type of resource we want to determine whether they match or not + * @param

the type of primary resources associated with the secondary resources we want to + * match + * @param ignorePaths are paths in the resource that are ignored on matching. Anny related change + * on a calculated JSON Patch between actual and desired will be ignored. + */ + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerMetadata, String... ignorePaths) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerMetadata, ignorePaths); + } + public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, Context

context, boolean considerMetadata) { 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 91a71067ca..d6248a9f05 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 @@ -35,8 +35,8 @@ void checksIfDesiredValuesAreTheSame() { final var matcher = GenericKubernetesResourceMatcher.matcherFor(Deployment.class, 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); + assertThat(matcher.match(actual, null, context).computedDesired()).isPresent(); + assertThat(matcher.match(actual, null, context).computedDesired()).contains(desired); actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(matcher.match(actual, null, context).matched()) @@ -59,6 +59,11 @@ void checksIfDesiredValuesAreTheSame() { .withFailMessage("Changed values are not ok") .isFalse(); + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, false, "/spec/replicas").matched()) + .withFailMessage("Ignored paths are not matched") + .isTrue(); + actual = new DeploymentBuilder(createDeployment()) .editOrNewMetadata() .addToAnnotations("test", "value") @@ -70,9 +75,15 @@ void checksIfDesiredValuesAreTheSame() { .isTrue(); assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true).matched()) + .match(dependentResource, actual, null, context, true, true).matched()) .withFailMessage("Annotations should matter when metadata is considered") .isFalse(); + + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, true, false).matched()) + .withFailMessage("Non strong equality on labels and annotations") + .isTrue(); + } Deployment createDeployment() { From 4e16456a20d40052faf43a8e113bf3cdbbb263dd Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 2 May 2023 15:35:20 +0200 Subject: [PATCH 02/17] tests --- .../GenericKubernetesResourceMatcher.java | 50 +++++++++++++++---- .../GenericKubernetesResourceMatcherTest.java | 47 ++++++++++++++--- 2 files changed, 80 insertions(+), 17 deletions(-) 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 abeead1747..96b9e7caf0 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 @@ -30,7 +30,8 @@ static Matcher matcherFor( @Override public Result match(R actualResource, P primary, Context

context) { var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, false, false); + return match(desired, actualResource, true, false, + false, Collections.emptyList()); } public static Result match(R desired, R actualResource, @@ -64,18 +65,27 @@ public static Result match(R desired, R actualResourc */ public static Result match(R desired, R actualResource, boolean considerMetadata, boolean equality) { - return match(desired, actualResource, considerMetadata, equality, Collections.emptyList()); + return match(desired, actualResource, considerMetadata, false, equality, + Collections.emptyList()); } public static Result match(R desired, R actualResource, boolean considerMetadata, String... ignoreList) { - return match(desired, actualResource, considerMetadata, false, Arrays.asList(ignoreList)); + return match(desired, actualResource, considerMetadata, false, false, + Arrays.asList(ignoreList)); } + public static Result match(R desired, R actualResource, + boolean considerMetadata, boolean metadataEquality, String... ignoreList) { + return match(desired, actualResource, considerMetadata, metadataEquality, false, + Arrays.asList(ignoreList)); + } private static Result match(R desired, R actualResource, - boolean considerMetadata, boolean equality, List ignoreList) { - if (equality && !ignoreList.isEmpty()) { + boolean considerMetadata, boolean metadataEquality, boolean specEquality, + List ignoreList) { + + if (specEquality && !ignoreList.isEmpty()) { throw new IllegalArgumentException( "Equality should be false in case of ignore list provided"); } @@ -86,10 +96,10 @@ private static Result match(R desired, R actualResour var actualNode = objectMapper.valueToTree(actualResource); var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode); - var considerIgnoreList = !equality && !ignoreList.isEmpty(); + var considerIgnoreList = !specEquality && !ignoreList.isEmpty(); if (considerMetadata) { - if (equality) { + if (metadataEquality) { final var desiredMetadata = desired.getMetadata(); final var actualMetadata = actualResource.getMetadata(); @@ -123,7 +133,7 @@ private static Result match(R desired, R actualResour // 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 && !specDiffJsonPatch.isEmpty()) { + if (specEquality && !specDiffJsonPatch.isEmpty()) { return Result.computed(false, desired); } if (considerIgnoreList) { @@ -216,8 +226,11 @@ public static Result match( * @param the type of resource we want to determine whether they match or not * @param

the type of primary resources associated with the secondary resources we want to * match - * @param ignorePaths are paths in the resource that are ignored on matching. Anny related change - * on a calculated JSON Patch between actual and desired will be ignored. + * @param ignorePaths are paths in the resource that are ignored on matching (basically an ignore + * list). All changes with a target prefix path on a calculated JSON Patch between actual + * and desired will be ignored. If there are other changes, non-present on ignore list + * match fails. + * */ public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, @@ -226,10 +239,27 @@ public static Result match( return match(desired, actualResource, considerMetadata, ignorePaths); } + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerMetadata, boolean metadataEquality, + String... ignorePaths) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerMetadata, metadataEquality, ignorePaths); + } + public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, Context

context, boolean considerMetadata) { final var desired = dependentResource.desired(primary, context); return match(desired, actualResource, considerMetadata, false); } + + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerMetadata, boolean metadataEquality, + boolean strongEquality) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerMetadata, metadataEquality, strongEquality, + Collections.emptyList()); + } } 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 d6248a9f05..1723d53a61 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 @@ -11,6 +11,7 @@ import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.Matcher; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; @@ -21,6 +22,12 @@ class GenericKubernetesResourceMatcherTest { private static final Context context = mock(Context.class); + Deployment actual = createDeployment(); + Deployment desired = createDeployment(); + TestDependentResource dependentResource = new TestDependentResource(desired); + Matcher matcher = + GenericKubernetesResourceMatcher.matcherFor(Deployment.class, dependentResource); + @BeforeAll static void setUp() { final var controllerConfiguration = mock(ControllerConfiguration.class); @@ -28,42 +35,68 @@ static void setUp() { } @Test - void checksIfDesiredValuesAreTheSame() { - var actual = createDeployment(); - final var desired = createDeployment(); - final var dependentResource = new TestDependentResource(desired); - final var matcher = - GenericKubernetesResourceMatcher.matcherFor(Deployment.class, dependentResource); + void matchesTrivialCases() { assertThat(matcher.match(actual, null, context).matched()).isTrue(); assertThat(matcher.match(actual, null, context).computedDesired()).isPresent(); assertThat(matcher.match(actual, null, context).computedDesired()).contains(desired); + } + @Test + void matchesAdditiveOnlyChanges() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(matcher.match(actual, null, context).matched()) .withFailMessage("Additive changes should be ok") .isTrue(); + } + @Test + void matchesWithStrongSpecEquality() { + actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true, true).matched()) .withFailMessage("Strong equality does not ignore additive changes on spec") .isFalse(); + } + @Test + void notMatchesRemovedValues() { actual = createDeployment(); assertThat(matcher.match(actual, createPrimary("removed"), context).matched()) .withFailMessage("Removed value should not be ok") .isFalse(); + } + @Test + void notMatchesChangedValues() { actual = createDeployment(); actual.getSpec().setReplicas(2); assertThat(matcher.match(actual, null, context).matched()) .withFailMessage("Changed values are not ok") .isFalse(); + } + @Test + void notMatchesIgnoredPaths() { + actual = createDeployment(); + actual.getSpec().setReplicas(2); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, false, "/spec/replicas").matched()) .withFailMessage("Ignored paths are not matched") .isTrue(); + } + @Test + void ignoresWholeSubPath() { + actual = createDeployment(); + actual.getSpec().getTemplate().getMetadata().getLabels().put("additionak-key", "val"); + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, false, "/spec/template").matched()) + .withFailMessage("Ignored sub-paths are not matched") + .isTrue(); + } + + @Test + void matchesMetadata() { actual = new DeploymentBuilder(createDeployment()) .editOrNewMetadata() .addToAnnotations("test", "value") @@ -75,7 +108,7 @@ void checksIfDesiredValuesAreTheSame() { .isTrue(); assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true, true).matched()) + .match(dependentResource, actual, null, context, true, true, true).matched()) .withFailMessage("Annotations should matter when metadata is considered") .isFalse(); From 2f0504d90b8a0ebc7a61b5123ec7102a2b089806 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 2 May 2023 15:39:19 +0200 Subject: [PATCH 03/17] refactoring implementation --- .../GenericKubernetesResourceMatcher.java | 81 +++++++++++-------- 1 file changed, 48 insertions(+), 33 deletions(-) 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 96b9e7caf0..8151db7807 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 @@ -99,23 +99,10 @@ private static Result match(R desired, R actualResour var considerIgnoreList = !specEquality && !ignoreList.isEmpty(); if (considerMetadata) { - if (metadataEquality) { - final var desiredMetadata = desired.getMetadata(); - final var actualMetadata = actualResource.getMetadata(); - - final var matched = - Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) && - Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels()); - if (!matched) { - return Result.computed(false, desired); - } - } else { - var metadataJSonDiffs = getDiffsWithPathSuffix(wholeDiffJsonPatch, - "/metadata/labels", - "/metadata/annotations"); - if (!allDiffsAreAddOps(metadataJSonDiffs)) { - return Result.computed(false, desired); - } + Optional> res = + matchMetadata(desired, actualResource, metadataEquality, wholeDiffJsonPatch); + if (res.isPresent()) { + return res.orElseThrow(); } } if (desired instanceof ConfigMap) { @@ -127,26 +114,54 @@ private static Result match(R desired, R actualResour ResourceComparators.compareSecretData((Secret) desired, (Secret) actualResource), desired); } else { - // reflection will be replaced by this: - // https://github.com/fabric8io/kubernetes-client/issues/3816 - var specDiffJsonPatch = getDiffsWithPathSuffix(wholeDiffJsonPatch, "/spec"); - // 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 (specEquality && !specDiffJsonPatch.isEmpty()) { + return matchSpec(desired, specEquality, ignoreList, wholeDiffJsonPatch, considerIgnoreList); + } + } + + private static Result matchSpec(R desired, boolean specEquality, + List ignoreList, JsonNode wholeDiffJsonPatch, boolean considerIgnoreList) { + // reflection will be replaced by this: + // https://github.com/fabric8io/kubernetes-client/issues/3816 + var specDiffJsonPatch = getDiffsWithPathSuffix(wholeDiffJsonPatch, "/spec"); + // 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 (specEquality && !specDiffJsonPatch.isEmpty()) { + return Result.computed(false, desired); + } + if (considerIgnoreList) { + if (!allDiffsOnIgnoreList(specDiffJsonPatch, ignoreList)) { + return Result.computed(false, desired); + } + } else { + if (!allDiffsAreAddOps(specDiffJsonPatch)) { return Result.computed(false, desired); } - if (considerIgnoreList) { - if (!allDiffsOnIgnoreList(specDiffJsonPatch, ignoreList)) { - return Result.computed(false, desired); - } - } else { - if (!allDiffsAreAddOps(specDiffJsonPatch)) { - return Result.computed(false, desired); - } + } + return Result.computed(true, desired); + } + + private static Optional> matchMetadata(R desired, + R actualResource, boolean metadataEquality, JsonNode wholeDiffJsonPatch) { + if (metadataEquality) { + final var desiredMetadata = desired.getMetadata(); + final var actualMetadata = actualResource.getMetadata(); + + final var matched = + Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) && + Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels()); + if (!matched) { + return Optional.of(Result.computed(false, desired)); + } + } else { + var metadataJSonDiffs = getDiffsWithPathSuffix(wholeDiffJsonPatch, + "/metadata/labels", + "/metadata/annotations"); + if (!allDiffsAreAddOps(metadataJSonDiffs)) { + return Optional.of(Result.computed(false, desired)); } - return Result.computed(true, desired); } + return Optional.empty(); } private static boolean allDiffsAreAddOps(List metadataJSonDiffs) { From a57fd4d353e721485bff886391868ce498bf1555 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 2 May 2023 17:19:10 +0200 Subject: [PATCH 04/17] integration test for ignore list --- .../operator/ServiceStrictMatcherIT.java | 52 ++++++++++++++++ .../ServiceDependentResource.java | 62 +++++++++++++++++++ .../ServiceStrictMatcherSpec.java | 15 +++++ ...erviceStrictMatcherTestCustomResource.java | 16 +++++ .../ServiceStrictMatcherTestReconciler.java | 29 +++++++++ .../io/javaoperatorsdk/operator/service.yaml | 12 ++++ 6 files changed, 186 insertions(+) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceDependentResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherSpec.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestReconciler.java create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/service.yaml diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java new file mode 100644 index 0000000000..da3160ed14 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java @@ -0,0 +1,52 @@ +package io.javaoperatorsdk.operator; + +import java.time.Duration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.servicestrictmatcher.ServiceDependentResource; +import io.javaoperatorsdk.operator.sample.servicestrictmatcher.ServiceStrictMatcherSpec; +import io.javaoperatorsdk.operator.sample.servicestrictmatcher.ServiceStrictMatcherTestCustomResource; +import io.javaoperatorsdk.operator.sample.servicestrictmatcher.ServiceStrictMatcherTestReconciler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +public class ServiceStrictMatcherIT { + + @RegisterExtension + LocallyRunOperatorExtension operator = + LocallyRunOperatorExtension.builder().withReconciler(new ServiceStrictMatcherTestReconciler()) + .build(); + + + @Test + void testTheMatchingDoesNoTTriggersFurtherUpdates() { + var resource = operator.create(testResource()); + + // make an update to spec to reconcile again + resource.getSpec().setValue(2); + operator.replace(resource); + + await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { + assertThat(operator.getReconcilerOfType(ServiceStrictMatcherTestReconciler.class) + .getNumberOfExecutions()).isEqualTo(2); + assertThat(ServiceDependentResource.updated.get()).isEqualTo(0); + }); + } + + + ServiceStrictMatcherTestCustomResource testResource() { + var res = new ServiceStrictMatcherTestCustomResource(); + res.setSpec(new ServiceStrictMatcherSpec()); + res.getSpec().setValue(1); + res.setMetadata(new ObjectMetaBuilder() + .withName("test1") + .build()); + return res; + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceDependentResource.java new file mode 100644 index 0000000000..1079642b33 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceDependentResource.java @@ -0,0 +1,62 @@ +package io.javaoperatorsdk.operator.sample.servicestrictmatcher; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import io.fabric8.kubernetes.api.model.Service; +import io.javaoperatorsdk.operator.ServiceStrictMatcherIT; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.Matcher; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; + +import static io.javaoperatorsdk.operator.ReconcilerUtils.loadYaml; + +@KubernetesDependent +public class ServiceDependentResource + extends CRUDKubernetesDependentResource { + + public static AtomicInteger updated = new AtomicInteger(0); + + public ServiceDependentResource() { + super(Service.class); + } + + @Override + protected Service desired(ServiceStrictMatcherTestCustomResource primary, + Context context) { + Service service = loadYaml(Service.class, ServiceStrictMatcherIT.class, "service.yaml"); + service.getMetadata().setName(primary.getMetadata().getName()); + service.getMetadata().setNamespace(primary.getMetadata().getNamespace()); + Map labels = new HashMap<>(); + labels.put("app", "deployment-name"); + service.getSpec().setSelector(labels); + return service; + } + + @Override + public Matcher.Result match(Service actualResource, + ServiceStrictMatcherTestCustomResource primary, + Context context) { + return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, false, + false, + "/spec/ports", + "/spec/clusterIP", + "/spec/clusterIPs", + "/spec/externalTrafficPolicy", + "/spec/internalTrafficPolicy", + "/spec/ipFamilies", + "/spec/ipFamilyPolicy", + "/spec/sessionAffinity"); + } + + @Override + public Service update(Service actual, Service target, + ServiceStrictMatcherTestCustomResource primary, + Context context) { + updated.addAndGet(1); + return super.update(actual, target, primary, context); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherSpec.java new file mode 100644 index 0000000000..1233b70914 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherSpec.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.servicestrictmatcher; + +public class ServiceStrictMatcherSpec { + + private int value; + + public int getValue() { + return value; + } + + public ServiceStrictMatcherSpec setValue(int value) { + this.value = value; + return this; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestCustomResource.java new file mode 100644 index 0000000000..6079ceb757 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestCustomResource.java @@ -0,0 +1,16 @@ +package io.javaoperatorsdk.operator.sample.servicestrictmatcher; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@ShortNames("ssm") +public class ServiceStrictMatcherTestCustomResource + extends CustomResource + implements Namespaced { + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestReconciler.java new file mode 100644 index 0000000000..64e81e7c31 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/servicestrictmatcher/ServiceStrictMatcherTestReconciler.java @@ -0,0 +1,29 @@ +package io.javaoperatorsdk.operator.sample.servicestrictmatcher; + +import java.util.concurrent.atomic.AtomicInteger; + +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; + +@ControllerConfiguration(dependents = {@Dependent(type = ServiceDependentResource.class)}) +public class ServiceStrictMatcherTestReconciler + implements Reconciler { + + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + + @Override + public UpdateControl reconcile( + ServiceStrictMatcherTestCustomResource resource, + Context context) { + numberOfExecutions.addAndGet(1); + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } +} diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/service.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/service.yaml new file mode 100644 index 0000000000..b9ef3b7b3d --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/service.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: Service +metadata: + name: "" +spec: + selector: + app: "" + ports: + - protocol: TCP + port: 80 + targetPort: 80 + type: NodePort \ No newline at end of file From c16200787e2bcc4525b4f6d5c2b8330e2cf7647e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 3 May 2023 09:24:56 +0200 Subject: [PATCH 05/17] minor improvements --- .../io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java index da3160ed14..725e4a1324 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java @@ -34,7 +34,7 @@ void testTheMatchingDoesNoTTriggersFurtherUpdates() { await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { assertThat(operator.getReconcilerOfType(ServiceStrictMatcherTestReconciler.class) .getNumberOfExecutions()).isEqualTo(2); - assertThat(ServiceDependentResource.updated.get()).isEqualTo(0); + assertThat(ServiceDependentResource.updated.get()).isZero(); }); } From f5ba7b4fec75731fbd870b28ed4dacc22f59b3e3 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 3 May 2023 09:54:32 +0200 Subject: [PATCH 06/17] test fix --- .../io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java index 725e4a1324..97caab21ee 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ServiceStrictMatcherIT.java @@ -27,6 +27,11 @@ public class ServiceStrictMatcherIT { void testTheMatchingDoesNoTTriggersFurtherUpdates() { var resource = operator.create(testResource()); + await().untilAsserted(() -> { + assertThat(operator.getReconcilerOfType(ServiceStrictMatcherTestReconciler.class) + .getNumberOfExecutions()).isEqualTo(1); + }); + // make an update to spec to reconcile again resource.getSpec().setValue(2); operator.replace(resource); From ee7dae0c830d80fe6ddb5cc0d3a22edd09c9d6dd Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 11:55:04 +0200 Subject: [PATCH 07/17] refactor: more accurate error messages, fix typos/grammar --- .../GenericKubernetesResourceMatcherTest.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) 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 1723d53a61..8a60ebe370 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 @@ -45,7 +45,7 @@ void matchesTrivialCases() { void matchesAdditiveOnlyChanges() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(matcher.match(actual, null, context).matched()) - .withFailMessage("Additive changes should be ok") + .withFailMessage("Additive changes should be not cause a mismatch by default") .isTrue(); } @@ -54,44 +54,44 @@ void matchesWithStrongSpecEquality() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true, true).matched()) - .withFailMessage("Strong equality does not ignore additive changes on spec") + .withFailMessage("Adding values should fail matching when strong equality is required") .isFalse(); } @Test - void notMatchesRemovedValues() { + void doesNotMatchRemovedValues() { actual = createDeployment(); assertThat(matcher.match(actual, createPrimary("removed"), context).matched()) - .withFailMessage("Removed value should not be ok") + .withFailMessage("Removing values in metadata should lead to a mismatch") .isFalse(); } @Test - void notMatchesChangedValues() { + void doesNotMatchChangedValues() { actual = createDeployment(); actual.getSpec().setReplicas(2); assertThat(matcher.match(actual, null, context).matched()) - .withFailMessage("Changed values are not ok") + .withFailMessage("Should not have matched because values have changed") .isFalse(); } @Test - void notMatchesIgnoredPaths() { + void doesNotMatchIgnoredPaths() { actual = createDeployment(); actual.getSpec().setReplicas(2); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, false, "/spec/replicas").matched()) - .withFailMessage("Ignored paths are not matched") + .withFailMessage("Should not have compared ignored paths") .isTrue(); } @Test void ignoresWholeSubPath() { actual = createDeployment(); - actual.getSpec().getTemplate().getMetadata().getLabels().put("additionak-key", "val"); + actual.getSpec().getTemplate().getMetadata().getLabels().put("additional-key", "val"); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, false, "/spec/template").matched()) - .withFailMessage("Ignored sub-paths are not matched") + .withFailMessage("Should match when only changes impact ignored sub-paths") .isTrue(); } @@ -114,7 +114,7 @@ void matchesMetadata() { assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true, false).matched()) - .withFailMessage("Non strong equality on labels and annotations") + .withFailMessage("Should match when strong equality is not considered and only additive changes are made") .isTrue(); } From 535d83ddeae0e833c901d214b915942edabf9cfd Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 11:58:21 +0200 Subject: [PATCH 08/17] fix: if there are no changes, they should not be on the ignore list --- .../dependent/kubernetes/GenericKubernetesResourceMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8151db7807..e99760e714 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 @@ -174,7 +174,7 @@ private static boolean allDiffsAreAddOps(List metadataJSonDiffs) { private static boolean allDiffsOnIgnoreList(List metadataJSonDiffs, List ignoreList) { if (metadataJSonDiffs.isEmpty()) { - return true; + return false; } return metadataJSonDiffs.stream().allMatch(n -> { var path = n.get("path").asText(); From 1620becf60989f53589375c008e6b28b5900e5d0 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 12:00:58 +0200 Subject: [PATCH 09/17] refactor: extract common code, rename method more correctly --- .../GenericKubernetesResourceMatcher.java | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) 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 e99760e714..a69d6ecaac 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 @@ -1,6 +1,11 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Optional; import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -122,7 +127,7 @@ private static Result matchSpec(R desired, boolean sp List ignoreList, JsonNode wholeDiffJsonPatch, boolean considerIgnoreList) { // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 - var specDiffJsonPatch = getDiffsWithPathSuffix(wholeDiffJsonPatch, "/spec"); + var specDiffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, "/spec"); // 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. @@ -154,7 +159,7 @@ private static Optional> matchMetadata(R desir return Optional.of(Result.computed(false, desired)); } } else { - var metadataJSonDiffs = getDiffsWithPathSuffix(wholeDiffJsonPatch, + var metadataJSonDiffs = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, "/metadata/labels", "/metadata/annotations"); if (!allDiffsAreAddOps(metadataJSonDiffs)) { @@ -176,20 +181,25 @@ private static boolean allDiffsOnIgnoreList(List metadataJSonDiffs, if (metadataJSonDiffs.isEmpty()) { return false; } - return metadataJSonDiffs.stream().allMatch(n -> { - var path = n.get("path").asText(); - return ignoreList.stream().anyMatch(path::startsWith); - }); + return metadataJSonDiffs.stream().allMatch(n -> nodeIsChildOf(n, ignoreList)); } - private static List getDiffsWithPathSuffix(JsonNode diffJsonPatch, - String... ignorePaths) { + private static boolean nodeIsChildOf(JsonNode n, List prefixes) { + var path = getPath(n); + return prefixes.stream().anyMatch(path::startsWith); + } + + private static String getPath(JsonNode n) { + return n.get("path").asText(); + } + + private static List getDiffsImpactingPathsWithPrefixes(JsonNode diffJsonPatch, + String... prefixes) { var res = new ArrayList(); - var prefixList = Arrays.asList(ignorePaths); + var prefixList = Arrays.asList(prefixes); for (int i = 0; i < diffJsonPatch.size(); i++) { var node = diffJsonPatch.get(i); - String path = diffJsonPatch.get(i).get("path").asText(); - if (prefixList.stream().anyMatch(path::startsWith)) { + if (nodeIsChildOf(node, prefixList)) { res.add(node); } } From 7a1ec515c8d26afbc567bc5ac7747118d37e643c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 12:32:08 +0200 Subject: [PATCH 10/17] refactor: simplify, make sure empty ignored paths are properly handled --- .../GenericKubernetesResourceMatcher.java | 44 ++++++++----------- .../GenericKubernetesResourceMatcherTest.java | 16 ++++++- 2 files changed, 33 insertions(+), 27 deletions(-) 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 a69d6ecaac..e6e35bc958 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 @@ -35,8 +35,7 @@ static Matcher matcherFor( @Override public Result match(R actualResource, P primary, Context

context) { var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, true, false, - false, Collections.emptyList()); + return match(desired, actualResource, true, false, false); } public static Result match(R desired, R actualResource, @@ -70,25 +69,25 @@ public static Result match(R desired, R actualResourc */ public static Result match(R desired, R actualResource, boolean considerMetadata, boolean equality) { - return match(desired, actualResource, considerMetadata, false, equality, - Collections.emptyList()); + return match(desired, actualResource, considerMetadata, false, equality); } public static Result match(R desired, R actualResource, boolean considerMetadata, String... ignoreList) { - return match(desired, actualResource, considerMetadata, false, false, - Arrays.asList(ignoreList)); + return match(desired, actualResource, considerMetadata, false, false, ignoreList); } public static Result match(R desired, R actualResource, boolean considerMetadata, boolean metadataEquality, String... ignoreList) { - return match(desired, actualResource, considerMetadata, metadataEquality, false, - Arrays.asList(ignoreList)); + return match(desired, actualResource, considerMetadata, metadataEquality, false, ignoreList); } private static Result match(R desired, R actualResource, boolean considerMetadata, boolean metadataEquality, boolean specEquality, - List ignoreList) { + String... ignoredPaths) { + final List ignoreList = + ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths) + : Collections.emptyList(); if (specEquality && !ignoreList.isEmpty()) { throw new IllegalArgumentException( @@ -195,15 +194,18 @@ private static String getPath(JsonNode n) { private static List getDiffsImpactingPathsWithPrefixes(JsonNode diffJsonPatch, String... prefixes) { - var res = new ArrayList(); - var prefixList = Arrays.asList(prefixes); - for (int i = 0; i < diffJsonPatch.size(); i++) { - var node = diffJsonPatch.get(i); - if (nodeIsChildOf(node, prefixList)) { - res.add(node); + if (prefixes != null && prefixes.length > 0) { + var res = new ArrayList(); + var prefixList = Arrays.asList(prefixes); + for (int i = 0; i < diffJsonPatch.size(); i++) { + var node = diffJsonPatch.get(i); + if (nodeIsChildOf(node, prefixList)) { + res.add(node); + } } + return res; } - return res; + return Collections.emptyList(); } /** @@ -272,19 +274,11 @@ public static Result match( return match(desired, actualResource, considerMetadata, metadataEquality, ignorePaths); } - public static Result match( - KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata) { - final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, false); - } - public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, Context

context, boolean considerMetadata, boolean metadataEquality, boolean strongEquality) { final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, metadataEquality, strongEquality, - Collections.emptyList()); + return match(desired, actualResource, considerMetadata, metadataEquality, strongEquality); } } 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 8a60ebe370..f9625c9876 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 @@ -76,7 +76,18 @@ void doesNotMatchChangedValues() { } @Test - void doesNotMatchIgnoredPaths() { + void doesNotMatchChangedValuesWhenNoIgnoredPathsAreProvided() { + actual = createDeployment(); + actual.getSpec().setReplicas(2); + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, true).matched()) + .withFailMessage( + "Should not have matched because values have changed and no ignored path is provided") + .isFalse(); + } + + @Test + void doesNotAttemptToMatchIgnoredPaths() { actual = createDeployment(); actual.getSpec().setReplicas(2); assertThat(GenericKubernetesResourceMatcher @@ -114,7 +125,8 @@ void matchesMetadata() { assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true, false).matched()) - .withFailMessage("Should match when strong equality is not considered and only additive changes are made") + .withFailMessage( + "Should match when strong equality is not considered and only additive changes are made") .isTrue(); } From 0630caec95d4a9265b33ed1f0901bc6cbd801b8f Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 12:47:10 +0200 Subject: [PATCH 11/17] docs: add implementation details --- .../operator/processing/dependent/Matcher.java | 2 +- .../GenericKubernetesResourceMatcher.java | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java index 750fe89cbf..f8bababb42 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java @@ -92,7 +92,7 @@ public Optional computedDesired() { * @return a {@link Result} encapsulating whether the resource matched its desired state and this * associated state if it was computed as part of the matching process. Use the static * convenience methods ({@link Result#nonComputed(boolean)} and - * {@link Result#computed(boolean, Object)}) + * {@link Result#computed(boolean, Object)}) to create your return {@link Result}. */ Result match(R actualResource, P primary, Context

context); } 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 e6e35bc958..aecc2ba906 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 @@ -32,6 +32,22 @@ static Matcher matcherFor( return new GenericKubernetesResourceMatcher(dependentResource); } + /** + * {@inheritDoc} + *

+ * This implementation attempts to cover most common cases out of the box by considering + * non-additive changes to metadata and to the resource's spec (if the resource in question has a + * {@code spec} field), making special provisions for {@link ConfigMap} and {@link Secret} + * resources. Additive changes (i.e. a field is added that previously didn't exist) are not + * considered as triggering a mismatch by default to account for validating webhooks that might + * add default values automatically when not present or some other controller adding labels and/or + * annotations. + *

+ * It should be noted that this implementation is potentially intensive because it generically + * attempts to cover common use cases by performing diffs on the JSON representation of objects. + * If performance is a concern, it might be easier / simpler to provide a {@link Matcher} + * implementation optimized for your use case. + */ @Override public Result match(R actualResource, P primary, Context

context) { var desired = dependentResource.desired(primary, context); From 14d542a3310ae724ff0e5bc50333f3849072d4b9 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 3 May 2023 13:50:42 +0200 Subject: [PATCH 12/17] furthere improvements --- .../GenericKubernetesResourceMatcher.java | 130 +++++++++--------- .../KubernetesDependentResource.java | 3 +- .../GenericKubernetesResourceMatcherTest.java | 4 +- 3 files changed, 73 insertions(+), 64 deletions(-) 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 aecc2ba906..677b76da0a 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 @@ -51,12 +51,7 @@ static Matcher matcherFor( @Override public Result match(R actualResource, P primary, Context

context) { var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, true, false, false); - } - - public static Result match(R desired, R actualResource, - boolean considerMetadata) { - return match(desired, actualResource, considerMetadata, false); + return match(desired, actualResource, false, false, false); } /** @@ -65,10 +60,14 @@ public static Result match(R desired, R actualResourc * * @param desired the desired resource * @param actualResource the actual resource - * @param considerMetadata {@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 equality if {@code false}, the algorithm checks if the properties in the desired + * @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. + * + * @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 @@ -84,22 +83,41 @@ public static Result match(R desired, R actualResourc * @param resource */ public static Result match(R desired, R actualResource, - boolean considerMetadata, boolean equality) { - return match(desired, actualResource, considerMetadata, false, equality); - } - - public static Result match(R desired, R actualResource, - boolean considerMetadata, String... ignoreList) { - return match(desired, actualResource, considerMetadata, false, false, ignoreList); + boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality, + boolean specEquality) { + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, specEquality, new String[0]); } + /** + * Determines whether the specified actual resource matches the specified desired resource, + * possibly considering metadata and deeper equality checks. + * + * @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. + * + * @param ignorePaths are paths in the resource that are ignored on matching (basically an ignore + * list). All changes with a target prefix path on a calculated JSON Patch between actual + * and desired will be ignored. If there are other changes, non-present on ignore list + * match fails. + * @return results of matching + * @param resource + */ public static Result match(R desired, R actualResource, - boolean considerMetadata, boolean metadataEquality, String... ignoreList) { - return match(desired, actualResource, considerMetadata, metadataEquality, false, ignoreList); + boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality, + String... ignorePaths) { + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, false, ignorePaths); } private static Result match(R desired, R actualResource, - boolean considerMetadata, boolean metadataEquality, boolean specEquality, + boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean specEquality, String... ignoredPaths) { final List ignoreList = ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths) @@ -120,7 +138,7 @@ private static Result match(R desired, R actualResour if (considerMetadata) { Optional> res = - matchMetadata(desired, actualResource, metadataEquality, wholeDiffJsonPatch); + matchMetadata(desired, actualResource, labelsAndAnnotationsEquality, wholeDiffJsonPatch); if (res.isPresent()) { return res.orElseThrow(); } @@ -162,8 +180,8 @@ private static Result matchSpec(R desired, boolean sp } private static Optional> matchMetadata(R desired, - R actualResource, boolean metadataEquality, JsonNode wholeDiffJsonPatch) { - if (metadataEquality) { + R actualResource, boolean labelsAndAnnotationsEquality, JsonNode wholeDiffJsonPatch) { + if (labelsAndAnnotationsEquality) { final var desiredMetadata = desired.getMetadata(); final var actualMetadata = actualResource.getMetadata(); @@ -224,31 +242,20 @@ private static List getDiffsImpactingPathsWithPrefixes(JsonNode diffJs return Collections.emptyList(); } - /** - * Determines whether the specified actual resource matches the desired state defined by the - * specified {@link KubernetesDependentResource} based on the observed state of the associated - * specified primary resource. - * - * @param dependentResource the {@link KubernetesDependentResource} implementation used to - * computed the desired state associated with the specified primary resource - * @param actualResource the observed dependent resource for which we want to determine whether it - * 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 considerMetadata {@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 - * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object - * @param the type of resource we want to determine whether they match or not - * @param

the type of primary resources associated with the secondary resources we want to - * match - * @param strongEquality if the resource should match exactly - */ + @Deprecated(forRemoval = true) + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerLabelsAndAnnotations, boolean specEquality) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerLabelsAndAnnotations, specEquality); + } + + @Deprecated(forRemoval = true) public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata, boolean strongEquality) { + Context

context, boolean considerLabelsAndAnnotations, String... ignorePaths) { final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, strongEquality); + return match(desired, actualResource, considerLabelsAndAnnotations, true, ignorePaths); } /** @@ -262,10 +269,12 @@ public static Result match( * 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 considerMetadata {@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 - * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object + * @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. * @param the type of resource we want to determine whether they match or not * @param

the type of primary resources associated with the secondary resources we want to * match @@ -273,28 +282,25 @@ public static Result match( * list). All changes with a target prefix path on a calculated JSON Patch between actual * and desired will be ignored. If there are other changes, non-present on ignore list * match fails. - * + * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object */ public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata, String... ignorePaths) { - final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, ignorePaths); - } - - public static Result match( - KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata, boolean metadataEquality, + Context

context, boolean considerLabelsAndAnnotations, + boolean labelsAndAnnotationsEquality, String... ignorePaths) { final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, metadataEquality, ignorePaths); + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, ignorePaths); } public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata, boolean metadataEquality, - boolean strongEquality) { + Context

context, boolean considerLabelsAndAnnotations, + boolean labelsAndAnnotationsEquality, + boolean specEquality) { final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerMetadata, metadataEquality, strongEquality); + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, specEquality); } } 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 index 5e2b209a71..a6676fd3f1 100644 --- 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 @@ -145,7 +145,8 @@ public Result match(R actualResource, P primary, Context

context) { @SuppressWarnings("unused") public Result match(R actualResource, R desired, P primary, Context

context) { - return GenericKubernetesResourceMatcher.match(desired, actualResource, false); + return GenericKubernetesResourceMatcher.match(desired, actualResource, false, + false, false); } protected void handleDelete(P primary, R secondary, Context

context) { 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 f9625c9876..26a13a422c 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 @@ -53,7 +53,9 @@ void matchesAdditiveOnlyChanges() { void matchesWithStrongSpecEquality() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(GenericKubernetesResourceMatcher - .match(dependentResource, actual, null, context, true, true).matched()) + .match(dependentResource, actual, null, context, true, true, + true) + .matched()) .withFailMessage("Adding values should fail matching when strong equality is required") .isFalse(); } From 54fa804e9610f12626bd62bb945ee46152030fd4 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 3 May 2023 13:54:05 +0200 Subject: [PATCH 13/17] order of methods --- .../GenericKubernetesResourceMatcher.java | 91 ++++++++++--------- 1 file changed, 46 insertions(+), 45 deletions(-) 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 677b76da0a..923733515c 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 @@ -116,6 +116,52 @@ public static Result match(R desired, R actualResourc labelsAndAnnotationsEquality, false, ignorePaths); } + /** + * Determines whether the specified actual resource matches the desired state defined by the + * specified {@link KubernetesDependentResource} based on the observed state of the associated + * specified primary resource. + * + * @param dependentResource the {@link KubernetesDependentResource} implementation used to compute + * the desired state associated with the specified primary resource + * @param actualResource the observed dependent resource for which we want to determine whether it + * 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. + * @param the type of resource we want to determine whether they match or not + * @param

the type of primary resources associated with the secondary resources we want to + * match + * @param ignorePaths are paths in the resource that are ignored on matching (basically an ignore + * list). All changes with a target prefix path on a calculated JSON Patch between actual + * and desired will be ignored. If there are other changes, non-present on ignore list + * match fails. + * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object + */ + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerLabelsAndAnnotations, + boolean labelsAndAnnotationsEquality, + String... ignorePaths) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, ignorePaths); + } + + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerLabelsAndAnnotations, + boolean labelsAndAnnotationsEquality, + boolean specEquality) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerLabelsAndAnnotations, + labelsAndAnnotationsEquality, specEquality); + } + private static Result match(R desired, R actualResource, boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean specEquality, String... ignoredPaths) { @@ -258,49 +304,4 @@ public static Result match( return match(desired, actualResource, considerLabelsAndAnnotations, true, ignorePaths); } - /** - * Determines whether the specified actual resource matches the desired state defined by the - * specified {@link KubernetesDependentResource} based on the observed state of the associated - * specified primary resource. - * - * @param dependentResource the {@link KubernetesDependentResource} implementation used to compute - * the desired state associated with the specified primary resource - * @param actualResource the observed dependent resource for which we want to determine whether it - * 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. - * @param the type of resource we want to determine whether they match or not - * @param

the type of primary resources associated with the secondary resources we want to - * match - * @param ignorePaths are paths in the resource that are ignored on matching (basically an ignore - * list). All changes with a target prefix path on a calculated JSON Patch between actual - * and desired will be ignored. If there are other changes, non-present on ignore list - * match fails. - * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object - */ - public static Result match( - KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerLabelsAndAnnotations, - boolean labelsAndAnnotationsEquality, - String... ignorePaths) { - final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerLabelsAndAnnotations, - labelsAndAnnotationsEquality, ignorePaths); - } - - public static Result match( - KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerLabelsAndAnnotations, - boolean labelsAndAnnotationsEquality, - boolean specEquality) { - final var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, considerLabelsAndAnnotations, - labelsAndAnnotationsEquality, specEquality); - } } From 98669296e7c1d98523450fab41217a425577dcd7 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 3 May 2023 15:29:01 +0200 Subject: [PATCH 14/17] refactor: use constants where possible --- .../GenericKubernetesResourceMatcher.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) 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 923733515c..c9fd9d5751 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 @@ -20,13 +20,20 @@ public class GenericKubernetesResourceMatcher implements Matcher { + private static final String SPEC = "/spec"; + private static final String METADATA_LABELS = "/metadata/labels"; + private static final String METADATA_ANNOTATIONS = "/metadata/annotations"; + private static final String ADD = "add"; + private static final String OP = "op"; + private static final String PATH = "path"; + private final static String[] EMPTY_ARRAY = {}; private final KubernetesDependentResource dependentResource; private GenericKubernetesResourceMatcher(KubernetesDependentResource dependentResource) { this.dependentResource = dependentResource; } - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings({"unchecked", "rawtypes", "unused"}) static Matcher matcherFor( Class resourceType, KubernetesDependentResource dependentResource) { return new GenericKubernetesResourceMatcher(dependentResource); @@ -86,7 +93,7 @@ public static Result match(R desired, R actualResourc boolean considerLabelsAndAnnotations, boolean labelsAndAnnotationsEquality, boolean specEquality) { return match(desired, actualResource, considerLabelsAndAnnotations, - labelsAndAnnotationsEquality, specEquality, new String[0]); + labelsAndAnnotationsEquality, specEquality, EMPTY_ARRAY); } /** @@ -206,7 +213,7 @@ private static Result matchSpec(R desired, boolean sp List ignoreList, JsonNode wholeDiffJsonPatch, boolean considerIgnoreList) { // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 - var specDiffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, "/spec"); + var specDiffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, SPEC); // 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. @@ -239,8 +246,8 @@ private static Optional> matchMetadata(R desir } } else { var metadataJSonDiffs = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, - "/metadata/labels", - "/metadata/annotations"); + METADATA_LABELS, + METADATA_ANNOTATIONS); if (!allDiffsAreAddOps(metadataJSonDiffs)) { return Optional.of(Result.computed(false, desired)); } @@ -252,7 +259,7 @@ private static boolean allDiffsAreAddOps(List metadataJSonDiffs) { if (metadataJSonDiffs.isEmpty()) { return true; } - return metadataJSonDiffs.stream().allMatch(n -> "add".equals(n.get("op").asText())); + return metadataJSonDiffs.stream().allMatch(n -> ADD.equals(n.get(OP).asText())); } private static boolean allDiffsOnIgnoreList(List metadataJSonDiffs, @@ -269,7 +276,7 @@ private static boolean nodeIsChildOf(JsonNode n, List prefixes) { } private static String getPath(JsonNode n) { - return n.get("path").asText(); + return n.get(PATH).asText(); } private static List getDiffsImpactingPathsWithPrefixes(JsonNode diffJsonPatch, From 91de02d7e825d7d076dd10c2c681d81550544951 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 4 May 2023 10:39:40 +0200 Subject: [PATCH 15/17] docs --- .../dependent/kubernetes/GenericKubernetesResourceMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c9fd9d5751..473504af28 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 @@ -43,7 +43,7 @@ static Matcher matcherFor( * {@inheritDoc} *

* This implementation attempts to cover most common cases out of the box by considering - * non-additive changes to metadata and to the resource's spec (if the resource in question has a + * non-additive changes the resource's spec (if the resource in question has a * {@code spec} field), making special provisions for {@link ConfigMap} and {@link Secret} * resources. Additive changes (i.e. a field is added that previously didn't exist) are not * considered as triggering a mismatch by default to account for validating webhooks that might From e9cb2d79b6a0a46338c38e3e4053170777f1c11a Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 4 May 2023 10:40:39 +0200 Subject: [PATCH 16/17] docs format --- .../kubernetes/GenericKubernetesResourceMatcher.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 473504af28..a3d9e33662 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 @@ -43,12 +43,11 @@ static Matcher matcherFor( * {@inheritDoc} *

* This implementation attempts to cover most common cases out of the box by considering - * non-additive changes the resource's spec (if the resource in question has a - * {@code spec} field), making special provisions for {@link ConfigMap} and {@link Secret} - * resources. Additive changes (i.e. a field is added that previously didn't exist) are not - * considered as triggering a mismatch by default to account for validating webhooks that might - * add default values automatically when not present or some other controller adding labels and/or - * annotations. + * non-additive changes the resource's spec (if the resource in question has a {@code spec} + * field), making special provisions for {@link ConfigMap} and {@link Secret} resources. Additive + * changes (i.e. a field is added that previously didn't exist) are not considered as triggering a + * mismatch by default to account for validating webhooks that might add default values + * automatically when not present or some other controller adding labels and/or annotations. *

* It should be noted that this implementation is potentially intensive because it generically * attempts to cover common use cases by performing diffs on the JSON representation of objects. From 71e31f8b4335e2c819d2d3561de9950b56bcd0af Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 4 May 2023 14:51:13 +0200 Subject: [PATCH 17/17] fix: typos [skip ci] --- .../dependent/kubernetes/GenericKubernetesResourceMatcher.java | 2 +- .../kubernetes/GenericKubernetesResourceMatcherTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 a3d9e33662..3c2d3e8401 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 @@ -43,7 +43,7 @@ static Matcher matcherFor( * {@inheritDoc} *

* This implementation attempts to cover most common cases out of the box by considering - * non-additive changes the resource's spec (if the resource in question has a {@code spec} + * non-additive changes to the resource's spec (if the resource in question has a {@code spec} * field), making special provisions for {@link ConfigMap} and {@link Secret} resources. Additive * changes (i.e. a field is added that previously didn't exist) are not considered as triggering a * mismatch by default to account for validating webhooks that might add default values 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 26a13a422c..a23af75d9c 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 @@ -45,7 +45,7 @@ void matchesTrivialCases() { void matchesAdditiveOnlyChanges() { actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); assertThat(matcher.match(actual, null, context).matched()) - .withFailMessage("Additive changes should be not cause a mismatch by default") + .withFailMessage("Additive changes should not cause a mismatch by default") .isTrue(); }