From 751d4b0ec269343fd1a325557bb6402d1ceec397 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 9 Apr 2025 19:57:31 +0200 Subject: [PATCH 1/3] feat: allow easier configuration of matcher Signed-off-by: Chris Laprun --- .../KubernetesDependentResource.java | 19 +++++++++++++++--- ...dGenericKubernetesResourceMatcherTest.java | 20 +++++++++++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-) 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 382ac7525c..e7d83b03bb 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 @@ -37,10 +37,18 @@ public abstract class KubernetesDependentResource> { private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); + + @SuppressWarnings("rawtypes") + private static final SSABasedGenericKubernetesResourceMatcher defaultMatcher = + SSABasedGenericKubernetesResourceMatcher.getInstance(); + private final boolean garbageCollected = this instanceof GarbageCollected; private KubernetesDependentResourceConfig kubernetesDependentResourceConfig; private volatile Boolean useSSA; + @SuppressWarnings("unchecked") + private SSABasedGenericKubernetesResourceMatcher matcher = defaultMatcher; + public KubernetesDependentResource(Class resourceType) { this(resourceType, null); } @@ -54,6 +62,13 @@ public void configureWith(KubernetesDependentResourceConfig config) { this.kubernetesDependentResourceConfig = config; } + public void setMatcher(SSABasedGenericKubernetesResourceMatcher matcher) { + if (this.matcher != defaultMatcher) { + throw new IllegalStateException("Can only set a matcher once."); + } + this.matcher = matcher; + } + @SuppressWarnings("unused") public R create(R desired, P primary, Context

context) { if (useSSA(context)) { @@ -111,9 +126,7 @@ public Result match(R actualResource, R desired, P primary, Context

contex final boolean matches; addMetadata(true, actualResource, desired, primary, context); if (useSSA(context)) { - matches = - SSABasedGenericKubernetesResourceMatcher.getInstance() - .matches(actualResource, desired, context); + matches = matcher.matches(actualResource, desired, context); } else { matches = GenericKubernetesResourceMatcher.match(desired, actualResource, false, false, context) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index 176531344c..e5a216a37f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -22,6 +22,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Context; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -39,6 +40,7 @@ void setup() { when(mockedContext.getClient()).thenReturn(client); final var configurationService = mock(ConfigurationService.class); + when(configurationService.shouldUseSSA(any(), any(), any())).thenReturn(true); final var controllerConfiguration = mock(ControllerConfiguration.class); when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); when(controllerConfiguration.fieldManager()).thenReturn("controller"); @@ -239,17 +241,31 @@ void testSanitizeState_daemonSetWithResources_withMismatch() { @ParameterizedTest @ValueSource(booleans = {true, false}) void testCustomMatcher_returnsExpectedMatchBasedOnReadOnlyLabel(boolean readOnly) { + var dr = new ConfigMapDR(); var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", ConfigMap.class); desiredConfigMap.getData().put("key1", "another value"); var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", ConfigMap.class); actualConfigMap.getMetadata().getLabels().put("readonly", Boolean.toString(readOnly)); - var matcher = new ReadOnlyAwareMatcher(); - assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)) + ConfigMap ignoredPrimary = null; + assertThat( + dr.match( + actualConfigMap, + desiredConfigMap, + ignoredPrimary, + (Context) mockedContext) + .matched()) .isEqualTo(readOnly); } + private static class ConfigMapDR extends KubernetesDependentResource { + public ConfigMapDR() { + super(ConfigMap.class); + setMatcher(new ReadOnlyAwareMatcher<>()); + } + } + private static class ReadOnlyAwareMatcher extends SSABasedGenericKubernetesResourceMatcher { @Override From a9868e0c81d9f114c8869167fb1053068d01238a Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 10 Apr 2025 14:59:30 +0200 Subject: [PATCH 2/3] refactor: avoid recording default matcher Signed-off-by: Chris Laprun --- .../KubernetesDependentResource.java | 19 ++++++++++++------- ...dGenericKubernetesResourceMatcherTest.java | 3 +-- 2 files changed, 13 insertions(+), 9 deletions(-) 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 e7d83b03bb..8a74690f1e 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 @@ -38,16 +38,13 @@ public abstract class KubernetesDependentResource kubernetesDependentResourceConfig; private volatile Boolean useSSA; - @SuppressWarnings("unchecked") - private SSABasedGenericKubernetesResourceMatcher matcher = defaultMatcher; + private SSABasedGenericKubernetesResourceMatcher matcher = + SSABasedGenericKubernetesResourceMatcher.getInstance(); + private volatile boolean matcherSet = false; public KubernetesDependentResource(Class resourceType) { this(resourceType, null); @@ -57,16 +54,24 @@ public KubernetesDependentResource(Class resourceType, String name) { super(resourceType, name); } + public KubernetesDependentResource( + Class resourceType, String name, SSABasedGenericKubernetesResourceMatcher matcher) { + this(resourceType, name); + this.matcher = matcher; + matcherSet = true; + } + @Override public void configureWith(KubernetesDependentResourceConfig config) { this.kubernetesDependentResourceConfig = config; } public void setMatcher(SSABasedGenericKubernetesResourceMatcher matcher) { - if (this.matcher != defaultMatcher) { + if (matcherSet) { throw new IllegalStateException("Can only set a matcher once."); } this.matcher = matcher; + matcherSet = true; } @SuppressWarnings("unused") diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index e5a216a37f..64b91225e8 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -261,8 +261,7 @@ void testCustomMatcher_returnsExpectedMatchBasedOnReadOnlyLabel(boolean readOnly private static class ConfigMapDR extends KubernetesDependentResource { public ConfigMapDR() { - super(ConfigMap.class); - setMatcher(new ReadOnlyAwareMatcher<>()); + super(ConfigMap.class, "foo", new ReadOnlyAwareMatcher<>()); } } From 4f9c19d25039340962878d9e0f1448c46fa2fddd Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 29 Apr 2025 16:00:05 +0200 Subject: [PATCH 3/3] refactor: make matcher configurable instead of settable Signed-off-by: Chris Laprun --- .../kubernetes/KubernetesDependent.java | 19 ++++++++++++++ .../KubernetesDependentConverter.java | 14 ++++++++++- .../KubernetesDependentResource.java | 25 ++++--------------- .../KubernetesDependentResourceConfig.java | 10 +++++++- ...ernetesDependentResourceConfigBuilder.java | 9 ++++++- ...dGenericKubernetesResourceMatcherTest.java | 6 ++++- 6 files changed, 59 insertions(+), 24 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java index 4e32246a38..484ffb64c8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java @@ -27,5 +27,24 @@ boolean createResourceOnlyIfNotExistingWithSSA() default */ BooleanWithUndefined useSSA() default BooleanWithUndefined.UNDEFINED; + /** + * The underlying Informer event source configuration + * + * @return the {@link Informer} configuration + */ Informer informer() default @Informer; + + /** + * The specific matcher implementation to use when Server-Side Apply (SSA) is used, when case the + * default one isn't working appropriately. Typically, this could be needed to cover border cases + * with some Kubernetes resources that are modified by their controllers to normalize or add + * default values, which could result in infinite loops with the default matcher. Using a specific + * matcher could also be an optimization decision if determination of whether two resources match + * can be done faster than what can be done with the default exhaustive algorithm. + * + * @return the class of the specific matcher to use for the associated dependent resources + * @since 5.1 + */ + Class matcher() default + SSABasedGenericKubernetesResourceMatcher.class; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java index 6f1d7e3a64..7d68b0e106 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentConverter.java @@ -22,10 +22,22 @@ public KubernetesDependentResourceConfig configFrom( DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA; Boolean useSSA = null; + SSABasedGenericKubernetesResourceMatcher matcher = + SSABasedGenericKubernetesResourceMatcher.getInstance(); if (configAnnotation != null) { createResourceOnlyIfNotExistingWithSSA = configAnnotation.createResourceOnlyIfNotExistingWithSSA(); useSSA = configAnnotation.useSSA().asBoolean(); + + // check if we have a specific matcher + Class> dependentResourceClass = + (Class>) spec.getDependentResourceClass(); + final var context = + Utils.contextFor( + controllerConfig, dependentResourceClass, configAnnotation.annotationType()); + matcher = + Utils.instantiate( + configAnnotation.matcher(), SSABasedGenericKubernetesResourceMatcher.class, context); } var informerConfiguration = @@ -35,7 +47,7 @@ public KubernetesDependentResourceConfig configFrom( controllerConfig); return new KubernetesDependentResourceConfig<>( - useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration); + useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration, matcher); } @SuppressWarnings({"unchecked"}) 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 8a74690f1e..ea7edbc1a0 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 @@ -42,10 +42,6 @@ public abstract class KubernetesDependentResource kubernetesDependentResourceConfig; private volatile Boolean useSSA; - private SSABasedGenericKubernetesResourceMatcher matcher = - SSABasedGenericKubernetesResourceMatcher.getInstance(); - private volatile boolean matcherSet = false; - public KubernetesDependentResource(Class resourceType) { this(resourceType, null); } @@ -54,26 +50,11 @@ public KubernetesDependentResource(Class resourceType, String name) { super(resourceType, name); } - public KubernetesDependentResource( - Class resourceType, String name, SSABasedGenericKubernetesResourceMatcher matcher) { - this(resourceType, name); - this.matcher = matcher; - matcherSet = true; - } - @Override public void configureWith(KubernetesDependentResourceConfig config) { this.kubernetesDependentResourceConfig = config; } - public void setMatcher(SSABasedGenericKubernetesResourceMatcher matcher) { - if (matcherSet) { - throw new IllegalStateException("Can only set a matcher once."); - } - this.matcher = matcher; - matcherSet = true; - } - @SuppressWarnings("unused") public R create(R desired, P primary, Context

context) { if (useSSA(context)) { @@ -131,7 +112,11 @@ public Result match(R actualResource, R desired, P primary, Context

contex final boolean matches; addMetadata(true, actualResource, desired, primary, context); if (useSSA(context)) { - matches = matcher.matches(actualResource, desired, context); + matches = + configuration() + .map(KubernetesDependentResourceConfig::matcher) + .orElse(SSABasedGenericKubernetesResourceMatcher.getInstance()) + .matches(actualResource, desired, context); } else { matches = GenericKubernetesResourceMatcher.match(desired, actualResource, false, false, context) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java index c3424750d2..bcfe2f9fe6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java @@ -10,14 +10,18 @@ public class KubernetesDependentResourceConfig { private final Boolean useSSA; private final boolean createResourceOnlyIfNotExistingWithSSA; private final InformerConfiguration informerConfig; + private final SSABasedGenericKubernetesResourceMatcher matcher; public KubernetesDependentResourceConfig( Boolean useSSA, boolean createResourceOnlyIfNotExistingWithSSA, - InformerConfiguration informerConfig) { + InformerConfiguration informerConfig, + SSABasedGenericKubernetesResourceMatcher matcher) { this.useSSA = useSSA; this.createResourceOnlyIfNotExistingWithSSA = createResourceOnlyIfNotExistingWithSSA; this.informerConfig = informerConfig; + this.matcher = + matcher != null ? matcher : SSABasedGenericKubernetesResourceMatcher.getInstance(); } public boolean createResourceOnlyIfNotExistingWithSSA() { @@ -31,4 +35,8 @@ public Boolean useSSA() { public InformerConfiguration informerConfig() { return informerConfig; } + + public SSABasedGenericKubernetesResourceMatcher matcher() { + return matcher; + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfigBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfigBuilder.java index 7694fe1d46..371fb700c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfigBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfigBuilder.java @@ -8,6 +8,7 @@ public final class KubernetesDependentResourceConfigBuilder informerConfiguration; + private SSABasedGenericKubernetesResourceMatcher matcher; public KubernetesDependentResourceConfigBuilder() {} @@ -29,8 +30,14 @@ public KubernetesDependentResourceConfigBuilder withKubernetesDependentInform return this; } + public KubernetesDependentResourceConfigBuilder withSSAMatcher( + SSABasedGenericKubernetesResourceMatcher matcher) { + this.matcher = matcher; + return this; + } + public KubernetesDependentResourceConfig build() { return new KubernetesDependentResourceConfig<>( - useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration); + useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration, matcher); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index 64b91225e8..e87842c103 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -242,6 +242,10 @@ void testSanitizeState_daemonSetWithResources_withMismatch() { @ValueSource(booleans = {true, false}) void testCustomMatcher_returnsExpectedMatchBasedOnReadOnlyLabel(boolean readOnly) { var dr = new ConfigMapDR(); + dr.configureWith( + new KubernetesDependentResourceConfigBuilder() + .withSSAMatcher(new ReadOnlyAwareMatcher()) + .build()); var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", ConfigMap.class); desiredConfigMap.getData().put("key1", "another value"); @@ -261,7 +265,7 @@ void testCustomMatcher_returnsExpectedMatchBasedOnReadOnlyLabel(boolean readOnly private static class ConfigMapDR extends KubernetesDependentResource { public ConfigMapDR() { - super(ConfigMap.class, "foo", new ReadOnlyAwareMatcher<>()); + super(ConfigMap.class); } }