Skip to content
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

fix: only care about ecosystem suffix if present in both ecosystems when determining equality #1007

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented May 30, 2024

This changes how we compare ecosystems so that the suffix is only considered when it is present for both ecosystems being compared, since we can't reliably extract that.

Resolves #769

@@ -141,6 +141,43 @@ Loaded filter from: <rootdir>/fixtures/go-project/osv-scanner.toml

---

[TestRun/PURL_SBOM_case_sensitivity_(api) - 1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that the offline output has vulnerabilities whereas the online one doesn't indicates there's a potential bug in the api's Alpine comparator 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is a bug in alpine version enumeration, just fixed this week in: google/osv.dev#2241 but not in production yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - I'd seen a few of those go around this/last week, so figured it might have been known 😅

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@46aee59). Learn more about missing BASE report.

Files Patch % Lines
internal/utility/vulns/vulnerability.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1007   +/-   ##
=======================================
  Coverage        ?   65.30%           
=======================================
  Files           ?      150           
  Lines           ?    12535           
  Branches        ?        0           
=======================================
  Hits            ?     8186           
  Misses          ?     3884           
  Partials        ?      465           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!. Going to wait till the next OSV.dev release (with the alpine enumeration fix) before merging this, so we'll have the correct snapshots.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 4, 2024

Turns out we're not accurately sorting the table output at least for local - I'll tackle that somewhere...

Copy link
Contributor

@hogo6002 hogo6002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@G-Rath G-Rath force-pushed the fix-sbom branch 3 times, most recently from 1efc103 to c2baa5d Compare June 12, 2024 19:33
@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 12, 2024

I cannot for the life of me replicate that sorting difference locally 😕

@hogo6002
Copy link
Contributor

I cannot for the life of me replicate that sorting difference locally 😕

I tried running update_snaps for this branch locally, and it did change the order, but it's still a little different from the Ubuntu results.
image

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 13, 2024

Yeah, it seems that every couple of hours the order changes - I'm not sure if its coinciding with something on the osv.dev side like work being landed or if it's just some clock-shift based randomness, but it seems to be "hours" based rather than "seconds" based...

@hogo6002
Copy link
Contributor

Yeah, it seems that every couple of hours the order changes - I'm not sure if its coinciding with something on the osv.dev side like work being landed or if it's just some clock-shift based randomness, but it seems to be "hours" based rather than "seconds" based...

Yeah I see what you mean. I just tested again and the result shows different.

@hogo6002
Copy link
Contributor

hogo6002 commented Jun 13, 2024

I had a look with @michaelkedar on this issue. We noticed that the result changes after the exporter cron job publishes a new all.zip. We think this is because the exporter runs in parallel (google/osv.dev#2167), which may cause the order to be slightly different each time. To solve this, I guess we need to either sort the all.zip on the OSV.dev side or order all vulnerabilities alphabetically on the OSV-Scanner side. What do you think @another-rex

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 13, 2024

oohh good find - fwiw locally I've been playing around with sorting the table rows within packages by their url though that got blocked by it being an interface{}; I've not gotten back to it yet so might be an easy fix

@another-rex
Copy link
Collaborator

Hmm... is this behaviour new? All.zip is built concurrently, but that has always been the case if I understand correctly.

@hogo6002
Copy link
Contributor

hogo6002 commented Jun 13, 2024

Hmm... is this behaviour new? All.zip is built concurrently, but that has always been the case if I understand correctly.

I don't think this is a new behaviour. Even though all.zip is built concurrently, the files are usually modified in alphabetical order, just a few are out of place. That's probably why most of our tests are passing. But this PR has a bunch of vulnerabilities with adjacency IDs (like 9840, 9841, 9842, and 9843), which is probably why it's failing. But I am not 100% sure if this is the issue as the modified order doesn't really align with the scanner output order.
image

@another-rex
Copy link
Collaborator

I think this just needs a flag change in the test to match the new download-offline-db flag and should be good to merge.

@another-rex another-rex merged commit f40457e into google:main Jun 24, 2024
13 checks passed
@G-Rath G-Rath deleted the fix-sbom branch June 24, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing vulnerabilities for debian purls for --experimental-local-db
5 participants