Skip to content

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

Merged
jvdm merged 3 commits into
masterfrom
jvdm/ROX-13794/proto-updates/pull
Jan 16, 2023
Merged

ROX-13794: Add content sets to Components proto#1053
jvdm merged 3 commits into
masterfrom
jvdm/ROX-13794/proto-updates/pull

Conversation

@jvdm

@jvdm jvdm commented Jan 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description and rationale

Add a field to scannerV1.Components to carry the list of Content Sets found in a node inventory scan.

scannerV1.Component is used by Compliance node inventorizer to send the list of components detected in a node inventory scan. The Scanner initially used that payload to hold CPEs mapped during the detection/analysis phase. But to map CPEs, Scanner needs access to repository-to-cpe.json. Compliance doesn't have access to that file, yet.

To avoid adding one additional channel to Compliance to keep repo-to-CPE data up-to-date, we will return the Content Sets found in the node. That will allow Scanner to do the mapping when Central calls Scanner's GetNodeVulnerabilities().

Why scannerV1.Components

Because content sets are currently attached to a single O.S. installation or an image layer for node scanning. This is acceptable for nodes because there are no layer compositions. But for images, this might be limiting. I foresee the possibility of multiple images adding different content sets, and it will be hard to tell, com scannerV1.Components which ones are for each package. But hopefully, this field will be ignored by image scanning.

By using scannerV1.Components as part of the node inventory scan, I feel we are coupling image scanning with node scanning concerning the design of the proto message.

I am open to discussing alternatives.

Tests

  • UTs.

@jvdm jvdm changed the title Add content sets to Components proto ROX-13794: Add content sets to Components proto Jan 12, 2023
@ghost

ghost commented Jan 12, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Images are ready for the commit at a2c102b.

To use the images, use the tag 2.27.x-29-ga2c102bbfa.

@jvdm

jvdm commented Jan 13, 2023

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jan 13, 2023

Copy link
Copy Markdown

@jvdm: 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/e2e-tests a2c102b link false /test 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
vikin91 approved these changes Jan 13, 2023
View reviewed changes

@vikin91 vikin91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code look great. The change is okay as well, but let me propose an alternative to it.

Adding the field will mainly have influence on the Scanner repo, as scannerV1.Components proto will be translated into a stackrox proto type when we merge #3757 - see func protoComponentsFromScanComponents(c *nodes.Components) *storage.NodeInventory_Components in #3757.

As the life of scannerV1.Components will be short within the scope of stackrox code (only few lines in the inventorizer.go) we might as well return the content sets as an additional parameter from Analyze - would that maybe help keeping things cleaner in the component.proto?

@vikin91 vikin91 mentioned this pull request Jan 13, 2023
Merged
5 tasks
@jvdm

jvdm commented Jan 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@vikin91:

Adding the field will mainly have influence on the Scanner repo, as scannerV1.Components proto will be translated into a stackrox proto type when we merge #3757 - see func protoComponentsFromScanComponents(c *nodes.Components) *storage.NodeInventory_Components in #3757.

I am glad you guys are moving in that direction. As I mentioned in the description:

By using scannerV1.Components as part of the node inventory scan, I feel we are coupling image scanning with node scanning concerning the design of the proto message.

I also saw the following comment in the PR:

    // 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

👍

@vikin91:

As the life of scannerV1.Components will be short within the scope of stackrox code (only few lines in the inventorizer.go) we might as well return the content sets as an additional parameter from Analyze - would that maybe help keeping things cleaner in the component.proto?

I want so suggest we do one the following:

  1. Add ContentSets to node.Components.RHELv2Components
  2. In Compliance add a new field to NodeInventory.Components, ex. rhel_content_sets,

And we won't need cpe in NodeInventory.Components.RHELComponent.

(1.) would be closer to the actual use case of content_sets since each package might come from different content sets. But, AFAIK, we don't determine which specific content set each package came from. I am suggesting (2.) for completeness. The way I see things, both are fine since it's OK to add a per-package field later and deprecate the common one.

This was referenced Jan 13, 2023
Closed
Merged
@jvdm

jvdm commented Jan 13, 2023

Copy link
Copy Markdown
Contributor Author

See follow up changes at: #1055

@jvdm jvdm closed this Jan 14, 2023
@jvdm jvdm reopened this Jan 16, 2023
@jvdm
jvdm enabled auto-merge (squash) January 16, 2023 17:18
@jvdm
jvdm merged commit 9c465e6 into master Jan 16, 2023
@jvdm
jvdm deleted the jvdm/ROX-13794/proto-updates/pull branch January 16, 2023 17:33
@vikin91 vikin91 mentioned this pull request Jan 18, 2023
Merged
2 tasks
@vikin91

vikin91 commented Jan 18, 2023

Copy link
Copy Markdown
Contributor

I have a follow-up to this change: #1059
PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vikin91 vikin91 vikin91 approved these changes
@daynewlee daynewlee daynewlee approved these changes
@Maddosaurus Maddosaurus Awaiting requested review from Maddosaurus
@RTann RTann Awaiting requested review from RTann

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jvdm @vikin91 @daynewlee