Skip to content

Conversation

@dcaravel
Copy link
Contributor

@dcaravel dcaravel commented Jan 9, 2026

Description

Adds a new DataSource field to storage.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:

debian-bookworm-updater::debian:12
alpine-main-v3.23-updater::alpine:3.23
osv/go
osv/maven

This is not an ideal solution and adds an uncomfortable reliance on the updater names and Central. To help with this:

  • unit tests and e2e tests have been created that check for changes to the updaters and will fail if attention is needed.
  • The field will be treated as opaque, it will not be serialized in API responses and will be cleared from storage.EmbeddedVulnerability at the datastore layer as part of ROX-30641
    • This PR added json:"-" 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/e2etests were 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 existing TestImage fails due to data differences - this is unrelated to the new TestUpdaterNames that runs successfully (see here).

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
  • modified existing tests

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[].datasource for various images

$ rctl image scan --image=datadog/agent -f 2>/dev/null | jq '.scan.components[] | select(.vulns | length > 0) | .vulns[] | [.datasource] | @tsv' -r | sort | uniq -c

   4 osv/go
   2 osv/pypi
  38 ubuntu/updater/noble::ubuntu:24.04

$ rctl image scan --image=node -f 2>/dev/null | jq '.scan.components[] | select(.vulns | length > 0) | .vulns[] | [.datasource] | @tsv' -r | sort | uniq -c

1947 debian/updater::debian:12
   3 osv/npm

$ rctl image scan --image=nginx -f 2>/dev/null | jq '.scan.components[] | select(.vulns | length > 0) | .vulns[] | [.datasource] | @tsv' -r | sort | uniq -c

 122 debian/updater::debian:13

$ rctl image scan --image=grafana/grafana -f 2>/dev/null | jq '.scan.components[] | select(.vulns | length > 0) | .vulns[] | [.datasource] | @tsv' -r | sort | uniq -c

   8 osv/go

$ rctl image scan --image=alpine:3.19.7 -f 2>/dev/null | jq '.scan.components[] | select(.vulns | length > 0) | .vulns[] | [.datasource] | @tsv' -r | sort | uniq -c

  10 alpine-main-v3.19-updater::alpine:3.19

Additionally, ran the new e2e tests against an already running instances of StackRox

$ go test -run ^TestUpdaterNames$ github.com/stackrox/rox/scanner/e2etests -v
=== RUN   TestUpdaterNames
    updater_names_test.go:91: Querying updaters
{"level":"debug","count":105,"time":"2026-01-09T17:13:34-06:00","message":"found updaters"}
    updater_names_test.go:96: Found 105 updaters in database
--- PASS: TestUpdaterNames (0.57s)
PASS
ok  	github.com/stackrox/rox/scanner/e2etests	1.971s

For good measure, modified one of the patterns in the test to prove failure (removed the x from rhel-vex:

$ go test -run ^TestUpdaterNames$ github.com/stackrox/rox/scanner/e2etests -v
=== RUN   TestUpdaterNames
    updater_names_test.go:91: Querying updaters
{"level":"debug","count":105,"time":"2026-01-09T17:14:46-06:00","message":"found updaters"}
    updater_names_test.go:96: Found 105 updaters in database
    updater_names_test.go:114: 
        	Error Trace:	/Users/dcaravel/dev/stackrox/stackrox-add-datasource-to-vulns/scanner/e2etests/updater_names_test.go:114
        	Error:      	Should be true
        	Test:       	TestUpdaterNames
        	Messages:   	Unknown updater name: "rhel-vex"
--- FAIL: TestUpdaterNames (0.57s)
FAIL
FAIL	github.com/stackrox/rox/scanner/e2etests	1.956s
FAIL

@openshift-ci
Copy link

openshift-ci bot commented Jan 9, 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

Base automatically changed from dc/add-updater-to-vulns to master January 9, 2026 03:06
@dcaravel dcaravel force-pushed the dc/add-datasource-to-vulns branch from f7272fc to f4a8063 Compare January 9, 2026 03:08
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Jan 9, 2026

Images are ready for the commit at a0a88c2.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.10.x-743-ga0a88c2653.

@codecov
Copy link

codecov bot commented Jan 9, 2026

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 48.86%. Comparing base (b002714) to head (a0a88c2).

Files with missing lines Patch % Lines
scanner/updater/export.go 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
go-unit-tests 48.86% <97.50%> (-0.05%) ⬇️

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.

@dcaravel dcaravel force-pushed the dc/add-datasource-to-vulns branch 2 times, most recently from b5e3ec4 to b48a82f Compare January 9, 2026 15:42
@dcaravel dcaravel force-pushed the dc/add-datasource-to-vulns branch from b48a82f to f6ed3fa Compare January 9, 2026 22:06
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:

  • 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.
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>

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.

@dcaravel dcaravel force-pushed the dc/add-datasource-to-vulns branch from f6ed3fa to 027f67e Compare January 9, 2026 23:44
@dcaravel dcaravel marked this pull request as ready for review January 10, 2026 01:23
@dcaravel dcaravel requested review from a team as code owners January 10, 2026 01:23
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 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.

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.

@dcaravel dcaravel force-pushed the dc/add-datasource-to-vulns branch from 027f67e to a0a88c2 Compare January 11, 2026 18:54
@dcaravel dcaravel added the auto-retest PRs with this label will be automatically retested if prow checks fails label Jan 12, 2026
@dcaravel
Copy link
Contributor Author

/retest

@openshift-ci
Copy link

openshift-ci bot commented Jan 12, 2026

@dcaravel: The following tests 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-ui-e2e-tests a0a88c2 link true /test gke-ui-e2e-tests
ci/prow/ocp-4-20-ui-e2e-tests a0a88c2 link false /test ocp-4-20-ui-e2e-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.

@dcaravel dcaravel removed the auto-retest PRs with this label will be automatically retested if prow checks fails label Jan 12, 2026
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.

2 participants