From acaaab0766211978edf0429e76956d2963dade62 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 10 Aug 2022 16:57:07 +0200 Subject: [PATCH 1/5] feat: make it possible to also check annotations/labels when matching Fixes #1392 --- .../GenericKubernetesResourceMatcher.java | 21 ++++++- .../GenericKubernetesResourceMatcherTest.java | 56 ++++++++++++++----- 2 files changed, 61 insertions(+), 16 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 f2c789e267..356598fbbf 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,5 +1,7 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.Objects; + import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Secret; @@ -41,8 +43,25 @@ static Matcher matcherFor( @Override public Result match(R actualResource, P primary, Context

context) { - final var objectMapper = ConfigurationServiceProvider.instance().getObjectMapper(); + return match(dependentResource, actualResource, primary, context, false); + } + + public static Result match( + KubernetesDependentResource dependentResource, R actualResource, P primary, + Context

context, boolean considerMetadata) { final var desired = dependentResource.desired(primary, context); + 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); + } + } + + final var objectMapper = ConfigurationServiceProvider.instance().getObjectMapper(); // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 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 bad10d551d..3af57a67c7 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 @@ -31,21 +31,9 @@ static void setUp() { void checksIfDesiredValuesAreTheSame() { var actual = createDeployment(); final var desired = createDeployment(); - final var matcher = GenericKubernetesResourceMatcher.matcherFor(Deployment.class, - new KubernetesDependentResource<>(Deployment.class) { - @Override - protected Deployment desired(HasMetadata primary, Context context) { - final var currentCase = Optional.ofNullable(primary) - .map(p -> p.getMetadata().getLabels().get("case")) - .orElse(null); - var d = desired; - if ("removed".equals(currentCase)) { - d = createDeployment(); - d.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - } - return d; - } - }); + final var dependentResource = new TestDependentResource(desired); + 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); @@ -65,6 +53,21 @@ protected Deployment desired(HasMetadata primary, Context context) { assertThat(matcher.match(actual, null, context).matched()) .withFailMessage("Changed values are not ok") .isFalse(); + + actual = new DeploymentBuilder(createDeployment()) + .editOrNewMetadata() + .addToAnnotations("test", "value") + .endMetadata() + .build(); + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, false).matched()) + .withFailMessage("Annotations shouldn't matter when metadata is not considered") + .isTrue(); + + assertThat(GenericKubernetesResourceMatcher + .match(dependentResource, actual, null, context, true).matched()) + .withFailMessage("Annotations should matter when metadata is not considered") + .isFalse(); } Deployment createDeployment() { @@ -79,4 +82,27 @@ HasMetadata createPrimary(String caseName) { .endMetadata() .build(); } + + private class TestDependentResource extends KubernetesDependentResource { + + private final Deployment desired; + + public TestDependentResource(Deployment desired) { + super(Deployment.class); + this.desired = desired; + } + + @Override + protected Deployment desired(HasMetadata primary, Context context) { + final var currentCase = Optional.ofNullable(primary) + .map(p -> p.getMetadata().getLabels().get("case")) + .orElse(null); + var d = desired; + if ("removed".equals(currentCase)) { + d = createDeployment(); + d.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); + } + return d; + } + } } From 6ad6eea6c7e7f047e4d212130aaa775e567ac542 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 11 Aug 2022 10:39:18 +0200 Subject: [PATCH 2/5] fix: format --- .../kubernetes/GenericKubernetesResourceMatcherTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 3af57a67c7..f4416c47b7 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 @@ -61,13 +61,13 @@ void checksIfDesiredValuesAreTheSame() { .build(); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, false).matched()) - .withFailMessage("Annotations shouldn't matter when metadata is not considered") - .isTrue(); + .withFailMessage("Annotations shouldn't matter when metadata is not considered") + .isTrue(); assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true).matched()) - .withFailMessage("Annotations should matter when metadata is not considered") - .isFalse(); + .withFailMessage("Annotations should matter when metadata is not considered") + .isFalse(); } Deployment createDeployment() { From e74d9281fd2af0deaa1f8c98ba32896ad0872268 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 24 Aug 2022 22:14:54 +0200 Subject: [PATCH 3/5] docs: add Matcher information, show how to use metadata matching --- docs/documentation/dependent-resources.md | 189 +++++++++++++--------- 1 file changed, 109 insertions(+), 80 deletions(-) diff --git a/docs/documentation/dependent-resources.md b/docs/documentation/dependent-resources.md index e1d073d1ee..720e78cd7b 100644 --- a/docs/documentation/dependent-resources.md +++ b/docs/documentation/dependent-resources.md @@ -86,6 +86,35 @@ possible to not implement any of these traits and therefore create read-only dep that will trigger your reconciler whenever a user interacts with them but that are never modified by your reconciler itself. +[`AbstractSimpleDependentResource`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractSimpleDependentResource.java) +and [`KubernetesDependentResource`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java) +sub-classes can also implement +the [`Matcher`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/Matcher.java) +interface to customize how the SDK decides whether or not the actual state of the dependent +matches the desired state. This makes it convenient to use these abstract base classes for your +implementation, only customizing the matching logic. Note that in many cases, there is no need +to customize that logic as the SDK already provides convenient default implementations in the +form +of [`DesiredEqualsMatcher`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DesiredEqualsMatcher.java) +and +[`GenericKubernetesResourceMatcher`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java) +implementations, respectively. If you want to provide custom logic, you only need your +`DependentResource` implementation to implement the `Matcher` interface as below, which shows +how to customize the default matching logic for Kubernetes resource to also consider annotations +and labels, which are ignored by default: + +```java +public class MyDependentResource extends KubernetesDependentResource + implements Matcher { + // your implementation + + public Result match(MyDependent actualResource, MyPrimary primary, + Context context) { + return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, true); + } +} +``` + ### Batteries included: convenient DependentResource implementations! JOSDK also offers several other convenient implementations building on top of @@ -116,7 +145,7 @@ Deleted (or set to be garbage collected). The following example shows how to cre @KubernetesDependent(labelSelector = WebPageManagedDependentsReconciler.SELECTOR) class DeploymentDependentResource extends CRUDKubernetesDependentResource { - public DeploymentDependentResource() { + public DeploymentDependentResource() { super(Deployment.class); } @@ -169,26 +198,26 @@ instances are managed by JOSDK, an example of which can be seen below: ```java @ControllerConfiguration( - labelSelector = SELECTOR, - dependents = { - @Dependent(type = ConfigMapDependentResource.class), - @Dependent(type = DeploymentDependentResource.class), - @Dependent(type = ServiceDependentResource.class) - }) + labelSelector = SELECTOR, + dependents = { + @Dependent(type = ConfigMapDependentResource.class), + @Dependent(type = DeploymentDependentResource.class), + @Dependent(type = ServiceDependentResource.class) + }) public class WebPageManagedDependentsReconciler - implements Reconciler, ErrorStatusHandler { + implements Reconciler, ErrorStatusHandler { - // omitted code + // omitted code - @Override - public UpdateControl reconcile(WebPage webPage, Context context) - throws Exception { + @Override + public UpdateControl reconcile(WebPage webPage, Context context) + throws Exception { - final var name = context.getSecondaryResource(ConfigMap.class).orElseThrow() - .getMetadata().getName(); - webPage.setStatus(createStatus(name)); - return UpdateControl.patchStatus(webPage); - } + final var name = context.getSecondaryResource(ConfigMap.class).orElseThrow() + .getMetadata().getName(); + webPage.setStatus(createStatus(name)); + return UpdateControl.patchStatus(webPage); + } } ``` @@ -215,69 +244,69 @@ an `Ingress`: @ControllerConfiguration public class WebPageStandaloneDependentsReconciler - implements Reconciler, ErrorStatusHandler, - EventSourceInitializer { - - private KubernetesDependentResource configMapDR; - private KubernetesDependentResource deploymentDR; - private KubernetesDependentResource serviceDR; - private KubernetesDependentResource ingressDR; - - public WebPageStandaloneDependentsReconciler(KubernetesClient kubernetesClient) { - // 1. - createDependentResources(kubernetesClient); - } - - @Override - public List prepareEventSources(EventSourceContext context) { - // 2. - return List.of( - configMapDR.initEventSource(context), - deploymentDR.initEventSource(context), - serviceDR.initEventSource(context)); - } - - @Override - public UpdateControl reconcile(WebPage webPage, Context context) - throws Exception { - - // 3. - if (!isValidHtml(webPage.getHtml())) { - return UpdateControl.patchStatus(setInvalidHtmlErrorMessage(webPage)); - } - - // 4. - configMapDR.reconcile(webPage, context); - deploymentDR.reconcile(webPage, context); - serviceDR.reconcile(webPage, context); - - // 5. - if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) { - ingressDR.reconcile(webPage, context); - } else { - ingressDR.delete(webPage, context); - } - - // 6. - webPage.setStatus( - createStatus(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName())); - return UpdateControl.patchStatus(webPage); - } - - private void createDependentResources(KubernetesClient client) { - this.configMapDR = new ConfigMapDependentResource(); - this.deploymentDR = new DeploymentDependentResource(); - this.serviceDR = new ServiceDependentResource(); - this.ingressDR = new IngressDependentResource(); - - Arrays.asList(configMapDR, deploymentDR, serviceDR, ingressDR).forEach(dr -> { - dr.setKubernetesClient(client); - dr.configureWith(new KubernetesDependentResourceConfig() - .setLabelSelector(DEPENDENT_RESOURCE_LABEL_SELECTOR)); - }); - } - - // omitted code + implements Reconciler, ErrorStatusHandler, + EventSourceInitializer { + + private KubernetesDependentResource configMapDR; + private KubernetesDependentResource deploymentDR; + private KubernetesDependentResource serviceDR; + private KubernetesDependentResource ingressDR; + + public WebPageStandaloneDependentsReconciler(KubernetesClient kubernetesClient) { + // 1. + createDependentResources(kubernetesClient); + } + + @Override + public List prepareEventSources(EventSourceContext context) { + // 2. + return List.of( + configMapDR.initEventSource(context), + deploymentDR.initEventSource(context), + serviceDR.initEventSource(context)); + } + + @Override + public UpdateControl reconcile(WebPage webPage, Context context) + throws Exception { + + // 3. + if (!isValidHtml(webPage.getHtml())) { + return UpdateControl.patchStatus(setInvalidHtmlErrorMessage(webPage)); + } + + // 4. + configMapDR.reconcile(webPage, context); + deploymentDR.reconcile(webPage, context); + serviceDR.reconcile(webPage, context); + + // 5. + if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) { + ingressDR.reconcile(webPage, context); + } else { + ingressDR.delete(webPage, context); + } + + // 6. + webPage.setStatus( + createStatus(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName())); + return UpdateControl.patchStatus(webPage); + } + + private void createDependentResources(KubernetesClient client) { + this.configMapDR = new ConfigMapDependentResource(); + this.deploymentDR = new DeploymentDependentResource(); + this.serviceDR = new ServiceDependentResource(); + this.ingressDR = new IngressDependentResource(); + + Arrays.asList(configMapDR, deploymentDR, serviceDR, ingressDR).forEach(dr -> { + dr.setKubernetesClient(client); + dr.configureWith(new KubernetesDependentResourceConfig() + .setLabelSelector(DEPENDENT_RESOURCE_LABEL_SELECTOR)); + }); + } + + // omitted code } ``` From 9242fc694e52d86bf36b214e451a0cc79b03213e Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 24 Aug 2022 22:43:49 +0200 Subject: [PATCH 4/5] docs: add javadoc --- .../processing/dependent/Matcher.java | 62 +++++++++++++++++++ .../GenericKubernetesResourceMatcher.java | 19 ++++++ 2 files changed, 81 insertions(+) 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 84290fe464..750fe89cbf 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 @@ -5,18 +5,68 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; +/** + * Implement this interface to provide custom matching logic when determining whether secondary + * resources match their desired state. This is used by some default implementations of the + * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} interface, notably + * {@link io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource}. + * + * @param the type associated with the secondary resources we want to match + * @param

