UUID parsing to include pre-pended shard ID by lkatalin · Pull Request #515 · sigstore/rekor

lkatalin · 2021-11-30T18:23:54Z

Summary

This is a draft! Feedback welcome, including overall design considerations. I've got some to-dos that I am still sorting out as listed here and in code comments. It's not many lines of code, but I did a fair amount of experimentation to get to this point as it's my first "real" Rekor PR. : )

Here is how it's working currently:

Ticket Link

Fixes #487

Release Note


This would change the format of the UUID that people use on the CLI and pre-pend the 64-digit hex UUID with 
a 6-digit human-readable decimal shard ID, like so: 
`ssssss-uuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu`
where `s` is a digit of the shard ID and `u` is a digit of the old UUID / hex-encoded trillian merkle leaf 
hash.

TODO: Allow for backwards-compatibility by letting people search for the old 64-digit UUIDs and internally prepending those with the current shard ID. Is there a way to allow two different valid lengths for a parameter in pflags.go and openapi.yaml? I'm looking into this.

lkatalin · 2021-11-30T18:28:51Z

@lukehinds @asraa @bobcallaway @dlorenc
This is what I've got so far... I'm actively working on the remaining TODOs. If you have any thoughts I'm happy to incorporate them. Thanks!

asraa · 2021-11-30T21:00:14Z

cmd/rekor-cli/app/pflags.go

-			return valueFactory(uuidFlag, validateString("required,len=64,hexadecimal"), "")
+			// this corresponds to the merkle leaf hash of entries, which is represented by a 64 character hexadecimal string, prepended by the 6-digit shard ID and '-'  separator
+			// TODO: does this have to be a literal? can we not use the const to define the len?
+			// TODO: is there a way to validate only the UUID part of this string as hexadecimal? or validate different parts of the string differently? can / should they be split into different flags?


you could make a custom validator! instead of validateString you could make validateShardedUUID with a custom impl like the ones found here

rekor/cmd/rekor-cli/app/pflags.go

Line 170 in 5025a24

func validateSHAValue(v string) error {

Thank you @asraa ! That's exactly what I want to do. I appreciate the example.

bobcallaway · 2021-12-01T14:33:17Z

pkg/api/entries.go


 	queuedLeaf := resp.getAddResult.QueuedLeaf.Leaf
-	uuid := hex.EncodeToString(queuedLeaf.GetMerkleLeafHash())
+	shardID := "000000"


is the thought that the shardID would be a base10 integer or base16 (hex) like the rest of the UUID?

@bobcallaway I was going to have it as a base10 integer so that it's more easily human-readable, but I'm definitely open to suggestions here. Is there an advantage to having it in hex?

lkatalin · 2021-12-06T23:40:24Z

I've got an updated version here that

Fixed the linter errors
Created a custom validator for the new fullID
Added some custom types / structs in sharding.go so the shardID is no longer hardcoded in entries.go

Outstanding todos:

Make sure tests pass
Don't hardcode the CurrentShardID as 0 but instead grab it from / increment it in... somewhere the state is kept? Maybe in a separate PR? I don't understand this part well yet.
Update custom validator function to allow the old 64-digit UUIDs
Maybe make the shardID hex? Do we want to do this?
Add some (possibly dummy at first) routing in get.go to route the request to the proper shard. Maybe in a separate PR?

lkatalin · 2021-12-06T23:45:12Z

Also, I am not sure whether it is normal with go code to have go.mod and go.sum update as part of a PR. I'm not doing this deliberately so if that's not meant to happen I can reset them.

lukehinds · 2021-12-13T13:50:49Z

Also, I am not sure whether it is normal with go code to have go.mod and go.sum update as part of a PR. I'm not doing this deliberately so if that's not meant to happen I can reset them.

That's fine, its different to cargo where you tend to only push the toml file.

lkatalin · 2021-12-14T00:29:49Z

I think this is almost ready for review, but I'm not sure how to reproduce the failing e2e test locally (and therefore figure out what's going on with it). It seems to pass:

[rekor@fedora rekor]$ sudo ./tests/e2e-test.sh 
starting services
rekor_redis-server_1 is up-to-date
rekor_mysql_1 is up-to-date
rekor_trillian-log-server_1 is up-to-date
rekor_trillian-log-signer_1 is up-to-date
rekor_rekor-server_1 is up-to-date
building CLI and server
waiting up to 60 sec for system to start
running tests
ok  	github.com/sigstore/rekor/tests	13.996s

dlorenc · 2021-12-14T14:04:22Z

Could be a flake if it passes locally trying to re-run it now.

The other thing to try might be making sure you're fully rebased.

lkatalin · 2021-12-14T14:57:40Z

Could be a flake if it passes locally trying to re-run it now.

The other thing to try might be making sure you're fully rebased.

Hmm, I had rebased on main last night before running the tests and I just did so again. Puzzling.

jeffvance · 2021-12-14T17:03:09Z

cmd/rekor-cli/app/pflags.go

+
+		// validate the ShardID
+		shardID := v[:sharding.ShardIDLen]
+		shardIDTag := "required," + "len=" + fmt.Sprintf("%v", sharding.ShardIDLen) + ",number"


