Skip to content

Commit 1d53df0

Browse files
committed
consolidating the diffs
also adding a test that removes a field not present in the desired
1 parent 2fbd8a0 commit 1d53df0

File tree

3 files changed

+50
-111
lines changed

3 files changed

+50
-111
lines changed

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java

Lines changed: 22 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
11
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
22

3-
import java.util.ArrayList;
43
import java.util.Arrays;
54
import java.util.Collections;
65
import java.util.List;
7-
import java.util.Objects;
8-
import java.util.Optional;
9-
import java.util.Set;
106

117
import io.fabric8.kubernetes.api.model.ConfigMap;
128
import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -21,13 +17,12 @@ public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends H
2117
implements Matcher<R, P> {
2218

2319
private static final String SPEC = "/spec";
20+
private static final String METADATA = "/metadata";
2421
private static final String ADD = "add";
2522
private static final String OP = "op";
23+
private static final List<String> IGNORED_FIELDS = List.of("/apiVersion", "/kind", "/status");
2624
public static final String METADATA_LABELS = "/metadata/labels";
2725
public static final String METADATA_ANNOTATIONS = "/metadata/annotations";
28-
// without knowing the CRD we cannot ignore status as it may not be a subresource, so if it's
29-
// included we expect it to match
30-
private static Set<String> NOT_OTHER_FIELDS = Set.of(SPEC, "/metadata", "/apiVersion", "/kind");
3126

3227
private static final String PATH = "path";
3328
private static final String[] EMPTY_ARRAY = {};
@@ -188,100 +183,39 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
188183
"Equality should be false in case of ignore list provided");
189184
}
190185

191-
if (considerMetadata) {
192-
Optional<Result<R>> res =
193-
matchMetadata(desired, actualResource, labelsAndAnnotationsEquality, context);
194-
if (res.isPresent()) {
195-
return res.orElseThrow();
196-
}
197-
}
198-
199-
final var matched = match(actualResource, desired, specEquality, context, ignoredPaths);
200-
return Result.computed(matched, desired);
201-
}
202-
203-
private static <R extends HasMetadata> boolean match(R actual, R desired, boolean equality,
204-
Context<?> context,
205-
String[] ignoredPaths) {
206-
207186
final var kubernetesSerialization = context.getClient().getKubernetesSerialization();
208187
var desiredNode = kubernetesSerialization.convertValue(desired, JsonNode.class);
209-
var actualNode = kubernetesSerialization.convertValue(actual, JsonNode.class);
188+
var actualNode = kubernetesSerialization.convertValue(actualResource, JsonNode.class);
210189
var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
211190

212-
final List<String> ignoreList =
213-
ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths)
214-
: Collections.emptyList();
215-
boolean specMatch = match(equality, wholeDiffJsonPatch, ignoreList, SPEC);
216-
if (!specMatch) {
217-
return false;
218-
}
219-
// expect everything else to be equal
220-
var names = desiredNode.fieldNames();
221-
List<String> prefixes = new ArrayList<>();
222-
while (names.hasNext()) {
223-
String prefix = "/" + names.next();
224-
if (!NOT_OTHER_FIELDS.contains(prefix)) {
225-
prefixes.add(prefix);
191+
boolean matched = true;
192+
for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) {
193+
var node = wholeDiffJsonPatch.get(i);
194+
if (nodeIsChildOf(node, List.of(SPEC))) {
195+
matched = match(specEquality, node, ignoreList);
196+
} else if (nodeIsChildOf(node, List.of(METADATA))) {
197+
// conditionally consider labels and annotations
198+
if (considerMetadata
199+
&& nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) {
200+
matched = match(labelsAndAnnotationsEquality, node, Collections.emptyList());
201+
}
202+
} else if (!nodeIsChildOf(node, IGNORED_FIELDS)) {
203+
matched = match(true, node, ignoreList);
226204
}
227205
}
228-
return match(true, wholeDiffJsonPatch, ignoreList, prefixes.toArray(String[]::new));
206+
207+
return Result.computed(matched, desired);
229208
}
230209

