Skip to content

ROX-31226: Store compliance operator custom rules#19037

Open
guzalv wants to merge 16 commits intomaster-base/gualvare/ROX-31226-custom-rules-protosfrom
master-base/gualvare/ROX-31226-custom-rules-tracking
Open

ROX-31226: Store compliance operator custom rules#19037
guzalv wants to merge 16 commits intomaster-base/gualvare/ROX-31226-custom-rules-protosfrom
master-base/gualvare/ROX-31226-custom-rules-tracking

Conversation

@guzalv
Copy link
Contributor

@guzalv guzalv commented Feb 15, 2026

Description

This PR implements watching for and storing Compliance Operator custom rules. Details about custom rules can be found in this Red Hat developer blog post.

Custom rules are stored reusing the existing ComplianceOperatorRuleV2 object, setting isCustom to true. While the full design decision is documented here, it can be summarized as follows:

  • Aligns with the reality that from Stackrox point of view rules and custom rules are mostly equivalent: both are included in profiles (or tailored profiles), checked as part of scans, and generate results that are then exposed to users. Stackrox only deals with rules/custom rules to show their details next to the associated results, which does not change based on whether or not a rule is custom.
  • Minimizes code duplication.
  • Enables reusing most of the existing code that deals with schedules, results and reports.
  • Automatically provides test coverage for generic functionality.
  • Makes it easy to provide a single API for UI consumption and therefore reduce the extent of UI changes required.

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

Tests in dispatcher are not needed here if we want to keep parallelism with other already tracked objects: none of the existing dispatchers (e.g. for profiles or scans) are tested.

Regarding pipelines, we are reusing the existing rules pipeline which is already tested. So far we only branch on the new field to decide whether an annotation is required, and a test has been added for that.

How I validated my change

CI, I have also deployed the image in a live cluster, created a new custom rule and verified it is stored in the database.

@openshift-ci
Copy link

openshift-ci bot commented Feb 15, 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 1 issue, and left some high level feedback:

  • The go directive in go.mod was changed to 1.25.3, which is not a valid Go toolchain version; consider reverting to a supported version (e.g., 1.22.x) or the project’s standard Go version to avoid tooling/build issues.
  • In sensor/kubernetes/complianceoperator/updater.go, IsResourceAvailable is called once per optional resource and performs ServerResourcesForGroupVersion each time; consider caching the discovery result per group-version and reusing it to avoid repeated discovery calls.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `go` directive in `go.mod` was changed to `1.25.3`, which is not a valid Go toolchain version; consider reverting to a supported version (e.g., 1.22.x) or the project’s standard Go version to avoid tooling/build issues.
- In `sensor/kubernetes/complianceoperator/updater.go`, `IsResourceAvailable` is called once per optional resource and performs `ServerResourcesForGroupVersion` each time; consider caching the discovery result per group-version and reusing it to avoid repeated discovery calls.

## Individual Comments

### Comment 1
<location> `sensor/kubernetes/listener/resource_event_handler.go:159` </location>
<code_context>
 	if err != nil {
 		log.Errorf("Failed to check the availability of Compliance Operator resources: %v", err)
 	}
