ROX-12975: Extend Enricher to accept NodeInventory as a second parameter by vikin91 · Pull Request #4492 · stackrox/stackrox

vikin91 · 2023-01-20T16:51:36Z

Description

This PR was created as a split from #4430 in order to make the change easier to review.
This PR is meant to replace #3940.

Here, we change the call to EnrichNode, so that it accepts two parameters: Node and NodeInventory. This allows us to keep Node and NodeInventory proto messages separate.

⚠️ This implementation is prone to a race condition between the central pipelines: Node and NodeInventory. This issue will be resolved in a follow-up (ROX-14484).

Checklist

Investigated and inspected CI test results
Unit test and regression tests added (to some degree...)

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)

Testing Performed

CI
Deploy locally on Colima, enable FF and check logs
Deploy locally on Colima with Support RHEL content sets in a call to GetNodeVulnerabilities scanner#1059 and ROX-12943: Sensor sets NodeID in NodeInventory #4484 and see this in the UI

Deploy on OpenShift 4.11 (with Scanner PR ROX-14592: Ensure that RHCOS Node scan is not skipped by Scanner scanner#1065) and see this in the UI

openshift-ci · 2023-01-20T16:51:41Z

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

vikin91 · 2023-01-20T16:51:51Z

Current dependencies on/for this PR:

master
- PR ROX-13722: Readd nodeID to NodeInventory proto #4483
  - PR ROX-12975: Extend Enricher to accept NodeInventory as a second parameter #4492 👈
  - PR ROX-12943: Sensor sets NodeID in NodeInventory #4484

This comment was auto-generated by Graphite.

ghost · 2023-01-20T17:25:45Z

Images are ready for the commit at 1684b59.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.73.x-646-g1684b59a61.

tools/generate-helpers/pg-table-bindings/list.go

central/imageintegration/service/service_impl_test.go

central/sensor/service/pipeline/nodeinventory/pipeline.go

pkg/nodes/enricher/enricher_impl.go

pkg/scanners/clairify/convert.go

pkg/scanners/clairify/convert_test.go

vikin91 · 2023-01-31T10:21:43Z

/retest

vikin91 · 2023-01-31T17:01:08Z

The CI failure ImageScanningTest: Image scanning test to check if scan time is not null is unrelated - see test-failures Slack channel and the ticket: https://issues.redhat.com/browse/ROX-14619

openshift-ci · 2023-02-01T09:42:45Z

@vikin91: 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-postgres-upgrade-tests	`23806e7`	link	false	`/test gke-postgres-upgrade-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/test-infra repository. I understand the commands that are listed here.

Maddosaurus

Thanks for the sync review!

vikin91 · 2023-02-01T12:52:53Z

Will rebase this again after reading the suggestions to do so on Slack.

This reverts commit f71ac7d.

- convertNodeToVulnRequest when inventory is nil - use scanner from branch - addd debug logging

lvalerom

The code looks good to me, just one comment regarding the go.mod.

lvalerom · 2023-02-01T13:38:17Z

go.mod

 	github.com/stackrox/helmtest v0.0.0-20220930104945-c4a3c15e834a
 	github.com/stackrox/k8s-istio-cve-pusher v0.0.0-20210422200002-d89f671ac4f5
-	github.com/stackrox/scanner v0.0.0-20230120022619-96107ead11f0
+	github.com/stackrox/scanner v0.0.0-20230127183442-f5521e941607


I saw you reverted the tag in SCANNER_VERSION should this also be reverted?

I will double check that. My intention was to take both from master for now.

We need to bump the SCANNER_VERSION after this stackrox/scanner#1065 is merged, but let's not make too many steps at once.

I did go get -u github.com/stackrox/scanner@master and go mod tidy and committed 1684b59. Will deploy it locally again to test that thoroughly.

It looks good locally. I will let the CI finish its checks

vikin91 · 2023-02-01T16:48:45Z

CI looks good so far! I will merge this on Thu in the morning to land it after the cut of RC.1 for 3.74 happens.

openshift-ci bot added the do-not-merge/work-in-progress label Jan 20, 2023

This was referenced Jan 20, 2023

ROX-13722: Readd nodeID to NodeInventory proto #4483

Merged

ROX-12943: Sensor sets NodeID in NodeInventory #4484

Merged

github-actions bot added area/central ci-all-qa-tests Tells CI to run all API tests (not just BAT). labels Jan 20, 2023

vikin91 changed the title ~~Extend Enricher to accept NodeInventory as a second parameter~~ ROX-12975: Extend Enricher to accept NodeInventory as a second parameter Jan 20, 2023

vikin91 mentioned this pull request Jan 20, 2023

ROX-12975: Use new Scanner API for Node scanning #3940

Closed

5 tasks

Base automatically changed from piotr/ROX-13722-readd-node-id to master January 23, 2023 09:09

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch from caff3f5 to f8ade4b Compare January 23, 2023 09:44

vikin91 marked this pull request as ready for review January 23, 2023 17:05

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 23, 2023

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch from f8ade4b to bef41cf Compare January 24, 2023 09:38

github-actions bot added the area/postgres label Jan 24, 2023

vikin91 commented Jan 24, 2023

View reviewed changes

tools/generate-helpers/pg-table-bindings/list.go Show resolved Hide resolved

vikin91 requested a review from Maddosaurus January 24, 2023 15:53

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch 2 times, most recently from 9114872 to 6e5392c Compare January 26, 2023 08:50

rhybrillou reviewed Jan 26, 2023

View reviewed changes

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch 2 times, most recently from 1e40e65 to 7bef2b1 Compare January 27, 2023 10:24

This was referenced Jan 27, 2023

ROX-14592: Ensure that RHCOS Node scan is not skipped by Scanner stackrox/scanner#1065

Merged

Add usr/share/buildinfo to node scan stackrox/scanner#1066

Merged

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch from 3f92550 to 58e11cf Compare January 27, 2023 14:50

vikin91 mentioned this pull request Jan 30, 2023

[ROX-12943,-12975,-13722] Send NodeInventory as a separate message between Sensor and Central #4430

Closed

5 tasks

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch from 58e11cf to 098fadc Compare January 30, 2023 11:08

vikin91 requested a review from fredrb January 31, 2023 12:44

vikin91 requested review from jschnath and lvalerom January 31, 2023 12:44

Maddosaurus approved these changes Feb 1, 2023

View reviewed changes

vikin91 added 18 commits February 1, 2023 13:53

Extend Enricher to accept NodeInventory as a second parameter

fb576e4

Add imageScanServiceClient to clairify node scanner

00af4e5

This reverts commit f71ac7d.

Fixes and tmp changes:

39573b3

- convertNodeToVulnRequest when inventory is nil - use scanner from branch - addd debug logging

Bump scanner version and dependency

b58390f

Update Scanner dep from master

fda68b9

Add comments explaining the data race between the pipelines

0aadc6d

Rerun code generation

f869fca

Add more test coverage for convertNodeToVulnRequest

acc35b2

Refactor & comment on nil-ness of EnrichNodeWithInventory params

b938957

Fix typo

a6cafdd

Rename import to riskManager

562e75e

Unexport nodeinventory.newPipeline

baa2b6e

Add informational debug logging

f5f5659

Tmp: Pull scanner code from Victor's branch to check content sets

ca9a113

Ignore requests to delete node inventories

06dab9c

Use scanner version and deps from master

dc23382

Run go mod tidy again

31e6b7d

Minor: Correct log message

4e20215

vikin91 force-pushed the piotr/ROX-12975-change-enrichNode branch from 23806e7 to 4e20215 Compare February 1, 2023 12:55

lvalerom approved these changes Feb 1, 2023

View reviewed changes

Take scanner deps from master

1684b59

vikin91 merged commit f7b5a78 into master Feb 2, 2023

vikin91 deleted the piotr/ROX-12975-change-enrichNode branch February 2, 2023 10:58

Conversation

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

Uh oh!

Description

Checklist

Not apply

Testing Performed

Uh oh!

openshift-ci bot commented Jan 20, 2023

Uh oh!

vikin91 commented Jan 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vikin91 commented Jan 31, 2023

Uh oh!

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

Uh oh!

Uh oh!

openshift-ci bot commented Feb 1, 2023 • edited 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!

vikin91 commented Feb 1, 2023

Uh oh!

lvalerom left a comment

Choose a reason for hiding this comment

Uh oh!

lvalerom Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

vikin91 Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

vikin91 Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

vikin91 Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vikin91 commented Jan 20, 2023 •

edited

Loading

ghost commented Jan 20, 2023 •

edited by ghost

Loading

vikin91 commented Jan 31, 2023 •

edited

Loading

openshift-ci bot commented Feb 1, 2023 •

edited

Loading

vikin91 commented Feb 1, 2023 •

edited

Loading