Repository navigation
Allow operator scans to exclude controls - #429
Conversation
Combine installation settings with request exclusions at the shared request builder. Store only request settings in recurring jobs so later configuration changes take effect when those jobs run. Signed-off-by: Harsh K. Singh <harshsinghrajpoot07@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This adds an optional control-exclusion setting for operator scans, and an empty setting keeps current behavior. The change is tested and documented, and no merge-blocking risk was found. External implementers of the config interface must add one method, which the README notes. Pre-merge checks |
|
matthyx
left a comment
There was a problem hiding this comment.
Request changes for reviewed head 23c3caf49ce2dffe7a8b37e958a2230e2ae3b1c2, targeting main at 54a3272fc3f2c63fd8f3b97d8ad4ec173f5322ed.
The feature is useful: discussion #3976 documents the CLI/operator gap for C-0069/C-0070. Current main deserializes into a shared request type without an exclusion field, so this setting cannot survive forwarding; custom frameworks are a workaround, not an equivalent operator setting.
HIGH — build-blocking dependency contract mismatch, mainhandler/kubescapehandlerhelper.go:74. The new reads/writes of PostScanRequest.ExcludeControls cannot compile against the unchanged opa-utils v0.0.285 pin (go.mod:29): its request definition has no such field. This affects every operator build, including installations with no exclusions. Please update the committed dependency to a released version containing the field and JSON tag, then test without a local replacement. This is consistent with the unfinished dependency work already acknowledged in the description.
The centralized merge before dispatch, copied configuration slices, blank rejection, and request-only scheduled persistence are sensible. Tests cover normalization, invalid input, replay after installation settings change, and continuous add/update/delete command construction. No additional confirmed security, concurrency, lifecycle, or scope blocker emerged from the two independent static reviews; architectural status is BLOCK solely for the shared contract above. Adding a method to exported IConfig also requires downstream implementations to update.
History: merged #398 established installation framework defaults; its earlier blank-target, continuous-scan intent, and duplicated-helper objections were explicitly resolved, and this PR retains those fixes. Merged #415 supplies the scheduled-request test extended here. #176 concerns security-framework defaulting; #420 concerns namespace filtering. None implements control exclusion. GitHub indexed searches across open/closed/merged operator PRs and issues used exclude controls, exclude-controls, excludeControls, exclusion, skip controls, custom framework, C-0069, kubelet scan, PostScanRequest, and kubescapehandlerhelper, capped at 100 results/query. No duplicate/superseding exclusion PR or relevant explicitly rejected closed approach was found; this is bounded search evidence, not proof of absence.
Validation: git diff --check passes; independent review found all seven changed Go files formatted. Targeted go test ./config ./mainhandler -run 'TestLoadControlExclusions|TestOperatorControlExclusions|TestContinuousControlExclusions|TestAgentRuntimeFrameworkRequestSurvivesScheduledTrigger' -count=1 was attempted in a container with read-only source, no host credentials or privileged access. The offline attempt failed during setup because dependencies were missing from its read-only cache; the writable-cache retry was interrupted before a result was collected. These are validation gaps, not observed test failures caused by this PR. No successful package test, full build, vet/lint, or installed-cluster run is claimed. Visible DCO, security, FOSSA and CodeRabbit contexts pass, but CodeRabbit explicitly skipped this draft and no Go build/test result is visible.
Before readiness, complete the shared API/scanner integration and demonstrate that excluded controls disappear from results and scoring in the installed workflow. The PR remains a draft; branch protection requires one approval. Verdict: request changes; not ready to merge.
Replay stored commands through the real HTTP dispatch path and confirm malformed exclusion lists never reach the scanner. Signed-off-by: Harsh K. Singh <harshsinghrajpoot07@gmail.com>
|
hi @matthyx, Thanks for catching this. you're right, the dependency version used by this PR doesn’t contain ExcludeControls, so the operator change won’t build with the committed dependency yet. The scanner change that applies the exclusions is also prepared, but I still need to open its PR. I’m keeping this operator PR as a draft until those pieces are connected and verified. |
|
One more update: the full Linux test suites now pass, and installed tests confirm that excluded controls disappear from the results and scoring. Removing the exclusions restores the original results. These checks still use temporary shared-library copies, so I’ll repeat them with the released dependency before marking this ready. I also noted your compatibility point about IConfig. I’ll document that external implementations need to add the new ExcludeControls() method. |
matthyx
left a comment
There was a problem hiding this comment.
Rechecked head 885a15d6f9594e94193176c88f4f177ef9d24366 against main at 54a3272fc3f2c63fd8f3b97d8ad4ec173f5322ed. Both independent review lanes still recommend blocking approval.
The only code delta since the previous review is mainhandler/control_exclusions_dispatch_test.go. It adds useful HTTP-dispatch coverage for changed/removed installation exclusions, preserved request exclusions, and malformed input rejection. No new blocker was found in that test.
The HIGH dependency blocker from the existing review remains: go.mod:29 still pins opa-utils v0.0.285, whose PostScanRequest lacks ExcludeControls, while mainhandler/kubescapehandlerhelper.go:74–87 accesses it. opa-utils #194 is now ready for review but remains unmerged. Please update to a released compatible dependency and rerun validation without temporary replacements.
The author-reported full Linux and installed tests improve the development evidence, but use patched libraries; I did not independently rerun them. This recheck used static source/API inspection and a clean gofmt check of the added test. Visible checks pass, but provide no Go build/test result. The draft and incomplete scanner integration still prevent approval.
The documented CLI/operator gap remains valid. Refreshed indexed searches (up to 100 results/query, across PR states, using exclude, PostScanRequest, and exclude controls) found no operator replacement. Related merged #398/#415 remain complementary; #398's prior concerns were explicitly resolved, not rejected approaches. Search results are bounded evidence, not proof of absence.
Verdict: request changes remains appropriate; not ready to merge. This update references the existing blocker rather than adding duplicate inline feedback. Open/draft status, head, main target, and reviews were rechecked before submission.
Signed-off-by: Harsh K. Singh <harshsinghrajpoot07@gmail.com>
|
Hi @matthyx, I’ve updated the operator to released opa-utils v0.0.315, which includes the field from #194, and documented the new method required by external IConfig implementations. The scanner counterpart is kubescape/kubescape#4116. Both PRs are now ready for review. Full Linux tests, go vet, and native builds pass using the committed dependencies, without temporary replacements. Installed tests confirm that installation and request exclusions combine correctly, affect results and scoring, and don’t leak into later requests. An automatic scheduled scan also passed, and the same saved schedule restored the original checks after installation exclusions were removed. Actual deletion requests carry the exclusions correctly. However, replaying the exact request with and without exclusions produces the same “no resources found” error in our test setup, so I’m not claiming a successful deletion assessment. |
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed c63d85949550ec64463a29dd873ffabc6d693069, targeting main now at 044b32e56057d26a65e7116141708b5efaa1ac31 (PR merge base 54a3272). The previous HIGH dependency blocker is resolved. No confirmed code blockers remain.
opa-utils #194 is merged, go.mod:29 pins released v0.0.315, the checksums are committed, and the released request type contains ExcludeControls with the correct JSON tag. README now documents the exported IConfig compatibility change. The only delta since my last review is this dependency update and documentation. Current main's admission changes do not overlap the feature. Independent code review recommends approval; architectural status is WATCH for release coordination.
Fresh isolated validation, with read-only source, no credentials/host socket, networking disabled during execution, GOWORK=off, GOFLAGS=-mod=readonly, Go 1.25.8, and no temporary dependency replacements:
go test -count=1 -p 2 ./config ./mainhandler -run 'TestLoadControlExclusions|TestOperatorControlExclusions|TestContinuousControlExclusions|TestControlExclusionsScheduledReplayAndInvalidRequestsAtDispatch|TestAgentRuntimeFrameworkRequestSurvivesScheduledTrigger'
Both packages PASS. This covers strict configuration decoding, union/normalization, HTTP dispatch and invalid-input rejection, schedule replay with changed/removed installation settings, and continuous add/modify/delete command construction. git diff --check and changed-Go-file formatting checks pass. The first cache-only attempt failed at dependency setup; dependencies were then downloaded separately before the successful offline run. I did not rerun the full suite, standalone vet/build, or installed cluster checks. The author reports those passing against committed dependencies; visible CI checks pass but do not supply a Go build/test result.
The feature remains needed for the documented CLI/operator gap. Refreshed all-state indexed operator PR/issue searches for excludeControls, exclude controls, and exclude-controls (100-result caps) found no superseding operator implementation. Prior #398/#415 context remains complementary; resolved concerns are not repeated. Searches are bounded, not proof of absence.
Remaining nonblocking release concern: scanner #4116 is open. Older scanners do not apply this field. Please identify the compatible scanner/operator image pairing and chart selection before publishing or enabling the feature. This is a rollout condition, not a new operator-code defect. Actual deletion assessment remains unproven in the reported fixture, although forwarding is tested.
Verdict: COMMENT / needs clarification on release coordination; the earlier request-changes finding is fixed. I am not submitting approval or retaining the old dependency objection as an unresolved defect. Open/non-draft state, head, current main, reviews, and inline comments were rechecked before submission.
|
Thanks for merging this, @matthyx. I have the Helm change prepared and will update and test it once releases containing both changes are available. |
|
@shipitdev which components are you waiting a release for? |
I meant the operator and Kubescape HTTP scanner images from these two PRs. I wasn’t sure which tags to use for the Helm change. Should I use existing tags or wait for new releases? |
I’ve seen similar requests about settings available in manual scans being missing from automatic scans, though I don’t remember where I first came across them.
The operator-specific reply in Kubescape discussion #3976 gives a concrete example: the CLI supports
--exclude-controls, but operator scans cannot pass that setting through. Seeing this encouraged me to work on bringing that option to automatic scans.This change adds an optional
excludeControlssetting. The operator passes it to Kubescape for scans started by a request, a schedule, or a resource change. Individual requests can add exclusions to the installation list. Leaving the setting empty keeps the current behavior.This would help users keep the same chosen checks across manual and automatic scans, without maintaining a separate custom framework. Excluding a check removes it from the assessment; it does not mark it as passing.
The operator now uses released
opa-utils v0.0.315, containing the shared field merged in opa-utils #194. Full uncached Linux tests,go vet ./..., and the native Linux build pass using the committed dependency files, without temporary replacements. These checks include configuration validation, request forwarding, saved schedule replay, and resource-change command construction. An independent agent reviewed the dependency selection, changes, and validation logs.Adding
ExcludeControls() []stringto the exportedconfig.IConfiginterface requires external implementations to add that method. Returning nil or an empty slice preserves behavior without installation exclusions; this is documented in the README.The scanner counterpart is kubescape #4116. Its full Linux suite, vet, and native HTTP scanner build also pass with the released dependency.
Installed tests use a three-control policy with two failing checks and one passing check. The normal scan scores 33.33; installation exclusions score 50; combined installation/request exclusions leave only the passing check and score 100. An empty next request restores the installation-only result, and a later direct scan restores all three checks. A naturally triggered CronJob completes with the expected exclusions. The same saved schedule restores all three checks after installation exclusions are removed and the operator restarts.
Actual DELETED-event forwarding is verified by capturing the exact HTTP request, deletion flag, exclusion list, scanner ID, and matching pod UID in the event log. Replaying that request with and without exclusions returns the same “no resources found” error in this fixture. This is a documented assessment limitation, not a successful deletion assessment.
This is ready for maintainer review. Compatible scanner/operator image releases and chart selection still need coordination before users can install the feature. Feedback on combining installation and per-request exclusions is welcome.
Summary by CodeRabbit
New Features
Bug Fixes