Skip to content

feat: allow easier configuration of matcher #2760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<? extends SSABasedGenericKubernetesResourceMatcher> matcher() default
SSABasedGenericKubernetesResourceMatcher.class;
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,22 @@ public KubernetesDependentResourceConfig<R> configFrom(
DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA;

Boolean useSSA = null;
SSABasedGenericKubernetesResourceMatcher<R> matcher =
SSABasedGenericKubernetesResourceMatcher.getInstance();
if (configAnnotation != null) {
createResourceOnlyIfNotExistingWithSSA =
configAnnotation.createResourceOnlyIfNotExistingWithSSA();
useSSA = configAnnotation.useSSA().asBoolean();

// check if we have a specific matcher
Class<? extends KubernetesDependentResource<?, ?>> dependentResourceClass =
(Class<? extends KubernetesDependentResource<?, ?>>) spec.getDependentResourceClass();
final var context =
Utils.contextFor(
controllerConfig, dependentResourceClass, configAnnotation.annotationType());
matcher =
Utils.instantiate(
configAnnotation.matcher(), SSABasedGenericKubernetesResourceMatcher.class, context);
}

var informerConfiguration =
Expand All @@ -35,7 +47,7 @@ public KubernetesDependentResourceConfig<R> configFrom(
controllerConfig);

return new KubernetesDependentResourceConfig<>(
useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration);
useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration, matcher);
}

@SuppressWarnings({"unchecked"})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
implements ConfiguredDependentResource<KubernetesDependentResourceConfig<R>> {

private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class);

private final boolean garbageCollected = this instanceof GarbageCollected;
private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
private volatile Boolean useSSA;
Expand Down Expand Up @@ -112,7 +113,9 @@ public Result<R> match(R actualResource, R desired, P primary, Context<P> contex
addMetadata(true, actualResource, desired, primary, context);
if (useSSA(context)) {
matches =
SSABasedGenericKubernetesResourceMatcher.getInstance()
configuration()
.map(KubernetesDependentResourceConfig::matcher)
.orElse(SSABasedGenericKubernetesResourceMatcher.getInstance())
.matches(actualResource, desired, context);
} else {
matches =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,18 @@ public class KubernetesDependentResourceConfig<R extends HasMetadata> {
private final Boolean useSSA;
private final boolean createResourceOnlyIfNotExistingWithSSA;
private final InformerConfiguration<R> informerConfig;
private final SSABasedGenericKubernetesResourceMatcher<R> matcher;

public KubernetesDependentResourceConfig(
Boolean useSSA,
boolean createResourceOnlyIfNotExistingWithSSA,
InformerConfiguration<R> informerConfig) {
InformerConfiguration<R> informerConfig,
SSABasedGenericKubernetesResourceMatcher<R> matcher) {
this.useSSA = useSSA;
this.createResourceOnlyIfNotExistingWithSSA = createResourceOnlyIfNotExistingWithSSA;
this.informerConfig = informerConfig;
this.matcher =
matcher != null ? matcher : SSABasedGenericKubernetesResourceMatcher.getInstance();
}

public boolean createResourceOnlyIfNotExistingWithSSA() {
Expand All @@ -31,4 +35,8 @@ public Boolean useSSA() {
public InformerConfiguration<R> informerConfig() {
return informerConfig;
}

public SSABasedGenericKubernetesResourceMatcher<R> matcher() {
return matcher;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ public final class KubernetesDependentResourceConfigBuilder<R extends HasMetadat
private boolean createResourceOnlyIfNotExistingWithSSA;
private Boolean useSSA = null;
private InformerConfiguration<R> informerConfiguration;
private SSABasedGenericKubernetesResourceMatcher<R> matcher;

public KubernetesDependentResourceConfigBuilder() {}

Expand All @@ -29,8 +30,14 @@ public KubernetesDependentResourceConfigBuilder<R> withKubernetesDependentInform
return this;
}

public KubernetesDependentResourceConfigBuilder<R> withSSAMatcher(
SSABasedGenericKubernetesResourceMatcher<R> matcher) {
this.matcher = matcher;
return this;
}

public KubernetesDependentResourceConfig<R> build() {
return new KubernetesDependentResourceConfig<>(
useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration);
useSSA, createResourceOnlyIfNotExistingWithSSA, informerConfiguration, matcher);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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");
Expand Down Expand Up @@ -239,17 +241,34 @@ void testSanitizeState_daemonSetWithResources_withMismatch() {
@ParameterizedTest
@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");
var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", ConfigMap.class);
actualConfigMap.getMetadata().getLabels().put("readonly", Boolean.toString(readOnly));

var matcher = new ReadOnlyAwareMatcher<ConfigMap>();
assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext))
ConfigMap ignoredPrimary = null;
assertThat(
dr.match(
actualConfigMap,
desiredConfigMap,
ignoredPrimary,
(Context<ConfigMap>) mockedContext)
.matched())
.isEqualTo(readOnly);
}

private static class ConfigMapDR extends KubernetesDependentResource<ConfigMap, ConfigMap> {
public ConfigMapDR() {
super(ConfigMap.class);
}
}

private static class ReadOnlyAwareMatcher<T extends HasMetadata>
extends SSABasedGenericKubernetesResourceMatcher<T> {
@Override
Expand Down