ROX-25797: Central Scanner v4 communication by Maddosaurus · Pull Request #12448 · stackrox/stackrox

Maddosaurus · 2024-08-19T12:02:48Z

Description

This PR adds part of the Node Indexing on Scanner v4 feature.
The pipeline is not fully implemented, as a conversion function is missing. This will be introduced in a follow up PR (see TODO(ROX-26089) comments in the code and #12798).

The most important changes summarized:

nodes/enricher.go now also creates Scanner v4 NodeScanners
nodes/enricher_impl.go's EnrichNodeWithInventory was enhanced with a third parameter, IndexReports

The goal is to ensure that an incoming IndexReport that was generated by a Node will be matched by Scanner v4's Matcher.
The resulting VulnerabilityReport will be converted into a storage.NodeScan and upserted to storage.Node in the aforementioned follow-up PR.

User-facing documentation

~~CHANGELOG is updated~~ OR update is not needed
~~documentation PR is created and is linked above~~ OR is not needed

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

How I validated my change

Create an OS4 Cluster on Infra making sure to set Ensure an SSD StorageClass is the default StorageClass for the cluster to true
Deploy this image with ROX_SCANNER_V4=true
Wait for Matcher to finish initial sync (check logs of both matchers)
Set ROX_NODE_INDEX_ENABLED=true for the Collector DaemonSet
Recommendation: Lower Node Scan intervals by setting on the Collector DaemonSet
- ROX_NODE_SCANNING_INTERVAL=45s and ROX_NODE_SCANNING_MAX_INITIAL_WAIT=10s
Set debug loglevel for Central
Observe the following log messages on Central:
- received node index report with %d packages from %d content sets for node %s (from pipeline.go)
- Received Vulnerability Report with %d vulnerabilities: %v (from scannerv4.go)

openshift-ci · 2024-08-19T12:02:53Z

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

rhacs-bot · 2024-08-20T15:00:13Z

Images are ready for the commit at 0626984.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.6.x-679-g0626984156.

central/enrichment/integration.go

codecov · 2024-09-27T10:34:48Z

Codecov Report

Attention: Patch coverage is 26.77596% with 134 lines in your changes missing coverage. Please review.

Project coverage is 48.15%. Comparing base (dd945a5) to head (0626984).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/scanners/scannerv4/scannerv4.go	0.00%	73 Missing ⚠️
compliance/collection/compliance/compliance.go	0.00%	31 Missing ⚠️
pkg/nodes/enricher/enricher.go	0.00%	10 Missing ⚠️
pkg/nodes/enricher/enricher_impl.go	75.00%	3 Missing and 4 partials ⚠️
central/enrichment/integration.go	73.91%	6 Missing ⚠️
pkg/scanners/clairify/clairify.go	0.00%	6 Missing ⚠️
central/enrichment/singleton.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12448      +/-   ##
==========================================
- Coverage   48.20%   48.15%   -0.06%     
==========================================
  Files        2442     2446       +4     
  Lines      175662   176067     +405     
==========================================
+ Hits        84671    84778     +107     
- Misses      84170    84462     +292     
- Partials     6821     6827       +6

Flag	Coverage Δ
go-unit-tests	`48.15% <26.77%> (-0.06%)`	⬇️

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.

vikin91 · 2024-09-27T11:44:46Z

making sure to set Ensure an SSD StorageClass is the default StorageClass for the cluster to true

Why is this important?

vikin91

Looks really good with no major blockers. I still mark it ❌ as there are many tiny issues that must be looked at before we can merge this.

central/enrichment/integration.go

central/sensor/service/pipeline/nodeindex/pipeline.go

pkg/scanners/scannerv4/scannerv4.go

pkg/scanners/scannerv4/scannerv4_test.go

scanner/e2etests/helmchart/templates/scanner-config.yaml

Maddosaurus · 2024-09-27T13:45:26Z

making sure to set Ensure an SSD StorageClass is the default StorageClass for the cluster to true

Why is this important?

This is important because the Matcher intialization with VEX data is currently not well optimized.
We have observed timeouts when not running on SSD-backed storage.
This leads to the Matcher never becoming ready, which leads to no VulnerabilityReports being generated.

pkg/scanners/clairv4/clairv4.go

vikin91 · 2024-09-30T15:08:05Z

I think you might have forgotten to push few things. I unresolved some comments as the code does not contain the fixes for them. Ping me again when you check what has happened

vikin91

This looks much better! I left few comments, but they are not blocking.
The two open things are:

renaming of interfaces
making sure the metrics for v4 are collected and the scanner version is indicated in the metrics.

I am fine if those two topics are addressed in a followup.

Please make the CI happy and test that in a deployed cluster. After that I can give it a final look and mark it ✅

compliance/collection/compliance/compliance.go

pkg/nodes/enricher/enricher_impl.go

pkg/nodes/enricher/metrics.go

pkg/scanners/clairify/clairify.go

pkg/nodes/enricher/enricher_impl.go

vikin91

Looks good and can be merged if CI is happy. Good job!

# Conflicts: # pkg/scannerv4/mappers/mappers.go # scanner/enricher/fixedby/fixedby.go # scanner/enricher/nvd/nvd.go # scanner/enricher/nvd/nvd_test.go # scanner/updater/manual/manual.go

Move to latest master claircore Tidy dependencies Switch to uber gomock Fix import cycle, add key to sensor deduper

Make Scanner v4 known to NewWithCreator Set up default Scannerv4 to be Node Scanner Expand GetNodeInventoryScan to handle v4 IndexReports

# Conflicts: # generated/storage/node_integration_vtproto.pb.go

- Remove duplication between log messages and error wraps - Document static layer SHA - Remove TODO and keep shared max concurrent config for Node Index and Image Scans

Improve conditional readability Co-authored-by: Piotr Rygielski <114479+vikin91@users.noreply.github.com>

openshift-ci · 2024-10-09T09:20:01Z

@Maddosaurus: The following test 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-sensor-integration-tests	`0626984`	link	false	`/test gke-sensor-integration-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.

central/imageintegration/store/defaults.go

RTann · 2024-10-28T21:49:04Z

central/sensor/service/pipeline/nodeindex/pipeline.go

+	cr := report.CloneVT()
+
+	// Read the node from the database, if not found we fail.
+	node, found, err := p.nodeDatastore.GetNode(ctx, event.GetId())


the node's ID is the same as the event ID? Is that allowed? Is it possible to set the Index Report's "HashID" to the node's ID instead?

RTann · 2024-10-28T21:52:55Z

central/sensor/service/pipeline/nodeindex/pipeline.go

+	}
+
+	// Send the Node and Index Report to Scanner for enrichment
+	err = p.enricher.EnrichNodeWithInventory(node, nil, cr)


Perhaps for a followup: I'm thinking a separate function. We seem to replace Inventory with Index, so I'm thinking we should have a separate Index function, too. If underneath it stays the same, that's ok, as at least the interface would look a bit cleaner

RTann · 2024-10-28T22:06:14Z

pkg/scanners/types/semaphore.go

+
+func NewNodeMatchSemaphoreWithValue(val int64) NodeMatchSemaphore {
+	return &nodeMatchSemaphoreImpl{
+		NewSemaphoreWithValue(val),


nit: explicitly set ScanSemaphore: NewSemaphoreWithValue(val), so it's clear what this is doing

RTann · 2024-10-28T22:08:45Z

pkg/scanners/scannerv4/scannerv4.go

+// The Scanner endpoint requires a digest for each image layer before analyzing it - TODO(ROX-25614)
+// As the Node contents are treated as one big image layer, they also need a bogus digest.
+// This digest is taken from the test of the digest library we're using (go-containerregistry).
+const mockDigest = "registry/repository@sha256:deadb33fdeadb33fdeadb33fdeadb33fdeadb33fdeadb33fdeadb33fdeadb33f"


it's unfortunate this is required, but I get why you did it. If not handled differently in the future, can we at least update this to make it clear this is for nodes?

RTann · 2024-10-28T22:10:00Z

pkg/scanners/scannerv4/scannerv4.go

+	nodeDigest, err := name.NewDigest(mockDigest)
+	if err != nil {
+		log.Errorf("Failed to parse digest from node %q: %v", node.GetName(), err)
+	}


since this is hardcoded, this can be put into an init() function or perhaps we ignore the error and just do this in a var block. No need to waste cycles recreating this every time this is called

RTann · 2024-10-28T22:37:38Z

pkg/scanners/clairify/clairify.go

 var (
 	_ scannerTypes.Scanner                  = (*clairify)(nil)
 	_ scannerTypes.ImageVulnerabilityGetter = (*clairify)(nil)
+	_ scannerTypes.NodeScanner              = (*clairify)(nil)


thanks for adding this 😃. I'm a fan of this, as it makes it clear (and will give us compilation errors once something deviates)

RTann · 2024-10-28T22:39:14Z

pkg/nodes/enricher/enricher.go

@@ -26,13 +28,20 @@ type CVESuppressor interface {

 // New returns a new NodeEnricher for the given Prometheus metrics subsystem and the Clair node scanner creator.
 func New(cves CVESuppressor, subsystem pkgMetrics.Subsystem) NodeEnricher {


how come this is all combined?

RTann · 2024-10-28T22:39:48Z

central/enrichment/integration.go

 	}

+	if integration.GetType() == scannerTypes.ScannerV4 {
+		log.Debugf("Scanner v4 is not an orchestrator Scanner, exiting")


RTann · 2024-10-28T22:40:43Z

central/enrichment/integration.go

+// Currently, only StackRox Scanner and Scanner v4 are supported node integrations.
+// Assumes integration.GetCategories() includes storage.ImageIntegrationCategory_NODE_SCANNER.
+func ImageIntegrationToNodeIntegration(integration *storage.ImageIntegration) (*storage.NodeIntegration, error) {
+	i := &storage.NodeIntegration{


we should do this after the switch. No need to allocate space for this when we may just exit early in the default

RTann · 2024-10-28T22:52:01Z

central/enrichment/integration.go

-		IntegrationConfig: &storage.NodeIntegration_Clairify{
+	}
+
+	switch integration.GetType() {


It's probably (hopefully) unlikely, but I guess it's possible GetType and the type of the IntegrationConfig may not match. I'm thinking we can revert back to the check before (check for the config's type rather than this string type). We can do something like:

switch integration.GetIntegrationConfig().(type) { case *v4.ImageIntegration_Clairify: case *v4.ImageIntegration_ScannerV4: }

openshift-ci bot added the do-not-merge/work-in-progress label Aug 19, 2024

github-actions bot added the area/central label Aug 19, 2024

Maddosaurus force-pushed the mm/rox-24117-nindexv4-acs branch 2 times, most recently from 63de2e2 to ab62d13 Compare August 22, 2024 14:09

Base automatically changed from mm/rox-24117-nindexv4-acs to master August 26, 2024 12:15

An error occurred while trying to automatically change base from mm/rox-24117-nindexv4-acs to master August 26, 2024 12:15

Maddosaurus mentioned this pull request Sep 4, 2024

ROX-25540: Update Helm charts #12608

Merged

9 tasks

Maddosaurus force-pushed the mm/rox-25797-enrich branch 2 times, most recently from c689a03 to b30f011 Compare September 26, 2024 10:32

github-actions bot added the area/scanner label Sep 26, 2024

Maddosaurus force-pushed the mm/rox-25797-enrich branch from bbabdd3 to 3463190 Compare September 27, 2024 08:13

Maddosaurus commented Sep 27, 2024

View reviewed changes

central/enrichment/integration.go Show resolved Hide resolved

Maddosaurus force-pushed the mm/rox-25797-enrich branch from 5f4fc42 to 351fa1b Compare September 27, 2024 09:40

Maddosaurus requested a review from a team September 27, 2024 09:42

Maddosaurus added the team/maple label Sep 27, 2024

Maddosaurus marked this pull request as ready for review September 27, 2024 11:01

Maddosaurus requested review from a team as code owners September 27, 2024 11:01

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 27, 2024

vikin91 requested changes Sep 27, 2024

View reviewed changes

Maddosaurus commented Sep 27, 2024

View reviewed changes

pkg/scanners/clairv4/clairv4.go Outdated Show resolved Hide resolved

vikin91 reviewed Oct 1, 2024

View reviewed changes

pkg/nodes/enricher/enricher_impl.go Outdated Show resolved Hide resolved

vikin91 approved these changes Oct 1, 2024

View reviewed changes

Maddosaurus and others added 17 commits October 7, 2024 09:48

Prefactor: Move scanner/mappers to pkg

3eec6b1

# Conflicts: # pkg/scannerv4/mappers/mappers.go # scanner/enricher/fixedby/fixedby.go # scanner/enricher/nvd/nvd.go # scanner/enricher/nvd/nvd_test.go # scanner/updater/manual/manual.go

Squash previously tracked parent branches

0591fc7

Point to CC fork for fixed scanners

0f7af0b

Housekeeping, WIP code

1794013

Move to latest master claircore Tidy dependencies Switch to uber gomock Fix import cycle, add key to sensor deduper

Scanner v4 is called on NodeIndex

49effb4

Make Scanner v4 known to NewWithCreator Set up default Scannerv4 to be Node Scanner Expand GetNodeInventoryScan to handle v4 IndexReports

Fix tests

5d3d70f

Clean up and remove rebase artifacts

02edb48

Clean up unneeded code, refine logs

3aff078

# Conflicts: # generated/storage/node_integration_vtproto.pb.go

Update protolock, add a test, fix test

05ef606

Review comments, pt1

7462877

Review comments, pt2

22aaa58

- Remove duplication between log messages and error wraps - Document static layer SHA - Remove TODO and keep shared max concurrent config for Node Index and Image Scans

Fix unit test and skip over unnecessary scanner calls

bf489c0

Rework loops, remove dead code

1a5c76b

Update pkg/nodes/enricher/enricher_impl.go

6446bbb

Improve conditional readability Co-authored-by: Piotr Rygielski <114479+vikin91@users.noreply.github.com>

Disable NodeInventory E2E tests on non-OpenShift environments

c8b4712

Move ignore annotation into test

f0aadf8

Rebase and regen proto code

66a7a13

Maddosaurus force-pushed the mm/rox-25797-enrich branch from 2d1b230 to 66a7a13 Compare October 7, 2024 07:53

Maddosaurus added 2 commits October 7, 2024 10:11

Remove skip condition from NodeInventoryTest

268826e

Ensure enricher only skips scanners on specific circumstances

0626984

Maddosaurus merged commit f45adfb into master Oct 9, 2024

Maddosaurus deleted the mm/rox-25797-enrich branch October 9, 2024 11:40

dcaravel reviewed Oct 9, 2024

View reviewed changes

central/imageintegration/store/defaults.go Show resolved Hide resolved

RTann reviewed Oct 28, 2024

View reviewed changes

		@@ -26,13 +28,20 @@ type CVESuppressor interface {

		// New returns a new NodeEnricher for the given Prometheus metrics subsystem and the Clair node scanner creator.
		func New(cves CVESuppressor, subsystem pkgMetrics.Subsystem) NodeEnricher {

Conversation

Maddosaurus commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-facing documentation

Testing and quality

Automated testing

How I validated my change

Uh oh!

openshift-ci bot commented Aug 19, 2024

Uh oh!

rhacs-bot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vikin91 commented Sep 27, 2024

Uh oh!

vikin91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maddosaurus commented Sep 27, 2024

Uh oh!

Uh oh!

vikin91 commented Sep 30, 2024

Uh oh!

vikin91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vikin91 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Maddosaurus commented Aug 19, 2024 •

edited

Loading

rhacs-bot commented Aug 20, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

openshift-ci bot commented Oct 9, 2024 •

edited

Loading