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

Return virtual index when creating and getting a log entry #725

Merged
merged 3 commits into from
Mar 11, 2022

Conversation

priyawadhwa
Copy link
Contributor

Use the virtual index when signing an entry on creation, and return that to the end user.
There shouldn't be any observable difference here at the moment, until we actually shard the log.

Summary

Ticket Link

Fixes

Release Note


@priyawadhwa priyawadhwa requested a review from a team as a code owner March 10, 2022 19:32
Use the virtual index when signing an entry on creation, and return that to the end user.
There shouldn't be any observable difference here at the moment, until we actually shard the log.

Signed-off-by: Priya Wadhwa <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #725 (eb98b43) into main (982ebd7) will increase coverage by 0.11%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   47.56%   47.68%   +0.11%     
==========================================
  Files          65       66       +1     
  Lines        5670     5696      +26     
==========================================
+ Hits         2697     2716      +19     
- Misses       2683     2691       +8     
+ Partials      290      289       -1     
Impacted Files Coverage Δ
pkg/sharding/ranges.go 40.00% <25.00%> (-40.00%) ⬇️
cmd/rekor-server/app/flags.go 64.44% <50.00%> (+3.66%) ⬆️
pkg/sharding/log_index.go 100.00% <100.00%> (ø)
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 982ebd7...eb98b43. Read the comment docs.

@bobcallaway
Copy link
Member

bobcallaway commented Mar 10, 2022

@priyawadhwa can you confirm there's no risk of api.logRanges changing underneath a call to VirtualLogIndex?

@priyawadhwa priyawadhwa force-pushed the universal-index branch 2 times, most recently from 7f87b47 to 98cd99e Compare March 10, 2022 23:18
Also make all fields private and only accessible via funcition calls

Signed-off-by: Priya Wadhwa <[email protected]>
@priyawadhwa
Copy link
Contributor Author

hey @bobcallaway for sure, made a couple changes:

  • removed the pointer from api.logRanges so it can't be modified
  • made logRanges.ranges private so it can only be accessed/modified via function call

@lkatalin
Copy link
Contributor

I'm testing this branch on top of #623 and got an index of 0 even though I have a few populated shards already:

[rekor@fedora rekor]$ rekor-cli upload --artifact tests/testfile06.txt --signature tests/testfile06.txt.sig --pki-format=ssh --public-key=/home/rekor/.ssh/id_rsa.pub --rekor_server http://127.0.0.1:3000
Created entry at index 0, available at: http://127.0.0.1:3000/api/v1/log/entries/7a9cbf47251dff69ed600735513c7ead7a7fe9d6d0c127581707031a352bc45d7d823db5a42e0345

I may be doing something wrong, though, my brain is a bit fried. If you don't mind waiting to merge until I can double check on it tomorrow, that would help!

@priyawadhwa
Copy link
Contributor Author

@lkatalin lmk if it still isn't working. I tried with #623 and it seems to be working as expected for me.

@lkatalin
Copy link
Contributor

It was user error / usage ambiguity: I was not manually passing in a logRangeMap, only a tlog_id, and so the VirtualLogIndex() did not detect multiple shards. I think we should implement a way to build up the logRangeMap internally to avoid problems like this (the user could still pass it in on the CLI at startup as well but tbh this would become redundant as there's no meaningful manual "override" for the actual shard map).

@lkatalin
Copy link
Contributor

I can rebase #727 on top of this once it's merged.

@dlorenc
Copy link
Member

dlorenc commented Mar 11, 2022

@bobcallaway is this good to merge now?

@bobcallaway bobcallaway merged commit da7c453 into sigstore:main Mar 11, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 11, 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
Development

Successfully merging this pull request may close these issues.

5 participants