ROX-13722: [Prefactor] Do not use scanner protos in node.proto by vikin91 · Pull Request #4392 · stackrox/stackrox

vikin91 · 2023-01-13T13:53:55Z

Description

This PR is a prefactor for #3757. It contains all changes that are safe
to land as they do not affect other parts of the system than RHCOS node
scanning feature.

The changes here are:

No longer use scanner protos in the node.proto
Add rhel_content_sets field in storage.NodeInventory_Components - see Scanner #1053
Use more realistic fake data in the fake node inventory

Checklist

Investigated and inspected CI test results
Unit test and regression tests added

Do not apply

Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

CI
Deployed a test cluster and investigated logs for errors

vikin91 · 2023-01-13T13:54:09Z

Current dependencies on/for this PR:

master
- PR ROX-13722: [Prefactor] Do not use scanner protos in node.proto #4392 👈

This comment was auto-generated by Graphite.

ghost · 2023-01-13T14:13:05Z

Images are ready for the commit at 2bed400.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.73.x-408-g2bed40071e.

Maddosaurus

I spotted some well placed small additions, addressing feedback we discussed in the initial review of #3757.
Well done, thanks for the updates!

Maddosaurus · 2023-01-13T15:19:48Z

central/sensor/service/pipeline/nodescansv2/pipeline.go


 	// TODO(ROX-12240, ROX-13053): Do something meaningful with the nodeInventory
-	log.Infof("Central received NodeInventory: %+v", nodeInventory)
+	log.Debugf("Central received NodeInventory: %+v", nodeInventory)


I see you already incorporated the feedback regarding loglevels. Great job!

That said, we're keeping the pipeline only until #3757 is merged, as NodeInventory is still an independent message here, and we depend on this pipeline to print the inventory, correct?

Yes, no changes to the pipeline here as it affects the decision that we need to do in #3757.

The nodescanv2 pipeline is a dud and does nothing except of printing the debug log message. Moreover, it is guarded by the feature-flag - see here

Maddosaurus · 2023-01-13T15:29:34Z

compliance/collection/nodeinventorizer/nodeinventory.go

+			Path:             executable.Path,
+			RequiredFeatures: nil,
+		}
+		if executable.GetRequiredFeatures() != nil {


Thanks for switchting to getters here, as discussed in the sync review!

jvdm · 2023-01-14T01:02:49Z

proto/storage/node.proto

+            string              version = 4;
+            string              arch = 5;
+            string              module = 6;
+            repeated string     cpes = 7;


With rhel_content_sets we don't need cpes anymore.

Got it removed in 36de34d

rhybrillou

Good work.
Some additional bulletproofing can be added in some places.

rhybrillou · 2023-01-16T10:57:14Z

compliance/collection/nodeinventorizer/nodeinventory.go


-	convertedComponents := make(map[string]*scannerV1.RHELComponent, 0)
+	convertedComponents := make(map[string]*storage.NodeInventory_Components_RHELComponent, 0)
 	for i, rhelc := range rc.Packages {


It might be worth adding a guard here for the case where rhelc is a nil pointer.

Done in fdbd8cd

rhybrillou · 2023-01-16T11:02:01Z

compliance/collection/nodeinventorizer/nodeinventory.go

+	arr := make([]*storage.NodeInventory_Components_RHELComponent_Executable, len(exe))
+	for i, executable := range exe {
+		arr[i] = &storage.NodeInventory_Components_RHELComponent_Executable{
+			Path:             executable.Path,


executable.GetPath()

Good point, added in fdbd8cd

rhybrillou · 2023-01-16T11:03:43Z

compliance/collection/nodeinventorizer/nodeinventory.go

+		if executable.GetRequiredFeatures() != nil {
+			arr[i].RequiredFeatures = make([]*storage.NodeInventory_Components_RHELComponent_Executable_FeatureNameVersion, len(executable.GetRequiredFeatures()))
+		}
+		for i2, fnv := range executable.GetRequiredFeatures() {


nit: would it make sense to nest this for loop in the previous block after the array allocation ?

dded in fdbd8cd

rhybrillou · 2023-01-16T11:15:17Z

proto/storage/node.proto

+    // Components represents a subset of the scannerV1.Components proto message containing only fields required for RHCOS node scanning
+    // We are not using scannerV1.Components here for the following reasons:
+    // - to avoid conflicts between v1 and scannerV1 APIs when generating the code in central/graphql/resolvers/generated.go
+    // - to not expose scanner v1 API over stackrox graphql API


Should there be a note to keep scannerV1.Components and storage.NodeInventory_Components in sync ?

Added that in 2bed400. Note that the function that does the reverse conversion in Central will be first added in a future PR, so I left the description generic here.

… in sync

jvdm

LGTM.

vikin91 · 2023-01-17T08:10:26Z

Thank you for all the reviews. I will merge this as it is as this state is a good starting point for further changes.

After meeting with Mandar yesterday, we might need to abandon #3757, readd node_id to NodeInventory and use a separate message (not Node) to transfer inventory between Sensor and Central.
I will make necessary experiments and explain why in the next days. Once we agree on that, I will readd the node_id in a followup to this PR.

vikin91 added 2 commits January 13, 2023 14:45

Do not use scanner protos in node.proto

967d082

Use more accurate data in fake NodeInventory

6adf1a9

github-actions bot added the area/central label Jan 13, 2023

vikin91 changed the title ~~Do not use scanner protos in node.proto~~ ROX-13722: [Prefactor] Do not use scanner protos in node.proto Jan 13, 2023

Update proto.lock

05a3dd9

vikin91 added 2 commits January 13, 2023 15:29

Add unit test for convertExecutables

1544adb

Update proto types used in TestNodeInventoryHandler

b539461

github-actions bot added the area/sensor label Jan 13, 2023

vikin91 requested review from Maddosaurus, jvdm and rhybrillou January 13, 2023 15:09

Maddosaurus approved these changes Jan 13, 2023

View reviewed changes

vikin91 mentioned this pull request Jan 13, 2023

ROX-13722: Move NodeInventory into Node #3757

Closed

8 tasks

jvdm requested changes Jan 14, 2023

View reviewed changes

jvdm mentioned this pull request Jan 14, 2023

ROX-13794: Add content sets to Components proto stackrox/scanner#1053

Merged

Remove CPEs from NodeInventory..RHELComponent

36de34d

rhybrillou approved these changes Jan 16, 2023

View reviewed changes

vikin91 added 2 commits January 16, 2023 16:10

Harden convertAndDedupRHELComponents

fdbd8cd

Add comment to keep scannerV1.Components and NodeInventory_Components…

2bed400

… in sync

vikin91 requested a review from jvdm January 16, 2023 16:59

md2119 approved these changes Jan 16, 2023

View reviewed changes

jvdm approved these changes Jan 16, 2023

View reviewed changes

vikin91 merged commit 39eab76 into master Jan 17, 2023

vikin91 deleted the piotr/ROX-13722-prefactor branch January 17, 2023 08:11

Conversation

vikin91 commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Do not apply

Testing Performed

Uh oh!

vikin91 commented Jan 13, 2023

Uh oh!

ghost commented Jan 13, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maddosaurus left a comment

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!

rhybrillou left a comment

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!

jvdm left a comment

Choose a reason for hiding this comment

Uh oh!

vikin91 commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vikin91 commented Jan 13, 2023 •

edited

Loading

ghost commented Jan 13, 2023 •

edited by ghost

Loading