-
Notifications
You must be signed in to change notification settings - Fork 170
ROX-32422: Populate vulnerability datasource #18416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
f7272fc to
f4a8063
Compare
|
Images are ready for the commit at a0a88c2. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18416 +/- ##
==========================================
- Coverage 48.90% 48.86% -0.05%
==========================================
Files 2629 2631 +2
Lines 197917 198124 +207
==========================================
+ Hits 96789 96805 +16
- Misses 93747 93936 +189
- Partials 7381 7383 +2
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:
|
b5e3ec4 to
b48a82f
Compare
b48a82f to
f6ed3fa
Compare
There was a problem hiding this 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:
- In
envOS, callingdistributions(report)without checking for a nilreportwill panic in cases the tests already model (e.g., non-nil env with nil report); consider adding a nil guard so the helper safely returns an empty string whenreportis nil.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `envOS`, calling `distributions(report)` without checking for a nil `report` will panic in cases the tests already model (e.g., non-nil env with nil report); consider adding a nil guard so the helper safely returns an empty string when `report` is nil.
## Individual Comments
### Comment 1
<location> `pkg/scanners/scannerv4/convert.go:98-107` </location>
<code_context>
+// envOS will return the operating system name and version associated with an
+// environment.
+func envOS(env *v4.Environment, report *v4.VulnerabilityReport) string {
+ if env == nil {
+ return ""
+ }
+
+ dists := distributions(report)
+ dist, ok := dists[env.GetDistributionId()]
+ if !ok || dist.GetDid() == "" || dist.GetVersionId() == "" {
+ return ""
+ }
+
+ return dist.GetDid() + ":" + dist.GetVersionId()
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** envOS assumes report is non-nil, which can panic if used defensively elsewhere
`envOS` dereferences `report` via `distributions(report)` without a nil check. If any caller passes a nil report (e.g., future code or tests), this will panic. Either add a nil guard for `report` in `envOS`, or clearly enforce/document that `report` must be non-nil at all call sites.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
f6ed3fa to
027f67e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- envOS calls distributions(report) without guarding against a nil report, but TestEnvOS includes cases with a non-nil env and nil report; either envOS or distributions should defensively handle a nil report to avoid a potential panic and align with the test expectations.
- vulnDataSource relies on a simple strings.HasPrefix(updater, "rhel") check to exclude Red Hat–sourced vulnerabilities; consider centralizing or tightening this Red Hat source detection (e.g., matching specific updater set namespaces) so that future updater naming changes don’t accidentally bypass the exclusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- envOS calls distributions(report) without guarding against a nil report, but TestEnvOS includes cases with a non-nil env and nil report; either envOS or distributions should defensively handle a nil report to avoid a potential panic and align with the test expectations.
- vulnDataSource relies on a simple strings.HasPrefix(updater, "rhel") check to exclude Red Hat–sourced vulnerabilities; consider centralizing or tightening this Red Hat source detection (e.g., matching specific updater set namespaces) so that future updater naming changes don’t accidentally bypass the exclusion.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
027f67e to
a0a88c2
Compare
|
/retest |
|
@dcaravel: The following tests 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
Adds a new
DataSourcefield tostorage.EmbeddedVulnerability. This datasource will be used to uniquely associate data with CVEs in Central.The initial use cases will track CVE fixed dates.
The DataSource will only be populated by Scanner V4 and only for vulnerabilities NOT sourced from Red Hat (because more fields are needed to uniquely identify a Red Hat vulnerability then what is exposed by Scanner V4).
The DataSource field will contain the concatenation of the updater name from Scanner V4/ClairCore + the package os/version (if applicable), examples:
This is not an ideal solution and adds an uncomfortable reliance on the updater names and Central. To help with this:
storage.EmbeddedVulnerabilityat the datastore layer as part of ROX-30641json:"-"to the new field which will omit it from REST API responses, however it is still populated in gRPC reponses until cleared at the datastore level.The
scanner/e2etestswere not starting prior this PR - the restart check was removed because the scanner pod was always restarting at least once before bundles were loaded. After removing the restart check the e2etest now run, however the existingTestImagefails due to data differences - this is unrelated to the newTestUpdaterNamesthat runs successfully (see here).User-facing documentation
Testing and quality
Automated testing
There is slight overlap between the added unit and e2e tests that check to see if updaters have been changed. The unit tests will cover the obvious changes to updater sets, but cannot validate the actual updater names since those are determined at run time. The actual updater names are evaluated as part of the e2e tests.
How I validated my change
Because the dataSource field is not filtered yet for gRPC requests - I used roxctl (gRPC) to verify that the field is being populated as expected, the below jq queries show the counts / values of
.scan.components[].vulns[].datasourcefor various imagesAdditionally, ran the new e2e tests against an already running instances of StackRox
For good measure, modified one of the patterns in the test to prove failure (removed the
xfromrhel-vex: