Repository navigation
Conversation
CalculatePostureReportV2 returned on the first framework with a zero worst-case score, so the frameworks after it and the summary were never scored. SetPostureReportComplianceScores then returned that error before setting any compliance score, leaving every framework at 0 and every control without a compliance score. Score the remaining frameworks and the summary, set the compliance scores, and return all framework errors joined at the end. Signed-off-by: KVR Vishnu <46535652+VISHNU0906@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Review of head 1b93c457 against main (a2dfde9a).
Verdict: approve.
Need. In main, CalculatePostureReportV2 returns on the first framework whose worst-case score is 0 and not all-irrelevant. This leaves later frameworks and SummaryDetails.Score unscored. SetPostureReportComplianceScores then returns before setting any compliance score. The problem is real and this is the same root-cause family as #158, which fixed only the all-irrelevant case.
History. I searched the PRs and issues in this repo for score, wcs and unscorable terms. I found no duplicate or superseding PR. #158 (merged) is the narrower predecessor. #164 and #186 are unrelated closed score PRs and raised no objection to this approach. The search tool has limits, so this is not proof that no overlap exists.
Soundness.
- The loop now collects errors and continues, and
errors.Joinreturns them at the end. The unscorable framework still gets score 0 and an error, so existing behaviour and the existing empty/skipped tests are preserved. SetPostureReportComplianceScoresnow sets all compliance scores and returns the error last.- Dropping the trailing
\nis sensible now that the messages are joined. - Callers that only check
err != nilare unaffected. Callers that previously saw scores unset on error now get them populated.
Blockers. None.
Validation.
- With the PR's
score.go:go vet ./score/passes andgo test -race ./score/passes. - With
score.goreverted tomainand the PR's tests kept, both new tests fail. The failures are control compliance scores being nil andempty-bmissing from the error. - I did not rebuild kubescape against this branch. The CI
buildcheck passes, and DCO and GitGuardian pass. - The CodeRabbit check was skipped, and it has no reviews on this PR.
I confirmed the PR was open, not a draft, and unchanged at that head just before submitting.
…able framework (#4750) * fix(score): bump opa-utils to v0.0.318 so one unscorable framework no longer blanks every score opa-utils v0.0.318 (kubescape/opa-utils#197) keeps scoring the remaining frameworks, the summary and all compliance scores when a framework has no weight, instead of returning on the first one. Add a test through ScoreWrapper.Calculate that fails on v0.0.317. Signed-off-by: KVR Vishnu <46535652+VISHNU0906@users.noreply.github.com> * test(score): check the namespace summary built from the new scores Signed-off-by: KVR Vishnu <46535652+VISHNU0906@users.noreply.github.com> --------- Signed-off-by: KVR Vishnu <46535652+VISHNU0906@users.noreply.github.com>
Hi @matthyx, I found this while reading the scoring code after #195.
CalculatePostureReportV2returns as soon as it hits a framework with a zero worst-case score that isn't all-irrelevant, for example a framework whose controls were all skipped (not evaluated, missing configuration, manual review). Two things follow:SummaryDetails.Scoreare never scoredSetPostureReportComplianceScoresreturns that error before setting any compliance score, so every framework stays at 0,SummaryDetails.ComplianceScoreis 0 and each control'sComplianceScorestays nil (GetComplianceScore()gives -1)Kubescape only logs "failed to calculate score" and carries on, so one framework like that (say, all of its controls marked not evaluated because the resources couldn't be listed) zeroes the compliance scores of the whole scan. The -1 gauges in kubescape/kubescape#4017 look like the same root cause, seen from the printer side.
This change keeps going after an unscorable framework, still sets all compliance scores, and returns the framework errors joined at the end. The unscorable framework still gets a score of 0 and an error, as before, so the existing empty and skipped framework tests pass unchanged. I also dropped the trailing
\nfrom that error message, since the messages are now joined.Tests:
score/unscorable_framework_test.goputs a skipped-only framework in front of the existing mock report and checks that the other frameworks, the summary and the control compliance scores come out the same as in the plain mock case, plus a case with two empty frameworks to check both are reported. Both fail on main.go vet ./...,go test -race ./...andgo build ./...pass, and golangci-lint shows no new findings. I also built kubescape against this branch and ran itsopaprocessor,scoreandcautilstests.