Skip to content

Commit 148dcb6

Browse files
csviricsviri
authored
fix: patchResourceAndStatus on status patch should generate diff only from status (#2973)
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
1 parent 57b2400 commit 148dcb6

File tree

5 files changed

+119
-26
lines changed

5 files changed

+119
-26
lines changed

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

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ public class ReconcilerUtils {
2828
protected static final String MISSING_GROUP_SUFFIX = ".javaoperatorsdk.io";
2929
private static final String GET_SPEC = "getSpec";
3030
private static final String SET_SPEC = "setSpec";
31+
private static final String SET_STATUS = "setStatus";
32+
private static final String GET_STATUS = "getStatus";
3133
private static final Pattern API_URI_PATTERN =
3234
Pattern.compile(".*http(s?)://[^/]*/api(s?)/(\\S*).*"); // NOSONAR: input is controlled
3335

@@ -135,11 +137,23 @@ public static Object getSpec(HasMetadata resource) {
135137
return cr.getSpec();
136138
}
137139

140+
return getSpecOrStatus(resource, GET_SPEC);
141+
}
142+
143+
public static Object getStatus(HasMetadata resource) {
144+
// optimize CustomResource case
145+
if (resource instanceof CustomResource cr) {
146+
return cr.getStatus();
147+
}
148+
return getSpecOrStatus(resource, GET_STATUS);
149+
}
150+
151+
private static Object getSpecOrStatus(HasMetadata resource, String getMethod) {
138152
try {
139-
Method getSpecMethod = resource.getClass().getMethod(GET_SPEC);
153+
Method getSpecMethod = resource.getClass().getMethod(getMethod);
140154
return getSpecMethod.invoke(resource);
141155
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
142-
throw noSpecException(resource, e);
156+
throw noMethodException(resource, e, getMethod);
143157
}
144158
}
145159

@@ -151,31 +165,46 @@ public static Object setSpec(HasMetadata resource, Object spec) {
151165
return null;
152166
}
153167

168+
return setSpecOrStatus(resource, spec, SET_SPEC);
169+
}
170+
171+
@SuppressWarnings("unchecked")
172+
public static Object setStatus(HasMetadata resource, Object status) {
173+
// optimize CustomResource case
174+
if (resource instanceof CustomResource cr) {
175+
cr.setStatus(status);
176+
return null;
177+
}
178+
return setSpecOrStatus(resource, status, SET_STATUS);
179+
}
180+
181+
private static Object setSpecOrStatus(
182+
HasMetadata resource, Object spec, String setterMethodName) {
154183
try {
155184
Class<? extends HasMetadata> resourceClass = resource.getClass();
156185

157186
// if given spec is null, find the method just using its name
158-
Method setSpecMethod;
187+
Method setMethod;
159188
if (spec != null) {
160-
setSpecMethod = resourceClass.getMethod(SET_SPEC, spec.getClass());
189+
setMethod = resourceClass.getMethod(setterMethodName, spec.getClass());
161190
} else {
162-
setSpecMethod =
191+
setMethod =
163192
Arrays.stream(resourceClass.getMethods())
164-
.filter(method -> SET_SPEC.equals(method.getName()))
193+
.filter(method -> setterMethodName.equals(method.getName()))
165194
.findFirst()
166-
.orElseThrow(() -> noSpecException(resource, null));
195+
.orElseThrow(() -> noMethodException(resource, null, setterMethodName));
167196
}
168197

169-
return setSpecMethod.invoke(resource, spec);
198+
return setMethod.invoke(resource, spec);
170199
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
171-
throw noSpecException(resource, e);
200+
throw noMethodException(resource, e, setterMethodName);
172201
}
173202
}
174203

175-
private static IllegalStateException noSpecException(
176-
HasMetadata resource, ReflectiveOperationException e) {
204+
private static IllegalStateException noMethodException(
205+
HasMetadata resource, ReflectiveOperationException e, String methodName) {
177206
return new IllegalStateException(
178-
"No spec found on resource " + resource.getClass().getName(), e);
207+
"No method: " + methodName + " found on resource " + resource.getClass().getName(), e);
179208
}
180209

181210
public static <T> T loadYaml(Class<T> clazz, Class loader, String yaml) {

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import io.fabric8.kubernetes.client.dsl.base.PatchContext;
1717
import io.fabric8.kubernetes.client.dsl.base.PatchType;
1818
import io.javaoperatorsdk.operator.OperatorException;
19+
import io.javaoperatorsdk.operator.ReconcilerUtils;
1920
import io.javaoperatorsdk.operator.api.config.Cloner;
2021
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
2122
import io.javaoperatorsdk.operator.api.reconciler.BaseControl;
@@ -477,6 +478,7 @@ public R patchStatus(R resource, R originalResource) {
477478
}
478479
}
479480

