Skip to content
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

feat: sanitization of resources for matching #2042

Merged
merged 15 commits into from
Sep 12, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.ConfigBuilder;
import io.fabric8.kubernetes.client.CustomResource;
Expand All @@ -19,6 +21,7 @@
import io.javaoperatorsdk.operator.api.monitoring.Metrics;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
import io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowFactory;

import static io.javaoperatorsdk.operator.api.config.ExecutorServiceManager.newThreadPoolExecutor;
Expand Down Expand Up @@ -335,7 +338,7 @@ default ExecutorServiceManager getExecutorServiceManager() {
* resources are created/updated and match was change to use
* <a href="https://kubernetes.io/docs/reference/using-api/server-side-apply/">Server-Side
* Apply</a> (SSA) by default.
*
* <p>
* SSA based create/update can be still used with the legacy matching, just overriding the match
* method of Kubernetes Dependent Resource.
*
Expand All @@ -345,4 +348,20 @@ default boolean ssaBasedCreateUpdateMatchForDependentResources() {
return true;
}

/**
* Returns the set of default resources for which Server-Side Apply (SSA) will not be used, even
* if it is the default behavior for dependent resources as specified by
* {@link #ssaBasedCreateUpdateMatchForDependentResources()}. The exception to this is in the case
* where the use of SSA is explicitly enabled on the dependent resource directly using
* {@link KubernetesDependent#useSSA()}.
* <p>
* By default, SSA is disabled for {@link ConfigMap} and {@link Secret} resources.
*
* @return The set of resource types for which SSA will not be used
* @since 4.4.0
*/
default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
return Set.of(ConfigMap.class, Secret.class);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.monitoring.Metrics;

Expand All @@ -35,6 +36,7 @@ public class ConfigurationServiceOverrider {
private Duration cacheSyncTimeout;
private ResourceClassResolver resourceClassResolver;
private Boolean ssaBasedCreateUpdateMatchForDependentResources;
private Set<Class<? extends HasMetadata>> defaultNonSSAResource;

ConfigurationServiceOverrider(ConfigurationService original) {
this.original = original;
Expand Down Expand Up @@ -150,6 +152,12 @@ public ConfigurationServiceOverrider withSSABasedCreateUpdateMatchForDependentRe
return this;
}

public ConfigurationServiceOverrider withDefaultNonSSAResource(
Set<Class<? extends HasMetadata>> defaultNonSSAResource) {
this.defaultNonSSAResource = defaultNonSSAResource;
return this;
}

public ConfigurationService build() {
return new BaseConfigurationService(original.getVersion(), cloner, client) {
@Override
Expand Down Expand Up @@ -256,6 +264,12 @@ public boolean ssaBasedCreateUpdateMatchForDependentResources() {
? ssaBasedCreateUpdateMatchForDependentResources
: super.ssaBasedCreateUpdateMatchForDependentResources();
}

@Override
public Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
return defaultNonSSAResource != null ? defaultNonSSAResource
: super.defaultNonSSAResource();
}
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.PersistentVolumeClaimStatus;
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
import io.javaoperatorsdk.operator.api.reconciler.Context;

public class DesiredResourceSanitizer {

private DesiredResourceSanitizer() {}

public static <R, P extends HasMetadata> void sanitizeDesired(R desired, R actual, P primary,
Context<P> context, boolean useSSA) {
if (useSSA) {
if (desired instanceof StatefulSet) {
fillDefaultsOnVolumeClaimTemplate((StatefulSet) desired);
}
if (desired instanceof Secret) {
checkIfStringDataUsed((Secret) desired);
}
}
}

private static void checkIfStringDataUsed(Secret secret) {
if (secret.getStringData() != null && !secret.getStringData().isEmpty()) {
throw new IllegalStateException(
"There is a known issue using StringData with SSA. Use data instead.");
}
}

private static void fillDefaultsOnVolumeClaimTemplate(StatefulSet statefulSet) {
if (!statefulSet.getSpec().getVolumeClaimTemplates().isEmpty()) {
metacosm marked this conversation as resolved.
Show resolved Hide resolved
statefulSet.getSpec().getVolumeClaimTemplates().forEach(t -> {
if (t.getSpec().getVolumeMode() == null) {
t.getSpec().setVolumeMode("Filesystem");
}
if (t.getStatus() == null) {
t.setStatus(new PersistentVolumeClaimStatus());
}
if (t.getStatus().getPhase() == null) {
t.getStatus().setPhase("pending");
}
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,19 @@ public void configureWith(InformerEventSource<R, P> informerEventSource) {
}

@SuppressWarnings("unused")
public R create(R target, P primary, Context<P> context) {
public R create(R desired, P primary, Context<P> context) {
if (useSSA(context)) {
// setting resource version for SSA so only created if it doesn't exist already
var createIfNotExisting = kubernetesDependentResourceConfig == null
? KubernetesDependentResourceConfig.DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA
: kubernetesDependentResourceConfig.createResourceOnlyIfNotExistingWithSSA();
if (createIfNotExisting) {
target.getMetadata().setResourceVersion("1");
desired.getMetadata().setResourceVersion("1");
}
}
addMetadata(false, null, target, primary);
final var resource = prepare(target, primary, "Creating");
addMetadata(false, null, desired, primary);
sanitizeDesired(desired, null, primary, context);
final var resource = prepare(desired, primary, "Creating");
return useSSA(context)
? resource
.fieldManager(context.getControllerConfiguration().fieldManager())
Expand All @@ -123,19 +124,20 @@ public R create(R target, P primary, Context<P> context) {
: resource.create();
}

public R update(R actual, R target, P primary, Context<P> context) {
public R update(R actual, R desired, P primary, Context<P> context) {
if (log.isDebugEnabled()) {
log.debug("Updating actual resource: {} version: {}", ResourceID.fromResource(actual),
actual.getMetadata().getResourceVersion());
}
R updatedResource;
addMetadata(false, actual, target, primary);
addMetadata(false, actual, desired, primary);
sanitizeDesired(desired, actual, primary, context);
if (useSSA(context)) {
updatedResource = prepare(target, primary, "Updating")
updatedResource = prepare(desired, primary, "Updating")
.fieldManager(context.getControllerConfiguration().fieldManager())
.forceConflicts().serverSideApply();
} else {
var updatedActual = updaterMatcher.updateResource(actual, target, context);
var updatedActual = updaterMatcher.updateResource(actual, desired, context);
updatedResource = prepare(updatedActual, primary, "Updating").update();
}
log.debug("Resource version after update: {}",
Expand All @@ -146,6 +148,7 @@ public R update(R actual, R target, P primary, Context<P> context) {
@Override
public Result<R> match(R actualResource, P primary, Context<P> context) {
final var desired = desired(primary, context);
sanitizeDesired(desired, actualResource, primary, context);
return match(actualResource, desired, primary, updaterMatcher, context);
}

Expand Down Expand Up @@ -189,9 +192,18 @@ protected void addMetadata(boolean forMatch, R actualResource, final R target, P
addReferenceHandlingMetadata(target, primary);
}

private boolean useSSA(Context<P> context) {
protected void sanitizeDesired(R desired, R actual, P primary, Context<P> context) {
DesiredResourceSanitizer.sanitizeDesired(desired, actual, primary, context, useSSA(context));
}

protected boolean useSSA(Context<P> context) {
Optional<Boolean> useSSAConfig =
configuration().flatMap(KubernetesDependentResourceConfig::useSSA);
var configService = context.getControllerConfiguration().getConfigurationService();
// don't use SSA for certain resources by default, only if explicitly overriden
if (useSSAConfig.isEmpty() && configService.defaultNonSSAResource().contains(resourceType())) {
return false;
}
return useSSAConfig.orElse(context.getControllerConfiguration().getConfigurationService()
.ssaBasedCreateUpdateMatchForDependentResources());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Set;

import io.javaoperatorsdk.operator.api.reconciler.Constants;
import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
import io.javaoperatorsdk.operator.processing.event.source.filter.GenericFilter;
import io.javaoperatorsdk.operator.processing.event.source.filter.OnAddFilter;
Expand All @@ -10,7 +11,7 @@

public final class KubernetesDependentResourceConfigBuilder<R> {

private Set<String> namespaces;
private Set<String> namespaces = Constants.SAME_AS_CONTROLLER_NAMESPACES_SET;
private String labelSelector;
private boolean createResourceOnlyIfNotExistingWithSSA;
private ResourceDiscriminator<R, ?> resourceDiscriminator;
Expand Down Expand Up @@ -77,7 +78,8 @@ public KubernetesDependentResourceConfigBuilder<R> withGenericFilter(
}

public KubernetesDependentResourceConfig<R> build() {
return new KubernetesDependentResourceConfig<>(namespaces, labelSelector, false,
return new KubernetesDependentResourceConfig<>(namespaces, labelSelector,
namespaces != Constants.SAME_AS_CONTROLLER_NAMESPACES_SET,
createResourceOnlyIfNotExistingWithSSA, resourceDiscriminator, useSSA, onAddFilter,
onUpdateFilter, onDeleteFilter, genericFilter);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.stream.Collectors;

import org.slf4j.Logger;
Expand Down Expand Up @@ -159,7 +152,7 @@ private static void fillResultsAndTraverseFurther(Map<String, Object> result,
Object managedFieldValue) {
var emptyMapValue = new HashMap<String, Object>();
result.put(keyInActual, emptyMapValue);
var actualMapValue = actualMap.get(keyInActual);
var actualMapValue = actualMap.getOrDefault(keyInActual, Collections.emptyMap());
log.debug("key: {} actual map value: {} managedFieldValue: {}", keyInActual,
actualMapValue, managedFieldValue);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,10 @@ private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOp
private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling,
String fieldManager) {
Operator operator = new Operator(client,
o -> o.withSSABasedCreateUpdateMatchForDependentResources(!legacyDependentHandling)
.withCloseClientOnStop(false));
operator.register(new DependentSSAReconciler(), o -> {
o -> o.withCloseClientOnStop(false));
var reconciler = new DependentSSAReconciler(!legacyDependentHandling);
reconciler.setKubernetesClient(client);
operator.register(reconciler, o -> {
o.settingNamespace(namespace);
if (fieldManager != null) {
o.withFieldManager(fieldManager);
Expand All @@ -155,7 +156,6 @@ private Operator createOperator(KubernetesClient client, boolean legacyDependent
return operator;
}


public DependnetSSACustomResource testResource() {
DependnetSSACustomResource resource = new DependnetSSACustomResource();
resource.setMetadata(new ObjectMetaBuilder()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package io.javaoperatorsdk.operator;

import java.time.Duration;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerCustomResource;
import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerDependentResource;
import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerReconciler;
import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerSpec;

import static org.assertj.core.api.Assertions.assertThat;
import static org.awaitility.Awaitility.await;

public class StatefulSetDesiredSanitizerIT {

public static final String TEST_1 = "test1";

@RegisterExtension
LocallyRunOperatorExtension extension =
LocallyRunOperatorExtension.builder()
.withReconciler(new StatefulSetDesiredSanitizerReconciler())
.build();

@Test
void testSSAMatcher() {
var resource = extension.create(testResource());

await().pollDelay(Duration.ofMillis(200)).untilAsserted(() -> {
var statefulSet = extension.get(StatefulSet.class, TEST_1);
assertThat(statefulSet).isNotNull();
});
// make sure reconciliation happens at least once more
resource.getSpec().setValue("changed value");
extension.replace(resource);

await().untilAsserted(
() -> assertThat(StatefulSetDesiredSanitizerDependentResource.nonMatchedAtLeastOnce)
.isFalse());
}

StatefulSetDesiredSanitizerCustomResource testResource() {
var res = new StatefulSetDesiredSanitizerCustomResource();
res.setMetadata(new ObjectMetaBuilder()
.withName(TEST_1)
.build());
res.setSpec(new StatefulSetDesiredSanitizerSpec());
res.getSpec().setValue("initial value");

return res;
}

}
Loading