Skip to content
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

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

Open
var-sdk opened this issue Sep 8, 2022 · 5 comments
Open

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

var-sdk opened this issue Sep 8, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@var-sdk
Copy link

var-sdk commented Sep 8, 2022

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.

@var-sdk var-sdk added the enhancement New feature or request label Sep 8, 2022
@asraa
Copy link
Contributor

asraa commented Sep 9, 2022

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
Copy link
Member

bobcallaway commented Oct 20, 2022

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
Copy link
Contributor

asraa commented Oct 20, 2022

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
Copy link
Member

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
Copy link
Member

Also should consider searching entries that span multiple shards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants