Repository navigation
fix(cleanup): judge the default namespace by its own running workloads and return its errors - #439
Conversation
…s and return its errors Signed-off-by: Youssef Omar Bouden <youssef.bouden2002@gmail.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
matthyx
left a comment
There was a problem hiding this comment.
Approve: this is a needed, scoped fix for #438. Reviewed head 11e584109d96e7bd301c1e5065136745f1bbcd59 against main at 43023fbb5d7e123d20a5ee1f24b846a1a55b11e1.
No blocking findings in either changed file; independent code and architecture reviews agree (architecture CLEAR). On the target branch, CleanupTask skips the default namespace's discovery, supplies empty WLID/template-hash sets, and drops the final cleanup error. That deletes live default-namespace summaries/profiles and image/instance records found only there. The new code fetches that inventory, preserves its namespace-specific sets, unions image/instance IDs with the other namespaces, and returns failures. Existing deletion, watch, host-resource, and ContainerProfile paths remain consistent. No dependency, permission, public API, or storage-format changes. The additional namespace discovery can defer cluster cleanup on discovery failure; retaining potentially live records is the correct failure behavior.
History: merged #251 and #252 established namespace-by-namespace discovery and SQLite namespace enumeration. Their discussion does not reject fetching the default namespace. Merged #412 fixes standalone-Pod WLIDs; #435 fixes deleted Node posture records. Neither covers this defect. #432's scanner sweep handles stale non-workload records and complements this preservation fix. Closed #119 raised an author concern about deleting temporarily empty files, which this PR does not do; no explicit maintainer rejection was recorded. Closed benchmark-only #219 has no stated rejection rationale. Closure alone does not establish rejection.
Searched open, closed, and merged PRs and related issues using cleanup, CleanupTask, FetchResources, cleanup.go, default namespace, kubescape namespace, template hash, and records disappear (up to 100 indexed results per query). No duplicate or superseding fix found within those searches; this is not exhaustive proof. This PR had no human reviews or inline concerns at the final recheck.
Validation: all three new tests passed with go test -race ./pkg/registry/file/ -run 'TestCleanupTask(KeepsWhatRunsInTheDefaultNamespace|StopsWhenTheDefaultNamespaceCannotBeFetched|ReportsAFailureInTheDefaultNamespace)$' -count=1 -v in a network-disabled, credential-free Bubblewrap sandbox. They cover live-record retention, stale-record deletion, fetch failure, and delete-error propagation. git diff --check and gofmt -d passed. CI for this head passed package tests, lint, cross-platform build, and CodeQL; perf-ab was skipped.
Limitations: no local full-suite or live-cluster run, and the new tests were not executed against the pre-fix code. The target-branch defect was verified by source tracing. A nonblocking coverage improvement would store a cluster record in kubescape whose image/instance runs only in another namespace, directly locking down cross-namespace aggregation.
|
Thanks for the review and the merge. |
Overview
CleanupTaskskipped the default namespace when it collected the running workloads, then cleaned that namespace with the other namespaces' image and instance ids and with empty template-hash and wlid sets. The default namespace is where the components run; when it is scanned, the records of its workloads (posture summaries, vulnerability manifest summaries, seccomp profiles) and the cluster level records of images and instances that run only there were removed on every tick and recreated by the next scan. The default namespace's cleanup error was also assigned and dropped, so a failed row delete there ended in "finished cleanup task".CleanupTasknow fetches the default namespace's resources last, adds its images and instances to the cluster level sets, cleans it with its own template hashes and wlids, and returns its errors: a fetch failure aborts the tick and a cleanup failure is returned, as for every other namespace.How to Test
TestCleanupTaskKeepsWhatRunsInTheDefaultNamespace: a per-namespace fetcher mock, records of a running image, instance, deployment (posture summary, vulnerability manifest summary, seccomp profile) and of a gone deployment in bothkubescapeandother; after the task only the gone deployments' summaries are missing and the fetcher was called for both namespaces. On main thekubescapenamespace's six records are all gone.TestCleanupTaskStopsWhenTheDefaultNamespaceCannotBeFetched: a failing fetch of the default namespace returns the error and leaves its records untouched. On main the fetch is never attempted and the records go.TestCleanupTaskReportsAFailureInTheDefaultNamespace: the first metadata row delete in the default namespace is interrupted through the package's write-statement hook; the task returns the error. On main it returns nil.Related issues/PRs: