ROX-33654: Add entity scope and query field support to report service#19861
ROX-33654: Add entity scope and query field support to report service#19861
Conversation
- Add bidirectional conversion between API and storage for entity scope resource scope type in report configurations and snapshots - Add query field passthrough in vulnerability report filters - Add validation for entity scope rules (unset entity/field, duplicate entity+field pairs, cluster annotation unsupported, label key=value format) - Add query field validation using search.ParseQuery - Store ResourceScope in report snapshot Partially generated with AI assistance.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19861 +/- ##
==========================================
+ Coverage 49.58% 49.60% +0.01%
==========================================
Files 2766 2766
Lines 208535 208708 +173
==========================================
+ Hits 103409 103530 +121
- Misses 97448 97494 +46
- Partials 7678 7684 +6
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:
|
🚀 Build Images ReadyImages are ready for commit 7aaa6ee. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-604-g7aaa6ee34a |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes extend the central/reports service's vulnerability report handling to support entity-scoped resource configurations alongside collection-scoped ones. This includes bidirectional conversion between API v2 and storage representations, validation logic for entity scope rules, and Query field parsing for vulnerability report filters. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
central/reports/validation/validate.go (1)
351-356:⚠️ Potential issue | 🔴 CriticalGuard
collectionbefore populatingCollectionSnapshot.Line 353 dereferences
collectionunconditionally, but lines 284-295 now leavecollection == nilfor the new entity-scope path. The first valid entity-scoped report request will panic here before the snapshot is created, so the feature is effectively unusable.🐛 Proposed fix
snapshot := &storage.ReportSnapshot{ ReportConfigurationId: config.GetId(), Name: config.GetName(), Description: config.GetDescription(), Type: storage.ReportSnapshot_VULNERABILITY, - Collection: &storage.CollectionSnapshot{ - Id: config.GetResourceScope().GetCollectionId(), - Name: collection.GetName(), - }, ResourceScope: config.GetResourceScope(), Schedule: config.GetSchedule(), ReportStatus: &storage.ReportStatus{ RunState: storage.ReportStatus_WAITING, ReportRequestType: requestType, ReportNotificationMethod: notificationMethod, }, } + if collection != nil { + snapshot.Collection = &storage.CollectionSnapshot{ + Id: config.GetResourceScope().GetCollectionId(), + Name: collection.GetName(), + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/reports/validation/validate.go` around lines 351 - 356, The code unconditionally dereferences collection when building the CollectionSnapshot even though collection can be nil for entity-scoped requests; modify the snapshot construction in validate.go so you only populate Collection: &storage.CollectionSnapshot{...} when collection != nil (i.e., guard the access to collection.GetName()/GetId()), otherwise leave Collection nil (or omit it) to avoid a panic; update the block that sets Collection: to conditionally assign based on collection and keep ResourceScope and Schedule assignments unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@central/reports/service/v2/convert.go`:
- Around line 483-486: The current conversion calls
convertProtoResourceScopeToV2 on snapshot.GetResourceScope(), which performs
live datastore collection lookups and can fail or rewrite historical names;
change the conversion logic to first build ResourceScope.CollectionScope from
snapshot.GetCollection() (use snapshot.GetCollection() to populate
CollectionScope fields) and only call convertProtoResourceScopeToV2 as a
fallback when snapshot.GetCollection() is empty/missing; implement this in a
new/updated helper (e.g., convertProtoSnapshotResourceScopeToV2) that accepts
the snapshot, prefers snapshot.GetCollection() for CollectionScope, and
delegates to convertProtoResourceScopeToV2(snapshot.GetResourceScope()) only
when snapshot collection metadata is absent.
---
Outside diff comments:
In `@central/reports/validation/validate.go`:
- Around line 351-356: The code unconditionally dereferences collection when
building the CollectionSnapshot even though collection can be nil for
entity-scoped requests; modify the snapshot construction in validate.go so you
only populate Collection: &storage.CollectionSnapshot{...} when collection !=
nil (i.e., guard the access to collection.GetName()/GetId()), otherwise leave
Collection nil (or omit it) to avoid a panic; update the block that sets
Collection: to conditionally assign based on collection and keep ResourceScope
and Schedule assignments unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e3908763-42bc-4423-9b33-71e46f51d3e9
📒 Files selected for processing (4)
central/reports/service/v2/convert.gocentral/reports/service/v2/convert_test.gocentral/reports/validation/validate.gocentral/reports/validation/validate_test.go
| resourceScope, err := s.convertProtoResourceScopeToV2(snapshot.GetResourceScope()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Use snapshot metadata instead of live collection lookups here.
Lines 483-486 now route snapshot ResourceScope through convertProtoResourceScopeToV2, which resolves collection-scoped names from the current datastore. That means one deleted collection will make the whole snapshot conversion fail, and a renamed collection will also rewrite the historical name, even though snapshot.GetCollection() already stores the snapshot-time metadata. Please build ResourceScope.CollectionScope from the snapshot copy first and only fall back to a live lookup when that snapshot metadata is absent.
💡 Suggested direction
- resourceScope, err := s.convertProtoResourceScopeToV2(snapshot.GetResourceScope())
- if err != nil {
- return nil, err
- }
+ resourceScope, err := s.convertProtoSnapshotResourceScopeToV2(
+ snapshot.GetResourceScope(),
+ snapshot.GetCollection(),
+ )
+ if err != nil {
+ return nil, err
+ }convertProtoSnapshotResourceScopeToV2 should prefer snapshot.GetCollection() for collection-scoped snapshots so historical report reads stay stable after collection rename/deletion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@central/reports/service/v2/convert.go` around lines 483 - 486, The current
conversion calls convertProtoResourceScopeToV2 on snapshot.GetResourceScope(),
which performs live datastore collection lookups and can fail or rewrite
historical names; change the conversion logic to first build
ResourceScope.CollectionScope from snapshot.GetCollection() (use
snapshot.GetCollection() to populate CollectionScope fields) and only call
convertProtoResourceScopeToV2 as a fallback when snapshot.GetCollection() is
empty/missing; implement this in a new/updated helper (e.g.,
convertProtoSnapshotResourceScopeToV2) that accepts the snapshot, prefers
snapshot.GetCollection() for CollectionScope, and delegates to
convertProtoResourceScopeToV2(snapshot.GetResourceScope()) only when snapshot
collection metadata is absent.
| sr := &storage.EntityScopeRule{ | ||
| Entity: storage.EntityType(rule.GetEntity()), | ||
| Field: storage.EntityField(rule.GetField()), | ||
| } | ||
| for _, rv := range rule.GetValues() { | ||
| sr.Values = append(sr.Values, &storage.RuleValue{ | ||
| Value: rv.GetValue(), | ||
| MatchType: storage.MatchType(rv.GetMatchType()), | ||
| }) | ||
| } |
There was a problem hiding this comment.
nit: Should we not convert these with a switch on the value to future proof this? Today the values line up, but they may not always.
| Entity: apiV2.ScopeEntity(rule.GetEntity()), | ||
| Field: apiV2.ScopeField(rule.GetField()), | ||
| } | ||
| for _, rv := range rule.GetValues() { | ||
| ar.Values = append(ar.Values, &apiV2.RuleValue{ | ||
| Value: rv.GetValue(), | ||
| MatchType: apiV2.MatchType(rv.GetMatchType()), |
There was a problem hiding this comment.
nit: same comment as above on converting by value to allow for divergence.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @c-du. * #19861 (comment) The following files were modified: * `central/reports/service/v2/convert.go` * `central/reports/validation/validate.go`
|
|
||
| // v2EntityTypeToStorage converts an API ScopeEntity to the corresponding storage EntityType. | ||
| func v2EntityTypeToStorage(e apiV2.ScopeEntity) storage.EntityType { | ||
| switch e { |
There was a problem hiding this comment.
why is input apiV2.ScopeEntity and not apiV2.EntityScope
| return v.validateCollectionScope(ref.CollectionScope) | ||
| case *apiV2.ResourceScope_EntityScope: | ||
| if !features.VulnerabilityReportsEnhancedFiltering.Enabled() { | ||
| return errors.Wrap(errox.InvalidArgs, "Report configuration must specify a valid resource scope") |
There was a problem hiding this comment.
reword error message - Report configuration must specify a valid collection as a resource scope
| seen := set.NewSet[entityFieldKey]() | ||
| for _, rule := range es.GetRules() { | ||
| if rule.GetEntity() == apiV2.ScopeEntity_SCOPE_ENTITY_UNSET { | ||
| return errors.Wrapf(errox.InvalidArgs, "Unexpected entity scope rule: %s", rule.GetEntity()) |
There was a problem hiding this comment.
reword error message to : "Unexpected entity in scope rule: %s", rule.GetEntity()"
| key := entityFieldKey{entity: rule.GetEntity(), field: rule.GetField()} | ||
| if !seen.Add(key) { | ||
| return errors.Wrapf(errox.InvalidArgs, | ||
| "Duplicate (entity, field) pair in entity scope rules: entity=%v field=%v", rule.GetEntity(), rule.GetField()) |
There was a problem hiding this comment.
nit: reword error message as - "Duplicate (entity, field) pair in entity scope rules found: entity=%v field=%v". Only one rule per (entity, field) pair is allowed,"
| if rule.GetField() == apiV2.ScopeField_FIELD_LABEL { | ||
| for _, rv := range rule.GetValues() { | ||
| if !strings.Contains(rv.GetValue(), "=") { | ||
| return errors.Wrap(errox.InvalidArgs, "Label values must be in 'key=value' format") |
There was a problem hiding this comment.
we should verify this for annotation as well
Description
Adds support for two new fields in the vulnerability report service (v2
API):
collection-based scoping that lets callers filter by
cluster/namespace/deployment entity rules directly, without needing a
pre-created collection.
string appended to CVE filter criteria.
Changes are confined to the service and validation layers:
convertV2ResourceScopeToProto / convertProtoResourceScopeToV2
entity+field pairs, cluster annotation unsupported, label key=value format,
query parseable via search.ParseQuery)
User-facing documentation
Testing and quality
Automated testing
How I validated my change
TestValidateReportFiltersQuery)
(TestConvertEntityScope)