From 71e7af246fa08899149f6324b992afd6ba1266c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 29 Jul 2026 18:25:25 +0200 Subject: [PATCH] fix: assorted robustness and clarity fixes Three unrelated small items found while auditing. ManagedInformerEventSource.get could throw instead of falling back to the informer cache. It compared a temporary-cache hit against `manager().lastSyncResourceVersion(namespace)` without guarding either failure mode: * `lastSyncResourceVersion` is null until an informer has completed its initial list, and `compareResourceVersions` dereferences it, so this threw a NullPointerException. * `InformerManager.lastSyncResourceVersion` does `getSource(ns) .orElseThrow()`, which throws `NoSuchElementException` for a namespace that is no longer watched after a dynamic namespace change - a window that only closes on the next re-list, when `checkGhostResources` prunes the entry. `TemporaryResourceCache.putResource` already guards the null case; the comparison is now extracted into a helper that guards both and falls back to the informer cache, which is the safe answer in either case. ExpectationResult.name() threw a bare NullPointerException when no expectation was registered, which is a normal result state produced by `ExpectationManager.checkExpectation`. It now throws `IllegalStateException` pointing at `isExpectationPresent()`, and says so in the javadoc. ResourceOperations cleanups: drops the `options` parameter of `desiredForJsonPatch`, which was never read at any of its six call sites, and rewrites the comment on `create` - it claimed the operation "check if the resource already exists", which it does not; the actual reason filtering is safe is that the API server rejects a duplicate create. --- .../api/reconciler/ResourceOperations.java | 20 ++++++++------- .../informer/ManagedInformerEventSource.java | 25 ++++++++++++++++--- .../expectation/ExpectationResult.java | 9 +++++++ 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java index c4532aa284..ac3f40960e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java @@ -406,7 +406,9 @@ public R update( * @return the created resource as returned by the API server */ public R create(R resource) { - // it is safe to do event filtering for create since check if the resource already exists. + // filtering the own event is safe here without optimistic locking or a matcher: a create either + // succeeds, in which case the event we filter is unambiguously ours, or the API server rejects + // it because the resource already exists. return create(resource, Options.forceFilterEvents()); } @@ -445,7 +447,7 @@ public R create( if (informerEventSource == null) { return create(resource); } - // it is safe to do event filtering for create since check if the resource already exists. + // see the note on create(R) about why filtering the own event is safe here return resourcePatch( resource, r -> context.getClient().resource(r).create(), informerEventSource, options); } @@ -556,7 +558,7 @@ public R jsonPatch(R actualResource, UnaryOperator un */ public R jsonPatch( R actualResource, UnaryOperator unaryOperator, Options options) { - R desired = desiredForJsonPatch(actualResource, unaryOperator, options); + R desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -580,7 +582,7 @@ public R jsonPatch( UnaryOperator unaryOperator, InformerEventSource informerEventSource, Options options) { - R desired = desiredForJsonPatch(actualResource, unaryOperator, options); + R desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -620,7 +622,7 @@ public R jsonPatchStatus( */ public R jsonPatchStatus( R actualResource, UnaryOperator unaryOperator, Options options) { - R desired = desiredForJsonPatch(actualResource, unaryOperator, options); + R desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -645,7 +647,7 @@ public R jsonPatchStatus( UnaryOperator unaryOperator, InformerEventSource informerEventSource, Options options) { - R desired = desiredForJsonPatch(actualResource, unaryOperator, options); + R desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -680,7 +682,7 @@ public P jsonPatchPrimary(P actualResource, UnaryOperator

unaryOperator) { * @return the patched resource as returned by the API server */ public P jsonPatchPrimary(P actualResource, UnaryOperator

unaryOperator, Options options) { - P desired = desiredForJsonPatch(actualResource, unaryOperator, options); + P desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -717,7 +719,7 @@ public P jsonPatchPrimaryStatus(P actualResource, UnaryOperator

unaryOperator */ public P jsonPatchPrimaryStatus( P actualResource, UnaryOperator

unaryOperator, Options options) { - P desired = desiredForJsonPatch(actualResource, unaryOperator, options); + P desired = desiredForJsonPatch(actualResource, unaryOperator); return resourcePatch( desired, actualResource, @@ -1435,7 +1437,7 @@ public enum Mode { } private T desiredForJsonPatch( - T actualResource, UnaryOperator unaryOperator, Options options) { + T actualResource, UnaryOperator unaryOperator) { var cloned = context .getControllerConfiguration() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 8352bef665..3cebdbf9a8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -224,10 +224,7 @@ public Optional get(ResourceID resourceID) { Optional resource = temporaryResourceCache.getResourceFromCache(resourceID); if (comparableResourceVersions && resource.isPresent() - && ReconcilerUtilsInternal.compareResourceVersions( - resource.get().getMetadata().getResourceVersion(), - manager().lastSyncResourceVersion(resource.get().getMetadata().getNamespace())) - > 0) { + && isLaterThanLastSyncResourceVersion(resource.orElseThrow())) { log.debug("Latest resource found in temporary cache for Resource ID: {}", resourceID); return resource; } else { @@ -242,6 +239,26 @@ public Optional get(ResourceID resourceID) { } } + /** + * A resource from the temporary cache is only preferred over the informer cache if we can tell + * that it is newer. The last sync resource version is not available before an informer has + * completed its initial list, and the namespace might not be watched anymore after a dynamic + * namespace change, in both of which cases the informer cache is used instead. + */ + private boolean isLaterThanLastSyncResourceVersion(R resource) { + var namespace = resource.getMetadata().getNamespace(); + if (!manager().isWatchingNamespace(namespace)) { + return false; + } + var lastSyncResourceVersion = manager().lastSyncResourceVersion(namespace); + if (lastSyncResourceVersion == null) { + return false; + } + return ReconcilerUtilsInternal.compareResourceVersions( + resource.getMetadata().getResourceVersion(), lastSyncResourceVersion) + > 0; + } + /** * @deprecated Use {@link #get(ResourceID)} instead. */ diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/expectation/ExpectationResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/expectation/ExpectationResult.java index 5b4081f7b3..e2981d7834 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/expectation/ExpectationResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/expectation/ExpectationResult.java @@ -36,7 +36,16 @@ public boolean isNotPresentOrFulfilled() { return !isExpectationPresent() || isFulfilled(); } + /** + * @return the name of the expectation this result refers to + * @throws IllegalStateException if there is no expectation, check {@link #isExpectationPresent()} + * first + */ public String name() { + if (expectation == null) { + throw new IllegalStateException( + "No expectation present for this result, check isExpectationPresent() first."); + } return expectation.name(); } }