Skip to content

Fix scope lookups in image and cluster Vulnerabilities sub-resolvers#2715

Merged
charmik-redhat merged 2 commits intomasterfrom
charmik/fix-scope-searching-inside-image-vulns-sub-resolvers
Aug 18, 2022
Merged

Fix scope lookups in image and cluster Vulnerabilities sub-resolvers#2715
charmik-redhat merged 2 commits intomasterfrom
charmik/fix-scope-searching-inside-image-vulns-sub-resolvers

Conversation

@charmik-redhat
Copy link
Contributor

@charmik-redhat charmik-redhat commented Aug 16, 2022

Description

Some image and cluster vulnerability sub resolvers used to search the default ctx sent to the sub-resolver. The default ctx would not have the entity scoping added when vulnerabilities were queried from another entity. This PR updates the sub-resolvers in image and cluster vulnerability sub-resolvers to use the parent ctx to lookup scopes.

Checklist

  • Investigated and inspected CI test results
  • [ ] Unit test and regression tests added
  • [ ] 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)

If any of these don't apply, please comment below.

Testing Performed

Manual testing

  • Check if fixedByVersion, isFixable and discoveredAtImage in ImageVulnerability graphQL work as expected when imageVulnerabilities are queries from other entities.
  • Check if fixedByVersion and isFixable in ClusterVulnerability graphQl work as expected.

@openshift-ci
Copy link

openshift-ci bot commented Aug 16, 2022

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

@charmik-redhat charmik-redhat marked this pull request as ready for review August 16, 2022 19:27
Copy link
Contributor

@keyallis keyallis left a comment

Choose a reason for hiding this comment

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

Good catch on these, they were probably changes I made when migrating old code over

Copy link
Contributor

@md2119 md2119 left a comment

Choose a reason for hiding this comment

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

Although conjunction query are built, make sure datastores are getting the correct context to reduce the confusion.

@@ -386,7 +388,7 @@ func (resolver *clusterCVEResolver) EnvImpact(ctx context.Context) (float64, err

func (resolver *clusterCVEResolver) FixedByVersion(ctx context.Context) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: _ context.Context

func (resolver *clusterCVEResolver) FixedByVersion(ctx context.Context) (string, error) {
defer metrics.SetGraphQLOperationDurationTime(time.Now(), pkgMetrics.ClusterCVEs, "FixedByVersion")
scope, hasScope := scoped.GetScope(ctx)
scope, hasScope := scoped.GetScope(resolver.ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

pass the resolver.ctx to the datastore


// check scoping, add as conjunction if needed
if scope, ok := scoped.GetScope(ctx); !ok || scope.Level != v1.SearchCategory_CLUSTER_VULNERABILITIES {
if scope, ok := scoped.GetScope(resolver.ctx); !ok || scope.Level != v1.SearchCategory_CLUSTER_VULNERABILITIES {
Copy link
Contributor

Choose a reason for hiding this comment

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

pass the resolver.ctx to the datastore

@ghost
Copy link

ghost commented Aug 16, 2022

Images are ready for the commit at 2080f43.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.71.x-315-g2080f4343e.

@charmik-redhat charmik-redhat force-pushed the charmik/fix-scope-searching-inside-image-vulns-sub-resolvers branch from 06defe8 to 2080f43 Compare August 17, 2022 20:24
@openshift-ci
Copy link

openshift-ci bot commented Aug 17, 2022

@charmik-redhat: 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-qa-e2e-tests 2080f43 link false /test gke-postgres-qa-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.

@charmik-redhat charmik-redhat merged commit 414d725 into master Aug 18, 2022
@charmik-redhat charmik-redhat deleted the charmik/fix-scope-searching-inside-image-vulns-sub-resolvers branch August 18, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants