Skip to content

Commit 94f005e

Browse files
csvirimetacosm
csviri
and
metacosm
authored
improve: owner reference mappers checks only group not apiVersion (#3302)
Signed-off-by: Attila Mészáros <a_meszaros@apple.com> Signed-off-by: Chris Laprun <metacosm@gmail.com> Co-authored-by: Chris Laprun <metacosm@gmail.com>
1 parent 21fd3b8 commit 94f005e

10 files changed

Lines changed: 434 additions & 10 deletions

File tree

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtilsInternal.java‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,13 @@ public class ReconcilerUtilsInternal {
4545
private static final String GET_STATUS = "getStatus";
4646
private static final Pattern API_URI_PATTERN =
4747
Pattern.compile(".*http(s?)://[^/]*/api(s?)/(\\S*).*"); // NOSONAR: input is controlled
48+
private static final String DOT = ".";
49+
private static final char DOT_CHAR = '.';
50+
private static final String SLASH = "/";
51+
private static final char SLASH_CHAR = '/';
52+
private static final String EMPTY = "";
53+
private static final char ZERO = '0';
54+
private static final char NINE = '9';
4855

4956
// prevent instantiation of util class
5057
private ReconcilerUtilsInternal() {}
@@ -55,7 +62,7 @@ public static boolean isFinalizerValid(String finalizer) {
5562

5663
public static String getResourceTypeNameWithVersion(Class<? extends HasMetadata> resourceClass) {
5764
final var version = HasMetadata.getVersion(resourceClass);
58-
return getResourceTypeName(resourceClass) + "/" + version;
65+
return getResourceTypeName(resourceClass) + SLASH + version;
5966
}
6067

6168
public static String getResourceTypeName(Class<? extends HasMetadata> resourceClass) {
@@ -69,7 +76,7 @@ public static String getDefaultFinalizerName(Class<? extends HasMetadata> resour
6976
public static String getDefaultFinalizerName(String resourceName) {
7077
// resource names for historic resources such as Pods are missing periods and therefore do not
7178
// constitute valid domain names as mandated by Kubernetes so generate one that does
72-
if (resourceName.indexOf('.') < 0) {
79+
if (resourceName.indexOf(DOT_CHAR) < 0) {
7380
resourceName = resourceName + MISSING_GROUP_SUFFIX;
7481
}
7582
return resourceName + FINALIZER_NAME_SUFFIX;
@@ -102,7 +109,7 @@ public static String getDefaultNameFor(Class<? extends Reconciler> reconcilerCla
102109

103110
public static String getDefaultReconcilerName(String reconcilerClassName) {
104111
// if the name is fully qualified, extract the simple class name
105-
final var lastDot = reconcilerClassName.lastIndexOf('.');
112+
final var lastDot = reconcilerClassName.lastIndexOf(DOT_CHAR);
106113
if (lastDot > 0) {
107114
reconcilerClassName = reconcilerClassName.substring(lastDot + 1);
108115
}
@@ -228,15 +235,15 @@ private static boolean matchesResourceType(
228235
final var regex = API_URI_PATTERN.matcher(message);
229236
if (regex.matches()) {
230237
var group = regex.group(3);
231-
if (group.endsWith(".")) {
238+
if (group.endsWith(DOT)) {
232239
group = group.substring(0, group.length() - 1);
233240
}
234241
final var segments =
235-
Arrays.stream(group.split("/")).filter(Predicate.not(String::isEmpty)).toList();
242+
Arrays.stream(group.split(SLASH)).filter(Predicate.not(String::isEmpty)).toList();
236243
if (segments.size() != 3) {
237244
return false;
238245
}
239-
final var targetResourceName = segments.get(2) + "." + segments.get(0);
246+
final var targetResourceName = segments.get(2) + DOT_CHAR + segments.get(0);
240247
return resourceTypeName.equals(targetResourceName);
241248
}
242249
}
@@ -349,16 +356,25 @@ private static int validateResourceVersion(String v1) {
349356
}
350357
for (int i = 0; i < v1Length; i++) {
351358
char char1 = v1.charAt(i);
352-
if (char1 == '0') {
359+
if (char1 == ZERO) {
353360
if (i == 0) {
354361
throw new NonComparableResourceVersionException(
355362
"Resource version cannot begin with 0: " + v1);
356363
}
357-
} else if (char1 < '0' || char1 > '9') {
364+
} else if (char1 < ZERO || char1 > NINE) {
358365
throw new NonComparableResourceVersionException(
359366
"Non numeric characters in resource version: " + v1);
360367
}
361368
}
362369
return v1Length;
363370
}
371+
372+
public static String getGroup(String apiVersion) {
373+
var index = apiVersion.indexOf(SLASH_CHAR);
374+
if (index < 0) {
375+
return EMPTY;
376+
} else {
377+
return apiVersion.substring(0, index);
378+
}
379+
}
364380
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import io.javaoperatorsdk.operator.processing.event.ResourceID;
2525
import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper;
2626

27+
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.getGroup;
28+
2729
public class Mappers {
2830

2931
public static final String DEFAULT_ANNOTATION_FOR_NAME = "io.javaoperatorsdk/primary-name";
@@ -98,10 +100,12 @@ public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerRefer
98100

99101
public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerReferences(
100102
String apiVersion, String kind, boolean clusterScope) {
101-
String correctApiVersion = apiVersion.startsWith("/") ? apiVersion.substring(1) : apiVersion;
102103
return resource ->
103104
resource.getMetadata().getOwnerReferences().stream()
104-
.filter(r -> r.getKind().equals(kind) && r.getApiVersion().equals(correctApiVersion))
105+
.filter(
106+
r ->
107+
r.getKind().equals(kind)
108+
&& getGroup(r.getApiVersion()).equals(getGroup(apiVersion)))
105109
.map(or -> ResourceID.fromOwnerReference(resource, or, clusterScope))
106110
.collect(Collectors.toSet());
107111
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsInternalTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.getDefaultFinalizerName;
4040
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.getDefaultNameFor;
4141
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.getDefaultReconcilerName;
42+
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.getGroup;
4243
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.handleKubernetesClientException;
4344
import static io.javaoperatorsdk.operator.ReconcilerUtilsInternal.isFinalizerValid;
4445
import static org.assertj.core.api.Assertions.assertThat;
@@ -308,6 +309,12 @@ void compareResourceVersionsWithHasMetadata() {
308309
assertThat(ReconcilerUtilsInternal.compareResourceVersions(resource2, resource1)).isPositive();
309310
}
310311

312+
@Test
313+
void extractsGroupFromApiVersion() {
314+
assertThat(getGroup("v1")).isEqualTo("");
315+
assertThat(getGroup("apps/v1")).isEqualTo("apps");
316+
}
317+
311318
private HasMetadata createResourceWithVersion(String resourceVersion) {
312319
return new PodBuilder()
313320
.withMetadata(

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/MappersTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import io.fabric8.kubernetes.api.model.ConfigMap;
2323
import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
24+
import io.fabric8.kubernetes.api.model.HasMetadata;
2425
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
2526
import io.javaoperatorsdk.operator.TestUtils;
2627
import io.javaoperatorsdk.operator.processing.event.ResourceID;
@@ -83,6 +84,30 @@ void secondaryToPrimaryMapperFromOwnerReferenceFiltersByType() {
8384
assertThat(res).isEmpty();
8485
}
8586

87+
@Test
88+
void fromOwnerReferenceIgnoresVersionFromApiVersion() {
89+
var primary = TestUtils.testCustomResource();
90+
primary.getMetadata().setUid(UUID.randomUUID().toString());
91+
var secondary =
92+
new ConfigMapBuilder()
93+
.withMetadata(
94+
new ObjectMetaBuilder()
95+
.withName("test1")
96+
.withNamespace(primary.getMetadata().getNamespace())
97+
.build())
98+
.build();
99+
secondary.addOwnerReference(primary);
100+
101+
var res =
102+
Mappers.fromOwnerReferences(
103+
HasMetadata.getGroup(TestCustomResource.class) + "/v2",
104+
HasMetadata.getKind(TestCustomResource.class),
105+
false)
106+
.toPrimaryResourceIDs(secondary);
107+
108+
assertThat(res).contains(ResourceID.fromResource(primary));
109+
}
110+
86111
private static ConfigMap getConfigMap(TestCustomResource primary) {
87112
return new ConfigMapBuilder()
88113
.withMetadata(

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/ownerreferencemultiversion/OwnerRefMultiVersionCR1.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* Copyright Java Operator SDK Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package io.javaoperatorsdk.operator.baseapi.ownerreferencemultiversion;
17+
18+
import io.fabric8.kubernetes.api.model.Namespaced;
19+
import io.fabric8.kubernetes.client.CustomResource;
20+
import io.fabric8.kubernetes.model.annotation.Group;
21+
import io.fabric8.kubernetes.model.annotation.Kind;
22+
import io.fabric8.kubernetes.model.annotation.Version;
23+
24+
@Group("sample.javaoperatorsdk")
25+
@Version("v1")
26+
@Kind("OwnerRefMultiVersionCR")
27+
public class OwnerRefMultiVersionCR1
28+
extends CustomResource<OwnerRefMultiVersionSpec, OwnerRefMultiVersionStatus>
29+
implements Namespaced {}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/ownerreferencemultiversion/OwnerRefMultiVersionCR2.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* Copyright Java Operator SDK Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package io.javaoperatorsdk.operator.baseapi.ownerreferencemultiversion;
17+
18+
import io.fabric8.kubernetes.api.model.Namespaced;
19+
import io.fabric8.kubernetes.client.CustomResource;
20+
import io.fabric8.kubernetes.model.annotation.Group;
21+
import io.fabric8.kubernetes.model.annotation.Kind;
22+
import io.fabric8.kubernetes.model.annotation.Version;
23+
24+
@Group("sample.javaoperatorsdk")
25+
@Version(value = "v2", storage = false)
26+
@Kind("OwnerRefMultiVersionCR")
27+
public class OwnerRefMultiVersionCR2
28+
extends CustomResource<OwnerRefMultiVersionSpec, OwnerRefMultiVersionStatus>
29+
implements Namespaced {}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/ownerreferencemultiversion/OwnerRefMultiVersionIT.java‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
/*
2+
* Copyright Java Operator SDK Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package io.javaoperatorsdk.operator.baseapi.ownerreferencemultiversion;
17+
18+
import java.time.Duration;
19+
20+
import org.junit.jupiter.api.Test;
21+
import org.junit.jupiter.api.extension.RegisterExtension;
22+
import org.slf4j.Logger;
23+
import org.slf4j.LoggerFactory;
24+
25+
import io.fabric8.kubernetes.api.model.ConfigMap;
26+
import io.fabric8.kubernetes.api.model.ObjectMeta;
27+
import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinitionVersion;
28+
import io.fabric8.kubernetes.client.KubernetesClient;
29+
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
30+
31+
import static org.assertj.core.api.Assertions.assertThat;
32+
import static org.awaitility.Awaitility.await;
33+
34+
/**
35+
* Integration test verifying that {@code Mappers.fromOwnerReferences} correctly maps secondary
36+
* resources back to primary resources even after a CRD version change. The mapper compares only the
37+
* group part of the apiVersion (ignoring the version), so owner references created under v1 should
38+
* still work when the CRD storage version switches to v2.
39+
*/
40+
class OwnerRefMultiVersionIT {
41+
42+
private static final Logger log = LoggerFactory.getLogger(OwnerRefMultiVersionIT.class);
43+
44+
private static final String CR_NAME = "test-ownerref-mv";
45+
private static final String CRD_NAME = "ownerrefmultiversioncrs.sample.javaoperatorsdk";
46+
47+
@RegisterExtension
48+
LocallyRunOperatorExtension operator =
49+
LocallyRunOperatorExtension.builder()
50+
.withReconciler(new OwnerRefMultiVersionReconciler())
51+
.withBeforeStartHook(
52+
ext -> {
53+
// The auto-generated CRD has both v1 (storage) and v2. Remove v2 so the
54+
// cluster initially only knows about v1.
55+
var client = ext.getKubernetesClient();
56+
var crd =
57+
client
58+
.apiextensions()
59+
.v1()
60+
.customResourceDefinitions()
61+
.withName(CRD_NAME)
62+
.get();
63+
if (crd != null) {
64+
crd.getSpec().getVersions().removeIf(v -> "v2".equals(v.getName()));
65+
crd.getMetadata().setResourceVersion(null);
66+
crd.getMetadata().setManagedFields(null);
67+
client.resource(crd).serverSideApply();
68+
log.info("Applied CRD with v1 only");
69+
}
70+
})
71+
.build();
72+
73+
@Test
74+
void mapperWorksAcrossVersionChange() {
75+
var reconciler = operator.getReconcilerOfType(OwnerRefMultiVersionReconciler.class);
76+
77+
// 1. Create a v1 custom resource
78+
var cr = createCR();
79+
operator.create(cr);
80+
81+
// 2. Wait for initial reconciliation: ConfigMap is created with owner ref to v1
82+
await()
83+
.atMost(Duration.ofSeconds(30))
84+
.pollInterval(Duration.ofMillis(300))
85+
.untilAsserted(
86+
() -> {
87+
var current = operator.get(OwnerRefMultiVersionCR1.class, CR_NAME);
88+
assertThat(current.getStatus()).isNotNull();
89+
assertThat(current.getStatus().getReconcileCount()).isPositive();
90+
91+
var cm = operator.get(ConfigMap.class, CR_NAME);
92+
assertThat(cm).isNotNull();
93+
assertThat(cm.getMetadata().getOwnerReferences()).hasSize(1);
94+
assertThat(cm.getMetadata().getOwnerReferences().get(0).getApiVersion())
95+
.isEqualTo("sample.javaoperatorsdk/v1");
96+
});
97+
98+
int countBeforeUpdate = reconciler.getReconcileCount();
99+
log.info("Reconcile count before CRD update: {}", countBeforeUpdate);
100+
101+
// 3. Update CRD to add v2 as the new storage version
102+
updateCrdWithV2AsStorage(operator.getKubernetesClient());
103+
104+
// 4. Modify the ConfigMap to trigger the informer event source.
105+
// The mapper should still map the ConfigMap (with v1 owner ref) to the primary CR.
106+
var cm = operator.get(ConfigMap.class, CR_NAME);
107+
cm.getData().put("updated", "true");
108+
operator.replace(cm);
109+
110+
// 5. Verify reconciliation was triggered again
111+
await()
112+
.atMost(Duration.ofSeconds(30))
113+
.pollInterval(Duration.ofMillis(300))
114+
.untilAsserted(
115+
() -> {
116+
assertThat(reconciler.getReconcileCount()).isGreaterThan(countBeforeUpdate);
117+
});
118+
119+
log.info("Reconcile count after CRD update: {}", reconciler.getReconcileCount());
120+
}
121+
122+
private void updateCrdWithV2AsStorage(KubernetesClient client) {
123+
var crd = client.apiextensions().v1().customResourceDefinitions().withName(CRD_NAME).get();
124+
125+
// Set v1 to non-storage
126+
for (var version : crd.getSpec().getVersions()) {
127+
if ("v1".equals(version.getName())) {
128+
version.setStorage(false);
129+
}
130+
}
131+
132+
// Add v2 as storage version, reusing v1's schema (specs are compatible)
133+
var v1 =
134+
crd.getSpec().getVersions().stream()
135+
.filter(v -> "v1".equals(v.getName()))
136+
.findFirst()
137+
.orElseThrow();
138+
139+
var v2 = new CustomResourceDefinitionVersion();
140+
v2.setName("v2");
141+
v2.setServed(true);
142+
v2.setStorage(true);
143+
v2.setSchema(v1.getSchema());
144+
v2.setSubresources(v1.getSubresources());
145+
crd.getSpec().getVersions().add(v2);
146+
147+
crd.getMetadata().setResourceVersion(null);
148+
crd.getMetadata().setManagedFields(null);
149+
client.resource(crd).serverSideApply();
150+
log.info("Updated CRD with v2 as storage version");
151+
}
152+
153+
private OwnerRefMultiVersionCR1 createCR() {
154+
var cr = new OwnerRefMultiVersionCR1();
155+
cr.setMetadata(new ObjectMeta());
156+
cr.getMetadata().setName(CR_NAME);
157+
cr.setSpec(new OwnerRefMultiVersionSpec());
158+
cr.getSpec().setValue("initial");
159+
return cr;
160+
}
161+
}

0 commit comments

Comments
 (0)