ROX-31226: Store compliance operator custom rules#19037
ROX-31226: Store compliance operator custom rules#19037guzalv wants to merge 16 commits intomaster-base/gualvare/ROX-31226-custom-rules-protosfrom
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 1 issue, and left some high level feedback:
- The
godirective ingo.modwas changed to1.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,IsResourceAvailableis called once per optional resource and performsServerResourcesForGroupVersioneach 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at e60c9e1. To use with deploy scripts, first |
c9d64ff to
e4a9fda
Compare
c5be1ad to
bb9af55
Compare
e4a9fda to
0117848
Compare
bb9af55 to
ffa133f
Compare
0117848 to
12ef7d0
Compare
ffa133f to
776083b
Compare
Codecov Report❌ Patch coverage is 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
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:
|
There was a problem hiding this comment.
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sensor/kubernetes/complianceoperator/dispatchers/complianceoperatorcustomrules.go
Show resolved
Hide resolved
f421c10 to
caf8290
Compare
4943eb8 to
a918c52
Compare
|
/retest |
| @@ -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 == "" { | |||
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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{ |
There was a problem hiding this comment.
Why only CustomRule is defined as optional resource?
And why we didn't have such separation before (required and optional)?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Didn't check code - just from reading change, this looks like not fitting with other names in the list.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
nitpick: customRule := &v1alpha1.CustomRule{} can be moved to be before this if, where it's used for the first time
There was a problem hiding this comment.
Thanks for noticing, done
2202f77 to
3a558a0
Compare
Extract this to separate PR?
c3bf9b9 to
bdaa47c
Compare
3a558a0 to
e60c9e1
Compare
|
@guzalv: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
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
ComplianceOperatorRuleV2object, settingisCustomtotrue. While the full design decision is documented here, it can be summarized as follows:User-facing documentation
Testing and quality
Automated testing
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.