ROX-13722: Move NodeInventory into Node by vikin91 · Pull Request #3757 · stackrox/stackrox

vikin91 · 2022-11-09T14:03:48Z

Description

This PR changes the scope in which the storage.NodeInventory is treated as a standalone proto message.

Before this PR, NodeInventory was used as a standalone message between: Compliance -> Sensor -> Central.
After this PR is merged, it is: Compliance -> Sensor.

Sensor is responsible for connecting the NodeInventory with Node (goal of #3892) and sending Node further to central.

The newly added NodeInventory field will not be saved into the DB as a separate field or table, but it will be included in the serialized data in the DB.
NodeInventory is meant to be used in the request to Scanner to scan the packages included in it.
The reply from Scanner will be converted in Central into a storage.NodeScan that will
be inserted into the DB the same way as the existing v1 node scan data (i.e., without any need for further refactoring as NodeScan message is already being used).

Currently, the API between Scanner and Central is unable to handle the
NodeInventory - support for this will be added in separate PRs:

This PR handles the proto part for ROX-12943.
The proper implementation of ROX-12943 will be done in a follow-up - see #3892.

⚠️ See also the prefactor PR: #4392 that was created from this PR by extracting the non-risky parts. This PR will be rebased when the prefactor is merged.

Checklist

Investigated and inspected CI test results

Checks that do not apply

Unit test and regression tests added
- we will cover the tests in ROX-12943 as this PR only changes the proto structure and defines no extra behaviors.
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

Regenerated code with make proto-generated-srcs generated-srcs and checked central/graphql/resolvers/generated.go for errors
CI
Manual test consisting deploying a cluster locally, looking at the
Central logs and seeing that the Node with NodeInventory arrives.

Query Central HTTP API

The goal was to query the API to see how the data is provided by Central.
The inventory scanning was disabled in this test (for each inventory item, scanner returned 0 vulns).
Query: GET https://localhost:8443/v1/nodes/{clusterID}/{nodeID}

Node contains v1 and v2 data (scanner scans v1 only)

Call result

{
	"id": "bf5bf7d4-2d77-4194-9ab5-570848c55777",
	"name": "colima-fake",
	"taints": [],
	"clusterId": "4b3f14e0-b7ce-461d-8a36-b8698ff51f15",
	"clusterName": "remote",
	"labels": {
		"fakeLabelsK": "fakeLabelsV"
	},
	"annotations": {
		"fakeAnnotationsK": "fakeAnnotationsV"
	},
	"joinedAt": "2023-01-06T15:33:45.324727501Z",
	"internalIpAddresses": [
		"192.168.255.254"
	],
	"externalIpAddresses": [
		"10.10.255.254"
	],
	"containerRuntimeVersion": "docker://20.10.18",
	"containerRuntime": {
		"type": "DOCKER_CONTAINER_RUNTIME",
		"version": "20.10.18"
	},
	"kernelVersion": "99.19.16",
	"operatingSystem": "Alpine Linux v3.16",
	"osImage": "RedHat CoreOS v99.66.33",
	"kubeletVersion": "v1.25.0+k3s1",
	"kubeProxyVersion": "v1.25.0+k3s1",
	"lastUpdated": "2023-01-06T15:33:45.561847530Z",
	"k8sUpdated": "2023-01-06T15:33:45.325838631Z",
	"nodeInventory": {
		"nodeName": "colima",
		"scanTime": "2023-01-06T15:33:45.324727501Z",
		"components": {
			"namespace": "Testme OS",
			"rhelComponents": [
				{
					"id": "0",
					"name": "vim-minimal",
					"namespace": "rhel:8",
					"version": "2:7.4.629-6.el8.x86_64",
					"arch": "x86_64",
					"module": "FakeMod",
					"cpes": [
						"cpe:/a:redhat:enterprise_linux:8::baseos"
					],
					"addedBy": "FakeLayer",
					"executables": []
				},
				{
					"id": "0",
					"name": "libsolv",
					"namespace": "rhel:8",
					"version": "0.7.7-1.el8.x86_64",
					"arch": "x86_64",
					"module": "FakeMod",
					"cpes": [],
					"addedBy": "FakeLayer",
					"executables": []
				}
			]
		},
		"notes": [
			"LANGUAGE_CVES_UNAVAILABLE"
		]
	},
...
}

Node contains v1 data only

Call result

{
	"id": "f723ae15-334c-4c1e-a38a-453f27eba5f1",
	"name": "colima",
	"taints": [],
	"clusterId": "4b3f14e0-b7ce-461d-8a36-b8698ff51f15",
	"clusterName": "remote",
	"labels": {...},
	"annotations": {...},
	"joinedAt": "2023-01-06T09:29:58Z",
	"internalIpAddresses": [
		"192.168.5.15"
	],
	"externalIpAddresses": [],
	"containerRuntimeVersion": "docker://20.10.20",
	"containerRuntime": {
		"type": "DOCKER_CONTAINER_RUNTIME",
		"version": "20.10.20"
	},
	"kernelVersion": "5.15.82-0-virt",
	"operatingSystem": "linux",
	"osImage": "Alpine Linux v3.16",
	"kubeletVersion": "v1.25.4+k3s1",
	"kubeProxyVersion": "v1.25.4+k3s1",
	"lastUpdated": "2023-01-06T14:16:11.495989645Z",
	"k8sUpdated": "2023-01-06T14:15:30.578707270Z",
	"nodeInventory": null,
...
}

Query Central GraphQL API

POST https://localhost:8443/api/graphql

gQL querries

query nodev1v2 {
	 node(id: "bf5bf7d4-2d77-4194-9ab5-570848c55777") {
		...fields
	}
}

query nodev1 {
	node(id: "f723ae15-334c-4c1e-a38a-453f27eba5f1") {
		...fields
	}
}

fragment fields on Node {
   # arbitrary selected fields including
   nodeInventory{...} # input to scanner
   scan{...} # output from scanner
}

Node contains v1 and v2 data (scanner scans v1 only)

Call result

{
	"data": {
		"node": {
			"id": "bf5bf7d4-2d77-4194-9ab5-570848c55777",
			"name": "colima-fake",
			"nodeInventory": {
				"nodeName": "colima",
				"scanTime": "2023-01-06T16:11:25.317593632Z",
				"notes": [
					"LANGUAGE_CVES_UNAVAILABLE"
				],
				"components": {
					"rhelComponents": [
						{
							"id": 0,
							"namespace": "rhel:8",
							"name": "vim-minimal",
							"version": "2:7.4.629-6.el8.x86_64",
							"arch": "x86_64",
							"cpes": [
								"cpe:/a:redhat:enterprise_linux:8::baseos"
							]
						},
						{
							"id": 0,
							"namespace": "rhel:8",
							"name": "libsolv",
							"version": "0.7.7-1.el8.x86_64",
							"arch": "x86_64",
							"cpes": []
						}
					]
				}
			},
			"scan": {
				"scanTime": "2023-01-06T16:11:25.511172079Z",
				"nodeComponents": [
					{
						"id": "docker#20.10.18#",
						"name": "docker",
						"nodeVulnerabilityCount": 0
					},
					{
						"id": "kernel#99.19.16#",
						"name": "kernel",
						"nodeVulnerabilityCount": 13
					},
					{
						"id": "kubelet#v1.25.0+k3s1#",
						"name": "kubelet",
						"nodeVulnerabilityCount": 0
					},
					{
						"id": "kube-proxy#v1.25.0+k3s1#",
						"name": "kube-proxy",
						"nodeVulnerabilityCount": 0
					}
				]
			}
		}
	}
}

Node contains v1 data only

Call result

{
	"data": {
		"node": {
			"id": "f723ae15-334c-4c1e-a38a-453f27eba5f1",
			"name": "colima",
			"nodeInventory": null,
			"scan": {
				"scanTime": "2023-01-06T14:16:11.4924593Z",
				"nodeComponents": [
					{
						"id": "docker#20.10.20#",
						"name": "docker",
						"nodeVulnerabilityCount": 0
					},
					{
						"id": "kernel#5.15.82-0-virt#",
						"name": "kernel",
						"nodeVulnerabilityCount": 181
					},
					{
						"id": "kubelet#v1.25.4+k3s1#",
						"name": "kubelet",
						"nodeVulnerabilityCount": 0
					},
					{
						"id": "kube-proxy#v1.25.4+k3s1#",
						"name": "kube-proxy",
						"nodeVulnerabilityCount": 0
					}
				]
			}
		}
	}
}

openshift-ci · 2022-11-09T14:03:51Z

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 · 2022-11-09T14:04:04Z

Current dependencies on/for this PR:

master
- PR ROX-13540: Allow Sensor Component NodeScanHandler to start max. once #3800
  - PR Rename NodeScanV2 to NodeInventory #3755
    - PR ROX-13722: Move NodeInventory into Node #3757 👈
      - PR ROX-12975: Use new Scanner API for Node scanning #3940
        
        PR Port changes from mm/ROX-12967-real-nodescan #3941
      - PR ROX-12943: Sensor maps the NodeInventory to Node #3892

This comment was auto-generated by Graphite.

proto/storage/node.proto

central/graphql/generator/codegen/utils.go

Maddosaurus · 2023-01-09T15:28:45Z

Besides the Style errors you're getting, this looks good to me!
I'll re-review once these are sorted out for the protobuf :)

vikin91 · 2023-01-09T15:33:56Z

Besides the Style errors you're getting, this looks good to me! I'll re-review once these are sorted out for the protobuf :)

