ROX-26089,ROX-26519: v4.VulnerabilityReport conversion#12798
ROX-26089,ROX-26519: v4.VulnerabilityReport conversion#12798Maddosaurus merged 25 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 449579b. To use with deploy scripts, first |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12798 +/- ##
==========================================
+ Coverage 48.24% 48.26% +0.02%
==========================================
Files 2452 2453 +1
Lines 176454 176585 +131
==========================================
+ Hits 85123 85226 +103
- Misses 84483 84507 +24
- Partials 6848 6852 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Maddosaurus
force-pushed
the
mm/ROX-26089-convert
branch
3 times, most recently
from
bfb6266 to
f2fb397
Compare
jschnath
left a comment
jschnath
left a comment
There was a problem hiding this comment.
Partial review since we just talked about this still being in draft. Didn't look at all the code yet.
Maddosaurus
force-pushed
the
mm/ROX-26089-convert
branch
from
f2fb397 to
3cc5cb9
Compare
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
The code looks good, but I am curious how you validated that additionally to the unit tests.
@vikin91 I did not verify this just yet. The plan is to do that in the follow up, ROX-26519. |
|
Side note: Looks like prow tests are skipping because of failing ARM builds, even though they are using x86 builds. |
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
Pls check the style failing and my additional comments on using Len assertion.
|
@Maddosaurus as I said, the code looks good and I cannot see any issues there. But I need to know how did you make sure that it works. |
Maddosaurus
force-pushed
the
mm/ROX-26089-convert
branch
2 times, most recently
from
0633b34 to
5f7808e
Compare
Maddosaurus
changed the title
Maddosaurus
changed the title
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
Still looks fine, thanks for testing this manually. I am leaving two minor cosmetic comments
Maddosaurus
dismissed
jschnath’s stale review
Jan is OOO this week and cannot re-review
Co-authored-by: Jan Schnathmeier <102952141+jschnath@users.noreply.github.com>
Co-authored-by: Piotr Rygielski <114479+vikin91@users.noreply.github.com>
Maddosaurus
force-pushed
the
mm/ROX-26089-convert
branch
from
42f3400 to
449579b
Compare
|
/retest |
|
@Maddosaurus: 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. |
Maddosaurus
changed the title
| ) | ||
|
|
||
| var ( | ||
| rhcosOSImageRegexp = regexp.MustCompile(`(Red Hat Enterprise Linux) (CoreOS) ([\d])([\d]+)`) |
There was a problem hiding this comment.
nit: rhcosPattern
Also, according to my IDE, the [] are not needed
| // Keep notes as they are for nodes other than RHCOS | ||
| return notes | ||
| } | ||
| fixedNotes := make([]storage.NodeScan_Note, 0) |
There was a problem hiding this comment.
filtered := notes[:0]
for _, note := range notes {
if note == storage.NodeScan_UNSUPPORTED {
continue
}
filtered = append(filtered, note)
}
return filtered
| if len(r) != 5 { | ||
| return "" | ||
| } | ||
| return fmt.Sprintf("rhcos:%s.%s", r[3], r[4]) |
There was a problem hiding this comment.
For Red Hat Enterprise Linux CoreOS 41712345.94.2024 it gives me rhcos:4.1712345. Is this expected?
|
|
||
| func toNodeScan(r *v4.VulnerabilityReport, osImageRef string) *storage.NodeScan { | ||
| // TODO(ROX-26593): Instead of fixing notes here, add RHCOS DistributionScanner to ClairCore | ||
| fixedNotes := fixNotes(toStorageNotes(r.Notes), osImageRef) |
There was a problem hiding this comment.
if we can't determine the OS below, though, wouldn't we want the OS unsupported or unknown note?
| func toOperatingSystem(ref string) string { | ||
| r := rhcosOSImageRegexp.FindStringSubmatch(ref) | ||
| if len(r) != 5 { | ||
| return "" |
There was a problem hiding this comment.
We do unknown for images. Why not do that here, too?
|
|
||
| return &storage.NodeScan{ | ||
| ScanTime: protocompat.TimestampNow(), | ||
| Components: toStorageComponents(r), |
There was a problem hiding this comment.
how much does this differ from components in convert.go? Can we just reuse that instead?
6 participants
Description
Introduces a converter to translate a
v4.VulnerabilityReportinto astorage.NodeScan.This converter allows us to reuse existing storage and UI logic, as well as API endpoints.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Ensure an SSD StorageClass is the default StorageClass for the clusterto trueROX_SCANNER_V4=trueROX_NODE_INDEX_ENABLED=truefor the Collector DaemonSetROX_NODE_SCANNING_INTERVAL=45sandROX_NODE_SCANNING_MAX_INITIAL_WAIT=10sReceived Vulnerability Report with %d packages containing %d vulnerabilitiesSuccessfully enriched node %s with %s report - found %d componentsDiscarding v2 NodeScan in favor of v4 NodeScan(If Scanner v2 is run in parallel)The last mentioned log message will log out the Node name, the Scanner Version string, and the number of discovered components.
The scanner version is expected to be 4, and the number of components bigger than 0.