Add logic to GET artifacts via old or new UUID by lkatalin · Pull Request #587 · sigstore/rekor

lkatalin · 2022-01-07T01:36:22Z

Summary

This PR has 5 commits:

Moves ranges.go and its testing into the sharding package because it is related to sharding and I was getting import cycle errors otherwise.
Updates the name FullID to EntryID
Adds unit tests and a few more helper functions to sharding package
Adds logic to handle both new and old UUIDs during a GET request
Adds an e2e test to make sure a GET request with a longer EntryID is still handled properly

TODO: I think this would benefit from an integration test before being merged, but I'm still looking into how to add that. I assume it should have its own function is tests/e2e_test.go?

Ticket Link

Related to #487
(needs another PR before that can be closed)

bobcallaway · 2022-01-07T22:39:26Z

pkg/sharding/sharding.go

+
+// Returns UUID (with no prepended TreeID) from a UUID or FullID string
+func GetUUIDFromIDString(id string) string {
+	return id[len(id)-UUIDHexStringLen:]


what happens here if someone passes in a string that isn't a valid ID? you could end up panicking if the lengths are wrong. I think we need some sort of a defensive check here (with potentially an error return value as well)

You are right @bobcallaway , sorry I missed it.

lkatalin · 2022-01-10T15:27:51Z

I'm still going to try to add an e2e test in here.

bobcallaway · 2022-01-11T14:13:30Z

cmd/rekor-cli/app/get.go

+			entryUUID, err := sharding.GetUUIDFromIDString(uuid)
+			if err != nil {
+				return nil, fmt.Errorf("unable to parse uuid: %w", err)
+			}
+			params.EntryUUID = entryUUID


Suggested change

entryUUID, err := sharding.GetUUIDFromIDString(uuid)

if err != nil {

return nil, fmt.Errorf("unable to parse uuid: %w", err)

}

params.EntryUUID = entryUUID

params.EntryUUID, err = sharding.GetUUIDFromIDString(uuid)

if err != nil {

return nil, fmt.Errorf("unable to parse UUID: %w", err)

}

bobcallaway · 2022-01-11T14:13:43Z

cmd/rekor-cli/app/get.go


 			for k, entry := range resp.Payload {
-				if k != uuid {
+				if k != entryUUID {


Suggested change

if k != entryUUID {

if k != params.entryUUID {

bobcallaway · 2022-01-11T16:56:57Z

pkg/sharding/sharding.go

+}
+
+// Returns UUID (with no prepended TreeID) from a UUID or EntryID string
+func GetUUIDFromIDString(id string) (string, error) {


Is there a way to move validateID logic from CLI into this package and just re-use it here? As written this doesn't check that id is a valid hex string. Its a tad paranoid given that the API layer will check the input values, but since its a exported function I think it would be good to be consistent.

Yeah, this is a good idea. I'm just not sure yet how to do it without tearing out the entire pflags.go logic and moving it into sharding. validateID relies on validateString, which uses the validationFunc, baseValue, etc. and I can't import those into sharding because rekor-cli has sharding as a dependency. Writing a duplicate version of validateString in sharding also seems suboptimal. Possibly would make sense to move pflags.go into a package accessible to both sharding and rekor-cli? What do you think?

I guess this function could also just move into pflags.

I think in order to do this, pflags.go and related files would have to move "up" and out of rekor-cli into a new pkg/validation or cmd/validation because that way GetUUIDFromIDString could be used in both cmd/rekor-cli/app/get.go and pkg/api/entries.go without import cycles. Still experimenting with this.

I ended up using hex.DecodeString instead of moving the pflags logic around.

bobcallaway · 2022-01-11T17:00:27Z

pkg/sharding/sharding.go

 }

-func CreateFullID(treeid string, uuid string) (FullID, error) {
+func CreateEntryID(treeid string, uuid string) (EntryID, error) {


can you please add unit tests for this package?

Definitely!

Done: the unit tests have their own commit.

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

bobcallaway

2 minor nits. otherwise thanks for addressing all of the other comments and i think this should be good to merge once these two are fixed.

bobcallaway · 2022-01-24T14:45:36Z

pkg/api/entries.go

+
+	entryUUID, err := sharding.GetUUIDFromIDString(params.EntryUUID)
+	if err != nil {
+		return handleRekorAPIError(params, http.StatusInternalServerError, err, "")


If the UUID isn't valid, this should return an http.StatusBadRequestError since the problem is invalid input from the client.

Makes sense!

bobcallaway · 2022-01-24T14:47:30Z

pkg/sharding/sharding.go

+		err := fmt.Errorf("id %v is not a valid hex string: %v", id, err)
+		return "", err


Suggested change

err := fmt.Errorf("id %v is not a valid hex string: %v", id, err)

return "", err

return "", fmt.Errorf("id %v is not a valid hex string: %v", id, err)

Also add a few helper functions and update names. Signed-off-by: Lily Sturmann <lsturman@redhat.com>

lkatalin · 2022-01-24T15:30:12Z

2 minor nits. otherwise thanks for addressing all of the other comments and i think this should be good to merge once these two are fixed.

@bobcallaway Thanks for catching these!

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

bobcallaway

i'd like @dlorenc to take one final look but LGTM

bobcallaway reviewed Jan 7, 2022

View reviewed changes

lkatalin force-pushed the uuidpr2 branch from dc54d13 to f1597e2 Compare January 10, 2022 15:25

lkatalin force-pushed the uuidpr2 branch 3 times, most recently from 23db617 to 45454a3 Compare January 11, 2022 16:04

lkatalin requested a review from bobcallaway January 11, 2022 16:22

lkatalin force-pushed the uuidpr2 branch from 45454a3 to 9e22550 Compare January 11, 2022 16:45

bobcallaway reviewed Jan 11, 2022

View reviewed changes

lkatalin force-pushed the uuidpr2 branch from 9e22550 to 208a099 Compare January 24, 2022 00:23

Move range.go into sharding package to avoid import cycles

881e522

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

lkatalin force-pushed the uuidpr2 branch from 208a099 to a0ad9bb Compare January 24, 2022 00:32

Change name of FullID to EntryID

8104f77

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

lkatalin force-pushed the uuidpr2 branch 4 times, most recently from 6ca60c3 to b461d47 Compare January 24, 2022 01:28

lkatalin requested a review from bobcallaway January 24, 2022 01:56

bobcallaway reviewed Jan 24, 2022

View reviewed changes

Add unit tests for sharding package

aa98b60

Also add a few helper functions and update names. Signed-off-by: Lily Sturmann <lsturman@redhat.com>

lkatalin force-pushed the uuidpr2 branch from b461d47 to 4436132 Compare January 24, 2022 15:28

lkatalin added 2 commits January 24, 2022 10:32

Add logic to GET artifacts via old UUID or new EntryID

7645cc3

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

Add e2e test for longer EntryID

5b05bb7

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

lkatalin force-pushed the uuidpr2 branch from 4436132 to 5b05bb7 Compare January 24, 2022 15:32

bobcallaway approved these changes Jan 24, 2022

View reviewed changes

dlorenc approved these changes Jan 24, 2022

View reviewed changes

dlorenc merged commit adab5c5 into sigstore:main Jan 25, 2022

github-actions bot added this to the v1.0.0 milestone Jan 25, 2022

lkatalin mentioned this pull request Feb 1, 2022

UUID parsing to include pre-pended shard ID #515

Closed

1 task

		err := fmt.Errorf("id %v is not a valid hex string: %v", id, err)
		return "", err

	err := fmt.Errorf("id %v is not a valid hex string: %v", id, err)
	return "", err
	return "", fmt.Errorf("id %v is not a valid hex string: %v", id, err)

Conversation

lkatalin commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkatalin commented Jan 10, 2022

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!

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!

bobcallaway left a comment

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 Jan 24, 2022

Uh oh!

bobcallaway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lkatalin commented Jan 7, 2022 •

edited

Loading