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

Use logRangesFlag in API, route reads based on TreeID #671

Merged
merged 5 commits into from
Mar 5, 2022

Conversation

lkatalin
Copy link
Contributor

@lkatalin lkatalin commented Feb 15, 2022

Summary

This PR hooks up the logRangeMap flag to be used in API construction. The map can then be accessed to do routing for uuid-based and log-index-based reads.

I made the logRangeMap flag into a string because the Set() method can be used for construction and it was easier than trying to cram the data into a struct from the flag. But if there's a clean way to do this that doesn't involve this change, I'm happy to do it.

WIP because I still need to add some tests, fix the e2e test, and do some cleanup, ex. maybe around the int types. And I need to think through 1) making sure this will not break old clients, 2) do we need to use this logic in the writes too before merging (I think this will actually be a change made to the pending PR that emits longer UUIDs upon artifact upload).

Questions:

  • How can the current TreeID be accessed during e2e testing?
    • Instead of this, I added a condition that if the treeID is zero, we use the api.logID instead
  • In the case that both a tlog_id and logRangeMap are passed in, which takes precedence? The logRangeMap's ActiveIndex() should be the tlog_id (currently using ActiveIndex()).
    • Should we add a check to make sure these are the same? Edit: not doing this
    • Should we base the tlog_id in the API on the ActiveIndex? Edit: no, I'm appending the tlog_id to the rangeMap
    • Or should the tlog_id function as a flag for signaling the need for a new shard? This seems to be how it is used currently via createAndInitTree. Edit: yes, still doing this

Note: Currently it looks like the tlog_id in the API is used in only one place in api.go, to check whether it is zero and determine if the program should createAndInitTree.

Edit: So I think my answer to the above is to add logic that will append any nonzero tlog_id passed in at server startup to the API's logRange, see code from this PR here. If this isn't the right way, let me know or I guess we will change it later.

Ticket Link

Fixes #670
Fixes #632
Fixes #629

@lkatalin lkatalin requested a review from a team as a code owner February 15, 2022 00:37
@lkatalin
Copy link
Contributor Author

lkatalin commented Feb 15, 2022

TODO:

  • there's a bug when passing in a --trillian_log_server.tlog_id other than zero, because it doesn't get updated to the current one since it doesn't go through createAndInitTree() Update: FIXED

@lkatalin lkatalin force-pushed the lrmflags-staged branch 2 times, most recently from 102a04b to b2032c6 Compare February 16, 2022 14:51
@lkatalin lkatalin changed the title [WIP] Use logRangesFlag in API, route reads based on TreeID Use logRangesFlag in API, route reads based on TreeID Feb 17, 2022
Copy link
Member

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

LGTM

rangeMap := viper.GetString("trillian_log_server.log_id_ranges")
if rangeMap != "" {
if err := logRangeMap.Set(rangeMap); err != nil {
log.Logger.Errorf("unable to set logRangeMap from flag: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

if we can't set the logRangeMap should this error be fatal?

tc := NewTrillianClient(ctx)
tid, resolvedIndex := api.logRanges.ResolveVirtualIndex(int(params.LogIndex))
tc := NewTrillianClientFromTreeID(ctx, int64(tid))
log.Logger.Debugf("Retrieving resolved index %v from TreeID %v", resolvedIndex, tid)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.Logger.Debugf("Retrieving resolved index %v from TreeID %v", resolvedIndex, tid)
log.RequestIDLogger(params.HTTPRequest).Debugf("Retrieving resolved index %v from TreeID %v", resolvedIndex, tid)

Comment on lines 292 to 293
tc := NewTrillianClientFromTreeID(params.HTTPRequest.Context(), tid)
log.Logger.Debugf("Retrieving UUID %v from TreeID %v", entryUUID, tid)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tc := NewTrillianClientFromTreeID(params.HTTPRequest.Context(), tid)
log.Logger.Debugf("Retrieving UUID %v from TreeID %v", entryUUID, tid)
tc := NewTrillianClientFromTreeID(ctx, tid)
log.RequestIDLogger(params.HTTPRequest).Debugf("Retrieving UUID %v from TreeID %v", entryUUID, tid)

// Returns TreeID from an EntryID string
// TODO: Add some tests
func GetTreeIDFromEntryIDString(id string) (int64, error) {
if len(id) == UUIDHexStringLen {
Copy link
Member

Choose a reason for hiding this comment

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

style: rewrite this as a switch statement on len(id)?

@lkatalin
Copy link
Contributor Author

lkatalin commented Feb 22, 2022

I made the requested changes but also ended up adding some testing for GetTreeIDFromIDString() that led to a bunch of refactoring in the sharding package that now has its own commit.

Also ended up changing the type of the ints in logRange to int64 because that's what trillian uses.

The handling of a treeID of 0 is currently a bit awkward as I'm torn between flagging it as invalid vs. allowing it to be a special value that signifies we should just use the API's current tlog_id and/or the API's logRange's ActiveIndex().

Edit: Unless there are objections, I've made a treeID of 0 invalid. I think this makes the most sense as that can't actually be routed and doesn't add information to a plain UUID.

- Adds a function to get a TreeID from an ID string
- Adds testing for the above
- Consolidates validation logic for UUID, TreeID, EntryID
- Removes code that attempts to use ActiveIndex() in the sharding
  package, as this is not accessible due to import cycles
- Other small cleanup and typo fixes

Signed-off-by: Lily Sturmann <[email protected]>
This is the type used by the trillian TreeID and saves from having to
convert in multiple places.

Signed-off-by: Lily Sturmann <[email protected]>
WARNING: breaks loginfo cmd to current prod server

Signed-off-by: Lily Sturmann <[email protected]>
@lkatalin lkatalin force-pushed the lrmflags-staged branch 2 times, most recently from 3d204bd to 49cda5c Compare March 1, 2022 01:32
@lkatalin
Copy link
Contributor Author

lkatalin commented Mar 1, 2022

I added the treeID as output to rekor-cli loginfo and it does allow the e2e test to pass. However, I'm not sure how often the Rekor public server is updated with new code - there is an error when a loginfo with treeID request is sent to the currently running public Rekor server since it doesn't return this data.

Would it make more sense to return the treeID as a separate API endpoint after all so that people can still query the current server with loginfo? Or is this not an issue? @dlorenc @bobcallaway what do you think?

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@1530fb1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #671   +/-   ##
=======================================
  Coverage        ?   47.57%           
=======================================
  Files           ?       65           
  Lines           ?     5669           
  Branches        ?        0           
=======================================
  Hits            ?     2697           
  Misses          ?     2682           
  Partials        ?      290           

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 1530fb1...3b9c3d4. Read the comment docs.

@lkatalin
Copy link
Contributor Author

lkatalin commented Mar 4, 2022

@lukehinds @bobcallaway @dlorenc Is this okay to merge?

@lukehinds
Copy link
Member

lukehinds commented Mar 5, 2022

Looks good to me. I will merge by 'lazy consensus' late Monday if nothing more from @bobcallaway / @dlorenc is posted before then.

@dlorenc
Copy link
Member

dlorenc commented Mar 5, 2022

Lgtm!

@dlorenc dlorenc merged commit 2c11841 into sigstore:main Mar 5, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants