From ff2dec7abacdaa630f58682ff3cf10dfa5d404ae Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 10:34:35 +0100 Subject: [PATCH 1/6] feat: generic matcher for kubernetes dependent resource --- .../operator/ReconcilerUtils.java | 2 +- .../api/config/ConfigurationService.java | 8 +++- .../kubernetes/DesiredValueMatcher.java | 37 +++++++++++++++++++ .../KubernetesDependentResource.java | 33 ++++++++++++----- .../dependent/kubernetes/ResourceMatcher.java | 9 +++++ .../kubernetes/DesiredValueMatcherTest.java | 7 ++++ .../sample/DeploymentDependentResource.java | 7 ---- .../operator/sample/WebPageReconciler.java | 13 +------ 8 files changed, 85 insertions(+), 31 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceMatcher.java create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java index 6f4e19692b..628f2e21fc 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java @@ -111,7 +111,7 @@ public static Object getSpec(HasMetadata resource) { Method getSpecMethod = resource.getClass().getMethod("getSpec"); return getSpecMethod.invoke(resource); } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) { - throw new IllegalStateException(e); + throw new IllegalStateException("No spec found on resource", e); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 2e4679efd3..6e5f44ebea 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -16,9 +16,9 @@ /** An interface from which to retrieve configuration information. */ public interface ConfigurationService { - Cloner DEFAULT_CLONER = new Cloner() { - private final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + Cloner DEFAULT_CLONER = new Cloner() { @Override public HasMetadata clone(HasMetadata object) { try { @@ -122,4 +122,8 @@ default ExecutorService getExecutorService() { default boolean closeClientOnStop() { return true; } + + default ObjectMapper getObjectMapper() { + return OBJECT_MAPPER; + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java new file mode 100644 index 0000000000..159c1ab8d1 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java @@ -0,0 +1,37 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +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.reconciler.Context; + +import com.fasterxml.jackson.databind.ObjectMapper; + +public class DesiredValueMatcher implements ResourceMatcher { + + private final ObjectMapper objectMapper; + + public DesiredValueMatcher(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } + + @Override + public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context) { + if (actualResource instanceof Secret || actualResource instanceof ConfigMap) { + // this comparison should be done ideally without metadata + return actualResource.equals(desiredResource); + } + var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desiredResource)); + var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); + var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); + for (int i = 0; i < diffJsonPatch.size(); i++) { + String operation = diffJsonPatch.get(i).get("op").asText(); + if (!operation.equals("add")) { + return false; + } + } + return true; + } +} 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 a805c12b11..7d61cf251a 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 @@ -8,7 +8,6 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.KubernetesClient; -import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.config.ConfigurationService; import io.javaoperatorsdk.operator.api.config.Utils; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; @@ -32,6 +31,7 @@ public abstract class KubernetesDependentResource informerEventSource; private boolean addOwnerReference; + protected ResourceMatcher resourceMatcher; @Override public void configureWith(KubernetesDependentResourceConfig config) { @@ -40,7 +40,7 @@ public void configureWith(KubernetesDependentResourceConfig config) { } @SuppressWarnings("unchecked") - private void configureWith(ConfigurationService service, String labelSelector, + private void configureWith(ConfigurationService configService, String labelSelector, Set namespaces, boolean addOwnerReference) { final var primaryResourcesRetriever = (this instanceof PrimaryResourcesRetriever) ? (PrimaryResourcesRetriever) this @@ -50,26 +50,28 @@ private void configureWith(ConfigurationService service, String labelSelector, ? (AssociatedSecondaryResourceIdentifier

) this : ResourceID::fromResource; InformerConfiguration ic = - InformerConfiguration.from(service, resourceType()) + InformerConfiguration.from(configService, resourceType()) .withLabelSelector(labelSelector) .withNamespaces(namespaces) .withPrimaryResourcesRetriever(primaryResourcesRetriever) .withAssociatedSecondaryResourceIdentifier(secondaryResourceIdentifier) .build(); - this.addOwnerReference = addOwnerReference; - informerEventSource = new InformerEventSource<>(ic, client); + configureWith(configService, new InformerEventSource<>(ic, client), addOwnerReference); } /** * Use to share informers between event more resources. - * + * + * @param configurationService get configs * @param informerEventSource informer to use * @param addOwnerReference to the created resource */ - public void configureWith(InformerEventSource informerEventSource, + public void configureWith(ConfigurationService configurationService, + InformerEventSource informerEventSource, boolean addOwnerReference) { this.informerEventSource = informerEventSource; this.addOwnerReference = addOwnerReference; + initResourceMatcherIfNotSet(configurationService); } protected void beforeCreateOrUpdate(R desired, P primary) { @@ -79,8 +81,8 @@ protected void beforeCreateOrUpdate(R desired, P primary) { } @Override - protected boolean match(R actual, R target, Context context) { - return ReconcilerUtils.specsEqual(actual, target); + protected boolean match(R actualResource, R desiredResource, Context context) { + return resourceMatcher.match(actualResource, desiredResource, context); } @SuppressWarnings("unchecked") @@ -107,6 +109,7 @@ protected R update(R actual, R target, P primary, Context context) { @Override public Optional eventSource(EventSourceContext

context) { + initResourceMatcherIfNotSet(context.getConfigurationService()); if (informerEventSource == null) { configureWith(context.getConfigurationService(), null, null, KubernetesDependent.ADD_OWNER_REFERENCE_DEFAULT); @@ -144,4 +147,16 @@ public Optional getResource(P primaryResource) { public void setKubernetesClient(KubernetesClient kubernetesClient) { this.client = kubernetesClient; } + + /** + * Override this method to configure resource matcher + * + * @param configurationService config service to mainly access object mapper + */ + protected void initResourceMatcherIfNotSet(ConfigurationService configurationService) { + if (resourceMatcher == null) { + resourceMatcher = new DesiredValueMatcher(configurationService.getObjectMapper()); + } + } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceMatcher.java new file mode 100644 index 0000000000..0d86709ca0 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceMatcher.java @@ -0,0 +1,9 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Context; + +public interface ResourceMatcher { + + boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context); +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java new file mode 100644 index 0000000000..42f85caf76 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java @@ -0,0 +1,7 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +import static org.junit.jupiter.api.Assertions.*; + +class DesiredValueMatcherTest { + +} diff --git a/sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/DeploymentDependentResource.java b/sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/DeploymentDependentResource.java index a0c22ce9b1..91da4523ed 100644 --- a/sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/DeploymentDependentResource.java +++ b/sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/DeploymentDependentResource.java @@ -43,11 +43,4 @@ protected Deployment desired(Tomcat tomcat, Context context) { .build(); return deployment; } - - @Override - public boolean match(Deployment fetched, Deployment target, Context context) { - return fetched.getSpec().getTemplate().getSpec().getContainers().get(0).getImage() - .equals(target.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()); - } - } diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java index cc9bce6c6d..c8a39a6274 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java @@ -62,7 +62,7 @@ public UpdateControl reconcile(WebPage webPage, Context context) { WebPageStatus status = new WebPageStatus(); waitUntilConfigMapAvailable(webPage); - status.setHtmlConfigMap(configMapDR.getResource(webPage).get().getMetadata().getName()); + status.setHtmlConfigMap(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName()); status.setAreWeGood("Yes!"); status.setErrorMessage(null); webPage.setStatus(status); @@ -114,12 +114,6 @@ protected Deployment desired(WebPage webPage, Context context) { return deployment; } - @Override - protected boolean match(Deployment actual, Deployment target, Context context) { - // todo comparator - return true; - } - @Override protected Class resourceType() { return Deployment.class; @@ -140,11 +134,6 @@ protected Service desired(WebPage webPage, Context context) { return service; } - protected boolean match(Service actual, Service target, Context context) { - // todo comparator - return true; - } - @Override protected Class resourceType() { return Service.class; From cfc2eda1a2fcc56fbd642c6d2c0ccc80f38e0434 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 10:38:06 +0100 Subject: [PATCH 2/6] docs: comment about reflection --- .../processing/dependent/kubernetes/DesiredValueMatcher.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java index 159c1ab8d1..9b6042b70a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java @@ -23,6 +23,8 @@ public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Co // this comparison should be done ideally without metadata return actualResource.equals(desiredResource); } + // reflection will be replaced by this: + // https://github.com/fabric8io/kubernetes-client/issues/3816 var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desiredResource)); var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource)); var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode); From 02c242601f18cbaa081a6c019fee5ccc4248c299 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 11:27:23 +0100 Subject: [PATCH 3/6] fix: test --- .../operator/ReconcilerUtils.java | 11 +++++ .../kubernetes/DesiredValueMatcherTest.java | 44 +++++++++++++++++++ .../kubernetes/nginx-deployment.yaml | 21 +++++++++ .../StandaloneDependentTestReconciler.java | 10 ----- 4 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java index 628f2e21fc..491910d13e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java @@ -1,11 +1,14 @@ package io.javaoperatorsdk.operator; +import java.io.IOException; +import java.io.InputStream; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Locale; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.ObjectMeta; +import io.fabric8.kubernetes.client.utils.Serialization; import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -115,4 +118,12 @@ public static Object getSpec(HasMetadata resource) { } } + public static T loadYaml(Class clazz, Class loader, String yaml) { + try (InputStream is = loader.getResourceAsStream(yaml)) { + return Serialization.unmarshal(is, clazz); + } catch (IOException ex) { + throw new IllegalStateException("Cannot find yaml on classpath: " + yaml); + } + } + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java index 42f85caf76..0f9015c06e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java @@ -1,7 +1,51 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.javaoperatorsdk.operator.ReconcilerUtils; + +import com.fasterxml.jackson.databind.ObjectMapper; + +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; class DesiredValueMatcherTest { + DesiredValueMatcher desiredValueMatcher = new DesiredValueMatcher(new ObjectMapper()); + + @Test + void checksIfDesiredValuesAreTheSame() { + var target1 = createTestDeploymentObject(); + var desired1 = createTestDeploymentObject(); + assertThat(desiredValueMatcher.match(target1, desired1, null)).isTrue(); + + var target2 = createTestDeploymentObject(); + var desired2 = createTestDeploymentObject(); + target2.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); + assertThat(desiredValueMatcher.match(target2, desired2, null)).isTrue() + .withFailMessage("Additive changes should be ok"); + + var target3 = createTestDeploymentObject(); + var desired3 = createTestDeploymentObject(); + desired3.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); + assertThat(desiredValueMatcher.match(target3, desired3, null)).isFalse() + .withFailMessage("Removed value should not be ok"); + + var target4 = createTestDeploymentObject(); + var desired4 = createTestDeploymentObject(); + target4.getSpec().setReplicas(2); + assertThat(desiredValueMatcher.match(target4, desired4, null)).isFalse() + .withFailMessage("Changed values are not ok"); + + } + + Deployment createTestDeploymentObject() { + Deployment deployment = + ReconcilerUtils.loadYaml(Deployment.class, DesiredValueMatcherTest.class, + "nginx-deployment.yaml"); + deployment.getMetadata().setName("test"); + return deployment; + } + } diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml new file mode 100644 index 0000000000..cea0f3c9d6 --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml @@ -0,0 +1,21 @@ +apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2 +kind: Deployment +metadata: + name: "" +spec: + progressDeadlineSeconds: 600 + revisionHistoryLimit: 10 + selector: + matchLabels: + app: "test-dependent" + replicas: 1 + template: + metadata: + labels: + app: "test-dependent" + spec: + containers: + - name: nginx + image: nginx:1.17.0 + ports: + - containerPort: 80 diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java index b168d8fb47..f0ac52458d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java @@ -3,7 +3,6 @@ import java.io.IOException; import java.io.InputStream; import java.util.List; -import java.util.Objects; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.fabric8.kubernetes.client.KubernetesClient; @@ -72,14 +71,5 @@ protected Deployment desired(StandaloneDependentTestCustomResource primary, Cont deployment.getMetadata().setNamespace(primary.getMetadata().getNamespace()); return deployment; } - - @Override - protected boolean match(Deployment actual, Deployment target, Context context) { - return Objects.equals(actual.getSpec().getReplicas(), target.getSpec().getReplicas()) && - actual.getSpec().getTemplate().getSpec().getContainers().get(0).getImage() - .equals( - target.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()); - } - } } From dc2c38043491015a62d20d64c91a717ead0202c6 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 13:06:44 +0100 Subject: [PATCH 4/6] fix: added unit tests --- .../kubernetes/DesiredValueMatcherTest.java | 70 +++++++++++++------ .../dependent/kubernetes/configmap.yaml | 8 +++ .../kubernetes/nginx-deployment.yaml | 2 +- .../dependent/kubernetes/secret.yaml | 12 ++++ 4 files changed, 71 insertions(+), 21 deletions(-) create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java index 0f9015c06e..d1b11b7884 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java @@ -2,13 +2,14 @@ import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.javaoperatorsdk.operator.ReconcilerUtils; import com.fasterxml.jackson.databind.ObjectMapper; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.*; class DesiredValueMatcherTest { @@ -16,36 +17,65 @@ class DesiredValueMatcherTest { @Test void checksIfDesiredValuesAreTheSame() { - var target1 = createTestDeploymentObject(); - var desired1 = createTestDeploymentObject(); + var target1 = createDeployment(); + var desired1 = createDeployment(); assertThat(desiredValueMatcher.match(target1, desired1, null)).isTrue(); - var target2 = createTestDeploymentObject(); - var desired2 = createTestDeploymentObject(); + var target2 = createDeployment(); + var desired2 = createDeployment(); target2.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - assertThat(desiredValueMatcher.match(target2, desired2, null)).isTrue() - .withFailMessage("Additive changes should be ok"); + assertThat(desiredValueMatcher.match(target2, desired2, null)) + .withFailMessage("Additive changes should be ok") + .isTrue(); - var target3 = createTestDeploymentObject(); - var desired3 = createTestDeploymentObject(); + var target3 = createDeployment(); + var desired3 = createDeployment(); desired3.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val"); - assertThat(desiredValueMatcher.match(target3, desired3, null)).isFalse() - .withFailMessage("Removed value should not be ok"); + assertThat(desiredValueMatcher.match(target3, desired3, null)) + .withFailMessage("Removed value should not be ok") + .isFalse(); - var target4 = createTestDeploymentObject(); - var desired4 = createTestDeploymentObject(); + var target4 = createDeployment(); + var desired4 = createDeployment(); target4.getSpec().setReplicas(2); - assertThat(desiredValueMatcher.match(target4, desired4, null)).isFalse() - .withFailMessage("Changed values are not ok"); + assertThat(desiredValueMatcher.match(target4, desired4, null)) + .withFailMessage("Changed values are not ok") + .isFalse(); + } + + @Test + void secretsComparedByEquals() { + var sec1 = createSecret(); + var sec2 = createSecret(); + assertThat(desiredValueMatcher.match(sec1, sec2, null)).isTrue(); + + sec2.getData().put("additional_key", "value"); + assertThat(desiredValueMatcher.match(sec1, sec2, null)).isFalse(); + } + + @Test + void configMapsComparedByEquals() { + var cm1 = createConfigMap(); + var cm2 = createConfigMap(); + assertThat(desiredValueMatcher.match(cm1, cm2, null)).isTrue(); + + cm2.getData().put("additional_key", "value"); + assertThat(desiredValueMatcher.match(cm1, cm2, null)).isFalse(); + } + ConfigMap createConfigMap() { + return ReconcilerUtils.loadYaml(ConfigMap.class, DesiredValueMatcherTest.class, + "configmap.yaml"); } - Deployment createTestDeploymentObject() { + Secret createSecret() { + return ReconcilerUtils.loadYaml(Secret.class, DesiredValueMatcherTest.class, "secret.yaml"); + } + + Deployment createDeployment() { Deployment deployment = - ReconcilerUtils.loadYaml(Deployment.class, DesiredValueMatcherTest.class, - "nginx-deployment.yaml"); - deployment.getMetadata().setName("test"); + ReconcilerUtils.loadYaml( + Deployment.class, DesiredValueMatcherTest.class, "nginx-deployment.yaml"); return deployment; } - } diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml new file mode 100644 index 0000000000..80f31ad966 --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +data: + key: value +kind: ConfigMap +metadata: + name: test + namespace: default + diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml index cea0f3c9d6..dcf90a8fc7 100644 --- a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/nginx-deployment.yaml @@ -1,7 +1,7 @@ apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2 kind: Deployment metadata: - name: "" + name: "test" spec: progressDeadlineSeconds: 600 revisionHistoryLimit: 10 diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml new file mode 100644 index 0000000000..8708407409 --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: Secret +metadata: + annotations: + kubernetes.io/service-account.name: default + kubernetes.io/service-account.uid: e3387bc3-6e72-4cf3-a74f-1acba56e224c + name: default-token-996km + namespace: default +type: kubernetes.io/service-account-token +data: + data: ZGVmYXVsdA== + namespace: ZGVmYXVsdA== From 832806ff2ecaf9e93ebc5e11cbc87fead3ae91a1 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 13:32:29 +0100 Subject: [PATCH 5/6] fix: comparators for secret and config map --- .../kubernetes/DesiredValueMatcher.java | 10 +++++-- .../kubernetes/ResourceComparators.java | 21 ++++++++++++++ .../kubernetes/DesiredValueMatcherTest.java | 29 ------------------- .../dependent/kubernetes/configmap.yaml | 8 ----- .../dependent/kubernetes/secret.yaml | 12 -------- 5 files changed, 28 insertions(+), 52 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java delete mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml delete mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java index 9b6042b70a..dcf1d10d84 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java @@ -9,6 +9,8 @@ import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.Objects; + public class DesiredValueMatcher implements ResourceMatcher { private final ObjectMapper objectMapper; @@ -19,9 +21,11 @@ public DesiredValueMatcher(ObjectMapper objectMapper) { @Override public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context) { - if (actualResource instanceof Secret || actualResource instanceof ConfigMap) { - // this comparison should be done ideally without metadata - return actualResource.equals(desiredResource); + if (actualResource instanceof Secret) { + return ResourceComparators.compareSecretData((Secret) desiredResource, (Secret) actualResource); + } + if (actualResource instanceof ConfigMap) { + return ResourceComparators.compareConfigMapData((ConfigMap) desiredResource, (ConfigMap) actualResource); } // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java new file mode 100644 index 0000000000..126cbd9441 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java @@ -0,0 +1,21 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.Secret; +import io.fabric8.kubernetes.api.model.Service; + +import java.util.Objects; + +public class ResourceComparators { + + public static boolean compareConfigMapData(ConfigMap c1,ConfigMap c2) { + return Objects.equals(c1.getData(),c2.getData()) && + Objects.equals(c1.getBinaryData(),c2.getBinaryData()); + } + + public static boolean compareSecretData(Secret s1, Secret s2) { + return Objects.equals(s1.getType(),s2.getType()) && + Objects.equals(s1.getData(),s2.getData()) && + Objects.equals(s1.getStringData(),s2.getStringData()); + } +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java index d1b11b7884..d69cbf39c9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java @@ -43,35 +43,6 @@ void checksIfDesiredValuesAreTheSame() { .isFalse(); } - @Test - void secretsComparedByEquals() { - var sec1 = createSecret(); - var sec2 = createSecret(); - assertThat(desiredValueMatcher.match(sec1, sec2, null)).isTrue(); - - sec2.getData().put("additional_key", "value"); - assertThat(desiredValueMatcher.match(sec1, sec2, null)).isFalse(); - } - - @Test - void configMapsComparedByEquals() { - var cm1 = createConfigMap(); - var cm2 = createConfigMap(); - assertThat(desiredValueMatcher.match(cm1, cm2, null)).isTrue(); - - cm2.getData().put("additional_key", "value"); - assertThat(desiredValueMatcher.match(cm1, cm2, null)).isFalse(); - } - - ConfigMap createConfigMap() { - return ReconcilerUtils.loadYaml(ConfigMap.class, DesiredValueMatcherTest.class, - "configmap.yaml"); - } - - Secret createSecret() { - return ReconcilerUtils.loadYaml(Secret.class, DesiredValueMatcherTest.class, "secret.yaml"); - } - Deployment createDeployment() { Deployment deployment = ReconcilerUtils.loadYaml( diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml deleted file mode 100644 index 80f31ad966..0000000000 --- a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.yaml +++ /dev/null @@ -1,8 +0,0 @@ -apiVersion: v1 -data: - key: value -kind: ConfigMap -metadata: - name: test - namespace: default - diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml deleted file mode 100644 index 8708407409..0000000000 --- a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/secret.yaml +++ /dev/null @@ -1,12 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - annotations: - kubernetes.io/service-account.name: default - kubernetes.io/service-account.uid: e3387bc3-6e72-4cf3-a74f-1acba56e224c - name: default-token-996km - namespace: default -type: kubernetes.io/service-account-token -data: - data: ZGVmYXVsdA== - namespace: ZGVmYXVsdA== From 7ebcb7315956f233aac312b2bb3a2357a8935e7e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 16 Feb 2022 13:41:16 +0100 Subject: [PATCH 6/6] fix: format --- .../kubernetes/DesiredValueMatcher.java | 8 +++---- .../kubernetes/ResourceComparators.java | 23 +++++++++---------- .../kubernetes/DesiredValueMatcherTest.java | 2 -- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java index dcf1d10d84..b8c9b1c7a5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcher.java @@ -9,8 +9,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; -import java.util.Objects; - public class DesiredValueMatcher implements ResourceMatcher { private final ObjectMapper objectMapper; @@ -22,10 +20,12 @@ public DesiredValueMatcher(ObjectMapper objectMapper) { @Override public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context) { if (actualResource instanceof Secret) { - return ResourceComparators.compareSecretData((Secret) desiredResource, (Secret) actualResource); + return ResourceComparators.compareSecretData((Secret) desiredResource, + (Secret) actualResource); } if (actualResource instanceof ConfigMap) { - return ResourceComparators.compareConfigMapData((ConfigMap) desiredResource, (ConfigMap) actualResource); + return ResourceComparators.compareConfigMapData((ConfigMap) desiredResource, + (ConfigMap) actualResource); } // reflection will be replaced by this: // https://github.com/fabric8io/kubernetes-client/issues/3816 diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java index 126cbd9441..037cb597f4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/ResourceComparators.java @@ -1,21 +1,20 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.Objects; + import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.Secret; -import io.fabric8.kubernetes.api.model.Service; - -import java.util.Objects; public class ResourceComparators { - public static boolean compareConfigMapData(ConfigMap c1,ConfigMap c2) { - return Objects.equals(c1.getData(),c2.getData()) && - Objects.equals(c1.getBinaryData(),c2.getBinaryData()); - } + public static boolean compareConfigMapData(ConfigMap c1, ConfigMap c2) { + return Objects.equals(c1.getData(), c2.getData()) && + Objects.equals(c1.getBinaryData(), c2.getBinaryData()); + } - public static boolean compareSecretData(Secret s1, Secret s2) { - return Objects.equals(s1.getType(),s2.getType()) && - Objects.equals(s1.getData(),s2.getData()) && - Objects.equals(s1.getStringData(),s2.getStringData()); - } + public static boolean compareSecretData(Secret s1, Secret s2) { + return Objects.equals(s1.getType(), s2.getType()) && + Objects.equals(s1.getData(), s2.getData()) && + Objects.equals(s1.getStringData(), s2.getStringData()); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java index d69cbf39c9..d3e013d54d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/DesiredValueMatcherTest.java @@ -2,8 +2,6 @@ import org.junit.jupiter.api.Test; -import io.fabric8.kubernetes.api.model.ConfigMap; -import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.javaoperatorsdk.operator.ReconcilerUtils;