Fix(compliance): Handle tailored profiles with enabled and disabled rules#19196
Fix(compliance): Handle tailored profiles with enabled and disabled rules#19196
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
effectiveRules := baseRules.Union(addedRules).Difference(removedRules)logic changes precedence between enabled and disabled rules (a rule present in both sets now ends up disabled); if this is intentional, consider adding a brief comment to document the expected precedence to avoid confusion for future changes. - Iterating over
effectiveRules(a set) may produce a non-deterministic rule order compared to the previous slice-based iteration; if rule order is used anywhere (e.g., for stable outputs or comparisons), consider sorting the keys or otherwise preserving a deterministic order.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `effectiveRules := baseRules.Union(addedRules).Difference(removedRules)` logic changes precedence between enabled and disabled rules (a rule present in both sets now ends up disabled); if this is intentional, consider adding a brief comment to document the expected precedence to avoid confusion for future changes.
- Iterating over `effectiveRules` (a set) may produce a non-deterministic rule order compared to the previous slice-based iteration; if rule order is used anywhere (e.g., for stable outputs or comparisons), consider sorting the keys or otherwise preserving a deterministic order.
## Individual Comments
### Comment 1
<location path="sensor/kubernetes/complianceoperator/dispatchers/complianceoperatortailoredprofiles.go" line_range="91-93" />
<code_context>
- })
- }
- for _, rule := range tailoredProfile.Spec.EnableRules {
+ effectiveRules := baseRules.Union(addedRules).Difference(removedRules)
+
+ for rule := range effectiveRules {
protoProfile.Rules = append(protoProfile.Rules, &storage.ComplianceOperatorProfile_Rule{
- Name: rule.Name,
</code_context>
<issue_to_address>
**issue (bug_risk):** Enable vs disable precedence has changed from "enable wins" to "disable wins" for the same rule name.
With the old logic we filtered out `removedRules` from the base, then always appended `EnableRules`, so an explicitly enabled rule “won” even if also disabled. With the new `(base ∪ added) − removed` logic, any rule in both `EnableRules` and `DisableRules` is now dropped. Please confirm whether this precedence change is intended per the Compliance Operator spec and existing consumers; if not, we should adjust the set operations to keep explicitly enabled rules.
</issue_to_address>
### Comment 2
<location path="sensor/kubernetes/complianceoperator/dispatchers/complianceoperatortailoredprofiles.go" line_range="93-85" />
<code_context>
- for _, rule := range tailoredProfile.Spec.EnableRules {
+ effectiveRules := baseRules.Union(addedRules).Difference(removedRules)
+
+ for rule := range effectiveRules {
protoProfile.Rules = append(protoProfile.Rules, &storage.ComplianceOperatorProfile_Rule{
- Name: rule.Name,
+ Name: rule,
</code_context>
<issue_to_address>
**issue (bug_risk):** Iteration over the set may introduce non-deterministic rule ordering compared to the previous slice-based approach.
The prior version produced a stable sequence: base profile rules (minus disabled) followed by enabled rules in declaration order. Iterating a set backed by a map will yield effectively random ordering between runs, which may affect consumers that expect deterministic or base‑then‑tailored ordering (e.g., diffs, caching, or UIs). If order is significant, keep using the set only for de‑duplication, but iterate the base and enabled rules in their original order and append only if not disabled and not yet seen.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| effectiveRules := baseRules.Union(addedRules).Difference(removedRules) | ||
|
|
||
| for rule := range effectiveRules { |
There was a problem hiding this comment.
issue (bug_risk): Enable vs disable precedence has changed from "enable wins" to "disable wins" for the same rule name.
With the old logic we filtered out removedRules from the base, then always appended EnableRules, so an explicitly enabled rule “won” even if also disabled. With the new (base ∪ added) − removed logic, any rule in both EnableRules and DisableRules is now dropped. Please confirm whether this precedence change is intended per the Compliance Operator spec and existing consumers; if not, we should adjust the set operations to keep explicitly enabled rules.
| for _, rule := range tailoredProfile.Spec.EnableRules { | ||
| addedRules.Add(rule.Name) | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Iteration over the set may introduce non-deterministic rule ordering compared to the previous slice-based approach.
The prior version produced a stable sequence: base profile rules (minus disabled) followed by enabled rules in declaration order. Iterating a set backed by a map will yield effectively random ordering between runs, which may affect consumers that expect deterministic or base‑then‑tailored ordering (e.g., diffs, caching, or UIs). If order is significant, keep using the set only for de‑duplication, but iterate the base and enabled rules in their original order and append only if not disabled and not yet seen.
|
Images are ready for the commit at 191e8ba. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master-base/gualvare/fix-compliance-make-tp-extends-optional #19196 +/- ##
================================================================================================
- Coverage 49.55% 49.53% -0.03%
================================================================================================
Files 2672 2668 -4
Lines 201670 201456 -214
================================================================================================
- Hits 99930 99783 -147
+ Misses 94278 94222 -56
+ Partials 7462 7451 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ofile This is relevant now that TPs without a base profile are supported. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Description
Previously, we were calculating effective rules incorrectly in cases where the same rule is both added and deleted. A profile that adds rules A and B and removes rule A would be shown as containing rules A and B, because we were handling rule removal before addition.
This is an edge case that should not be possible to implement in practice because CO does not allow listing the same rule in both
enableRulesanddisableRules. However there's yet another edge case where editing a profile to be invalid after being already tracked as valid could end up in this scenario. While this additional edge case will also be fixed in Stackrox, it is good to make the code robust and more clear.User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI