feat(apis): add SubStatusNotEvaluated for missing-GVR skips#166
Conversation
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new skipped sub-status ChangesNotEvaluated sub-status introduction and propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reporthandling/results/v1/reportsummary/controlsummarymethods.go (1)
65-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign skipped-substatus precedence with
CompareStatusAndSubStatus(and handleintegration).Line 72 onward omits
SubStatusIntegration, andmanual reviewis checked beforerequires review. That diverges fromreporthandling/apis/statuses.go(CompareStatusAndSubStatus) and can yield inconsistent aggregate substatuses across code paths.Suggested patch
case apis.StatusSkipped: if subStatus == apis.SubStatusNotEvaluated || controlSummary.StatusInfo.SubStatus == apis.SubStatusNotEvaluated { controlSummary.StatusInfo.SubStatus = apis.SubStatusNotEvaluated controlSummary.StatusInfo.InnerInfo = string(apis.SubStatusNotEvaluatedInfo) } else if subStatus == apis.SubStatusConfiguration || controlSummary.StatusInfo.SubStatus == apis.SubStatusConfiguration { controlSummary.StatusInfo.SubStatus = apis.SubStatusConfiguration controlSummary.StatusInfo.InnerInfo = string(apis.SubStatusConfigurationInfo) - } else if subStatus == apis.SubStatusManualReview || controlSummary.StatusInfo.SubStatus == apis.SubStatusManualReview { - controlSummary.StatusInfo.SubStatus = apis.SubStatusManualReview - controlSummary.StatusInfo.InnerInfo = string(apis.SubStatusManualReviewInfo) + } else if subStatus == apis.SubStatusIntegration || controlSummary.StatusInfo.SubStatus == apis.SubStatusIntegration { + controlSummary.StatusInfo.SubStatus = apis.SubStatusIntegration + controlSummary.StatusInfo.InnerInfo = "" } else if subStatus == apis.SubStatusRequiresReview || controlSummary.StatusInfo.SubStatus == apis.SubStatusRequiresReview { controlSummary.StatusInfo.SubStatus = apis.SubStatusRequiresReview controlSummary.StatusInfo.InnerInfo = string(apis.SubStatusRequiresReviewInfo) + } else if subStatus == apis.SubStatusManualReview || controlSummary.StatusInfo.SubStatus == apis.SubStatusManualReview { + controlSummary.StatusInfo.SubStatus = apis.SubStatusManualReview + controlSummary.StatusInfo.InnerInfo = string(apis.SubStatusManualReviewInfo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reporthandling/results/v1/reportsummary/controlsummarymethods.go` around lines 65 - 78, In the apis.StatusSkipped branch (case apis.StatusSkipped) the precedence and coverage for skipped substatuses diverges from CompareStatusAndSubStatus: add a branch to handle apis.SubStatusIntegration (set controlSummary.StatusInfo.SubStatus = apis.SubStatusIntegration and InnerInfo = string(apis.SubStatusIntegrationInfo)) and reorder the checks so apis.SubStatusRequiresReview is evaluated before apis.SubStatusManualReview to match CompareStatusAndSubStatus; keep comparisons against both the local subStatus variable and controlSummary.StatusInfo.SubStatus and update InnerInfo values for each substatus accordingly.
🧹 Nitpick comments (1)
reporthandling/apis/statuses_test.go (1)
33-38: ⚡ Quick winExpand precedence tests to cover
integrationandrequires review.Line 33 says “other skipped substatuses,” but current additions only assert configuration/manual-review cases. Add explicit checks against
SubStatusIntegrationandSubStatusRequiresReviewto lock the full precedence contract.Suggested test additions
// notEvaluated should win over other skipped substatuses since it signals a real cluster/RBAC gap assert.Equal(t, makeIS(StatusSkipped, SubStatusNotEvaluated), makeIS(CompareStatusAndSubStatus(StatusSkipped, StatusPassed, SubStatusNotEvaluated, SubStatusUnknown))) assert.Equal(t, makeIS(StatusSkipped, SubStatusNotEvaluated), makeIS(CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusNotEvaluated, SubStatusConfiguration))) assert.Equal(t, makeIS(StatusSkipped, SubStatusNotEvaluated), makeIS(CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusManualReview, SubStatusNotEvaluated))) + assert.Equal(t, makeIS(StatusSkipped, SubStatusNotEvaluated), makeIS(CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusIntegration, SubStatusNotEvaluated))) + assert.Equal(t, makeIS(StatusSkipped, SubStatusNotEvaluated), makeIS(CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusRequiresReview, SubStatusNotEvaluated))) // failed still beats notEvaluated assert.Equal(t, makeIS(StatusFailed, SubStatusUnknown), makeIS(CompareStatusAndSubStatus(StatusFailed, StatusSkipped, SubStatusUnknown, SubStatusNotEvaluated)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reporthandling/apis/statuses_test.go` around lines 33 - 38, Add tests asserting that SubStatusNotEvaluated wins over SubStatusIntegration and SubStatusRequiresReview by extending the existing assertions around CompareStatusAndSubStatus and makeIS: add two assertions similar to the existing ones that call CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusNotEvaluated, SubStatusIntegration) and CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusNotEvaluated, SubStatusRequiresReview) and expect makeIS(StatusSkipped, SubStatusNotEvaluated). Also add cross-order checks (e.g., CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusIntegration, SubStatusNotEvaluated)) to ensure precedence is symmetric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@reporthandling/results/v1/reportsummary/controlsummarymethods.go`:
- Around line 65-78: In the apis.StatusSkipped branch (case apis.StatusSkipped)
the precedence and coverage for skipped substatuses diverges from
CompareStatusAndSubStatus: add a branch to handle apis.SubStatusIntegration (set
controlSummary.StatusInfo.SubStatus = apis.SubStatusIntegration and InnerInfo =
string(apis.SubStatusIntegrationInfo)) and reorder the checks so
apis.SubStatusRequiresReview is evaluated before apis.SubStatusManualReview to
match CompareStatusAndSubStatus; keep comparisons against both the local
subStatus variable and controlSummary.StatusInfo.SubStatus and update InnerInfo
values for each substatus accordingly.
---
Nitpick comments:
In `@reporthandling/apis/statuses_test.go`:
- Around line 33-38: Add tests asserting that SubStatusNotEvaluated wins over
SubStatusIntegration and SubStatusRequiresReview by extending the existing
assertions around CompareStatusAndSubStatus and makeIS: add two assertions
similar to the existing ones that call CompareStatusAndSubStatus(StatusSkipped,
StatusSkipped, SubStatusNotEvaluated, SubStatusIntegration) and
CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusNotEvaluated,
SubStatusRequiresReview) and expect makeIS(StatusSkipped,
SubStatusNotEvaluated). Also add cross-order checks (e.g.,
CompareStatusAndSubStatus(StatusSkipped, StatusSkipped, SubStatusIntegration,
SubStatusNotEvaluated)) to ensure precedence is symmetric.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 699a9ea3-33b0-42a3-9a6f-2f71026c4d71
📒 Files selected for processing (3)
reporthandling/apis/statuses.goreporthandling/apis/statuses_test.goreporthandling/results/v1/reportsummary/controlsummarymethods.go
|
Hey @matthyx, kubescape/kubescape#2015 got merged (the part that populates |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
I found one blocker.
ResourceAssociatedControl.SetStatus() in reporthandling/results/v1/resourcesresults/controls.go can still overwrite an already-aggregated skipped/notEvaluated result with skipped/configuration when actionRequired == configuration and configs are missing. That contradicts the new precedence in CompareStatusAndSubStatus and can hide the RBAC/API/GVR collection gap this PR is trying to preserve.
Please preserve SubStatusNotEvaluated when it is already present in this path, and add a regression test for it.
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-checked after the follow-up commit.
The previous blocker in reporthandling/results/v1/resourcesresults/controls.go is fixed: SubStatusNotEvaluated is no longer overwritten by the configuration fallback, and the new regression test covers that path.
I still see the older reporthandling/results/v1/reportsummary/controlsummarymethods.go precedence divergence around integration / requires review / manual review, but that looks pre-existing and non-blocking for this PR.
2 participants
What this does
Adds a new
SubStatusNotEvaluatedskipped substatus so controls that get skipped because their required GVRs could not be collected (RBAC denied, missing CRDs, API errors) can be told apart from controls that were skipped for configuration / review reasons.Right now everything ends up as a generic
skipped, which makes it impossible for downstream consumers to filter on the actual cluster-gap case. Free-textinfois not enough since you can't filter on it cleanly.Why this is here and not in kubescape
This is the prerequisite piece for kubescape/kubescape#2010. The kubescape side wants to mark controls as
notEvaluatedwhen their GVRs fail to pull, but the substatus enum and the precedence logic both live here, so this has to land first.Once this is released, kubescape will bump the dep and start setting the substatus from its resource collection path.
Changes
reporthandling/apis/statuses.go: newSubStatusNotEvaluatedconstant +SubStatusNotEvaluatedInfomessage. UpdatedCompareStatusAndSubStatussonotEvaluatedwins over the other skipped substatuses, since a real cluster/RBAC gap is more actionable than a configuration or review skip.failedstill beatsnotEvaluated.reporthandling/results/v1/reportsummary/controlsummarymethods.go: extendedcalculateNSetSubStatusskipped branch to handle the new substatus when aggregating control summaries.reporthandling/apis/statuses_test.go: added precedence tests covering the new substatus against the existing skipped ones and against failed.Out of scope
InfoMapwork. Kept this PR to enum + precedence only so it's easy to review on its own.Notes
go test ./reporthandling/apis/... ./reporthandling/results/...Refs: kubescape/kubescape#2010
Summary by CodeRabbit
New Features
Improvements
Tests
Chores