fix: do not NPE when adding a finalizer to a resource deleted mid-retry - #3530
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents a NullPointerException when addFinalizer is executed through the conflict-retry patch path and the primary resource is deleted between retries (i.e., the re-read get() returns null). This makes addFinalizer behave consistently with existing removeFinalizer handling in ResourceOperations.
Changes:
- Add a null-guard to the
addFinalizerprecondition inResourceOperationsto avoid dereferencing a deleted resource during retry. - Add the same null-guard to the deprecated
PrimaryUpdateAndCacheUtils.addFinalizerretry precondition (and log when the resource is gone).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java | Adds null-guarded precondition for addFinalizer during conflict-retry patch. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java | Adds null-guarded precondition for deprecated addFinalizer during conflict-retry patch. |
`conflictRetryingPatchPrimary` / `conflictRetryingPatch` re-read the
resource from the API server after a 409 or 422 and then re-evaluate the
precondition:
resource = operation.inNamespace(ns).withName(name).get();
`get()` returns null if the resource was deleted in the meantime, so the
next iteration calls the precondition with null. `removeFinalizer`
anticipates this:
r -> {
if (r == null) {
log.warn("Cannot remove finalizer since resource not exists.");
return false;
}
return r.hasFinalizer(finalizerName);
}
but `addFinalizer` passes `r -> !r.hasFinalizer(finalizerName)`, which
throws a NullPointerException instead of exiting cleanly.
Gives `addFinalizer` the same null guard, in both `ResourceOperations` and
the deprecated `PrimaryUpdateAndCacheUtils`.
No test is added: reaching the retry path requires stubbing the client to
answer 409/422 and then 404 through the whole fabric8 DSL chain, which the
existing unit tests are not set up for.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
e92fb53 to
3b0a833
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java:1094
addFinalizercan now returnnullwhen the resource is deleted mid-retry (because the retry loop re-fetches with.get()which may return null, and the precondition returns false in that case). The Javadoc currently says it returns the updated resource, which no longer fully describes the behavior; please document the possiblenullreturn so callers know they must handle it.
r -> {
if (r == null) {
log.warn("Cannot add finalizer since resource no longer exists.");
return false;
}
return !r.hasFinalizer(finalizerName);
},
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java:298
addFinalizercan now returnnullwhen the resource is deleted mid-retry (the retry loop re-fetches with.get()which may return null, and the precondition returns false). The Javadoc currently states it returns the updated resource, which is no longer complete; please document the possiblenullreturn value for callers.
r -> {
if (r == null) {
log.warn("Cannot add finalizer since resource no longer exists.");
return false;
}
return !r.hasFinalizer(finalizerName);
});
conflictRetryingPatchPrimary/conflictRetryingPatchre-read theresource from the API server after a 409 or 422 and then re-evaluate the
precondition:
get()returns null if the resource was deleted in the meantime, so thenext iteration calls the precondition with null.
removeFinalizeranticipates this:
but
addFinalizerpassesr -> !r.hasFinalizer(finalizerName), whichthrows a NullPointerException instead of exiting cleanly.
Gives
addFinalizerthe same null guard, in bothResourceOperationsandthe deprecated
PrimaryUpdateAndCacheUtils.No test is added: reaching the retry path requires stubbing the client to
answer 409/422 and then 404 through the whole fabric8 DSL chain, which the
existing unit tests are not set up for.
Part of #3517