Skip to content

Commit 71a952d

Browse files
robobariorobobario
committed
Refactor to only use the labels from the podTemplate
We want control over which podTemplateSpec fields are used Signed-off-by: Robert Young <robeyoun@redhat.com>
1 parent 76580ad commit 71a952d

File tree

6 files changed

+92
-71
lines changed

6 files changed

+92
-71
lines changed

‎kroxylicious-operator/src/main/java/io/kroxylicious/kubernetes/operator/ProxyDeployment.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@
66
package io.kroxylicious.kubernetes.operator;
77

88
import java.util.Map;
9+
import java.util.Optional;
910

1011
import io.fabric8.kubernetes.api.model.Container;
1112
import io.fabric8.kubernetes.api.model.ContainerBuilder;
13+
import io.fabric8.kubernetes.api.model.ObjectMeta;
1214
import io.fabric8.kubernetes.api.model.PodTemplateSpec;
1315
import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder;
1416
import io.fabric8.kubernetes.api.model.apps.Deployment;
@@ -77,11 +79,14 @@ static Map<String, String> podLabels() {
7779

7880
private PodTemplateSpec podTemplate(KafkaProxy primary,
7981
Context<KafkaProxy> context) {
80-
PodTemplateSpec podTemplate = primary.getSpec().getPodTemplate();
81-
PodTemplateSpecBuilder builder = podTemplate != null ? podTemplate.edit() : new PodTemplateSpecBuilder();
82+
Map<String, String> labelsFromSpec = Optional.ofNullable(primary.getSpec().getPodTemplate())
83+
.map(PodTemplateSpec::getMetadata)
84+
.map(ObjectMeta::getLabels)
85+
.orElse(Map.of());
8286
// @formatter:off
83-
return builder
87+
return new PodTemplateSpecBuilder()
8488
.editOrNewMetadata()
89+
.addToLabels(labelsFromSpec)
8590
.addToLabels(podLabels())
8691
.endMetadata()
8792
.editOrNewSpec()

‎kroxylicious-operator/src/test/java/io/kroxylicious/kubernetes/operator/DerivedResourcesTest.java‎

Lines changed: 73 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535

3636
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
3737
import io.fabric8.kubernetes.api.model.HasMetadata;
38+
import io.fabric8.kubernetes.api.model.ObjectMeta;
3839
import io.javaoperatorsdk.operator.api.reconciler.Context;
3940
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext;
4041
import io.javaoperatorsdk.operator.processing.dependent.BulkDependentResource;
@@ -47,6 +48,7 @@
4748
import edu.umd.cs.findbugs.annotations.NonNull;
4849

4950
import static org.assertj.core.api.Assertions.assertThat;
51+
import static org.assertj.core.api.Assertions.entry;
5052
import static org.mockito.Mockito.doReturn;
5153
import static org.mockito.Mockito.mock;
5254

@@ -195,75 +197,84 @@ record ConditionStruct(ConditionType type,
195197
private static List<DynamicTest> testsForDir(List<DesiredFn<KafkaProxy, ?>> dependentResources,
196198
Path testDir)
197199
throws IOException {
198-
var unusedFiles = childFilesMatching(testDir, "*");
199-
Path input = testDir.resolve("in-KafkaProxy.yaml");
200-
KafkaProxy kafkaProxy = kafkaProxyFromFile(input);
201-
assertMinimalMetadataPresent(kafkaProxy);
202-
unusedFiles.remove(input);
203-
unusedFiles.remove(testDir.resolve("operator-config.yaml"));
204-
unusedFiles.removeAll(childFilesMatching(testDir, "in-*"));
205-
206-
Context<KafkaProxy> context;
207200
try {
208-
context = buildContext(kafkaProxy, testDir);
209-
}
210-
catch (IOException e) {
211-
throw new UncheckedIOException(e);
212-
}
213-
214-
List<DynamicTest> tests = new ArrayList<>();
215-
216-
var dr = dependentResources.stream()
217-
.flatMap(r -> r.invokeDesired(kafkaProxy, context).values().stream().map(x -> Map.entry(r.resourceType(), x)))
218-
.collect(Collectors.groupingBy(Map.Entry::getKey))
219-
.entrySet()
220-
.stream()
221-
.collect(Collectors.toMap(Map.Entry::getKey,
222-
e -> e.getValue().stream().map(Map.Entry::getValue).collect(Collectors.toCollection(() -> new TreeSet<>(
223-
Comparator.comparing(hasMetadata -> hasMetadata.getMetadata().getName()))))));
224-
for (var entry : dr.entrySet()) {
225-
var resourceType = entry.getKey();
226-
var actualResources = entry.getValue();
227-
for (var actualResource : actualResources) {
228-
String kind = resourceType.getSimpleName();
229-
String name = actualResource.getMetadata().getName();
230-
var expectedFile = testDir.resolve("out-" + kind + "-" + name + ".yaml");
231-
tests.add(DynamicTest.dynamicTest(kind + " '" + name + "' should have the same content as " + testDir.relativize(expectedFile),
232-
() -> {
233-
assertThat(Files.exists(expectedFile)).isTrue();
234-
var expected = loadExpected(expectedFile, resourceType);
235-
assertSameYaml(actualResource, expected);
236-
unusedFiles.remove(expectedFile);
237-
}));
201+
var unusedFiles = childFilesMatching(testDir, "*");
202+
String inFileName = "in-KafkaProxy.yaml";
203+
Path input = testDir.resolve(inFileName);
204+
KafkaProxy kafkaProxy = kafkaProxyFromFile(input);
205+
assertMinimalMetadata(kafkaProxy.getMetadata(), inFileName);
206+
207+
unusedFiles.remove(input);
208+
unusedFiles.remove(testDir.resolve("operator-config.yaml"));
209+
unusedFiles.removeAll(childFilesMatching(testDir, "in-*"));
210+
211+
Context<KafkaProxy> context;
212+
try {
213+
context = buildContext(kafkaProxy, testDir);
238214
}
239-
for (var cluster : kafkaProxy.getSpec().getClusters()) {
240-
ClusterCondition actualClusterCondition = SharedKafkaProxyContext.clusterCondition(context, cluster);
241-
if (actualClusterCondition.type() == ConditionType.Accepted && actualClusterCondition.status().equals(Conditions.Status.TRUE)) {
242-
continue;
243-
}
244-
else {
245-
var expectedFile = testDir.resolve("cond-" + actualClusterCondition.type() + "-" + actualClusterCondition.cluster() + ".yaml");
246-
tests.add(DynamicTest.dynamicTest(
247-
"Condition " + actualClusterCondition.type() + " for cluster " + actualClusterCondition.cluster() + " matches contents of expected file "
248-
+ expectedFile,
215+
catch (IOException e) {
216+
throw new UncheckedIOException(e);
217+
}
218+
219+
List<DynamicTest> tests = new ArrayList<>();
220+
221+
var dr = dependentResources.stream()
222+
.flatMap(r -> r.invokeDesired(kafkaProxy, context).values().stream().map(x -> Map.entry(r.resourceType(), x)))
223+
.collect(Collectors.groupingBy(Map.Entry::getKey))
224+
.entrySet()
225+
.stream()
226+
.collect(Collectors.toMap(Map.Entry::getKey,
227+
e -> e.getValue().stream().map(Map.Entry::getValue).collect(Collectors.toCollection(() -> new TreeSet<>(
228+
Comparator.comparing(hasMetadata -> hasMetadata.getMetadata().getName()))))));
229+
for (var entry : dr.entrySet()) {
230+
var resourceType = entry.getKey();
231+
var actualResources = entry.getValue();
232+
for (var actualResource : actualResources) {
233+
String kind = resourceType.getSimpleName();
234+
String name = actualResource.getMetadata().getName();
235+
var expectedFile = testDir.resolve("out-" + kind + "-" + name + ".yaml");
236+
tests.add(DynamicTest.dynamicTest(kind + " '" + name + "' should have the same content as " + testDir.relativize(expectedFile),
249237
() -> {
250-
var expected = loadExpected(expectedFile, ClusterCondition.class);
251-
assertSameYaml(actualClusterCondition, expected);
238+
assertThat(Files.exists(expectedFile)).isTrue();
239+
var expected = loadExpected(expectedFile, resourceType);
240+
assertSameYaml(actualResource, expected);
252241
unusedFiles.remove(expectedFile);
253242
}));
254243
}
244+
for (var cluster : kafkaProxy.getSpec().getClusters()) {
245+
ClusterCondition actualClusterCondition = SharedKafkaProxyContext.clusterCondition(context, cluster);
246+
if (actualClusterCondition.type() == ConditionType.Accepted && actualClusterCondition.status().equals(Conditions.Status.TRUE)) {
247+
continue;
248+
}
249+
else {
250+
var expectedFile = testDir.resolve("cond-" + actualClusterCondition.type() + "-" + actualClusterCondition.cluster() + ".yaml");
251+
tests.add(DynamicTest.dynamicTest(
252+
"Condition " + actualClusterCondition.type() + " for cluster " + actualClusterCondition.cluster() + " matches contents of expected file "
253+
+ expectedFile,
254+
() -> {
255+
var expected = loadExpected(expectedFile, ClusterCondition.class);
256+
assertSameYaml(actualClusterCondition, expected);
257+
unusedFiles.remove(expectedFile);
258+
}));
259+
}
260+
}
255261
}
256-
}
257262

258-
tests.add(DynamicTest.dynamicTest("There should be no unused files in " + testDir,
259-
() -> assertThat(unusedFiles).isEmpty()));
260-
return tests;
263+
tests.add(DynamicTest.dynamicTest("There should be no unused files in " + testDir,
264+
() -> assertThat(unusedFiles).isEmpty()));
265+
return tests;
266+
}
267+
catch (AssertionError e) {
268+
return List.of(DynamicTest.dynamicTest("failed to initialize test", () -> {
269+
throw e;
270+
}));
271+
}
261272
}
262273

263-
// sanity check since we can omit fields that the k8s API will ensure are present in reality
264-
private static void assertMinimalMetadataPresent(HasMetadata resource) {
265-
assertThat(resource.getMetadata().getName()).isNotNull().isNotEmpty();
266-
assertThat(resource.getMetadata().getNamespace()).isNotNull().isNotEmpty();
274+
private static void assertMinimalMetadata(ObjectMeta metadata, String inFileName) {
275+
// sanity check since we can omit fields that the k8s API will ensure are present in reality
276+
assertThat(metadata.getName()).describedAs("metadata.name in " + inFileName).isNotNull().isNotEmpty();
277+
assertThat(metadata.getNamespace()).describedAs("metadata.namespace in " + inFileName).isNotNull().isNotEmpty();
267278
}
268279

269280
private static <T> void assertSameYaml(T actualResource, T expected) throws JsonProcessingException {
@@ -305,10 +316,11 @@ private static Context<KafkaProxy> buildContext(KafkaProxy kafkaProxy, Path test
305316
var runtimeDecl = Files.exists(configFile) ? configFromFile(configFile) : new RuntimeDecl(List.of());
306317
Set<GenericKubernetesResource> filterInstances = new HashSet<>();
307318
for (var filterApi : runtimeDecl.filterApis()) {
308-
try (var dirStream = Files.newDirectoryStream(testDir, "in-" + filterApi.kind() + "-*.yaml")) {
319+
String fileName = "in-" + filterApi.kind() + "-*.yaml";
320+
try (var dirStream = Files.newDirectoryStream(testDir, fileName)) {
309321
for (Path p : dirStream) {
310322
GenericKubernetesResource resource = YAML_MAPPER.readValue(p.toFile(), GenericKubernetesResource.class);
311-
assertMinimalMetadataPresent(resource);
323+
assertMinimalMetadata(resource.getMetadata(), fileName);
312324
filterInstances.add(resource);
313325
}
314326
}

‎kroxylicious-operator/src/test/resources/DerivedResourcesTest/use-pod-template-spec/in-KafkaProxy.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ kind: KafkaProxy
99
apiVersion: kroxylicious.io/v1alpha1
1010
metadata:
1111
name: use-pod-template-spec
12+
namespace: proxy-ns
1213
spec:
1314
clusters:
1415
- name: "one"
@@ -17,4 +18,4 @@ spec:
1718
podTemplate:
1819
metadata:
1920
labels:
20-
environment: "production"
21+
environment: "production-from-podTemplate"

‎kroxylicious-operator/src/test/resources/DerivedResourcesTest/use-pod-template-spec/out-Deployment-use-pod-template-spec.yaml‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ metadata:
1616
app.kubernetes.io/instance: "use-pod-template-spec"
1717
app.kubernetes.io/component: "proxy"
1818
name: "use-pod-template-spec"
19+
namespace: proxy-ns
1920
ownerReferences:
2021
- apiVersion: "kroxylicious.io/v1alpha1"
2122
kind: "KafkaProxy"
@@ -28,15 +29,15 @@ spec:
2829
template:
2930
metadata:
3031
labels:
31-
environment: "production"
32+
environment: "production-from-podTemplate"
3233
app: "kroxylicious"
3334
spec:
3435
containers:
3536
- name: "proxy"
3637
image: "quay.io/kroxylicious/kroxylicious:0.9.0-SNAPSHOT"
3738
args:
3839
- "--config"
39-
- "/opt/kroxylicious/config/config.yaml"
40+
- "/opt/kroxylicious/config/proxy-config.yaml"
4041
ports:
4142
- containerPort: 9190
4243
name: "metrics"
@@ -45,9 +46,9 @@ spec:
4546
- containerPort: 9294
4647
- containerPort: 9295
4748
volumeMounts:
48-
- mountPath: "/opt/kroxylicious/config/config.yaml"
49+
- mountPath: "/opt/kroxylicious/config/proxy-config.yaml"
4950
name: "config-volume"
50-
subPath: "config.yaml"
51+
subPath: "proxy-config.yaml"
5152
volumes:
5253
- name: "config-volume"
5354
secret:

‎kroxylicious-operator/src/test/resources/DerivedResourcesTest/use-pod-template-spec/out-Secret-use-pod-template-spec.yaml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,13 @@ metadata:
1515
app.kubernetes.io/instance: "use-pod-template-spec"
1616
app.kubernetes.io/component: "proxy"
1717
name: "use-pod-template-spec"
18+
namespace: proxy-ns
1819
ownerReferences:
1920
- apiVersion: "kroxylicious.io/v1alpha1"
2021
kind: "KafkaProxy"
2122
name: "use-pod-template-spec"
2223
stringData:
23-
config.yaml: |
24+
proxy-config.yaml: |
2425
---
2526
adminHttp:
2627
host: "0.0.0.0"
@@ -35,6 +36,6 @@ stringData:
3536
type: "PortPerBrokerClusterNetworkAddressConfigProvider"
3637
config:
3738
bootstrapAddress: "localhost:9292"
38-
brokerAddressPattern: "one"
39+
brokerAddressPattern: "one.proxy-ns.svc.cluster.local"
3940
brokerStartPort: 9293
4041
numberOfBrokerPorts: 3

‎kroxylicious-operator/src/test/resources/DerivedResourcesTest/use-pod-template-spec/out-Service-one.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ metadata:
1515
app.kubernetes.io/instance: "use-pod-template-spec"
1616
app.kubernetes.io/component: "proxy"
1717
name: "one"
18+
namespace: proxy-ns
1819
ownerReferences:
1920
- apiVersion: "kroxylicious.io/v1alpha1"
2021
kind: "KafkaProxy"

0 commit comments

Comments
 (0)