nit: is there a reason this is not shardIDTag := "required, len=" + fmt.Sprintf("%v",?
Ditto for L215

@jeffvance No, there isn't. : ) Thanks for the catch, I will update this.

lkatalin · 2021-12-15T21:38:40Z

It does look like it's an empty attestation on the failing test:

--- FAIL: TestIntoto (0.23s)
    e2e_test.go:439: g.Attesation:  []
    e2e_test.go:443: unexpected end of JSON input

dlorenc · 2021-12-18T15:05:59Z

I've managed to reproduce this locally! Sorry it took a bit longer than expected, I'm on a new M1 Macbook and it took a bit to get the dev environment running there.

--- FAIL: TestIntoto (0.52s)
    e2e_test.go:439: g.Attesation:  []
    e2e_test.go:443: unexpected end of JSON input
FAIL
FAIL	github.com/sigstore/rekor/tests	0.710s
FAIL

I think I have a guess at the issue...

dlorenc · 2021-12-18T15:10:52Z

pkg/api/entries.go

 	queuedLeaf := resp.getAddResult.QueuedLeaf.Leaf
-	uuid := hex.EncodeToString(queuedLeaf.GetMerkleLeafHash())
+	shardID := sharding.NewCurrent()
+	uuid := shardID.ShardIDString + sharding.FullIDSeparator + hex.EncodeToString(queuedLeaf.GetMerkleLeafHash())


I think we need to make this a helper function, and call it anywhere we go from a MerkleLeafHash to a UUID.

To fix the e2e test, we also need it right here: https://github.com/sigstore/rekor/blob/main/pkg/api/entries.go#L95

@dlorenc I can change it to a helper function, that's a great idea.
Thank you for figuring out what's going on with the e2e test!

dlorenc · 2021-12-18T19:38:15Z

OK, got a passing test over here! #548

I think I see the issue now. Let me know if you want to chat a bit more this week, I have a few ideas.

lkatalin · 2021-12-19T00:03:31Z

OK, got a passing test over here! #548

I think I see the issue now. Let me know if you want to chat a bit more this week, I have a few ideas.

Thanks @dlorenc , I really appreciate you looking into it. I still need to understand how the changes you suggested are related to the e2e test. I'm also happy to incorporate those changes in whatever way you think is best to get this to pass CI and merge it asap.

dlorenc · 2021-12-19T01:51:19Z

I still need to understand how the changes you suggested are related to the e2e test

At a high level it's just a case of confusion between MerkleLeafHash and UUID - the code was storing something using MerkleTreeHash (an In-Toto Attestation), and then trying to retrieve it later by UUID.

The In-Toto Attestations are stored separately from the Transparency Log itself, to make them redactable if we ever need to. The failing test tests that behavior. The change in entries.go fixes that up so we retrieve it by the UUID instead of the MerkleTreeHash, so we're consistent.

The change in get.go is unnecessary, but helped me when debugging and is otherwise safe. You can drop it if you prefer and we can handle that one separately.

dlorenc · 2021-12-19T02:01:09Z

cmd/rekor-cli/app/sharding/sharding.go

+// TODO: The shardID will be part of the state and will be updated. Store it in state.go?
+// TODO: Add a check that the CurrentShardID cannot be updated beyond 999,999
+
+var CurrentShardID uint32 = 0


I think this will need to be passed or parsed out of the log index range flag, it would basically be the last one in the list: https://github.com/sigstore/rekor/blob/main/cmd/rekor-server/app/flags.go#L29

The client shouldn't need to know the active shard id - it's purely controlled server side by that flag.

Thanks, I will use the ActiveIndex() from here #549

dlorenc · 2021-12-19T02:02:49Z

cmd/rekor-cli/app/sharding/sharding.go

+}
+
+// Create a 6-digit string from shardID
+func IntToString(i uint32) string {


You can use strconv.ParseInt here, it'll also handle the error cases.

dlorenc · 2021-12-19T02:06:20Z

pkg/api/entries.go

 	metricNewEntries.Inc()

 	queuedLeaf := resp.getAddResult.QueuedLeaf.Leaf
-	uuid := hex.EncodeToString(queuedLeaf.GetMerkleLeafHash())


Actually, I'm not 100% sure this change is safe as is - I don't think it would be backwards compatible (we need a test for this). It means the server would start returning the shard-prefixed UUID to clients who create one - but older clients wouldn't have the change you added in here to get.go.

Older clients would then have a UUID that's invalid to them. I think we might need to drop this part for a release or two, to make sure clients can handle the new format.

Interesting, I had not considered that older clients might still be used. I can write a test to make sure this will work when we include it.

This should help out with sigstore#515. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

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

lkatalin · 2021-12-21T03:01:15Z

I still need to understand how the changes you suggested are related to the e2e test

At a high level it's just a case of confusion between MerkleLeafHash and UUID - the code was storing something using MerkleTreeHash (an In-Toto Attestation), and then trying to retrieve it later by UUID.

The In-Toto Attestations are stored separately from the Transparency Log itself, to make them redactable if we ever need to. The failing test tests that behavior. The change in entries.go fixes that up so we retrieve it by the UUID instead of the MerkleTreeHash, so we're consistent.

The change in get.go is unnecessary, but helped me when debugging and is otherwise safe. You can drop it if you prefer and we can handle that one separately.

@dlorenc Thank you for this explanation - I had totally missed that there is another part of the code using the UUID. I'm glad the CI caught this.

lkatalin · 2021-12-21T03:27:33Z

I'm still making changes to this.

This should help out with #515. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

bobcallaway · 2021-12-21T16:14:07Z

cmd/rekor-cli/app/get.go

+			} else if len(uuid) == sharding.FullIDLen {
+				params.EntryUUID = uuid[sharding.FullIDLen-sharding.UUIDLen:]
+			} else {
+				return nil, fmt.Errorf("invalid UUID length: %v", len(uuid))


should we augment this error message to tell the user what the valid length/format should be?

bobcallaway · 2021-12-21T16:25:33Z

cmd/rekor-cli/app/get.go

-					continue
-				}
-
+				// TODO: understand why we don't need k to match params.EntryUUID


