ROX-12942: Introduce Compliance Full Node Scan message#3298
ROX-12942: Introduce Compliance Full Node Scan message#3298Maddosaurus merged 9 commits intomasterfrom
Conversation
|
Images are ready for the commit at 9fd6488. To use with deploy scripts, first |
| @@ -0,0 +1,22 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
About the file name compliance_nodescan.proto. I think, the fact that we put Scanner in Compliance is just an implementation detail that we shouldn't promote to the contract. As a thought exercise, compliance_ part will become misleading if we decide to set up a dedicated DaemonSet for node scanner.
There was a problem hiding this comment.
I think it would make more sense to actually put this in proto/storage/node.proto next to Node and the older NodeScan. That way we can store it in Central once all the vulns are filled in, and we can use it in other places as well. It also decouples it from the internal API. I believe internalapi/... can import from storage/, right?
| @@ -0,0 +1,22 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
I think it would make more sense to actually put this in proto/storage/node.proto next to Node and the older NodeScan. That way we can store it in Central once all the vulns are filled in, and we can use it in other places as well. It also decouples it from the internal API. I believe internalapi/... can import from storage/, right?
|
|
||
| // Names and data types are designed to follow proto/storage/node.proto as close as possible | ||
| // Next tag: 7 | ||
| message FullNodeScan { |
There was a problem hiding this comment.
Mentioned on Slack, but I wonder if something like NodeScanV2 would also work? Makes it clear this is the newer version of node scanning
| @@ -0,0 +1,18 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Why not just update internalapi/sensor/compliance_iservice.proto? It already is implemented to control the communication between Compliance pods and Sensor. Node scanning can just be added to the oneof in MsgFromCompliance. Similarly, if we ever want to have Sensor trigger node scans or something, we can just update the MsgToCompliance message since the bidi stream already exists
There was a problem hiding this comment.
Originally, I wanted to make sure we have completely separated processing pipelines for the two different NodeScan messages.
With a better idea of the work involved to set up a new bidi stream, your idea makes way more sense. Let me set that up!
|
/retest |
- Remove standalone iservice and use existing bidi stream - Rename message FullNodeScan -> NodeScanV2 for better semantics - Move NodeScanV2 message into storages' Node proto
3473439 to
9fd6488
Compare
|
@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/test-infra repository. I understand the commands that are listed here. |
Description
As a prereq for later work, a new Protobuf message is introduced to represent a full node scan result (generated in compliance).
When the timer fires, Compliance scans the Node and then sends a
FullNodeScanRequestwith the scan results to Scanner.Checklist
[ ] Determined and documented upgrade steps[ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)As this PR only introduces a new protobuf message, no specific upgrade steps or user-facing changes are introduced
Testing Performed
None