The style errors are wrong in this case - I made the changes to avoid potential compatibility issues and the protolock started complaining. The solution here would be to overwrite the protolock file and force-ignore the errors, but I wanted to consult with @rhybrillou first.

vikin91 · 2023-01-11T17:31:54Z

In 1398927 I addressed one of @md2119 comments from slack thread - point E

vikin91 · 2023-01-12T14:34:49Z

proto/storage/node.proto

+    string                         node_name  = 1;
+    google.protobuf.Timestamp      scan_time  = 2;
+
+    // Components represents a subset of the scannerV1.Components proto message containing only fields required for RHCOS node scanning


Put also comment in Scanner proto that this field is related to scannerV1.Components and should be updated in pair.

vikin91 · 2023-01-12T14:37:04Z

compliance/collection/nodeinventorizer/nodeinventory.go

 }

-func makeComponentKey(component *scannerV1.RHELComponent) string {
+func convertExecutables(exe []*scannerV1.Executable) []*storage.NodeInventory_Components_RHELComponent_Executable {


Make sure to use getters here to avoid nils

vikin91 · 2023-01-12T14:39:53Z

compliance/collection/nodeinventorizer/nodeinventory.go

 			Module:      rhelc.Module,
 			Cpes:        rc.CPEs,
-			Executables: rhelc.Executables,
+			Executables: convertExecutables(rhelc.Executables),


Make sure to use getters here to avoid nils

vikin91 · 2023-01-12T14:43:15Z

proto/internalapi/central/sensor_events.proto

        storage.NamespaceMetadata      namespace          = 6;
        storage.Secret                 secret             = 7;
        storage.Node                   node               = 9;
-        storage.NodeInventory          node_inventory     = 25;


Consider Next tag: 25. vs. leaving 26

Try reserving 25 outside of oneof

vikin91 · 2023-01-13T16:12:03Z

Less risky parts of this PR have been moved into a separate prefactor PR: #4392

jvdm · 2023-01-13T18:26:37Z

proto/storage/node.proto

+        message RHELComponent {
+            int64               id          = 1;
+            string              name        = 2;
+            string              namespace   = 3;
+            string              version     = 4;
+            string              arch        = 5;
+            string              module      = 6;
+            repeated string     cpes        = 7;
+            string              added_by    = 8;
+            message Executable {
+                string                      path              = 1;
+                message FeatureNameVersion {
+                    string name    = 1;
+                    string version = 2;
+                }
+                repeated FeatureNameVersion required_features = 2;
+            }
+            repeated Executable executables = 9;
+        }


As part of ROX-13794, we will not map CPEs in the node "invetorization" (aka. scanning). Instead, we will return the content sets found in the O.S. installation. The CPE mapping will be performed by Central Scanner when matching vulnerabilities. Because of that, we need to replace cpe with content_sets. Two options:

Replace the cpe field in RHELComponent with content_sets.

Remove the cpe field and add a content_sets field to Component.

Option 2 exists because today, we cannot determine precisely which content set each package came from, so we assume the package might have come from any of the content sets specified in the O.S. installation. This may be possible in the long future, so we might just use individual fields just as we do with CPEs, especially considering the size is neglectable.

CC @vikin91

See related PR: stackrox/scanner#1053

Thanks! As #4392 is a prefactor to this PR, the change will be addressed in #4392 (option 2) and then this PR will be rebased.

openshift-ci · 2023-01-17T10:03:44Z

@vikin91: The following tests 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/style-checks	`96bb013`	link	true	`/test style-checks`
ci/prow/openshift-oldest-operator-e2e-tests	`96bb013`	link	false	`/test openshift-oldest-operator-e2e-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.

vikin91 · 2023-01-19T15:16:09Z

Converting this to draft, as this PR will be replaced by #4430 (or its descendants)

vikin91 · 2023-01-20T15:42:39Z

Closing in favor of #4483 and its descendants

openshift-ci bot added the do-not-merge/work-in-progress label Nov 9, 2022

vikin91 mentioned this pull request Nov 9, 2022

ROX-12835: Add support for NodeScanV2 to Sensor #3533

Merged

2 tasks

github-actions bot added the area/central label Nov 9, 2022

vikin91 mentioned this pull request Nov 9, 2022

Rename NodeScanV2 to NodeInventory #3755

Merged

7 tasks

github-actions bot added the area/sensor label Nov 9, 2022

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 1a86416 to 3cdf143 Compare November 9, 2022 14:21

vikin91 force-pushed the pr/rename-nodeScanV2 branch from 1c6b5a7 to 7ffbcf8 Compare November 9, 2022 14:32

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 3cdf143 to 1ec1f69 Compare November 9, 2022 14:32

vikin91 force-pushed the pr/rename-nodeScanV2 branch 2 times, most recently from af46a2d to ce78183 Compare November 15, 2022 08:52

vikin91 mentioned this pull request Nov 15, 2022

ROX-13540: Allow Sensor Component NodeScanHandler to start max. once #3800

Merged

9 tasks

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 1ec1f69 to 2ae3d03 Compare November 15, 2022 09:04

vikin91 changed the title ~~Proto: Move NodeInventory into Node~~ ROX-12943: Move NodeInventory into Node Nov 15, 2022

vikin91 force-pushed the pr/rename-nodeScanV2 branch from ce78183 to 681ff0b Compare November 15, 2022 13:08

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from dbb52af to d45f428 Compare November 15, 2022 13:09

vikin91 commented Nov 15, 2022

View reviewed changes

proto/storage/node.proto Outdated Show resolved Hide resolved

vikin91 force-pushed the pr/rename-nodeScanV2 branch from 681ff0b to 66d340b Compare November 16, 2022 15:40

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from d45f428 to 5bff80b Compare November 16, 2022 15:42

vikin91 force-pushed the pr/rename-nodeScanV2 branch from 66d340b to b5caa72 Compare November 21, 2022 10:55

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 5bff80b to 0a7218e Compare November 21, 2022 10:57

vikin91 mentioned this pull request Nov 22, 2022

ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory stackrox/scanner#1004

Merged

4 tasks

vikin91 force-pushed the pr/rename-nodeScanV2 branch from b5caa72 to 9e75bcd Compare November 23, 2022 09:12

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 0a7218e to ca013ad Compare November 23, 2022 09:13

vikin91 mentioned this pull request Nov 23, 2022

ROX-12943: Sensor maps the NodeInventory to Node #3892

Closed

5 tasks

vikin91 force-pushed the pr/rename-nodeScanV2 branch from 9e75bcd to 9414f33 Compare November 23, 2022 14:29

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from ca013ad to c3df70d Compare November 23, 2022 14:30

vikin91 force-pushed the pr/rename-nodeScanV2 branch from 9414f33 to 39f20f4 Compare November 29, 2022 12:11

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from c3df70d to f6906a5 Compare November 29, 2022 12:22

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch 3 times, most recently from 6bf493c to c94f16d Compare January 6, 2023 10:46

Maddosaurus reviewed Jan 9, 2023

View reviewed changes

proto/storage/node.proto Outdated Show resolved Hide resolved

Maddosaurus reviewed Jan 9, 2023

View reviewed changes

central/graphql/generator/codegen/utils.go Outdated Show resolved Hide resolved

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from 615abd4 to 1398927 Compare January 11, 2023 17:12

vikin91 commented Jan 12, 2023

View reviewed changes

vikin91 mentioned this pull request Jan 13, 2023

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

Merged

5 tasks

jvdm reviewed Jan 13, 2023

View reviewed changes

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from fb20a0e to d7da815 Compare January 17, 2023 08:28

vikin91 added 2 commits January 17, 2023 09:29

Proto: Move NodeInventory into Node

3ea8b25

Regenerate graphql resolvers

96bb013

vikin91 force-pushed the pr/add-nodeComponents-to-Node branch from d7da815 to 96bb013 Compare January 17, 2023 09:44

vikin91 mentioned this pull request Jan 17, 2023

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

Closed

5 tasks

vikin91 marked this pull request as draft January 19, 2023 15:15

openshift-ci bot added the do-not-merge/work-in-progress label Jan 19, 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

vikin91 closed this Jan 20, 2023

vikin91 deleted the pr/add-nodeComponents-to-Node branch March 26, 2024 10:27

Conversation

vikin91 commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Checks that do not apply

Testing Performed

Query Central HTTP API

Node contains v1 and v2 data (scanner scans v1 only)

Node contains v1 data only

Query Central GraphQL API

Node contains v1 and v2 data (scanner scans v1 only)

Node contains v1 data only

Uh oh!

openshift-ci bot commented Nov 9, 2022

Uh oh!

vikin91 commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maddosaurus commented Jan 9, 2023

Uh oh!

vikin91 commented Jan 9, 2023

Uh oh!

vikin91 commented Jan 11, 2023

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!

vikin91 commented Jan 13, 2023

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!

openshift-ci bot commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikin91 commented Jan 19, 2023

Uh oh!

vikin91 commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vikin91 commented Nov 9, 2022 •

edited

Loading

vikin91 commented Nov 9, 2022 •

edited

Loading

openshift-ci bot commented Jan 17, 2023 •

edited

Loading