fix(etcd): initialize ResourceVersion in NewObjectStoreCluster (#352)#353
Open
arpitjain099 wants to merge 2 commits into
Open
fix(etcd): initialize ResourceVersion in NewObjectStoreCluster (#352)#353arpitjain099 wants to merge 2 commits into
arpitjain099 wants to merge 2 commits into
Conversation
The cluster store's inner objectStoreNamespaced literal left the embedded *versioning.ResourceVersion nil, so the first UseResourceVersion call made during Create of a cluster-scoped report dereferenced a nil pointer and panicked at versioning.go:33. This crashed the apiserver request handler for any ClusterEphemeralReport/ClusterPolicyReport/ClusterReport on the etcd backend. NewObjectStoreNamespaced already sets this field; mirror it in the cluster constructor and add a regression test. Fixes kyverno#352 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
aerosouund
reviewed
Jun 29, 2026
| // NewObjectStoreCluster left the embedded *versioning.ResourceVersion nil. The | ||
| // first call to UseResourceVersion (made during Create of a cluster-scoped | ||
| // report) then dereferenced that nil pointer and panicked, taking down the | ||
| // apiserver request handler. See kyverno/reports-server#352. |
Member
There was a problem hiding this comment.
That comment is unneeded. Its fair to only leave the test and ensure the fix is present rather than leave the whole context in the comment on the test
Member
|
@arpitjain099 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #353 +/- ##
========================================
+ Coverage 0.00% 0.05% +0.05%
========================================
Files 33 33
Lines 19095 19096 +1
========================================
+ Hits 0 11 +11
+ Misses 19095 19085 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ion test Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Author
|
Removed the comment, the test and fix are unchanged. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #352.
Creating any cluster-scoped report against the etcd backend panics the apiserver with a nil pointer dereference. The cluster store wraps an
objectStoreNamespacedwhose embedded*versioning.ResourceVersionis left nil, so the firstUseResourceVersion()call during Create dereferences nil. The stack lands at versioning/versioning.go:33 via etcd/store.go:223, which matches the trace in the issue. The namespaced constructor already sets this field; the cluster one just forgot. Fix is to addResourceVersion: versioning.NewVersioning()inNewObjectStoreCluster, same as the namespaced path.Added a small regression test that calls
NewObjectStoreCluster(...).UseResourceVersion()(a nil KV client is fine, that path only touches the in-memory versioning struct). It panics before the fix and passes after.@aerosouund you said on the issue you'd leave a fix to whoever picked it up, so here's one. Happy to move or rename the test if you'd prefer it elsewhere. Thanks!