Skip to content

Commit

Permalink
feat: sanitization of resources for matching (#2042)
Browse files Browse the repository at this point in the history
Signed-off-by: Attila Mészáros <[email protected]>
  • Loading branch information
csviri committed Sep 18, 2023
1 parent 2bbb016 commit a42c8a0
Show file tree
Hide file tree
Showing 18 changed files with 348 additions and 38 deletions.
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()) {
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

0 comments on commit a42c8a0

Please sign in to comment.