From ff8adb3a25e818dea291f6cc2d902b96d448407a Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 24 Jan 2023 11:11:59 +0100 Subject: [PATCH 1/7] feat: string equality matcher --- .../GenericKubernetesResourceMatcher.java | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 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 9952763e6a..a13521c0fb 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 @@ -29,11 +29,16 @@ static Matcher matcherFor( @Override public Result match(R actualResource, P primary, Context

context) { var desired = dependentResource.desired(primary, context); - return match(desired, actualResource, false); + return match(desired, actualResource, false, false); } public static Result match(R desired, R actualResource, - boolean considerMetadata) { + boolean considerMetadata) { + return match(desired,actualResource,considerMetadata,false); + } + + public static Result match(R desired, R actualResource, + boolean considerMetadata, boolean strongEquality) { if (considerMetadata) { final var desiredMetadata = desired.getMetadata(); final var actualMetadata = actualResource.getMetadata(); @@ -61,6 +66,9 @@ public static Result match(R desired, R actualResourc var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); + if (strongEquality && diffJsonPatch.size() > 0) { + return Result.computed(false, desired); + } for (int i = 0; i < diffJsonPatch.size(); i++) { String operation = diffJsonPatch.get(i).get("op").asText(); if (!operation.equals("add")) { @@ -92,8 +100,15 @@ public static Result match(R desired, R actualResourc */ public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata) { + Context

context, boolean considerMetadata, boolean strongEquality) { + final var desired = dependentResource.desired(primary, context); + return match(desired, actualResource, considerMetadata, strongEquality); + } + + 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); + return match(desired, actualResource, considerMetadata, false); } } From c14c8ade5e81edb87ec4a9ed6d1e608da26376f0 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 25 Jan 2023 09:41:15 +0100 Subject: [PATCH 2/7] feat: strong equality matcher --- .../kubernetes/GenericKubernetesResourceMatcher.java | 8 ++++---- .../kubernetes/GenericKubernetesResourceMatcherTest.java | 5 +++++ 2 files changed, 9 insertions(+), 4 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 a13521c0fb..b2823b0f21 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 @@ -33,8 +33,8 @@ public Result match(R actualResource, P primary, Context

context) { } public static Result match(R desired, R actualResource, - boolean considerMetadata) { - return match(desired,actualResource,considerMetadata,false); + boolean considerMetadata) { + return match(desired, actualResource, considerMetadata, false); } public static Result match(R desired, R actualResource, @@ -106,8 +106,8 @@ public static Result match( } public static Result match( - KubernetesDependentResource dependentResource, R actualResource, P primary, - Context

context, boolean considerMetadata) { + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerMetadata) { final var desired = dependentResource.desired(primary, context); return match(desired, actualResource, considerMetadata, false); } 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 d39db6ced6..91a71067ca 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 @@ -43,6 +43,11 @@ void checksIfDesiredValuesAreTheSame() { .withFailMessage("Additive changes should be ok") .isTrue(); + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, true, true).matched()) + .withFailMessage("Strong equality does not ignore additive changes on spec") + .isFalse(); + actual = createDeployment(); assertThat(matcher.match(actual, createPrimary("removed"), context).matched()) .withFailMessage("Removed value should not be ok") From fe5264070353e5357f119c465e8e26384ccad8b7 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 26 Jan 2023 11:31:03 +0100 Subject: [PATCH 3/7] CR fixes --- .../GenericKubernetesResourceMatcher.java | 24 +++++++++++++++++-- 1 file changed, 22 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 b2823b0f21..b17e32f52d 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 @@ -37,8 +37,28 @@ public static Result match(R desired, R actualResourc return match(desired, actualResource, considerMetadata, false); } + /** + * + * @param desired - desired resource + * @param actualResource - actual resource + * @param considerMetadata - if changes of metadata should be considered. If yes labels and + * annotations will be checked for equality. + * @param equality - if 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 + * strongEquality to 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. + * + * @return results of matching + * @param resource + */ public static Result match(R desired, R actualResource, - boolean considerMetadata, boolean strongEquality) { + boolean considerMetadata, boolean equality) { if (considerMetadata) { final var desiredMetadata = desired.getMetadata(); final var actualMetadata = actualResource.getMetadata(); @@ -66,7 +86,7 @@ public static Result match(R desired, R actualResourc var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); - if (strongEquality && diffJsonPatch.size() > 0) { + if (equality && diffJsonPatch.size() > 0) { return Result.computed(false, desired); } for (int i = 0; i < diffJsonPatch.size(); i++) { From 60ee663946608a80b940df8b1c641908bd80f03c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 26 Jan 2023 15:18:29 +0100 Subject: [PATCH 4/7] docs: try to clarify javadoc a little --- .../GenericKubernetesResourceMatcher.java | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 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 b17e32f52d..7c897b3681 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 @@ -38,21 +38,25 @@ public static Result match(R desired, R actualResourc } /** - * - * @param desired - desired resource - * @param actualResource - actual resource - * @param considerMetadata - if changes of metadata should be considered. If yes labels and - * annotations will be checked for equality. - * @param equality - if 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 - * strongEquality to 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. + * 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 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 + * 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. * * @return results of matching * @param resource From 4a7c6424798c625c4f31f5efe9010e1250ef5a91 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 26 Jan 2023 15:41:12 +0100 Subject: [PATCH 5/7] fix: format [skip ci] --- .../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 7c897b3681..3b7736f69b 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 @@ -40,7 +40,7 @@ public static Result match(R desired, R actualResourc /** * 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 considerMetadata {@code true} if labels and annotations will be checked for equality, From 8caee91a648fd2906aad37363aaa911d2a65ebf1 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 8 Feb 2023 09:41:03 +0100 Subject: [PATCH 6/7] added comment --- .../dependent/kubernetes/GenericKubernetesResourceMatcher.java | 3 +++ 1 file changed, 3 insertions(+) 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 3b7736f69b..e9070f0727 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 @@ -90,6 +90,9 @@ public static Result match(R desired, R actualResourc var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); + // In case of equality no diffs are allowed, so the both will return if found any with + // non-match. On contrary (if equality is false), "add" is allowed for cases when for some + // resources Kubernetes fills-in values into spec. if (equality && diffJsonPatch.size() > 0) { return Result.computed(false, desired); } From 7d01c7c3234572b55b4e5a11aa692e4a554b299c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 8 Feb 2023 10:17:00 +0100 Subject: [PATCH 7/7] fix: improve wording --- .../kubernetes/GenericKubernetesResourceMatcher.java | 4 ++-- 1 file 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 e9070f0727..352e4ee419 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 @@ -90,8 +90,8 @@ public static Result match(R desired, R actualResourc var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desired)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); - // In case of equality no diffs are allowed, so the both will return if found any with - // non-match. On contrary (if equality is false), "add" is allowed for cases when for some + // In case of equality is set to true, no diffs are allowed, so we return early if diffs exist + // On contrary (if equality is false), "add" is allowed for cases when for some // resources Kubernetes fills-in values into spec. if (equality && diffJsonPatch.size() > 0) { return Result.computed(false, desired);