Skip to content

Commit 6bf05ec

Browse files
fhussonnoisclaude
fhussonnois
and
claude
committed
fix(kafka): label selectors no longer break topic reconciliation (#698)
When using label selectors, reconciliation incorrectly attempted CREATE for existing topics because the selector filtered out actual (collected) topics that lack user-defined labels. Fix by propagating labels from expected resources to matching actual resources before applying the selector, so both sides are filtered consistently. Also rejects the unsafe combination of label selectors with delete-orphans=true, which would incorrectly delete all non-matching topics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 79aeed5 commit 6bf05ec

6 files changed

Lines changed: 317 additions & 9 deletions

File tree

‎core/src/main/java/io/streamthoughts/jikkou/core/selector/Selectors.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,4 +52,19 @@ public static Selector anyMatch(@NotNull List<Selector> selectors) {
5252
public static Selector noneMatch(@NotNull List<Selector> selectors) {
5353
return new NoneMatchSelector(selectors);
5454
}
55+
56+
/**
57+
* Checks whether the given selector contains a {@link LabelSelector},
58+
* either directly or nested within an {@link AggregateSelector}.
59+
*
60+
* @param selector the selector to check.
61+
* @return {@code true} if the selector contains a {@link LabelSelector}.
62+
*/
63+
public static boolean containsLabelSelector(@NotNull Selector selector) {
64+
if (selector instanceof LabelSelector) return true;
65+
if (selector instanceof AggregateSelector agg) {
66+
return agg.selectors.stream().anyMatch(Selectors::containsLabelSelector);
67+
}
68+
return false;
69+
}
5570
}

‎core/src/test/java/io/streamthoughts/jikkou/core/selector/SelectorsTest.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
*/
77
package io.streamthoughts.jikkou.core.selector;
88

9+
import static org.junit.jupiter.api.Assertions.assertFalse;
10+
import static org.junit.jupiter.api.Assertions.assertTrue;
11+
912
import io.streamthoughts.jikkou.core.models.HasMetadata;
1013
import java.util.List;
1114
import org.jetbrains.annotations.NotNull;
@@ -40,6 +43,41 @@ void shouldGetSelectorForNoneMatch() {
4043
Assertions.assertEquals(SelectorMatchingStrategy.NONE, selector.getSelectorMatchingStrategy());
4144
}
4245

46+
@Test
47+
void shouldReturnFalseForNoSelector() {
48+
assertFalse(Selectors.containsLabelSelector(Selectors.NO_SELECTOR));
49+
}
50+
51+
@Test
52+
void shouldReturnTrueForLabelSelector() {
53+
PreparedExpression expr = new PreparedExpression("env", ExpressionOperator.EXISTS, List.of());
54+
LabelSelector labelSelector = new LabelSelector(expr);
55+
assertTrue(Selectors.containsLabelSelector(labelSelector));
56+
}
57+
58+
@Test
59+
void shouldReturnFalseForFieldSelector() {
60+
PreparedExpression expr = new PreparedExpression("metadata.name", ExpressionOperator.IN, List.of("test"));
61+
FieldSelector fieldSelector = new FieldSelector(expr);
62+
assertFalse(Selectors.containsLabelSelector(fieldSelector));
63+
}
64+
65+
@Test
66+
void shouldReturnTrueForAggregateSelectorWithNestedLabelSelector() {
67+
PreparedExpression expr = new PreparedExpression("env", ExpressionOperator.EXISTS, List.of());
68+
LabelSelector labelSelector = new LabelSelector(expr);
69+
Selector aggregate = Selectors.allMatch(List.of(labelSelector));
70+
assertTrue(Selectors.containsLabelSelector(aggregate));
71+
}
72+
73+
@Test
74+
void shouldReturnFalseForAggregateSelectorWithoutLabelSelector() {
75+
PreparedExpression expr = new PreparedExpression("metadata.name", ExpressionOperator.IN, List.of("test"));
76+
FieldSelector fieldSelector = new FieldSelector(expr);
77+
Selector aggregate = Selectors.allMatch(List.of(fieldSelector));
78+
assertFalse(Selectors.containsLabelSelector(aggregate));
79+
}
80+
4381
@NotNull
4482
private static Selector getSelectorForExpressionString(String expressonString) {
4583
return new Selector() {

‎providers/jikkou-provider-kafka/src/integration-test/java/io/streamthoughts/jikkou/kafka/reconciler/AdminClientKafkaTopicControllerIT.java‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import io.streamthoughts.jikkou.core.ReconciliationContext;
1111
import io.streamthoughts.jikkou.core.ReconciliationMode;
1212
import io.streamthoughts.jikkou.core.config.Configuration;
13+
import io.streamthoughts.jikkou.core.exceptions.JikkouRuntimeException;
1314
import io.streamthoughts.jikkou.core.io.Jackson;
1415
import io.streamthoughts.jikkou.core.io.ResourceLoader;
1516
import io.streamthoughts.jikkou.core.io.reader.ResourceReaderFactory;
@@ -20,6 +21,9 @@
2021
import io.streamthoughts.jikkou.core.reconciler.ChangeResult;
2122
import io.streamthoughts.jikkou.core.reconciler.DefaultChangeResult;
2223
import io.streamthoughts.jikkou.core.reconciler.Operation;
24+
import io.streamthoughts.jikkou.core.selector.ExpressionOperator;
25+
import io.streamthoughts.jikkou.core.selector.LabelSelector;
26+
import io.streamthoughts.jikkou.core.selector.PreparedExpression;
2327
import io.streamthoughts.jikkou.core.selector.Selectors;
2428
import io.streamthoughts.jikkou.kafka.BaseExtensionProviderIT;
2529
import io.streamthoughts.jikkou.kafka.collections.V1KafkaTopicList;
@@ -36,6 +40,7 @@ public class AdminClientKafkaTopicControllerIT extends BaseExtensionProviderIT {
3640
public static final String CLASSPATH_RESOURCE_TOPICS = "test-kafka-topics.yaml";
3741
public static final String CLASSPATH_RESOURCE_TOPIC_ALL_DELETE = "test-kafka-topics-with-all-delete.yaml";
3842
public static final String CLASSPATH_RESOURCE_TOPIC_SINGLE_DELETE = "test-kafka-topics-with-single-delete.yaml";
43+
public static final String CLASSPATH_RESOURCE_TOPICS_WITH_LABELS = "test-kafka-topics-with-labels.yaml";
3944
public static final String TOPIC_TEST_A = "topic-test-A";
4045
public static final String TOPIC_TEST_B = "topic-test-B";
4146
public static final String TOPIC_TEST_C = "topic-test-C";
@@ -260,6 +265,66 @@ public void shouldReconcileKafkaTopicForModeApplyAndDeleteOrphansTrue() throws I
260265
Assertions.assertTrue(delete);
261266
}
262267

268+
@Test
269+
public void shouldReconcileExistingTopicsWithLabelSelectorWithoutCreatingDuplicates() {
270+
// GIVEN - Create topics that already exist on the cluster
271+
createTopic(TOPIC_TEST_A);
272+
createTopic(TOPIC_TEST_B);
273+
274+
// Load resources that define the same topics but with user-defined labels
275+
var resources = loader.loadFromClasspath(CLASSPATH_RESOURCE_TOPICS_WITH_LABELS);
276+
277+
// Build a label selector: team IN (my-service)
278+
LabelSelector labelSelector = new LabelSelector(
279+
new PreparedExpression("team", ExpressionOperator.IN, List.of("my-service"))
280+
);
281+
282+
var context = ReconciliationContext.builder()
283+
.selector(Selectors.allMatch(List.of(labelSelector)))
284+
.dryRun(false)
285+
.build();
286+
287+
// WHEN
288+
List<ChangeResult> results = api.reconcile(resources, ReconciliationMode.UPDATE, context).results();
289+
290+
// THEN - Both topics should be UPDATE or NONE, never CREATE
291+
Map<String, ResourceChange> changeByName = results.stream()
292+
.map(ChangeResult::change)
293+
.collect(Collectors.toMap(c -> c.getMetadata().getName(), c -> c));
294+
295+
Assertions.assertEquals(2, changeByName.size(), "Expected changes for both topics");
296+
297+
// Neither topic should be CREATE — they already exist and the selector must not filter them out
298+
changeByName.forEach((name, change) -> {
299+
Assertions.assertNotEquals(
300+
Operation.CREATE,
301+
change.getSpec().getOp(),
302+
"Topic '" + name + "' should not be CREATE (it already exists on the cluster)"
303+
);
304+
});
305+
}
306+
307+
@Test
308+
public void shouldThrowWhenLabelSelectorUsedWithDeleteOrphans() {
309+
// GIVEN
310+
var resources = loader.loadFromClasspath(CLASSPATH_RESOURCE_TOPICS_WITH_LABELS);
311+
312+
LabelSelector labelSelector = new LabelSelector(
313+
new PreparedExpression("team", ExpressionOperator.IN, List.of("my-service"))
314+
);
315+
316+
var context = ReconciliationContext.builder()
317+
.selector(Selectors.allMatch(List.of(labelSelector)))
318+
.configuration(Configuration.of("delete-orphans", true))
319+
.dryRun(false)
320+
.build();
321+
322+
// WHEN / THEN
323+
Assertions.assertThrows(JikkouRuntimeException.class, () ->
324+
api.reconcile(resources, ReconciliationMode.FULL, context)
325+
);
326+
}
327+
263328
@NotNull
264329
private static List<String> topicNames(ResourceList<V1KafkaTopic> items) {
265330
return items.stream().map(V1KafkaTopic::getMetadata).map(ObjectMeta::getName).toList();

‎providers/jikkou-provider-kafka/src/integration-test/resources/test-kafka-topics-with-labels.yaml‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
apiVersion: "kafka.jikkou.io/v1"
2+
kind: KafkaTopicList
3+
metadata:
4+
annotations: {}
5+
labels: {}
6+
items:
7+
- metadata:
8+
name: "topic-test-A"
9+
labels:
10+
team: my-service
11+
spec:
12+
partitions: 1
13+
replicas: 1
14+
configs:
15+
cleanup.policy: compact
16+
17+
- metadata:
18+
name: "topic-test-B"
19+
labels:
20+
team: my-service
21+
spec:
22+
partitions: 1
23+
replicas: 1
24+
configs:
25+
cleanup.policy: delete

‎providers/jikkou-provider-kafka/src/main/java/io/streamthoughts/jikkou/kafka/reconciler/AdminClientKafkaTopicController.java‎

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import io.streamthoughts.jikkou.core.ReconciliationContext;
1212
import io.streamthoughts.jikkou.core.annotation.SupportedResource;
1313
import io.streamthoughts.jikkou.core.config.ConfigProperty;
14+
import io.streamthoughts.jikkou.core.exceptions.JikkouRuntimeException;
1415
import io.streamthoughts.jikkou.core.extension.ContextualExtension;
1516
import io.streamthoughts.jikkou.core.extension.ExtensionContext;
1617
import io.streamthoughts.jikkou.core.models.Configs;
@@ -20,6 +21,8 @@
2021
import io.streamthoughts.jikkou.core.reconciler.ChangeResult;
2122
import io.streamthoughts.jikkou.core.reconciler.Controller;
2223
import io.streamthoughts.jikkou.core.reconciler.annotations.ControllerConfiguration;
24+
import io.streamthoughts.jikkou.core.selector.Selector;
25+
import io.streamthoughts.jikkou.core.selector.Selectors;
2326
import io.streamthoughts.jikkou.kafka.ApiVersions;
2427
import io.streamthoughts.jikkou.kafka.KafkaExtensionProvider;
2528
import io.streamthoughts.jikkou.kafka.change.topics.*;
@@ -29,7 +32,9 @@
2932
import io.streamthoughts.jikkou.kafka.models.V1KafkaTopicSpec;
3033
import java.util.Collection;
3134
import java.util.List;
35+
import java.util.Map;
3236
import java.util.regex.Pattern;
37+
import java.util.stream.Collectors;
3338
import org.apache.kafka.clients.admin.AdminClient;
3439
import org.jetbrains.annotations.NotNull;
3540
import org.slf4j.Logger;
@@ -124,33 +129,79 @@ public List<ResourceChange> plan(
124129

125130
LOG.info("Computing reconciliation change for '{}' resources", resources.size());
126131

127-
// Get the list of described resource that are candidates for this reconciliation
128-
List<V1KafkaTopic> expectedKafkaTopics = resources.stream()
129-
.filter(context.selector()::apply)
132+
Selector selector = context.selector();
133+
boolean deleteOrphans = Config.IS_DELETE_ORPHANS_ENABLED.get(context.configuration());
134+
135+
if (deleteOrphans && Selectors.containsLabelSelector(selector)) {
136+
throw new JikkouRuntimeException(
137+
"Cannot use label selectors together with delete-orphans=true. " +
138+
"Label selectors filter out actual resources that lack user-defined labels, " +
139+
"which would cause all non-matching topics to be deleted."
140+
);
141+
}
142+
143+
// Get the list of expected resources with flattened configs (unfiltered)
144+
List<V1KafkaTopic> allExpectedKafkaTopics = resources.stream()
130145
.map(resource -> {
131146
V1KafkaTopicSpec spec = resource.getSpec();
132147
Configs configs = spec.getConfigs();
133148
return configs == null ? resource : resource.withSpec(spec.withConfigs(configs.flatten()));
134149
})
135150
.toList();
136151

137-
// Get the list of remote resources that are candidates for this reconciliation
152+
// Get the list of actual remote resources (unfiltered)
138153
AdminClientKafkaTopicCollector collector = new AdminClientKafkaTopicCollector(adminClientContextFactory);
139154
collector.init(extensionContext().contextForExtension(AdminClientKafkaTopicCollector.class));
155+
List<V1KafkaTopic> allActualKafkaTopics = collector.listAll().getItems();
156+
157+
// Enrich actual topics with labels from expected topics so label selectors work on both sides
158+
enrichLabelsFromExpected(allActualKafkaTopics, allExpectedKafkaTopics);
159+
160+
// Now apply the selector to both sides
161+
List<V1KafkaTopic> expectedKafkaTopics = allExpectedKafkaTopics.stream()
162+
.filter(selector::apply)
163+
.toList();
140164

141-
List<V1KafkaTopic> actualKafkaTopics = collector.listAll()
142-
.stream()
143-
.filter(context.selector()::apply)
144-
.toList();
165+
List<V1KafkaTopic> actualKafkaTopics = allActualKafkaTopics.stream()
166+
.filter(selector::apply)
167+
.toList();
145168

146169
TopicChangeComputer changeComputer = new TopicChangeComputer(
147170
topicDeleteExcludePatterns,
148-
Config.IS_DELETE_ORPHANS_ENABLED.get(context.configuration()),
171+
deleteOrphans,
149172
Config.IS_CONFIG_DELETE_ORPHANS_ENABLED.get(context.configuration())
150173
);
151174
return changeComputer.computeChanges(actualKafkaTopics, expectedKafkaTopics);
152175
}
153176

177+
/**
178+
* Enriches actual topics with labels from matching expected topics.
179+
* Labels from expected topics are propagated to actual topics (joined by name),
180+
* preserving any existing labels on the actual topics (e.g., system labels).
181+
*
182+
* @param actual the list of actual (collected) topics.
183+
* @param expected the list of expected (input) topics.
184+
*/
185+
static void enrichLabelsFromExpected(
186+
@NotNull List<V1KafkaTopic> actual,
187+
@NotNull List<V1KafkaTopic> expected) {
188+
189+
Map<String, Map<String, Object>> labelsByName = expected.stream()
190+
.collect(Collectors.toMap(
191+
t -> t.getMetadata().getName(),
192+
t -> t.getMetadata().getLabels(),
193+
(a, b) -> b // last-one-wins for duplicate names
194+
));
195+
196+
for (V1KafkaTopic topic : actual) {
197+
Map<String, Object> expectedLabels = labelsByName.get(topic.getMetadata().getName());
198+
if (expectedLabels == null || expectedLabels.isEmpty()) {
199+
continue;
200+
}
201+
expectedLabels.forEach(topic.getMetadata()::addLabelIfAbsent);
202+
}
203+
}
204+
154205
/**
155206
* {@inheritDoc}
156207
**/

0 commit comments

Comments
 (0)