+	customRulesAvailable, err := complianceoperator.IsResourceAvailable(k.client.Kubernetes(), complianceoperator.CustomRule)
+	if err != nil {
+		log.Errorf("Failed to check the availability of Compliance Operator Custom Rules, they won't be tracked: %v", err)
+	}
 	if coAvailable {
</code_context>

<issue_to_address>
**suggestion (performance):** Avoid calling IsResourceAvailable for custom rules when the compliance operator itself is not available.

`customRulesAvailable` is computed even when `coAvailable` is false, and both checks trigger discovery. When CO CRDs are missing, this causes an unnecessary discovery call and log noise for custom rules that aren’t relevant. Move the custom-rules availability check inside the `if coAvailable` block (and reuse any existing discovery info if possible) to avoid extra API calls and logs.

```suggestion
	coCrdWatcher := crd.NewCRDWatcher(&k.stopSig, dynamicSif)
	if err != nil {
		log.Errorf("Failed to check the availability of Compliance Operator resources: %v", err)
	}
	if coAvailable {
		log.Info("Initializing compliance operator informers")
		customRulesAvailable, err := complianceoperator.IsResourceAvailable(k.client.Kubernetes(), complianceoperator.CustomRule)
		if err != nil {
			log.Errorf("Failed to check the availability of Compliance Operator Custom Rules, they won't be tracked: %v", err)
		}

		complianceResultInformer = crdSharedInformerFactory.ForResource(complianceoperator.ComplianceCheckResult.GroupVersionResource()).Informer()
		complianceScanInformer = crdSharedInformerFactory.ForResource(complianceoperator.ComplianceScan.GroupVersionResource()).Informer()
		complianceSuiteInformer = crdSharedInformerFactory.ForResource(complianceoperator.ComplianceSuite.GroupVersionResource()).Informer()
		complianceRemediationInformer = crdSharedInformerFactory.ForResource(complianceoperator.ComplianceRemediation.GroupVersionResource()).Informer()

		if customRulesAvailable {
```
</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.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 16, 2026

Images are ready for the commit at e60c9e1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-280-ge60c9e18d9.

@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-protos branch 2 times, most recently from c9d64ff to e4a9fda Compare February 24, 2026 09:47
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch 3 times, most recently from c5be1ad to bb9af55 Compare February 27, 2026 14:54
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-protos branch from e4a9fda to 0117848 Compare February 27, 2026 15:21
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch from bb9af55 to ffa133f Compare February 27, 2026 15:21
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-protos branch from 0117848 to 12ef7d0 Compare March 2, 2026 09:45
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch from ffa133f to 776083b Compare March 2, 2026 12:00
@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 4.21053% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.59%. Comparing base (086a7ec) to head (3a558a0).
⚠️ Report is 2 commits behind head on master-base/gualvare/ROX-31226-custom-rules-protos.

Files with missing lines Patch % Lines
...rator/dispatchers/complianceoperatorcustomrules.go 0.00% 53 Missing ⚠️
...nsor/kubernetes/listener/resource_event_handler.go 0.00% 22 Missing ⚠️
sensor/kubernetes/complianceoperator/updater.go 0.00% 11 Missing ⚠️
sensor/kubernetes/listener/resources/dispatcher.go 0.00% 3 Missing ⚠️
...v2/pipelines/complianceoperatorrulesv2/pipeline.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@                                  Coverage Diff                                   @@
##           master-base/gualvare/ROX-31226-custom-rules-protos   #19037      +/-   ##
======================================================================================
- Coverage                                               49.61%   49.59%   -0.02%     
======================================================================================
  Files                                                    2680     2681       +1     
  Lines                                                  202195   202284      +89     
======================================================================================
+ Hits                                                   100326   100332       +6     
- Misses                                                  94392    94475      +83     
  Partials                                                 7477     7477              
Flag Coverage Δ
go-unit-tests 49.59% <4.21%> (-0.02%) ⬇️

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 guzalv marked this pull request as ready for review March 3, 2026 11:11
@guzalv guzalv requested a review from a team as a code owner March 3, 2026 11:11
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 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="sensor/kubernetes/complianceoperator/dispatchers/complianceoperatorcustomrules.go" line_range="38-41" />
<code_context>
+		return nil
+	}
+
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObject.Object, customRule); err != nil {
+		log.Errorf("error converting unstructured to compliance custom rule: %v", err)
+		return nil
+	}
</code_context>
<issue_to_address>
**suggestion:** Include object identity in the conversion error log for easier debugging.

This log doesn’t include any identifying info about the object that failed conversion. Please include at least name/namespace or GVK from `unstructuredObject` with the error so we can tell which CustomRule is failing.