481+
@SuppressWarnings("unchecked")
480482
private R editStatus(R resource, R originalResource) {
481483
String resourceVersion = resource.getMetadata().getResourceVersion();
482484
// the cached resource should not be changed in any circumstances
@@ -486,7 +488,11 @@ private R editStatus(R resource, R originalResource) {
486488
clonedOriginal.getMetadata().setResourceVersion(null);
487489
resource.getMetadata().setResourceVersion(null);
488490
var res = resource(clonedOriginal);
489-
return res.editStatus(r -> resource);
491+
return res.editStatus(
492+
r -> {
493+
ReconcilerUtils.setStatus(r, ReconcilerUtils.getStatus(resource));
494+
return r;
495+
});
490496
} finally {
491497
// restore initial resource version
492498
clonedOriginal.getMetadata().setResourceVersion(resourceVersion);

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.fabric8.kubernetes.api.model.apps.Deployment;
99
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
1010
import io.fabric8.kubernetes.api.model.apps.DeploymentSpec;
11+
import io.fabric8.kubernetes.api.model.apps.DeploymentStatus;
1112
import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
1213
import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
1314
import io.fabric8.kubernetes.client.CustomResource;
@@ -116,6 +117,29 @@ void setsSpecCustomResourceWithReflection() {
116117
assertThat(tomcat.getSpec().getReplicas()).isEqualTo(1);
117118
}
118119

120+
@Test
121+
void setsStatusWithReflection() {
122+
Deployment deployment = new Deployment();
123+
DeploymentStatus status = new DeploymentStatus();
124+
status.setReplicas(2);
125+
126+
ReconcilerUtils.setStatus(deployment, status);
127+
128+
assertThat(deployment.getStatus().getReplicas()).isEqualTo(2);
129+
}
130+
131+
@Test
132+
void getsStatusWithReflection() {
133+
Deployment deployment = new Deployment();
134+
DeploymentStatus status = new DeploymentStatus();
135+
status.setReplicas(2);
136+
deployment.setStatus(status);
137+
138+
var res = ReconcilerUtils.getStatus(deployment);
139+
140+
assertThat(((DeploymentStatus) res).getReplicas()).isEqualTo(2);
141+
}
142+
119143
@Test
120144
void loadYamlAsBuilder() {
121145
DeploymentBuilder builder =

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAIT.java‎

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.javaoperatorsdk.operator.baseapi.patchresourceandstatusnossa;
22

3+
import java.util.Map;
34
import java.util.concurrent.TimeUnit;
45

56
import org.junit.jupiter.api.Test;
@@ -9,39 +10,62 @@
910
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
1011
import io.javaoperatorsdk.operator.support.TestUtils;
1112

13+
import static io.javaoperatorsdk.operator.baseapi.patchresourceandstatusnossa.PatchResourceAndStatusNoSSAReconciler.TEST_ANNOTATION;
14+
import static io.javaoperatorsdk.operator.baseapi.patchresourceandstatusnossa.PatchResourceAndStatusNoSSAReconciler.TEST_ANNOTATION_VALUE;
1215
import static org.assertj.core.api.Assertions.assertThat;
1316
import static org.awaitility.Awaitility.await;
1417

1518
class PatchResourceAndStatusNoSSAIT {
1619
@RegisterExtension
17-
LocallyRunOperatorExtension operator =
20+
LocallyRunOperatorExtension extension =
1821
LocallyRunOperatorExtension.builder()
1922
.withConfigurationService(o -> o.withUseSSAToPatchPrimaryResource(false))
2023
.withReconciler(PatchResourceAndStatusNoSSAReconciler.class)
2124
.build();
2225

2326
@Test
2427
void updatesSubResourceStatus() {
28+
extension
29+
.getReconcilerOfType(PatchResourceAndStatusNoSSAReconciler.class)
30+
.setRemoveAnnotation(false);
2531
PatchResourceAndStatusNoSSACustomResource resource = createTestCustomResource("1");
26-
operator.create(resource);
32+
extension.create(resource);
2733

2834
awaitStatusUpdated(resource.getMetadata().getName());
2935
// wait for sure, there are no more events
3036
TestUtils.waitXms(300);
3137

3238
PatchResourceAndStatusNoSSACustomResource customResource =
33-
operator.get(
39+
extension.get(
3440
PatchResourceAndStatusNoSSACustomResource.class, resource.getMetadata().getName());
3541

36-
assertThat(TestUtils.getNumberOfExecutions(operator)).isEqualTo(1);
42+
assertThat(TestUtils.getNumberOfExecutions(extension)).isEqualTo(1);
3743
assertThat(customResource.getStatus().getState())
3844
.isEqualTo(PatchResourceAndStatusNoSSAStatus.State.SUCCESS);
39-
assertThat(
40-
customResource
41-
.getMetadata()
42-
.getAnnotations()
43-
.get(PatchResourceAndStatusNoSSAReconciler.TEST_ANNOTATION))
44-
.isNotNull();
45+
assertThat(customResource.getMetadata().getAnnotations().get(TEST_ANNOTATION)).isNotNull();
46+
}
47+
48+
@Test
49+
void removeAnnotationCorrectlyUpdatesStatus() {
50+
extension
51+
.getReconcilerOfType(PatchResourceAndStatusNoSSAReconciler.class)
52+
.setRemoveAnnotation(true);
53+
PatchResourceAndStatusNoSSACustomResource resource = createTestCustomResource("1");
54+
resource.getMetadata().setAnnotations(Map.of(TEST_ANNOTATION, TEST_ANNOTATION_VALUE));
55+
extension.create(resource);
56+
57+
awaitStatusUpdated(resource.getMetadata().getName());
58+
// wait for sure, there are no more events
59+
TestUtils.waitXms(300);
60+
61+
PatchResourceAndStatusNoSSACustomResource customResource =
62+
extension.get(
63+
PatchResourceAndStatusNoSSACustomResource.class, resource.getMetadata().getName());
64+
65+
assertThat(TestUtils.getNumberOfExecutions(extension)).isEqualTo(1);
66+
assertThat(customResource.getStatus().getState())
67+
.isEqualTo(PatchResourceAndStatusNoSSAStatus.State.SUCCESS);
68+
assertThat(customResource.getMetadata().getAnnotations().get(TEST_ANNOTATION)).isNull();
4569
}
4670

4771
void awaitStatusUpdated(String name) {
@@ -50,7 +74,7 @@ void awaitStatusUpdated(String name) {
5074
.untilAsserted(
5175
() -> {
5276
PatchResourceAndStatusNoSSACustomResource cr =
53-
operator.get(PatchResourceAndStatusNoSSACustomResource.class, name);
77+
extension.get(PatchResourceAndStatusNoSSACustomResource.class, name);
5478
assertThat(cr).isNotNull();
5579
assertThat(cr.getStatus()).isNotNull();
5680
assertThat(cr.getStatus().getState())

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/patchresourceandstatusnossa/PatchResourceAndStatusNoSSAReconciler.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ public class PatchResourceAndStatusNoSSAReconciler
2222
public static final String TEST_ANNOTATION_VALUE = "TestAnnotationValue";
2323
private final AtomicInteger numberOfExecutions = new AtomicInteger(0);
2424

25+
private volatile boolean removeAnnotation = false;
26+
2527
@Override
2628
public UpdateControl<PatchResourceAndStatusNoSSACustomResource> reconcile(
2729
PatchResourceAndStatusNoSSACustomResource resource,
@@ -30,8 +32,12 @@ public UpdateControl<PatchResourceAndStatusNoSSACustomResource> reconcile(
3032

3133
log.info("Value: " + resource.getSpec().getValue());
3234

33-
resource.getMetadata().setAnnotations(new HashMap<>());
34-
resource.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE);
35+
if (removeAnnotation) {
36+
resource.getMetadata().getAnnotations().remove(TEST_ANNOTATION);
37+
} else {
38+
resource.getMetadata().setAnnotations(new HashMap<>());
39+
resource.getMetadata().getAnnotations().put(TEST_ANNOTATION, TEST_ANNOTATION_VALUE);
40+
}
3541
ensureStatusExists(resource);
3642
resource.getStatus().setState(PatchResourceAndStatusNoSSAStatus.State.SUCCESS);
3743

@@ -49,4 +55,8 @@ private void ensureStatusExists(PatchResourceAndStatusNoSSACustomResource resour
4955
public int getNumberOfExecutions() {
5056
return numberOfExecutions.get();
5157
}
58+
59+
public void setRemoveAnnotation(boolean removeAnnotation) {
60+
this.removeAnnotation = removeAnnotation;
61+
}
5262
}

0 commit comments

Comments
 (0)