fix(api): follow continuation token when listing summaries (kubescape/storage#337) - #52
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a generic ChangesPaginated list aggregation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Summary:
|
The storage backend (kubescape/storage) applies a default page size of 500 when a client lists without an explicit limit and returns a continue token for the remaining objects. The exporter listed summaries with ResourceVersionFullSpec but never consumed that token, so on clusters with more than 500 scanned objects the Prometheus metrics were computed from only the first page: whole namespaces silently disappeared and per-namespace severity counters were wrong (kubescape/storage#337). Add a generic listAllPages helper that follows the continuation token across pages while preserving ResourceVersionFullSpec (set on ResourceVersion, which is allowed alongside a continue token), and route all four Get* list methods through it. Virtual resources that ignore list options return a single page, so they are unaffected. ConfigurationScanSummaryStorage.GetList (storage repo) is out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
da43e46 to
7a3d00d
Compare
matthyx
left a comment
There was a problem hiding this comment.
Review: no blockers
I traced the fix against the storage backend and apimachinery and it holds up. Summary of what I verified:
The load-bearing claim is correct. ValidateListOptions (apimachinery v0.35.0, pkg/apis/meta/internalversion/validation/validation.go) only forbids resourceVersionMatch + continue. Plain resourceVersion + continue is not rejected, so re-sending ResourceVersion: fullSpec on every page is accepted at the handler layer. The etcd3 store's "resource version is not allowed when using continue" guard does not apply here because kubescape/storage uses a custom StorageImpl, not etcd3.
The server contract matches the fix. In kubescape/storage pkg/registry/file/storage.go, GetListWithConn honors opts.Predicate.Continue and Limit, recognizes ResourceVersionFullSpec to return full specs on every page, and sets the continue token only when a page is exactly full (len(list) == Limit). So the boundary case where the total is an exact multiple of 500 terminates safely after one extra empty request.
No infinite-loop risk on the virtual resources. VulnerabilitySummaryStorage.GetList and ConfigurationScanSummaryStorage.GetList build a fresh aggregated list with no continue token, so GetContinue() returns "" and listAllPages resolves in a single iteration — consistent with the PR description.
Helper logic is sound. Token threading reads page.GetContinue() (current page, not the accumulating list), the first page is adopted by reference and later pages appended, and the aggregate's stale token is cleared before return. The unit tests cover multi-page threading, single-page, and error propagation.
Non-blocking notes (optional)
- Only
GetVulnerabilityManifestSummariesandGetWorkloadConfigurationScanSummaries(the real resources) actually benefit from this client-side pagination.GetVulnerabilitySummaries/GetConfigScanSummaries(virtual) still receive a server-side-truncated aggregation untilkubescape/storage#337lands — the PR says this, but a one-line code comment on those two methods pointing at #337 would save the next reader the trip. listAllPagesis intentionally unbounded (accumulates all objects in memory, which is the goal). Consider a debug-level log of the page count / total items on large clusters so a future "why is this slow / using memory" question has a breadcrumb. Not required.- Tests exercise the generic helper directly; the wiring of each
Get*method (correct append func +fullSpec) is type-checked but not asserted. Trivial to leave as-is.
LGTM — safe to merge.
|
Summary:
|
Problem
The storage backend (kubescape/storage) applies a hard-coded default page size of 500 when a client lists objects without an explicit limit, and returns a
metadata.continuetoken for the rest:api/api.golists summaries withResourceVersionFullSpecbut never consumed that continue token. On any cluster with more than 500 scanned objects (common in production), the exporter aggregated metrics from only the first page:kubescape_controls_total_*/ vulnerability metrics;high: 0via LIST while a direct GET returnedhigh: 91);Reported in kubescape/storage#337.
Fix
Add a generic
listAllPageshelper that follows the continuation token across pages, accumulating every object, and route all fourGet*list methods through it.ResourceVersionFullSpecis preserved on every page request so each page still carries the full object spec. It is set viaResourceVersion(notResourceVersionMatch), which is the only one allowed alongside a continue token —ValidateListOptionsin apimachinery forbidsResourceVersionMatch+Continue, but notResourceVersion+Continue.VulnerabilitySummaries,ConfigurationScanSummaries) ignore list options server-side and never return a continue token, so they resolve in a single iteration — unaffected.Scope
ConfigurationScanSummaryStorage.GetList(in the storage repo) is the server-side aggregation and is out of scope for this PR — it is tracked separately in kubescape/storage#337.Tests
Added
api/api_test.gocovering:ResourceVersionFullSpecre-sent on every page;go build ./...,go vet ./..., andgo test ./...all pass.Fixes part of kubescape/storage#337.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests