ROX-31226: Add compliance operator custom rule protos#19032
ROX-31226: Add compliance operator custom rule protos#19032
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
Images are ready for the commit at bdaa47c. 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 #19032 +/- ##
==========================================
- Coverage 49.65% 49.64% -0.01%
==========================================
Files 2689 2689
Lines 202505 202505
==========================================
- Hits 100548 100541 -7
- Misses 94463 94470 +7
Partials 7494 7494
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:
|
563ac1e to
51b14e8
Compare
|
/test all |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider documenting
CustomRuleDetailsdirectly in the proto comment (e.g., explicitly stating it is only populated for CEL-based custom rules and nil for XDCC rules) so downstream consumers don’t need to rely on external docs to understand the semantics. - Since
CustomRuleDetailsis added to both internalapi and storage protos, double-check any conversion/mapping code between these layers is updated to correctly populate and persist the new field for custom rules to avoid silent data loss. - If
CustomRuleDetailsis effectively acting as a discriminator between custom and non-custom rules, you might want to consider aoneofor an explicit enum/flag instead of using presence vs. absence of the message to signal rule type, which can make the API contract clearer and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider documenting `CustomRuleDetails` directly in the proto comment (e.g., explicitly stating it is only populated for CEL-based custom rules and nil for XDCC rules) so downstream consumers don’t need to rely on external docs to understand the semantics.
- Since `CustomRuleDetails` is added to both internalapi and storage protos, double-check any conversion/mapping code between these layers is updated to correctly populate and persist the new field for custom rules to avoid silent data loss.
- If `CustomRuleDetails` is effectively acting as a discriminator between custom and non-custom rules, you might want to consider a `oneof` or an explicit enum/flag instead of using presence vs. absence of the message to signal rule type, which can make the API contract clearer and less error-prone.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0117848 to
12ef7d0
Compare
f421c10 to
caf8290
Compare
| string instructions = 13; | ||
|
|
||
| enum ComplianceOperatorRuleKind { | ||
| RULE = 0; |
There was a problem hiding this comment.
one consideration (not sure if it applies in this case): Usually = 0 is used for UNSET. I'm not sure if this is relevant in this case or not. Also if we would start with UNSET = 0 - probably migration is required, where in this case not, since default 0 will be mapped to provided rules.
There was a problem hiding this comment.
Thanks, I used 0 for RULE because I found it the most convenient for the reason you wrote, but I was not aware of the UNSPECIFIED best practice, I will implement that. @rukletsov also highlighted this in another comment. I have not dealt with migrations here yet, so if that is needed I will learn something new :).
I will look around and figure this out. Thanks!
| string instructions = 17; | ||
|
|
||
| enum ComplianceOperatorRuleKind { | ||
| RULE = 0; |
There was a problem hiding this comment.
Make 0 a sentinel value. I know our codebase is inconsistent but let's not add to it. See https://protobuf.dev/programming-guides/style/#enums . So — OPERATOR_KIND_UNSPECIFIED = 0;
There was a problem hiding this comment.
Thanks for the pointer, I was not aware of this and since I saw both styles in this file I went with the one that seemed simplest. Done:
enum OperatorKind {
OPERATOR_KIND_UNSPECIFIED = 0;
OPERATOR_KIND_RULE = 1;
OPERATOR_KIND_CUSTOM_RULE = 2;
}
OperatorKind operator_kind = 18;| string parent_rule = 16; | ||
| string instructions = 17; | ||
|
|
||
| enum ComplianceOperatorRuleKind { |
There was a problem hiding this comment.
If enum is nested, no need to repeat the message name. Enums (contrary to enum values) are already scoped. Just call it RuleKind.
There was a problem hiding this comment.
The intent was to show that this reflects the kind of a rule at the compliance operator side, and avoid confusion with the existing rule_type field. But it is clear that what it says is "author thought prefix is needed" :D.
I liked your suggestion below "operator_kind", so I have used it here:
enum OperatorKind {
OPERATOR_KIND_UNSPECIFIED = 0;
OPERATOR_KIND_RULE = 1;
OPERATOR_KIND_CUSTOM_RULE = 2;
}
OperatorKind operator_kind = 18;I had to do: docker run --platform linux/amd64 --rm -v "$(pwd):/src" -w /src quay.io/stackrox-io/apollo-ci:stackrox-test-0.5.2 bash -c "rm -rf .gotools && go mod tidy && make proto-generated-srcs"
c3bf9b9 to
bdaa47c
Compare
rukletsov
left a comment
There was a problem hiding this comment.
Only checked two .proto files.
| OPERATOR_KIND_RULE = 1; | ||
| OPERATOR_KIND_CUSTOM_RULE = 2; |
There was a problem hiding this comment.
You don't have to prefix any fields except the first one. They will be scoped to the message so if the chance that another enum in this message would use the same enum value, you can skip the prefix. The sentinel 0 is important because it's collide with any enum in the message.
Description
This PR prepares the proto definitions for upcoming support of Compliance Operator custom rules by adding a new enum field called
ComplianceOperatorRuleKindto the existingComplianceOperatorRuleV2definition. This field will beCUSTOM_RULEfor custom rules (CEL based), andRULEfor regular XDCC rules. Details about custom rules can be found in this Red Hat developer blog post.Note on the naming of the new field: The
compliance_operator_prefix on the field and enum name distinguishes them from the existingrule_typefield and reflects that this classification belongs to the Compliance Operator domain, not StackRox's own rule model.While whether a rule is custom or not could possibly be inferred by looking at other fields, an explicit enum is preferred:
kind: Rule->ComplianceOperatorRuleKind: RULEkind: CustomRule->ComplianceOperatorRuleKind: CUSTOM_RULEUser-facing documentation
Testing and quality
Automated testing
How I validated my change
CI