k should match the UUID returned but as Dan mentioned in his other review comment there would be a concern about backward compatibility.

bobcallaway · 2021-12-21T16:31:49Z

cmd/rekor-cli/app/get.go

+			if len(uuid) == sharding.UUIDLen {
+				params.EntryUUID = uuid
+			} else if len(uuid) == sharding.FullIDLen {
+				params.EntryUUID = uuid[sharding.FullIDLen-sharding.UUIDLen:]
+			} else {
+				return nil, fmt.Errorf("invalid UUID length: %v", len(uuid))


also, if you end up writing a helper method for creating this within the sharding package then would it make sense to move the validation logic in there, as well as add a new helper method to parse the input?

lkatalin · 2022-02-01T15:17:39Z

Closed in favor of #583 #587 and #623 which together resolve #487

asraa reviewed Nov 30, 2021

View reviewed changes

bobcallaway reviewed Dec 1, 2021

View reviewed changes

lkatalin force-pushed the uuidparsing branch from bd5a874 to 22dcf10 Compare December 6, 2021 23:33

lkatalin force-pushed the uuidparsing branch from 22dcf10 to 9935f6d Compare December 6, 2021 23:42

lkatalin force-pushed the uuidparsing branch from 9935f6d to 2c88ec8 Compare December 13, 2021 23:09

lkatalin changed the title ~~Preliminary draft of UUID parsing (wip with todos)~~ UUID parsing to include pre-pended shard ID Dec 13, 2021

lkatalin force-pushed the uuidparsing branch from 2c88ec8 to d55d754 Compare December 14, 2021 00:09

jeffvance reviewed Dec 14, 2021

View reviewed changes

lkatalin marked this pull request as ready for review December 14, 2021 17:37

lkatalin force-pushed the uuidparsing branch 2 times, most recently from af3e1bc to 1f7be40 Compare December 15, 2021 21:24

dlorenc reviewed Dec 18, 2021

View reviewed changes

dlorenc reviewed Dec 19, 2021

View reviewed changes

dlorenc added a commit to dlorenc/rekor-cli that referenced this pull request Dec 19, 2021

Add another helper to return the active shard from the flag set.

c3e9f7b

This should help out with sigstore#515. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

dlorenc mentioned this pull request Dec 19, 2021

Add another helper to return the active shard from the flag set. #549

Merged

dlorenc added a commit to dlorenc/rekor-cli that referenced this pull request Dec 19, 2021

Add another helper to return the active shard from the flag set.

aad05c0

This should help out with sigstore#515. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

UUID parsing to include pre-pended shard ID

2819f5a

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

lkatalin force-pushed the uuidparsing branch from 1f7be40 to 2819f5a Compare December 21, 2021 02:56

lukehinds pushed a commit that referenced this pull request Dec 21, 2021

Add another helper to return the active shard from the flag set. (#549)

68f830e

This should help out with #515. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

bobcallaway reviewed Dec 21, 2021

View reviewed changes

dlorenc mentioned this pull request Dec 23, 2021

Refactor the shard map parsing so we can pass it down into the API ob… #564

Merged

lkatalin closed this Feb 1, 2022

Conversation

lkatalin commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Release Note

Uh oh!

lkatalin commented Nov 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkatalin commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkatalin commented Dec 6, 2021

Uh oh!

lukehinds commented Dec 13, 2021

Uh oh!

lkatalin commented Dec 14, 2021

Uh oh!

dlorenc commented Dec 14, 2021

Uh oh!

lkatalin commented Dec 14, 2021

Uh oh!

jeffvance Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkatalin commented Dec 15, 2021

Uh oh!

dlorenc commented Dec 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlorenc commented Dec 18, 2021

Uh oh!

lkatalin commented Dec 19, 2021

Uh oh!

dlorenc commented Dec 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkatalin commented Dec 21, 2021

Uh oh!

lkatalin commented Dec 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkatalin commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lkatalin commented Nov 30, 2021 •

edited

Loading

lkatalin commented Dec 6, 2021 •

edited

Loading

jeffvance Dec 14, 2021 •

edited

Loading