```suggestion
	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObject.Object, customRule); err != nil {
		log.Errorf(
			"error converting unstructured to compliance custom rule %s/%s (%s): %v",
			unstructuredObject.GetNamespace(),
			unstructuredObject.GetName(),
			unstructuredObject.GetObjectKind().GroupVersionKind().String(),
			err,
		)
		return nil
	}
```
</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.

@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-protos branch from f421c10 to caf8290 Compare March 4, 2026 10:41
@guzalv guzalv requested a review from a team as a code owner March 4, 2026 10:41
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch from 4943eb8 to a918c52 Compare March 4, 2026 12:23
@guzalv
Copy link
Contributor Author

guzalv commented Mar 5, 2026

/retest

@guzalv guzalv changed the title ROX-31226: Add compliance operator custom rules tracking ROX-31226: Store compliance operator custom rules Mar 5, 2026
@guzalv guzalv requested review from mtodor and removed request for a team March 5, 2026 11:53
@@ -81,7 +81,10 @@ func (s *pipelineImpl) Run(ctx context.Context, clusterID string, msg *central.M
rule := event.GetComplianceOperatorRuleV2()

if val := rule.GetAnnotations()[v1alpha1.RuleIDAnnotationKey]; val == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick:

	if rule.GetComplianceOperatorKind() == central.ComplianceOperatorRuleV2_RULE && rule.GetAnnotations()[v1alpha1.RuleIDAnnotationKey] == ""  {
		return errors.Errorf("Rule %s is missing the annotation %s", rule.GetName(), v1alpha1.RuleIDAnnotationKey)
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to switch-based logic to handle updates to enum field: enforce annotation for UNSPECIFIED and RULE, and reject unexpected values

RuleRefId: BuildNameRefID(clusterID, parentRule),
Instructions: sensorData.GetInstructions(),
ParentRule: parentRule,
ComplianceOperatorKind: storage.ComplianceOperatorRuleV2_ComplianceOperatorRuleKind(sensorData.GetComplianceOperatorKind()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I'm not sure if it is common in code base to directly cast int from one enum to another. I would prefer dedicated getAFromB function (or something like that) with maybe map behind or if you still would like to keep direct cast then with good test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, this was suggested by AI as "it's just fine", and I overlooked it.

Group: GetGroupVersion().Group,
Version: GetGroupVersion().Version,
})
CustomRule = registerOptionalAPIResource(v1.APIResource{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only CustomRule is defined as optional resource?
And why we didn't have such separation before (required and optional)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it has only been introduced in compliance operator v1.8.0, which is the most recent version. I haven't checked how many CO versions we support, but I was assuming we will still support v1.7 when we release this, which does not have custom rules.

If you think we should handle this differently please let me know!

"groupVersion",
"requiredAPIResources",
"optionalAPIResources",
"CustomRule",
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't check code - just from reading change, this looks like not fitting with other names in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was a bit puzzling to understand for me. What I concluded is that other CO resources like Profile or Rule are not here because they are part of the CO required resources, and that list is returned by ac.GetResources() in line 38.

That is why here we only need our own variables and the optional resources.

Perhaps we could refactor the test or the checker to account for optional resources, but I think that's overkill here, and the checker's job seems to be to check for required resources specifically.

return nil
}

if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObject.Object, customRule); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: customRule := &v1alpha1.CustomRule{} can be moved to be before this if, where it's used for the first time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing, done

@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch 2 times, most recently from 2202f77 to 3a558a0 Compare March 6, 2026 10:57
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-protos branch from c3bf9b9 to bdaa47c Compare March 6, 2026 13:28
@guzalv guzalv force-pushed the master-base/gualvare/ROX-31226-custom-rules-tracking branch from 3a558a0 to e60c9e1 Compare March 6, 2026 13:28
@openshift-ci
Copy link

openshift-ci bot commented Mar 6, 2026

@guzalv: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-sensor-integration-tests e60c9e1 link false /test gke-sensor-integration-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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.

3 participants