231-
private static boolean match(boolean equality, JsonNode wholeDiffJsonPatch,
232-
final List<String> ignoreList, String... prefixes) {
233-
var diffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, prefixes);
234-
// In case of equality is set to true, no diffs are allowed, so we return early if diffs exist
235-
// On contrary (if equality is false), "add" is allowed for cases when for some
236-
// resources Kubernetes fills-in values into spec.
237-
if (diffJsonPatch.isEmpty()) {
238-
return true;
239-
}
210+
private static boolean match(boolean equality, JsonNode diff,
211+
final List<String> ignoreList) {
240212
if (equality) {
241213
return false;
242214
}
243215
if (!ignoreList.isEmpty()) {
244-
return allDiffsOnIgnoreList(diffJsonPatch, ignoreList);
245-
}
246-
return allDiffsAreAddOps(diffJsonPatch);
247-
}
248-
249-
private static boolean allDiffsOnIgnoreList(List<JsonNode> metadataJSonDiffs,
250-
List<String> ignoreList) {
251-
if (metadataJSonDiffs.isEmpty()) {
252-
return false;
253-
}
254-
return metadataJSonDiffs.stream().allMatch(n -> nodeIsChildOf(n, ignoreList));
255-
}
256-
257-
private static <R extends HasMetadata, P extends HasMetadata> Optional<Result<R>> matchMetadata(
258-
R desired,
259-
R actualResource,
260-
boolean labelsAndAnnotationsEquality, Context<P> context) {
261-
262-
if (labelsAndAnnotationsEquality) {
263-
final var desiredMetadata = desired.getMetadata();
264-
final var actualMetadata = actualResource.getMetadata();
265-
266-
final var matched =
267-
Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) &&
268-
Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels());
269-
if (!matched) {
270-
return Optional.of(Result.computed(false, desired));
271-
}
272-
} else {
273-
final var objectMapper = context.getClient().getKubernetesSerialization();
274-
var desiredNode = objectMapper.convertValue(desired, JsonNode.class);
275-
var actualNode = objectMapper.convertValue(actualResource, JsonNode.class);
276-
var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
277-
var metadataJSonDiffs = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch,
278-
METADATA_LABELS,
279-
METADATA_ANNOTATIONS);
280-
if (!allDiffsAreAddOps(metadataJSonDiffs)) {
281-
return Optional.of(Result.computed(false, desired));
282-
}
216+
return nodeIsChildOf(diff, ignoreList);
283217
}
284-
return Optional.empty();
218+
return ADD.equals(diff.get(OP).asText());
285219
}
286220

287221
static boolean nodeIsChildOf(JsonNode n, List<String> prefixes) {
@@ -293,29 +227,6 @@ static String getPath(JsonNode n) {
293227
return n.get(PATH).asText();
294228
}
295229

296-
static boolean allDiffsAreAddOps(List<JsonNode> metadataJSonDiffs) {
297-
if (metadataJSonDiffs.isEmpty()) {
298-
return true;
299-
}
300-
return metadataJSonDiffs.stream().allMatch(n -> ADD.equals(n.get(OP).asText()));
301-
}
302-
303-
public static List<JsonNode> getDiffsImpactingPathsWithPrefixes(JsonNode diffJsonPatch,
304-
String... prefixes) {
305-
if (prefixes != null && prefixes.length > 0) {
306-
var res = new ArrayList<JsonNode>();
307-
var prefixList = Arrays.asList(prefixes);
308-
for (int i = 0; i < diffJsonPatch.size(); i++) {
309-
var node = diffJsonPatch.get(i);
310-
if (nodeIsChildOf(node, prefixList)) {
311-
res.add(node);
312-
}
313-
}
314-
return res;
315-
}
316-
return Collections.emptyList();
317-
}
318-
319230
@Deprecated(forRemoval = true)
320231
public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
321232
KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import io.fabric8.kubernetes.api.model.ServiceAccountBuilder;
1111
import io.fabric8.kubernetes.api.model.apps.Deployment;
1212
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
13+
import io.fabric8.kubernetes.api.model.apps.DeploymentStatusBuilder;
1314
import io.javaoperatorsdk.operator.MockKubernetesClient;
1415
import io.javaoperatorsdk.operator.ReconcilerUtils;
1516
import io.javaoperatorsdk.operator.api.reconciler.Context;
@@ -79,6 +80,15 @@ void doesNotMatchChangedValues() {
7980
.isFalse();
8081
}
8182

83+
@Test
84+
void ignoreStatus() {
85+
actual = createDeployment();
86+
actual.setStatus(new DeploymentStatusBuilder().withReadyReplicas(1).build());
87+
assertThat(matcher.match(actual, null, context).matched())
88+
.withFailMessage("Should ignore status in actual")
89+
.isTrue();
90+
}
91+
8292
@Test
8393
void doesNotMatchChangedValuesWhenNoIgnoredPathsAreProvided() {
8494
actual = createDeployment();

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdaterMatcherTest.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.util.HashMap;
44
import java.util.List;
5+
import java.util.Map;
56

67
import org.junit.jupiter.api.BeforeAll;
78
import org.junit.jupiter.api.Test;
@@ -99,6 +100,23 @@ void checkSecret() {
99100
assertThat(secret.getData()).containsOnlyKeys("foo");
100101
}
101102

103+
@Test
104+
void checkSeviceAccount() {
105+
var processor = GenericResourceUpdaterMatcher.updaterMatcherFor(ServiceAccount.class);
106+
var desired = new ServiceAccountBuilder()
107+
.withMetadata(new ObjectMetaBuilder().addToLabels("new", "label").build())
108+
.build();
109+
var actual = new ServiceAccountBuilder()
110+
.withMetadata(new ObjectMetaBuilder().addToLabels("a", "label").build())
111+
.withImagePullSecrets(new LocalObjectReferenceBuilder().withName("secret").build())
112+
.build();
113+
114+
final var serviceAccount = processor.updateResource(actual, desired, context);
115+
assertThat(serviceAccount.getMetadata().getLabels())
116+
.isEqualTo(Map.of("a", "label", "new", "label"));
117+
assertThat(serviceAccount.getImagePullSecrets()).isNullOrEmpty();
118+
}
119+
102120
Deployment createDeployment() {
103121
return ReconcilerUtils.loadYaml(
104122
Deployment.class, GenericResourceUpdaterMatcherTest.class, "nginx-deployment.yaml");

0 commit comments

Comments
 (0)