Skip to content

refactoring: using the new GraphQL resolver to get vuln requests for image vulns#381

Merged
sachaudh merged 4 commits intomasterfrom
saif/effective-vuln-requests-resolver
Jan 20, 2022
Merged

refactoring: using the new GraphQL resolver to get vuln requests for image vulns#381
sachaudh merged 4 commits intomasterfrom
saif/effective-vuln-requests-resolver

Conversation

@sachaudh
Copy link
Copy Markdown
Contributor

@sachaudh sachaudh commented Jan 20, 2022

Description

Given the way we're getting the vuln requests for the image vulns right now, there are a few bugs that pop up related to scoping and cause incorrect data to show in the tables in the Image Findings section. Mandar and Nakul provided a GraphQL resolver that would help with that issue. This PR makes UI changes in order to utilize this new resolver

This should fix the two must-have issues in https://docs.google.com/document/d/1hQHvY1ORnCN_RBlO6vRSDPzjNQnBLwJrZrh4ZnfmulI/edit#

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added

Testing Performed

The pending deferral request for CVE-2005-2541 in image gke.gcr.io/gcp-compute-persistent-disk-csi-driver:v1.2.4-gke.0 should not be shown for k8s.gcr.io/networking/ip-masq-agent-amd64:v2.6.0
Screen Shot 2022-01-19 at 7 49 51 PM
Screen Shot 2022-01-19 at 7 50 03 PM

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022 •
edited
Loading

@theencee @md2119 I noticed when I update a deferral to indefinitely by passing a 0 for expiresOn, the GraphQL query returns an unexpected date

Screen Shot 2022-01-19 at 8 10 45 PM
Screen Shot 2022-01-19 at 8 09 41 PM
Screen Shot 2022-01-19 at 8 14 08 PM

It's weird because I didn't see that earlier today. Not sure what happened

md2119
md2119 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Chic!

@sachaudh sachaudh added the pls-merge Indicates to the *reviewer* that the PR can be merged once tests pass and approval is granted label Jan 20, 2022
@md2119
Copy link
Copy Markdown
Contributor

md2119 commented Jan 20, 2022 •
edited
Loading

@sachaudh what was the API returning before? 1969 is a default value. Was the API returning the year 1969 before? It safe to assume any date prior to 2021 = indefinitely (but let's keep it tight <2000)

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022 •
edited
Loading

@sachaudh was the API returning the year 1969 before?

I don't think so. I remember it was returning a null before. The table would show a Never for the Expires column before

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022 •
edited
Loading

@md2119 what does this mean on the backend? Is it just a bug in the data being returned and it actually is indefinitely? If so, I can just do a manual check for the value and assume it's indefinitely, and we can fix it after release? If it isn't, maybe we should fix that

@md2119
Copy link
Copy Markdown
Contributor

md2119 commented Jan 20, 2022 •
edited
Loading

No, I don't think there is bug or regression for date field. It safe to assume any date prior to 2021 = indefinitely (but let's keep it tight <2000)

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022

Since we need to get this in for release, I'll do that, but I think we should ideally return a null since 1970-01-01T00:00:00Z feels like an odd value to return

@ghost
Copy link
Copy Markdown

ghost commented Jan 20, 2022 •
edited by ghost
Loading

Tag for build #125808 is 3.68.x-8-g9b1622f49c.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.68.x-8-g9b1622f49c'

📦 You can also generate an installation bundle with:

docker run -i --rm stackrox/main:3.68.x-8-g9b1622f49c central generate interactive > bundle.zip

🕹️ A roxctl binary artifact can be downloaded from CircleCI.

@sachaudh sachaudh force-pushed the saif/effective-vuln-requests-resolver branch from 2a01c08 to b6459e1 Compare January 20, 2022 04:25
@md2119
Copy link
Copy Markdown
Contributor

md2119 commented Jan 20, 2022

@sachaudh , I don't believe it ever worked that way. You are passing a non-nil value, which is 0 which defaults to default value of timestamp, that is 1970-01-01 00:00:00 +0000 UTC, which is what you are seeing. You have to nullify the field.

