ROX-29284: set last affected as empty fixed by version#15457
ROX-29284: set last affected as empty fixed by version#15457
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 080ba01. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15457 +/- ##
==========================================
+ Coverage 49.23% 49.25% +0.01%
==========================================
Files 2578 2581 +3
Lines 189205 189330 +125
==========================================
+ Hits 93157 93251 +94
- Misses 88711 88739 +28
- Partials 7337 7340 +3
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:
|
There was a problem hiding this comment.
Hey @daynewlee - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Could there be a case where a vulnerability is fixable, but claircore's |
The interesting thing is that we decide if a vuln is fixable based on if there's a fixedIn version: (Let me know if you see other logics making this judgement differently. ) So what we do is: sees lastAffected => find nothing useful => telling the user not fixable. Since we can't assume a fixed in version from "last Affected" so we decide it's not fixablecc: @BradLugo |
|
Ah, so this PR makes what we display consistent with the logic that determines if the vulnerability is fixable? That makes sense. |
|
@daynewlee: The following test 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. |
| } else if err == nil && q.Has("lastAffected") { | ||
| // lastAffected doesn't give us anything informative so return empty string | ||
| return "" |
There was a problem hiding this comment.
probably unlikely to happen, but I wonder if we'd ever see introduced, only. In that case, then we will still be showing introduced=<whatever>. @daynewlee can we consider a followup to maybe also consider looking for introduced?
another consideration:
q, err := url.ParseQuery(fixedIn)
if err != nil {
// v.FixedInVersion is not url encoded, so just return it as-is.
return fixedIn
}
if q.Has("fixed") {
return q.Get("fixed")
}
return ""
Description
fixed in version in format of introduced=9.0.13&lastAffected=9.0.62 doesn't provide anything useful, so set it as empty
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Unit tests added, let me know if manual tests are needed although I think unit tests are good enough.