the type associated with the primary resources with which the related + * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} + * implementation is associated + */ public interface Matcher { + + /** + * Abstracts the matching result letting implementations also return the desired state if it has + * been computed as part of their logic. This allows the SDK to avoid re-computing it if not + * needed. + * + * @param the type associated with the secondary resources we want to match + */ interface Result { + + /** + * Whether or not the actual resource matched the desired state + * + * @return {@code true} if the observed resource matched the desired state, {@code false} + * otherwise + */ boolean matched(); + /** + * Retrieves the associated desired state if it has been computed during the matching process or + * empty if not. + * + * @return an {@link Optional} holding the desired state if it has been computed during the + * matching process or {@link Optional#empty()} if not + */ default Optional computedDesired() { return Optional.empty(); } + /** + * Creates a result stating only whether the resource matched the desired state without having + * computed the desired state. + * + * @param matched whether the actual resource matched the desired state + * @return a {@link Result} with an empty computed desired state + * @param the type of resources being matched + */ static Result nonComputed(boolean matched) { return () -> matched; } + /** + * Creates a result stating whether the resource matched and the associated computed desired + * state so that the SDK can use it downstream without having to recompute it. + * + * @param matched whether the actual resource matched the desired state + * @param computedDesired the associated desired state as computed during the matching process + * @return a {@link Result} with the associated desired state + * @param the type of resources being matched + */ static Result computed(boolean matched, T computedDesired) { return new Result<>() { @Override @@ -32,5 +82,17 @@ public Optional computedDesired() { } } + /** + * Determines whether the specified secondary resource matches the desired state as defined from + * the specified primary resource, given the specified {@link Context}. + * + * @param actualResource the resource we want to determine whether it's matching the desired state + * @param primary the primary resource from which the desired state is inferred + * @param context the context in which the resource is being matched + * @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)}) + */ 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 356598fbbf..e294b1c938 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 @@ -46,6 +46,25 @@ public Result match(R actualResource, P primary, Context

context) { return match(dependentResource, actualResource, primary, context, false); } + /** + * 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 + */ public static Result match( KubernetesDependentResource dependentResource, R actualResource, P primary, Context

context, boolean considerMetadata) { From 07fbe6d2ff4456cdaedd33988fe7899cb19d1048 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 25 Aug 2022 09:54:38 +0200 Subject: [PATCH 5/5] typo --- .../kubernetes/GenericKubernetesResourceMatcherTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f4416c47b7..d39db6ced6 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 @@ -66,7 +66,7 @@ void checksIfDesiredValuesAreTheSame() { assertThat(GenericKubernetesResourceMatcher .match(dependentResource, actual, null, context, true).matched()) - .withFailMessage("Annotations should matter when metadata is not considered") + .withFailMessage("Annotations should matter when metadata is considered") .isFalse(); }