Repository navigation
fix: scope exceptions by framework - #188
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Signed-off-by: Namanbhatt-01 <the.namanbhatt109@gmail.com>
b72c923 to
01d62a8
Compare
matthyx
left a comment
There was a problem hiding this comment.
The filters.go change itself (trimming PosturePolicies per-exception instead of keeping/dropping the whole exception, and fixing the duplicate-append bug) is a solid, well-tested fix. But the overall approach doesn't achieve the PR's stated goal end-to-end, and I'd like that resolved before merging — two inline comments plus the bigger integration issue below.
Critical: the real production pipeline never reaches the new per-framework check.
rules.go#GetStatus now only re-evaluates exceptions dynamically when rule.Status == apis.StatusFailed. But in kubescape's actual pipeline (core/pkg/opaprocessor/processorhandlerutils.go: updateResults), exceptions are applied first, via Result.SetExceptions → control.setExceptions → rule.SetStatus(rule.GetStatus(nil).Status(), nil) (both calls always pass f = nil, unchanged by this PR). Since f == nil there, SetStatus's f == nil branch still unconditionally overwrites rule.Status to Passed (+ SubStatusException) as soon as any exception is attached — with no framework awareness, because FilterExceptions is only framework-aware when it's actually given a populated Filters.
Only after that does SummaryDetails.InitResourcesSummary build the per-framework rollup by calling control.GetStatus(f) with a real per-framework filter (confirmed via summarydetails.go / frameworksummarymethods.go). By that point rule.Status is already Passed, not Failed, so the new rule.Status == apis.StatusFailed guard in GetStatus(f) can never fire — the dynamic per-framework recheck this PR adds is unreachable through the real flow, and the original bug ("exception in one framework suppresses a rule failure in another framework") will still reproduce end-to-end.
The new unit tests don't catch this because they construct ResourceAssociatedRule/ResourceAssociatedControl via struct literals with Status: StatusFailed set directly, bypassing SetStatus entirely — so they never exercise the code path that actually runs in production.
To really fix this, the exception-baking step in exceptions.go (rule.SetStatus(rule.GetStatus(nil).Status(), nil)) needs to stop permanently collapsing the raw failed status into Passed when no framework context is available — e.g. keep rule.Status as the framework-agnostic ground truth (Failed) and let GetStatus(f) be the sole place exceptions get applied, for both the f == nil and f != nil cases. As it stands, SetStatus's f == nil branch and GetStatus's new dynamic check are fighting each other. I'd like to see this validated with a test that goes through SetExceptions → per-framework GetStatus(f), not just direct struct construction, before this merges.
Also flagged two more issues inline:
rules.goSetStatus'sf != nilmatching-exception branch silently no-ops instead of persisting anything, which can leaverule.Statusempty in theisOldControlbackward-compat path.controls.go's newf != nilbranch drops theactionRequired(manual review / requires review / configuration) skip semantics thatcontrol.SetStatus(c)computes, since it recomputes purely from rule pass/fail.
Signed-off-by: Namanbhatt-01 <the.namanbhatt109@gmail.com>
matthyx
left a comment
There was a problem hiding this comment.
Both blockers from the previous review are resolved in b8930c3:
- Exception baking removed.
exceptions.gono longer callsrule.SetStatus(rule.GetStatus(nil).Status(), nil)after attaching exceptions, sorule.Statuskeeps its rawFailedground truth instead of being permanently collapsed toPassed.GetStatus(f)is now the single place exceptions get applied, dynamically, for bothf == nilandf != nil. Confirmed byTestSetExceptionsKeepsRuleStatusForFrameworkScopedEvaluation, which goes through the realResult.SetExceptions()entry point (the actual call kubescape's pipeline uses) and checks the NSA/MITRE split holds afterward — this is exactly the integration path that was previously unreachable. actionRequiredskip semantics preserved. The newcontrol.actionRequiredfield +applyActionRequiredStatus()correctly reapplies requires-review/manual-review/configuration-skip on top of the per-framework rule aggregate, instead of bypassing it.TestFilteredControlStatusPreservesActionRequiredcovers all three action-required kinds against a matching vs. non-matching framework and checks both status and sub-status.SetStatuson the rule is now a plainrule.Status = s(no more silent no-op branch), andGetStatus/GetSubStatusno longer mutate rule state as a side effect (TestOldControlFrameworkStatusDoesNotMutateRuleschecks this explicitly) — also resolves theisOldControlconcern from the last round.
Nit (non-blocking): ResourceAssociatedRule.SubStatus/GetSubStatus() (the raw field, not the interface method returned by GetStatus()) is no longer written anywhere, so it's effectively dead now that exceptions are applied dynamically — worth a follow-up cleanup, but I checked kubescape's only external consumer (httphandler/storage/apiserver.go) and it already reads through rule.GetStatus(f).GetSubStatus(), not the raw field, so there's no behavioral regression.
Approving.
Summary
Tests
go test -count=1 ./...go test -count=1 ./reporthandling/helpers/v1/...git diff --checkAll checks pass.