Skip to content

Fix(compliance): Handle tailored profiles with enabled and disabled rules#19196

Draft
guzalv wants to merge 7 commits intomasterfrom
master-base/gualvare/fix-compliance-handle-no-base-and-disabled-rules
Draft

Fix(compliance): Handle tailored profiles with enabled and disabled rules#19196
guzalv wants to merge 7 commits intomasterfrom
master-base/gualvare/fix-compliance-handle-no-base-and-disabled-rules

Conversation

@guzalv
Copy link
Contributor

@guzalv guzalv commented Feb 25, 2026

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 enableRules and disableRules. 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

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

CI

@guzalv
Copy link
Contributor Author

guzalv commented Feb 25, 2026

@openshift-ci
Copy link

openshift-ci bot commented Feb 25, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +91 to +93
effectiveRules := baseRules.Union(addedRules).Difference(removedRules)

for rule := range effectiveRules {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 25, 2026

Images are ready for the commit at 191e8ba.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-121-g191e8bac4d.

@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.53%. Comparing base (717a244) to head (191e8ba).
⚠️ Report is 2 commits behind head on master-base/gualvare/fix-compliance-make-tp-extends-optional.

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     
Flag Coverage Δ
go-unit-tests 49.53% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

guzalv and others added 2 commits February 26, 2026 10:04
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>
Base automatically changed from master-base/gualvare/fix-compliance-make-tp-extends-optional to master February 27, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants