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 @@ -345,4 +348,15 @@ default boolean ssaBasedCreateUpdateMatchForDependentResources() {
return true;
}

/**
* There are certain resources where the intention is not use SSA in Kubernetes. Typical are
* ConfigMaps or Secrets. For these resources, if {@link KubernetesDependent#useSSA()} is not
* explicitly overriden for a dependent resource instance SSA will not be used, event if in
* general SSA is used for Dependent Resources, see
* {@link #ssaBasedCreateUpdateMatchForDependentResources()}.
*/
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,21 @@ 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())
.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 +149,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 +193,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
Expand Up @@ -159,7 +159,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, Map.of());
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;
}

}
Original file line number Diff line number Diff line change
@@ -1,21 +1,43 @@
package io.javaoperatorsdk.operator.sample.dependentssa;

import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.reconciler.*;
import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent;
import io.javaoperatorsdk.operator.junit.KubernetesClientAware;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfigBuilder;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider;

@ControllerConfiguration(dependents = {@Dependent(type = SSAConfigMapDependent.class)})
@ControllerConfiguration
public class DependentSSAReconciler
implements Reconciler<DependnetSSACustomResource>, TestExecutionInfoProvider {
implements Reconciler<DependnetSSACustomResource>, TestExecutionInfoProvider,
KubernetesClientAware,
EventSourceInitializer<DependnetSSACustomResource> {

private final AtomicInteger numberOfExecutions = new AtomicInteger(0);

private SSAConfigMapDependent ssaConfigMapDependent = new SSAConfigMapDependent();
private KubernetesClient kubernetesClient;

public DependentSSAReconciler() {
this(true);
}

public DependentSSAReconciler(boolean useSSA) {
ssaConfigMapDependent.configureWith(new KubernetesDependentResourceConfigBuilder<ConfigMap>()
.withUseSSA(useSSA)
.build());
}

@Override
public UpdateControl<DependnetSSACustomResource> reconcile(
DependnetSSACustomResource resource,
Context<DependnetSSACustomResource> context) {

ssaConfigMapDependent.reconcile(resource, context);
numberOfExecutions.addAndGet(1);
return UpdateControl.noUpdate();
}
Expand All @@ -24,4 +46,21 @@ public int getNumberOfExecutions() {
return numberOfExecutions.get();
}

@Override
public KubernetesClient getKubernetesClient() {
return kubernetesClient;
}

@Override
public void setKubernetesClient(KubernetesClient kubernetesClient) {
this.kubernetesClient = kubernetesClient;
ssaConfigMapDependent.setKubernetesClient(kubernetesClient);
}

@Override
public Map<String, EventSource> prepareEventSources(
EventSourceContext<DependnetSSACustomResource> context) {
return EventSourceInitializer.nameEventSourcesFromDependentResource(context,
ssaConfigMapDependent);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ protected ConfigMap desired(DependnetSSACustomResource primary,
}

@Override
public ConfigMap update(ConfigMap actual, ConfigMap target,
public ConfigMap update(ConfigMap actual, ConfigMap desired,
DependnetSSACustomResource primary,
Context<DependnetSSACustomResource> context) {
NUMBER_OF_UPDATES.incrementAndGet();
return super.update(actual, target, primary, context);
return super.update(actual, desired, primary, context);
}
}
Loading