-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
httptransport: GET vuln report returns 404 when indexing in-progress #1963
Conversation
Can one of the admins verify this patch? |
@crozzy sorry for the bump - just checking if this PR has been overlooked? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea. Thinking through re-indexing, will there be a situation where the report is 200OK then is being re-indexed so will be 404Not Found the 200OK again?
@@ -12,6 +12,7 @@ import ( | |||
|
|||
"github.com/google/uuid" | |||
"github.com/quay/claircore" | |||
indexerController "github.com/quay/claircore/indexer/controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother with the alias here.
// now check bool only after confirming no err | ||
if !ok { | ||
// now check present and finished only after confirming no err | ||
if !ok || indexReport.State != indexerController.IndexFinished.String() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit but it might be nicer to compare the state types as opposed to the strings:
var state controller.State
state.FromString(indexReport.State)
if !ok || state != controller.IndexFinished {
...
I think that will happen, from the user's point of view the vuln report seems to be unavailable for a brief period. That's pretty much true, however. While (re-)indexing is in progress, there is no valid index report to base a vuln report on, so better to say it's unavailable than give an incorrect one? As a future extension, there could be a new response instead of HTTP404, to communicate "come back shortly", used when we know we ought to have a completed vuln report available shortly (as indexing is in progress, but hasn't completed) .... but this seems like an edge case that few will benefit from. |
5bf85e1
to
43282ce
Compare
43282ce
to
e24c65d
Compare
Sorry, sorted now |
429b560
to
7e4648d
Compare
Signed-off-by: Mark Frost <[email protected]>
7e4648d
to
546fedc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
For issue #1962
GET /matcher/api/v1/vulnerability_report/<digest>
returns HTTP404 when the index report is not complete, instead of a partial vulnerability report based on whatever is present in the indexThis doesn't distinguish between in-progress states (a later request will likely be successful) and the final error state
IndexError
(a later request will likely still be unsuccessful). I'm not sure what would be preferred - HTTP404? HTTP500 with some error body?