Repository navigation
fix(cleanup): remove posture records of deleted Nodes - #435
Conversation
The cleanup task kept the WorkloadConfigurationScan and summary records of a Node after the Node was deleted, because deleteByWlid only cleans pods and the six workload kinds. Use the node list the cleanup task already fetches: a Node record is removed when no Node with that name exists, and kept when the node list fails. Signed-off-by: Sarthak <sarthakshreshtha345@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 |
There was a problem hiding this comment.
🟢 Approval recommended
The scoped change preserves discovery-failure safeguards and has regression coverage, with no blocking issues identified; tests were not executed.
No open issues 🎉
What changed in this PR
Extends storage cleanup to remove posture scans and summaries for deleted Nodes, addressing the Node-specific portion of #432.
Changes:
- Checks Node posture records against existing Node names.
- Preserves records when Node discovery fails and leaves other resource kinds unchanged.
- Adds unit and integration regression coverage.
| File | Description |
|---|---|
| pkg/registry/file/cleanup_host.go | Adds Node-name-based posture cleanup. |
| pkg/registry/file/cleanup_host_test.go | Covers cleanup decisions and fallback behavior. |
| pkg/registry/file/cleanup_host_integration_test.go | Verifies record retention and deletion through cleanup. |
Review effort: Balanced
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Approve: this is a needed, scoped fix for the Node portion of #432. Reviewed b27cea11fd8104eac1cac90ba855c8a6a47532fc against main at 8db6c15a13d4e85dd37a59a05683f66f78d797fc.
No blocking findings across the three changed files; independent code and architecture reviews agree. The new branch matches canonical Node WLIDs against Node names, preserves records on failed discovery, and uses the existing deletion path. Machine IDs do not determine Node posture ownership; other resource kinds and user-managed exemptions retain their existing behavior. No new dependencies, permissions, public API or format changes.
History: #209 deferred host/Node cleanup rather than rejecting it; #410 added the Node inventory for host SBOMs/profiles, and #317 addressed Pods. These were merged and do not fix Node posture records. operator#443 handles delete events, complementing this periodic recovery when events are missed. #432's preferred scanner sweep is a broader proposal, not an implemented replacement or maintainer rejection of this scoped approach.
Searched open PRs plus closed/merged PRs and issues using cleanup, node, orphan, posture, WorkloadConfigurationScan, deleteByWlid and cleanup_host (100 results per query). No duplicate or superseding fix found within those indexed searches; this is not an exhaustive proof. Existing PR discussion contains one nonblocking bot review and no inline concerns.
Validation in a network-disabled, credential-free Bubblewrap sandbox (Go 1.26.0): go test ./pkg/registry/file/ -count=1 passed (70.004s), and go vet ./pkg/registry/file/ passed. Targeted cleanup/discovery tests also passed. Replacing only cleanup_host.go with the target branch version and running go test ./pkg/registry/file/ -run '^TestCleanupNodePostureRecords$' -count=1 failed exactly in deleted_node: both payloads and metadata incorrectly remained. Thus the regression fails before and passes after the fix. git diff --check and formatting checks passed.
Remaining limitation: cleanup uses a point-in-time Node snapshot, like the existing host cleanup; concurrent Node/report creation was not race-tested. No live-cluster validation or repository-wide test suite was run. GitHub currently reports successful DCO, GitGuardian and CodeRabbit status, but no unit-test/lint run for this head. These gaps do not change the approval verdict.
Overview
The cleanup task keeps the
WorkloadConfigurationScanandWorkloadConfigurationScanSummaryrecords of a deleted Node forever.deleteByWlidonly cleans thepodkind and the six workload kinds, so a record with the wlidwlid://cluster-<name>/namespace-/node-<node>is never removed. On clusters with node churn (Karpenter, autoscaling) these pile up: 10 of the 40 orphans in #432 are Nodes.Storage already lists Nodes on every cleanup run, for the host SBOMs and profiles. This PR uses the same node list for these records too: they are removed when no Node with that name exists. If the node list fails, they are kept, as the host resources already are. Other cluster-scoped kinds (Secrets, PVs, RBAC objects and so on) are left as they are, since storage cannot list them. That needs the larger change discussed in #432.
How to Test
go test ./pkg/registry/file/TestCleanupNodePostureRecordsruns the cleanup task over Node and Secret records of both kinds. The Node's records are removed after the Node is gone, and kept while it exists or when the node list fails. The Secret's records are kept either way. On main, the "deleted node" case fails because the records stay.TestHostResourceCleanupDecisiongains cases for both record kinds: an existing node, a deleted node, a machine ID that is not a node name, a failed node list, malformed wlids, and Pod or Secret wlids, which keep usingdeleteByWlid.Related issues/PRs: