/v1/log/entries/retrieve needs systematic test coverage.

sigstore / rekor

Software Supply Chain Transparency Log

https://sigstore.dev

Apache License 2.0

880 stars 163 forks source link

/v1/log/entries/retrieve needs systematic test coverage. #1041

Open var-sdk opened 2 years ago

var-sdk commented 2 years ago

Description

/v1/log/entries/retrieve is one of the more complicated APIs but lacks systematic test coverage and some code paths, like query by proposed entry, appear to have any test cases at all. There continue to be edge case bugs filed against this API. A systematic approach to testing would help harden the API for positive and negative cases.

Would adding this API to rekor-cli help? A lot of the e2e tests use the cli. The API is currently only used in a constrained way in verify.

asraa commented 2 years ago

I've always been curious why the rekor-cli get did not support getting by propsed entry.

I assumed that get should use SearchLogQuery which can handle the UUID, log index, and propsed entry. Meanwhile, get uses GetLogEntryByIndex or GetLogEntryByUUID.

IMO we should remove the redundancy here... if you have the index or entry UUID then use the v1/log/entries or v1/log/entries/UUID. I think log/retrieve should not re-implement those. But that's a pretty major breaking change, and I can understand why clients may want a handle-all endpoint.

bobcallaway commented 1 year ago

IMO we should remove the redundancy here... if you have the index or entry UUID then use the v1/log/entries or v1/log/entries/UUID. I think log/retrieve should not re-implement those. But that's a pretty major breaking change, and I can understand why clients may want a handle-all endpoint.

The main benefit would be to fetch multiple log entries in the context of a single HTTP request.

asraa commented 1 year ago

The main benefit would be to fetch multiple log entries in the context of a single HTTP request.

OH I see. That was probably the entire intent of the separate endpoint? In that case I change my opinion. Fetching a range is good.

bobcallaway commented 1 year ago

some notes I took while working #1144:

the API takes array of entryUUIDs, logIndices, and/or entries; for each of these three:

empty (expect empty array & HTTP 200)
one validly formed, and in log (expect success)
valid formed, but not in log (expect empty array & HTTP 200)
mix of validly formed (some correct, some incorrect) (expect only correct ones, HTTP 200)
malformed input (expect HTTP 400)
mix of validly formed and malformed (expect HTTP 400)
more than 10 total requested lookups (expect HTTP 429)

bobcallaway commented 1 year ago

Also should consider searching entries that span multiple shards