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.

Sign up for GitHub

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

fix: use SSA matcher to filter events #2749

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: use SSA matcher to filter events #2749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 22 additions & 6 deletions ...dk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.apps.ReplicaSet;
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.api.reconciler.Context;
Expand Down Expand Up @@ -79,10 +80,14 @@ public static <L extends HasMetadata> SSABasedGenericKubernetesResourceMatcher<L
private static final Logger log =
LoggerFactory.getLogger(SSABasedGenericKubernetesResourceMatcher.class);

@SuppressWarnings("unchecked")
public boolean matches(R actual, R desired, Context<?> context) {
var optionalManagedFieldsEntry =
checkIfFieldManagerExists(actual, context.getControllerConfiguration().fieldManager());
return matches(
actual, desired, context.getControllerConfiguration().fieldManager(), context.getClient(), false);
}

@SuppressWarnings("unchecked")
public boolean matches(R actual, R desired, String fieldManager, KubernetesClient client, boolean compareActual) {
var optionalManagedFieldsEntry = checkIfFieldManagerExists(actual, fieldManager);
// If no field is managed by our controller, that means the controller hasn't touched the
// resource yet and the resource probably doesn't match the desired state. Not matching here
// means that the resource will need to be updated and since this will be done using SSA, the
Expand All @@ -93,7 +98,7 @@ public boolean matches(R actual, R desired, Context<?> context) {

var managedFieldsEntry = optionalManagedFieldsEntry.orElseThrow();

var objectMapper = context.getClient().getKubernetesSerialization();
var objectMapper = client.getKubernetesSerialization();
var actualMap = objectMapper.convertValue(actual, Map.class);
var desiredMap = objectMapper.convertValue(desired, Map.class);
if (LoggingUtils.isNotSensitiveResource(desired)) {
Expand All @@ -108,7 +113,18 @@ public boolean matches(R actual, R desired, Context<?> context) {
managedFieldsEntry.getFieldsV1().getAdditionalProperties(),
objectMapper);

removeIrrelevantValues(desiredMap);
if (compareActual) {
var prunedDesired = new HashMap<String, Object>(actualMap.size());
keepOnlyManagedFields(
prunedDesired,
desiredMap,
managedFieldsEntry.getFieldsV1().getAdditionalProperties(),
objectMapper);

desiredMap = prunedDesired;
} else {
removeIrrelevantValues(desiredMap);
}

var matches = prunedActual.equals(desiredMap);
if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
Expand Down Expand Up @@ -438,7 +454,7 @@ private static Map.Entry<Integer, Map<String, Object>> selectListEntryBasedOnKey
return new AbstractMap.SimpleEntry<>(lastIndex, possibleTargets.get(0));
}

private Optional<ManagedFieldsEntry> checkIfFieldManagerExists(R actual, String fieldManager) {
public Optional<ManagedFieldsEntry> checkIfFieldManagerExists(R actual, String fieldManager) {
var targetManagedFields =
actual.getMetadata().getManagedFields().stream()
// Only the apply operations are interesting for us since those were created properly be
Expand Down
33 changes: 26 additions & 7 deletions ...ava/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.fabric8.kubernetes.client.informers.ResourceEventHandler;
import io.javaoperatorsdk.operator.api.config.informer.InformerEventSourceConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.SSABasedGenericKubernetesResourceMatcher;
import io.javaoperatorsdk.operator.processing.event.Event;
import io.javaoperatorsdk.operator.processing.event.EventHandler;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
Expand Down Expand Up @@ -68,6 +69,8 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata>
private final PrimaryToSecondaryIndex<R> primaryToSecondaryIndex;
private final PrimaryToSecondaryMapper<P> primaryToSecondaryMapper;
private final String id = UUID.randomUUID().toString();
private final KubernetesClient client;
private final String fieldManager;

public InformerEventSource(
InformerEventSourceConfiguration<R> configuration, EventSourceContext<P> context) {
Expand All @@ -77,18 +80,20 @@ public InformerEventSource(
context
.getControllerConfiguration()
.getConfigurationService()
.parseResourceVersionsForEventFilteringAndCaching());
.parseResourceVersionsForEventFilteringAndCaching(),
context.getControllerConfiguration().fieldManager());
}

InformerEventSource(InformerEventSourceConfiguration<R> configuration, KubernetesClient client) {
this(configuration, client, false);
this(configuration, client, false, "manager");
}

@SuppressWarnings({"unchecked", "rawtypes"})
private InformerEventSource(
InformerEventSourceConfiguration<R> configuration,
KubernetesClient client,
boolean parseResourceVersions) {
boolean parseResourceVersions,
String fieldManager) {
super(
configuration.name(),
configuration
Expand All @@ -112,6 +117,8 @@ private InformerEventSource(
onUpdateFilter = informerConfig.getOnUpdateFilter();
onDeleteFilter = informerConfig.getOnDeleteFilter();
genericFilter = informerConfig.getGenericFilter();
this.client = client;
this.fieldManager = fieldManager;
}

@Override
Expand Down Expand Up @@ -215,10 +222,22 @@ private boolean isEventKnownFromAnnotation(R newObject, R oldObject) {
if (id.equals(parts[0])) {
if (oldObject == null && parts.length == 1) {
known = true;
} else if (oldObject != null
&& parts.length == 2
&& oldObject.getMetadata().getResourceVersion().equals(parts[1])) {
known = true;
} else if (oldObject != null && parts.length == 2) {
if (oldObject.getMetadata().getResourceVersion().equals(parts[1])) {
known = true;
} else {
// if the resource version doesn't match, there's a chance that it's due
// to a change that is not meaningful, but causes the matcher logic between
// desired and newObject to see a change.
// TODO: this likely should be conditional on useSSA, not just the presence of the
// field manager
var ssaMatcher = SSABasedGenericKubernetesResourceMatcher.getInstance();
if (ssaMatcher.checkIfFieldManagerExists(newObject, fieldManager).isPresent()) {
known = ssaMatcher.matches(oldObject, newObject, fieldManager, client, true);
} else {
// do we even need to worry about this case
}
}
}
}
}
Expand Down
18 changes: 18 additions & 0 deletions ...perator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,25 @@ void addedLabelInDesiredMakesMatchFail() {

assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse();
}

@Test
void compareActualSame() {
var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", ConfigMap.class);

assertThat(matcher.matches(actualConfigMap, actualConfigMap, mockedContext.getControllerConfiguration().fieldManager(), mockedContext.getClient(), true)).isTrue();
}

@Test
void compareActualSameWithUnownedChange() {
var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", ConfigMap.class);

var newConfigMap = loadResource("configmap.empty-owner-reference.yaml", ConfigMap.class);
newConfigMap.getMetadata().setLabels(Map.of("newlabel", "val"));

assertThat(matcher.matches(newConfigMap, actualConfigMap, mockedContext.getControllerConfiguration().fieldManager(), mockedContext.getClient(), true)).isTrue();
assertThat(matcher.matches(actualConfigMap, newConfigMap, mockedContext.getControllerConfiguration().fieldManager(), mockedContext.getClient(), true)).isTrue();
}

@Test
@SuppressWarnings("unchecked")
void sortListItemsTest() {
Expand Down
Loading