ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory - #1004
Conversation
|
Skipping CI for Draft Pull Request. |
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
|
/test all |
|
/test all |
1 similar comment
|
/test all |
|
Images are ready for the commit at d26da56. To use the images, use the tag |
|
/test all |
61bf04c to
38050ee
Compare
|
/test all |
2 similar comments
|
/test all |
|
/test all |
f634efe to
a778e72
Compare
RTann
left a comment
There was a problem hiding this comment.
some comments. Also be sure to rebase to make the ose-jenkins E2E test pass
|
|
||
| $(PROTOC_FILE): $(PROTOC_DOWNLOADS_DIR) | ||
| @echo "+ $@" | ||
| ifeq (, $(shell which wget)) |
There was a problem hiding this comment.
should this be added to the stackrox repo, too?
There was a problem hiding this comment.
Not sure, it works for me in when I build stackrox without any additional changes. I will look how it is done there.
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| resp.InventoryFeatures = imagescan.ConvertFeatures(layer.Features) |
There was a problem hiding this comment.
hm maybe we should put this in a more common package so we don't mix image and node scanning libraries
There was a problem hiding this comment.
We could - we kind of intentionally planned to call the imagescan package in the design doc, so having it here is a consequence and it is visible (maybe that is good?).
I would rely here on your suggestion here if you have some particular pkg in mind.
There was a problem hiding this comment.
I moved it into a separate package now. Many existing pkgs caused import cycle, so I created a separate one in e693353
| Severity: "Moderate", | ||
| } | ||
|
|
||
| func TestGRPCGetRHCOSNodeVulnerabilities(t *testing.T) { |
There was a problem hiding this comment.
does this pass with the feature flag off right now? May need to enable it for tests?
There was a problem hiding this comment.
I will make sure that this is being executed with the FF.
There was a problem hiding this comment.
Yes, this is being executed in the CI. I know this because the test has failed :)
{Failed node_scan_rhcos_test.go:141:
Error Trace: /go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:141
Error: Should be true
Test: TestGRPCGetRHCOSNodeVulnerabilities/case-0
Messages: expected to find feat 'libksba:1.3.5-7.el8.x86_64' in the reply, but got none
node_scan_rhcos_test.go:141:
Error Trace: /go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:141
Error: Should be true
Test: TestGRPCGetRHCOSNodeVulnerabilities/case-0
Messages: expected to find feat 'tar:1.27.1.el8.x86_64' in the reply, but got none}
Do you know why the vulns are being found when running this e2e test locally but not on CI?
There was a problem hiding this comment.
Hoping to see the test being run in CI after applying 229e12d
There was a problem hiding this comment.
Apparently adding the feature-flag env to the chart did not enable it and the test was skipped. Do we have any other ideas how to make the test run on PRs and master?
There was a problem hiding this comment.
Sorry, I should have mentioned adding the env var to the chart will just enable the feature in the Scanner deployment. The tests run on CI (not in the deployment), so we'd have to set the var there as well. We never really had a need to skip a test due to a feature flag before, so we do not really have that infrastructure as of yet. I'm curious if the stackrox repo has this. I know we used to with CircleCI by updating the config.yml. Maybe @gavin-stackrox knows and there exists a solution which, preferably, avoids OpenShift CI configuration changes. Perhaps just a bash script in the ci/scripts directory which sets environment variables at the beginning of each step?
There was a problem hiding this comment.
I decided to remove the feature-flag. The feature will be used only if the arriving request to Scanner has non-empty node inventory. This does the same job that the feature flag is doing.
| Version: "0.0.1", | ||
| }, | ||
| NodeInventory: &v1.Components{ | ||
| Namespace: "Red Hat CoreOS", |
There was a problem hiding this comment.
just curious I wonder what this value actually would be
There was a problem hiding this comment.
Ups, confused it with the OS name. Not sure to be honest what is the correct value - should it be the cluster name or Openshift project name?
There was a problem hiding this comment.
I finally decided to put rhcos:4.11 here.
|
I will rebase as well as suggested. |
0af4f77 to
5952462
Compare
|
/retest |
1 similar comment
|
/retest |
|
After enabling the feature flag in code (setting default to true), the test executes properly: Now I would be interested how can we enable the feature flags for e2e tests in the PRs and master, but keep it disabled (if the default is false) for releases. |
Scanner has never really differentiated much between release and dev before for this kind of thing. Something like this, for example, cannot even be used unless Central requests for it anyway, as the network policy dictates only Central can make requests to Scanner. Scanner does not even enforce use of the default, only, when it's a release vs dev (like the stackrox repo does) because it never really had such a need before |
Would this suggest to use the feature flag in central and remove it from scanner? |
I'd say for now just enable the flag, and we can disable it before next release, if needed |
- Fix assertions when 0 vulns or 0 features are expected - Rename functions as suggested - Add comments when dummy values are used - Add comments in tents when expected & got params are switched in place (assert.Len)
1e07c0e to
6692db4
Compare
| name: "Uncertified scan is unsupported for RHCOS and returns no features", | ||
| request: buildRequestCase([]v1.Note{v1.Note_CERTIFIED_RHEL_SCAN_UNAVAILABLE}), | ||
| responseContains: &v1.GetNodeVulnerabilitiesResponse{ | ||
| Features: []*v1.Feature{}, | ||
| Features: []*v1.Feature{}, |
There was a problem hiding this comment.
Well, the problem is that grep which currently serves as an example package without vulnerabilities may get vulnerabilities detected in the future. The comparison with 0 will start failing then.
We can either:
- do nothing now and address the issue by changing the package when tests start breaking,
- find a different package that does so little that it will not have vulnerabilities (e.g. in Debian there are meta-packages that have empty contents but bring many transitive dependencies once installed),
- try see how to use a snapshot of vulnerability data in test so that you can be confident that detected vulnerabilities don't change.
I'm fine with any of these options.
Co-authored-by: Misha Sugakov <537715+msugakov@users.noreply.github.com>
|
I wanted to answer to this comment, but cannot find a way to do so :/
I acknowledge these options. While I tend to move on with the first option (keep as it is), I will add a comment stating that these tests may start failing in the future when new vulnerabilities are discovered for Regarding the different package than |
|
@vikin91: The following test 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. |
|
The e2e test failure is consistently appearing but unrelated to this PR: |
The answer was couple more attempts away (10 minutes). The ones which seemed most innocent: |
msugakov
left a comment
There was a problem hiding this comment.
Looks good to ship. Feel free to treat new comments as optional suggestions.
|
|
||
| for _, c := range cases { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| c := c |
There was a problem hiding this comment.
The page fails to call out explicitly is that this matters when you start goroutines (or the code that's called starts goroutines). We don't do start goroutines here and can get by without c := c.
| { | ||
| Name: "grep", | ||
| Version: "3.1-6.el8.x86_64", | ||
| Name: "grep", |
There was a problem hiding this comment.
See my .noarch suggestion #1004 (comment) . Optionally, up to you.
There was a problem hiding this comment.
Unfortunately golang-docs.noarch returns a vulnerability (the same that can be found for golang-docs:1.19.2.noarch).
Error Trace: /Users/prygiels/go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:192
Error: "[name:"CVE-2022-23806" description:"DOCUMENTATION: A flaw was found in the elliptic package of the crypto library in golang (...)" metadata_v2:<last_modified_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:L/A:H" score:8.2 exploitability_score:3.9 impact_score:4.2 > > severity:"Moderate" ]" should have 0 item(s), but has 1
Test: TestGRPCGetRHCOSNodeVulnerabilities/Selected_vulnerabilities_should_be_returned_by_the_certified_scan
Messages: Expected to find 0 vulnerabilities for feature 'golang-docs:.noarch'
There was a problem hiding this comment.
It looks like the CVE is for golang and not for the golang-docs, but this is what our scanner returns 🤷
There was a problem hiding this comment.
How about these?
git-core-doc.noarch
iso-codes.noarch
tzdata.noarch
There was a problem hiding this comment.
Following up in https://srox.slack.com/archives/C04213W844U/p1673027151155069
This PR extends the
GetNodeVulnerabilitiesAPI to accept aNodemessage that containsNodeInventory(see stackrox/stackrox#3755 and stackrox/stackrox#3757). The respective Central part of this request is still in progress and there is no PR for it yet.This is required to support RHCOS Node scanning over the
GetNodeVulnerabilitiesAPI.Additionally, this PR includes:
RHCOSNodeScanningPATHColleagues who do not havewgetbut havecurlHow tested
make imagemake deploy-localgo test -tags e2e -timeout=10s -v -run ^TestGRPCGetRHCOSNodeVulnerabilities$ github.com/stackrox/scanner/e2etestsmake clean-proto-depsand doing the steps described in https://github.com/stackrox/scanner#steps