image

image

image

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022

@sachaudh , I don't believe it ever worked that way. You are passing a non-nil value, which is 0 which defaults to default value of timestamp, that is 1970-01-01 00:00:00 +0000 UTC, which is what you are seeing. You have to nullify the field.

Would they both be considered indefinite to the backend?

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022 •
edited
Loading

Hey folks i'm also seeing some more weird data. If you look at the images I pasted, you'll see that I'm querying for Vulnerability State:FALSE_POSITIVE yet one of the vulns shows that it's effective vulnerability request is OBSERVED. I'm not sure exactly what I did. I just tried to approve deferrals/false positives and then tried to reobserve them.

Screen Shot 2022-01-19 at 9 03 19 PM
Screen Shot 2022-01-19 at 9 03 16 PM

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022

@md2119 I ended up nullifying the field

vjwilson
vjwilson approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@vjwilson vjwilson left a comment

Choose a reason for hiding this comment

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

I do have a couple of nits and stylistic suggestions, but I'm OK with this as-is for the release testing.

I am worried that this is a significant amount of refactoring, and implies a significant amount of refactoring in the API, too, at the very end of development, right before the release.

@vjwilson vjwilson modified the milestones: 67.2-rc.2, 68.0-rc.2 Jan 20, 2022
@sachaudh sachaudh force-pushed the saif/effective-vuln-requests-resolver branch from 7adcde8 to 4f96a26 Compare January 20, 2022 17:57
@md2119
Copy link
Copy Markdown
Contributor

md2119 commented Jan 20, 2022

Saif, note the id is empty i.e. there is no request associated. The observed value is just the default value of state field for that object. Nothing to worry about. I am going to force the whole object to be nil so that FE does not see default values when it is empty.

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022

Saif, note the id is empty i.e. there is no request associated. The observed value is just the default value of state field for that object. Nothing to worry about. I am going to force the whole object to be nil so that FE does not see default values when it is empty.

Regarding the screenshot I sent, then doesn't that mean I should receive an empty array for vulns? If I reobserved it, I wouldn't expect there to be anything in that table anymore

@md2119
Copy link
Copy Markdown
Contributor

md2119 commented Jan 20, 2022

Saif, note the id is empty i.e. there is no request associated. The observed value is just the default value of state field for that object. Nothing to worry about. I am going to force the whole object to be nil so that FE does not see default values when it is empty.

Regarding the screenshot I sent, then doesn't that mean I should receive an empty array for vulns? If I reobserved it, I wouldn't expect there to be anything in that table anymore

Yup, this PR addresses your concern.

@sachaudh
Copy link
Copy Markdown
Contributor Author

sachaudh commented Jan 20, 2022

Yup, this PR addresses your concern.

Nice 👍🏼

@sachaudh sachaudh force-pushed the saif/effective-vuln-requests-resolver branch from 4f96a26 to 9b1622f Compare January 20, 2022 19:17
@sachaudh sachaudh merged commit cccf471 into master Jan 20, 2022
@sachaudh sachaudh deleted the saif/effective-vuln-requests-resolver branch January 20, 2022 23:04
vikin91 pushed a commit that referenced this pull request Jan 21, 2022
@sachaudh @vikin91
refactoring: using the new GraphQL resolver to get vuln requests for …
5a9b2fb 
…image vulns (#381)

(cherry picked from commit cccf471)
RTann pushed a commit that referenced this pull request Apr 6, 2022
@sachaudh
refactoring: using the new GraphQL resolver to get vuln requests for …
e28c833 
…image vulns (#381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedrottimark pedrottimark pedrottimark left review comments

@vjwilson vjwilson vjwilson approved these changes

@theencee theencee Awaiting requested review from theencee

+1 more reviewer

@md2119 md2119 md2119 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/ui pls-merge Indicates to the *reviewer* that the PR can be merged once tests pass and approval is granted

Projects

None yet

Milestone

68.0-rc.2

Development

Successfully merging this pull request may close these issues.

4 participants

@sachaudh @md2119 @vjwilson @pedrottimark