Search inactive trees for GET by UUID requests #750

lkatalin · 2022-04-04T13:49:05Z

UPDATED:
This fix is useful for client v0.5 when there is more than one shard. When a GET by UUID request is made with a UUID (as opposed to the EntryID that has the prepended TreeID), this change will first check the active tree, then iterate through inactive trees to see if the UUID can be found in any of them.

Fixes the "expected bug" mentioned in #711

Add temporary fix to check old prod treeID for entry if UUID is not
found in active tree. This can be removed once client v0.5 is phased
out.

~~Related to #711 (the "expected bug") - this fix is only needed if we plan to shard the production server before the client v0.5 is phased out.~~

priyawadhwa · 2022-04-04T16:59:12Z

pkg/api/entries.go

+		// temporary workaround while users update to client v0.6, which has correct routing based on
+		// tree IDs. if entry is not found in currently active tree, tries again to retrieve entry
+		// from previously active tree.
+		formerTID, err := strconv.ParseInt("3904496407287907110", 10, 64)


Instead of hardcoding the tree ID, could we go through all inactive shards in ranges and see if the UUID exists in them?

Great idea! Done in updated version.

priyawadhwa · 2022-04-04T17:02:15Z

pkg/api/entries.go

@@ -303,7 +303,24 @@ func GetLogEntryByUUIDHandler(params entries.GetLogEntryByUUIDParams) middleware
 	switch resp.status {
 	case codes.OK:
 	case codes.NotFound:
-		return handleRekorAPIError(params, http.StatusNotFound, fmt.Errorf("grpc error: %w", resp.err), "")
+		// TODO: remove later


Instead of doing this logic here, I think it would make more sense in this conditional where we don't have a tree ID:

https://github.com/sigstore/rekor/blob/main/pkg/api/entries.go#L283-L290

so basically, if we don't have a tree id:

Check the active tree ID

If that fails, start going through each inactive shard & check those.

If none of those work, return an error

lkatalin · 2022-04-07T14:27:22Z

Some notes on the new version:

I made a new function called GetTrees() in api.go that can be used in entries.go to avoid doing a bunch of imports in that file.
I made a new function GetLogServerAndClients() for setting up the logClient and logAdminClient since this is used in both GetTrees() and NewAPI()
Pulled out logic to check a tree for a UUID into its own function AttemptRetrieveUUID() since this is now called multiple times in GetLogEntryByUUIDHandler()
Did some refactoring of GetLogEntryByUUIDHandler() because it had a lot of if/else statements. I think the switch makes it more readable but maybe it can be improved further.

I don't like returning the error from AttemptRetrieveUUID() as a bool, but could not quickly find a way to check a middleware.Responder to see if it is an error type - any ideas here?

codecov-commenter · 2022-04-07T14:30:41Z

Codecov Report

Merging #750 (ff9f2fd) into main (34d242e) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #750      +/-   ##
==========================================
+ Coverage   49.02%   49.11%   +0.08%     
==========================================
  Files          61       61              
  Lines        5566     5566              
==========================================
+ Hits         2729     2734       +5     
+ Misses       2542     2538       -4     
+ Partials      295      294       -1

Impacted Files	Coverage Δ
pkg/types/alpine/v0.0.1/entry.go	`63.17% <0.00%> (+1.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34d242e...ff9f2fd. Read the comment docs.

lkatalin · 2022-04-07T14:59:27Z

pkg/api/entries.go

+// Attempt to retrieve a UUID from a backend tree
+// TODO: don't return a bool that specifies whether it is an error, but instead check
+// the middleware.Responder to see if it is an error type (how?)
+func AttemptRetrieveUUID(params entries.GetLogEntryByUUIDParams, uuid string, tid int64) (middleware.Responder, bool) {


I would love to not return the bool here (it's just a marker for whether this is an error state) - is there some way to check a middleware.Responder to see if it's an error type? Or should we implement something? I wanted to throw this up quickly so didn't go down this path of investigation. Could also return a separate error but it would be great if even that is not necessary.

priyawadhwa · 2022-04-07T15:25:13Z

pkg/api/entries.go

+// Attempt to retrieve a UUID from a backend tree
+// TODO: don't return a bool that specifies whether it is an error, but instead check
+// the middleware.Responder to see if it is an error type (how?)
+func AttemptRetrieveUUID(params entries.GetLogEntryByUUIDParams, uuid string, tid int64) (middleware.Responder, bool) {


nit: I would just call this RetrieveUUID, it's implied that we're attempting to do so

That's fair :)

priyawadhwa · 2022-04-07T15:35:05Z

pkg/api/entries.go

-	case codes.NotFound:
-		return handleRekorAPIError(params, http.StatusNotFound, fmt.Errorf("grpc error: %w", resp.err), "")
+	// If there is an error, see if it is because the EntryID is a UUID and if so,
+	// handle it
 	default:


The switch statement within the switch statement is a little hard to read. How about a structure like this:

tidString, err := sharding.GetTreeIDFromIDString(params.EntryUUID) if err == nil { // Get the UUID from the specified TreeID } // This should get the active tree ID and the inactive IDs trees, err := GetTrees() if err != nil { return handleRekorAPIError(params, http.StatusBadRequest, err, "") } for _, t := range trees { logEntry, err := RetrieveUUID(params, uuid, t) if err != nil { continue } return entries.NewGetLogEntryByUUIDOK().WithPayload(logEntry) } // If we've reached this point, none of the shards have worked out, so return an error return handleRekorAPIError(params, http.StatusBadRequest, fmt.Errorf("unable to find UUID"), "")

I changed it to be more like this, let me know what you think about the readability.

priyawadhwa · 2022-04-07T15:45:55Z

pkg/api/entries.go

+		switch err.Error() {
+		// If EntryID is plain UUID (ex. from client v0.5), first try the active tree
+		case "cannot get treeID from plain UUID":
+			tid := api.logRanges.ActiveTreeID()


Won't GetTrees include the active Tree ID?

priyawadhwa · 2022-04-07T15:46:42Z

pkg/api/api.go

@@ -66,18 +66,41 @@ type API struct {
 	certChainPem string              // PEM encoded timestamping cert chain
 }

-func NewAPI(ranges sharding.LogRanges, treeID uint) (*API, error) {
+func GetTrees() (*trillian.ListTreesResponse, error) {


I think we should be getting this list of trees from the config and the tlog_id flag, rather than from the server. Right now, this will return all trees, but it doesn't make sense to check them if the user hasn't specified them in the config.

lkatalin · 2022-04-07T15:54:58Z

Thanks for the good comments @priyawadhwa , will incorporate these and get a new one up.

lkatalin · 2022-04-07T21:49:37Z

@priyawadhwa It's ready!

priyawadhwa

Thanks for the speedy changes @lkatalin ! Just a couple more comments and we should be good to go :)

priyawadhwa · 2022-04-07T23:26:00Z

pkg/api/entries.go

-		return handleRekorAPIError(params, http.StatusNotFound, fmt.Errorf("grpc error: %w", resp.err), "")
-	default:
-		return handleRekorAPIError(params, http.StatusInternalServerError, fmt.Errorf("grpc error: %w", resp.err), trillianUnexpectedResult)
+		resp, _ := RetrieveUUID(params, uuid, tid)


We probably shouldn't hide this error, might be useful to return it for the user if it's non-nil

This makes sense now when returning (models.LogEntry, error) from RetrieveUUID

priyawadhwa · 2022-04-07T23:30:41Z

pkg/api/entries.go

-		return handleRekorAPIError(params, http.StatusNotFound, errors.New("grpc returned 0 leaves with success code"), "")
-	}
+	// If EntryID is plain UUID (ex. from client v0.5), check all trees
+	if err.Error() == "cannot get treeID from plain UUID" {


Best practice would be not to hardcode this string in case someone changes the wording on the error later on. You could either:

Create an error type to check for (example here which uses errors.Is)

Change the GetUUIDFromIDString function to return "", nil in the case that there is no treeID. That way the error is nil, but we know if the treeID is empty that we have to go through all the shards

Yes, these options make more sense.

priyawadhwa · 2022-04-07T23:32:46Z

pkg/api/entries.go

+// Attempt to retrieve a UUID from a backend tree
+func RetrieveUUID(params entries.GetLogEntryByUUIDParams, uuid string, tid int64) (middleware.Responder, error) {
+	ctx := params.HTTPRequest.Context()
+	hashValue, _ := hex.DecodeString(uuid)


We should check this error in case someone passes in a malformed UUID?

I just moved this line of code from where it was earlier, but checking it seems like a good thing to do.

thanks ! :)

priyawadhwa · 2022-04-07T23:34:58Z

pkg/api/entries.go

@@ -438,3 +421,36 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo

 	return entries.NewSearchLogQueryOK().WithPayload(resultPayload)
 }
+
+// Attempt to retrieve a UUID from a backend tree
+func RetrieveUUID(params entries.GetLogEntryByUUIDParams, uuid string, tid int64) (middleware.Responder, error) {


I think it would still make more sense to return (models.LogEntry,, error) here and create the response in GetLogEntryByIndexHandler

Looks like creating a models.LogEntry will require turning a trillian.GetEntryAndProofResponse into a models.LogEntryAnon, is that right? Is there a straightforward way to do that? I'm looking at logEntryFromLeaf as an example but not really seeing how the fields match up.

Make a LogEntryAnon and unmarshal into it?

I think logEntryFromLeaf returns models.LogEntry, so after line 443 you could just return logEntry, nil?

Nice! You're so right. Thanks!

priyawadhwa

Couple small comments!

priyawadhwa · 2022-04-08T18:16:26Z

pkg/api/entries.go

@@ -438,3 +427,39 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo

 	return entries.NewSearchLogQueryOK().WithPayload(resultPayload)
 }
+
+var ErrNotFound = errors.New("grpc returned 0 leaves with success code")
+var ErrUnexpected = errors.New("unexpected error")


I don't think we need this ErrUnexpected since we don't check for it anywhere

Okay cool, I took it out!

priyawadhwa · 2022-04-08T18:16:35Z

pkg/api/entries.go

+
+		logEntry, err := logEntryFromLeaf(ctx, api.signer, tc, leaf, result.SignedLogRoot, result.Proof, tid, api.logRanges)
+		if err != nil {
+			return models.LogEntry{}, ErrUnexpected


as mentioned above we can just return err here

Signed-off-by: Lily Sturmann <lsturman@redhat.com>

priyawadhwa

LGTM! 🎉

lkatalin · 2022-04-08T19:03:17Z

lkatalin requested a review from a team as a code owner April 4, 2022 13:49

priyawadhwa reviewed Apr 4, 2022

View reviewed changes

lkatalin changed the title ~~Temporary fix for GET with UUID from inactive tree~~ Search inactive trees for GET by UUID requests Apr 7, 2022

lkatalin force-pushed the temp_uuid_fix branch from 02c5376 to ff9f2fd Compare April 7, 2022 14:19

lkatalin force-pushed the temp_uuid_fix branch from ff9f2fd to 9dde3f4 Compare April 7, 2022 14:41

lkatalin commented Apr 7, 2022

View reviewed changes

lkatalin mentioned this pull request Apr 7, 2022

Sharding open questions and todos remaining #711

Closed

14 tasks

priyawadhwa reviewed Apr 7, 2022

View reviewed changes

lkatalin force-pushed the temp_uuid_fix branch from 9dde3f4 to 79c6cb1 Compare April 7, 2022 21:13

priyawadhwa reviewed Apr 7, 2022

View reviewed changes

lkatalin force-pushed the temp_uuid_fix branch 2 times, most recently from 276f7bb to aa7e7b6 Compare April 8, 2022 17:20

priyawadhwa approved these changes Apr 8, 2022

View reviewed changes

Add search through inactive shards for GET by UUID

713be4c

Signed-off-by: Lily Sturmann <lsturman@redhat.com>

lkatalin force-pushed the temp_uuid_fix branch from aa7e7b6 to 713be4c Compare April 8, 2022 18:24

priyawadhwa approved these changes Apr 8, 2022

View reviewed changes

dlorenc approved these changes Apr 9, 2022

View reviewed changes

dlorenc merged commit 408b752 into sigstore:main Apr 11, 2022

github-actions bot added this to the v1.0.0 milestone Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search inactive trees for GET by UUID requests #750

Search inactive trees for GET by UUID requests #750

lkatalin commented Apr 4, 2022 •

edited

Loading

priyawadhwa Apr 4, 2022

lkatalin Apr 7, 2022

priyawadhwa Apr 4, 2022

lkatalin commented Apr 7, 2022

codecov-commenter commented Apr 7, 2022

lkatalin Apr 7, 2022

priyawadhwa Apr 7, 2022

lkatalin Apr 7, 2022

priyawadhwa Apr 7, 2022

lkatalin Apr 7, 2022

priyawadhwa Apr 7, 2022

priyawadhwa Apr 7, 2022

lkatalin commented Apr 7, 2022

lkatalin commented Apr 7, 2022

priyawadhwa left a comment

priyawadhwa Apr 7, 2022

lkatalin Apr 8, 2022

priyawadhwa Apr 7, 2022

lkatalin Apr 8, 2022

priyawadhwa Apr 7, 2022

lkatalin Apr 8, 2022

priyawadhwa Apr 8, 2022

priyawadhwa Apr 7, 2022

lkatalin Apr 8, 2022

lkatalin Apr 8, 2022

priyawadhwa Apr 8, 2022

lkatalin Apr 8, 2022

priyawadhwa left a comment

priyawadhwa Apr 8, 2022

lkatalin Apr 8, 2022

priyawadhwa Apr 8, 2022

priyawadhwa left a comment

lkatalin commented Apr 8, 2022

Search inactive trees for GET by UUID requests #750

Search inactive trees for GET by UUID requests #750

Conversation

lkatalin commented Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Apr 7, 2022

codecov-commenter commented Apr 7, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Apr 7, 2022

lkatalin commented Apr 7, 2022

priyawadhwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyawadhwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyawadhwa left a comment

Choose a reason for hiding this comment

lkatalin commented Apr 8, 2022

lkatalin commented Apr 4, 2022 •

edited

Loading