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

Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai review

🚀 Build Images Ready

Images are ready for commit 15d1419. To use with deploy scripts:

export MAIN_IMAGE_TAG=4.12.x-116-g15d1419620

/test all

1. Question:

does this layer intentionally rely on PackageVulnerabilities already being advisory-deduped upstream when ROX_SCANNER_V4_RED_HAT_CVES=false?

I noticed vulnerabilities() now merges by ccVuln.GetName(). In isolation that would collapse two entries like:

- RHSA-2024:10775 -> ...CVE-2024-24789...
- RHSA-2024:10775 -> ...CVE-2024-24790...

into one result.

I think toProtoV4PackageVulnerabilitiesMap() / dedupeAdvisories() currently makes that unreachable, so this may be fine. Just wanted to confirm that this function is deliberately depending on that upstream invariant.

That's correct, by the time the the information gets to Central there should only be a single RHSA, and those RHSAs do not map to a single CVE, they generally represent many CVEs - the behavior in dedupeAdvisories addresses that.

Regardless - ROX_SCANNER_V4_RED_HAT_CVES was to be removed when the dedicated advisory field was added (ROX-26672) but never was. I wouldn't put too much focus on that path, but either way it should behave OK.

2. Potential issue:

I think mergeFixFields() may regress remediation data in a real way, not just reshuffle metadata. Could we add a regression test for the case where a newer advisory has no FixedInVersion, but an older duplicate does? Something like:

t.Run("newer advisory does not erase existing fix version", func(t *testing.T) {
	vulnMap := map[string]*v4.VulnerabilityReport_Vulnerability{
		"a": {
			Id: "a", Name: "CVE-2024-1",
			Advisory:       &v4.VulnerabilityReport_Advisory{Name: "RHSA-2024:100"},
			FixedInVersion: "1.0.0",
		},
		"b": {
			Id: "b", Name: "CVE-2024-1",
			Advisory: &v4.VulnerabilityReport_Advisory{Name: "RHSA-2024:200"},
		},
	}

	got := vulnerabilities(vulnMap, []string{"a", "b"}, "", "")
	require.Len(t, got, 1)

	// Prefer the newer advisory metadata, but do not lose known fixability.
	assert.Equal(t, "RHSA-2024:200", got[0].GetAdvisory().GetName())
	assert.Equal(t, "1.0.0", got[0].GetFixedBy())
})

This would help answer whether losing fixability here is really intended.

That is the opposite of the existing test case:

t.Run("advisory wins with no fix version", func(t *testing.T) {
		vulnMap := map[string]*v4.VulnerabilityReport_Vulnerability{
			"a": {
				Id: "a", Name: "CVE-2024-1",
				Advisory:       &v4.VulnerabilityReport_Advisory{Name: "RHSA-2024:100"},
				FixedInVersion: "1.0.0",
			},
			"b": {
				Id: "b", Name: "CVE-2024-1",
				Advisory: &v4.VulnerabilityReport_Advisory{Name: "RHSA-2024:200"},
			},
		}
		got := vulnerabilities(vulnMap, []string{"a", "b"}, "", "")
		require.Len(t, got, 1)
		// "b" wins on advisory — fix version is empty from "b".
		assert.Equal(t, "RHSA-2024:200", got[0].GetAdvisory().GetName())
		assert.Equal(t, "", got[0].GetFixedBy())
	})

This scenario shouldn't occur - an RHSA describes a 'fix', any versions prior to the version mentioned are considered vulnerable, an RHSA wouldn't be published for a package it doesn't fix.

But lets say that can happen - if we surface to the user that this vulnerability is fixed in 1.0.0, but a later RHSA says the package is no longer fixed (not possible), that user will still be affected by the vuln in that case. I could argue that 'no fix' is the more accurate conclusion (which is why I created that test / expected outcome)

If we feel that we should carry the fix version forward in this case, I'm OK changing it - either way should not occur in practice.

Thank you for answering! The answers make sense!
I can mark the PR green, but because I am not 100% in the topic I would suggest that maybe someone else should give this a look too.

@dcaravel: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard.