Create EntryID for new artifacts and return EntryID to user #623

lkatalin · 2022-01-26T23:24:53Z

Summary

This will complete the UUID parsing component of the sharding work. It creates and returns a longer UUID (EntryID) to the user, so this change should go into a new release.

Edit:
As far as I understand, we should wait to merge this PR until we're ready for a new Rekor release - are there other changes that would go into that release? Do we have a timeline?

Ticket Link

Fixes #487

Release Note

    TODO

bobcallaway · 2022-01-28T12:39:48Z

pkg/api/entries.go

+		err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %v", hexEncodedHash, err)
+		return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))


Suggested change

err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %v", hexEncodedHash, err)

return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))

err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %w", hexEncodedHash, err)

return nil, handleRekorAPIError(params, http.StatusInternalServerError, err, fmt.Sprintf(validationError, err))

at this point we've already committed the change to the log... so while I expect this code path to never actually get executed, the burden wouldn't be on the client to resolve anything so we should return a 500 error.

dlorenc · 2022-02-01T01:45:39Z

Edit: As far as I understand, we should wait to merge this PR until we're ready for a new Rekor release - are there other changes that would go into that release? Do we have a timeline?

This is correct! We should release a version of the client that supports longer versions ASAP - then release this some time later.

@cpanato any idea if we can kick off another Rekor CLI release?

dlorenc · 2022-02-01T02:12:55Z

Just to confirm my understanding:

If you build rekor-cli from HEAD, it should work with rekor-server running this change AND before this change
The currently released rekor-cli will not work with this change

cpanato · 2022-02-01T11:00:34Z

Edit: As far as I understand, we should wait to merge this PR until we're ready for a new Rekor release - are there other changes that would go into that release? Do we have a timeline?

This is correct! We should release a version of the client that supports longer versions ASAP - then release this some time later.

@cpanato any idea if we can kick off another Rekor CLI release?

we can cut a v0.5.0 release, I don't think we are ready for a v1.0.0

I will check the changelog and maybe we can cut that today

dlorenc · 2022-02-01T11:57:29Z

+1 to v0.5!

lkatalin · 2022-02-01T15:10:45Z

Just to confirm my understanding:

* If you build rekor-cli from HEAD, it should work with rekor-server running this change AND before this change

* The currently released rekor-cli will not work with this change

@dlorenc This is also my understanding. There is an e2e test right now that includes a rekor-cli GET command for the new EntryID here.

cpanato · 2022-02-04T13:45:13Z

+1 to v0.5!

https://github.com/sigstore/rekor/releases/tag/v0.5.0

lukehinds

code looks good to me, once the cli is up to speed.

bobcallaway

lgtm other than minor error wrapping nit

bobcallaway · 2022-02-18T13:47:20Z

pkg/api/entries.go

+	hexEncodedHash := hex.EncodeToString(queuedLeaf.GetMerkleLeafHash())
+	entryID, err := sharding.CreateEntryIDWithActiveTreeID(hexEncodedHash)
+	if err != nil {
+		err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %v", hexEncodedHash, err)


nit if you end up re-spinning this:

Suggested change

err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %v", hexEncodedHash, err)

err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %w", hexEncodedHash, err)

lkatalin · 2022-02-22T15:14:40Z

Note to self: after #671 is merged, we will have to use CreateEntryIDFromParts(api.logRanges.ActiveIndex(), uuid) instead of CreateEntryIDWithActiveTreeID(uuid). This moves some logic out of the sharding package.

lkatalin · 2022-03-01T02:11:57Z

I think this will require #671 to be merged first to pass CI.

dlorenc · 2022-03-26T02:53:04Z

What's the plan for merging and rolling this out?

lkatalin · 2022-03-28T15:31:18Z

What's the plan for merging and rolling this out?

@dlorenc Once we merge #727 , that should take care of the last known bug. @priyawadhwa and I are double checking on anything else that needs to happen from the roadmap before merging this, but I think most/all of the outstanding work should be optional.

priyawadhwa

small nits around naming, otherwise LGTM!

priyawadhwa · 2022-04-11T17:48:10Z

tests/e2e_test.go

@@ -223,7 +223,10 @@ func TestGet(t *testing.T) {
 	out := runCli(t, "upload", "--artifact", artifactPath, "--signature", sigPath, "--public-key", pubPath)
 	outputContains(t, out, "Created entry at")

-	uuid := getUUIDFromUploadOutput(t, out)
+	uuid, err := sharding.GetUUIDFromIDString(getUUIDFromUploadOutput(t, out))


should getUUIDFromUploadOutput be renamed to getEntryIDFromUploadOutput?

Agree, though this is a larger problem all throughout the code. #625 is tracking this. Pulling on that thread will definitely create enough changes for another PR.

we can save it for later then!

priyawadhwa · 2022-04-11T17:49:20Z

pkg/api/entries.go

@@ -187,7 +187,14 @@ func createLogEntry(params entries.CreateLogEntryParams) (models.LogEntry, middl
 	metricNewEntries.Inc()

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


hexEncodedHash is the same as uuid right? might be clearer to keep the name as uuid since entry id = tree id + uuid

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

priyawadhwa

lgtm

lkatalin force-pushed the uuidpr3 branch 3 times, most recently from 4613650 to 565128a Compare January 27, 2022 18:38

lkatalin marked this pull request as ready for review January 27, 2022 18:54

bobcallaway reviewed Jan 29, 2022

View reviewed changes

lkatalin force-pushed the uuidpr3 branch from 565128a to cf2d81f Compare January 31, 2022 15:57

lkatalin mentioned this pull request Feb 1, 2022

UUID parsing to include pre-pended shard ID #515

Closed

1 task

lukehinds approved these changes Feb 18, 2022

View reviewed changes

bobcallaway approved these changes Feb 18, 2022

View reviewed changes

lkatalin force-pushed the uuidpr3 branch from cf2d81f to 49203b4 Compare March 1, 2022 02:10

lkatalin requested a review from a team as a code owner March 1, 2022 02:10

priyawadhwa added a commit to priyawadhwa/rekor that referenced this pull request Mar 4, 2022

lily sigstore#623

c8d99ce

bobcallaway approved these changes Mar 7, 2022

View reviewed changes

This was referenced Mar 7, 2022

Sharding open questions and todos remaining #711

Closed

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

Merged

lkatalin force-pushed the uuidpr3 branch from 49203b4 to c523361 Compare April 11, 2022 16:04

priyawadhwa approved these changes Apr 11, 2022

View reviewed changes

lkatalin force-pushed the uuidpr3 branch 3 times, most recently from 39b4607 to aff3f76 Compare April 11, 2022 18:22

lkatalin force-pushed the uuidpr3 branch 3 times, most recently from 91f36ee to 6bac6ad Compare April 11, 2022 19:39

Create EntryID for new artifacts and return EntryID to user

6135efa

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

lkatalin force-pushed the uuidpr3 branch from 6bac6ad to 6135efa Compare April 11, 2022 19:40

lkatalin requested a review from priyawadhwa April 11, 2022 19:58

priyawadhwa approved these changes Apr 11, 2022

View reviewed changes

dlorenc merged commit 72a7b00 into sigstore:main Apr 12, 2022

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

priyawadhwa mentioned this pull request Sep 7, 2022

Fix rekor-cli backwards incompatibility & run harness tests against HEAD #1030

Merged

stephen-fox mentioned this pull request Jul 30, 2024

sign: Include rekor entry ID in output sigstore/cosign#3805

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create EntryID for new artifacts and return EntryID to user #623

Create EntryID for new artifacts and return EntryID to user #623

lkatalin commented Jan 26, 2022 •

edited

Loading

bobcallaway Jan 28, 2022

bobcallaway Jan 28, 2022

dlorenc commented Feb 1, 2022

dlorenc commented Feb 1, 2022

cpanato commented Feb 1, 2022

dlorenc commented Feb 1, 2022

lkatalin commented Feb 1, 2022

cpanato commented Feb 4, 2022

lukehinds left a comment

bobcallaway left a comment

bobcallaway Feb 18, 2022

lkatalin commented Feb 22, 2022

lkatalin commented Mar 1, 2022

dlorenc commented Mar 26, 2022

lkatalin commented Mar 28, 2022

priyawadhwa left a comment

priyawadhwa Apr 11, 2022

lkatalin Apr 11, 2022

priyawadhwa Apr 11, 2022

priyawadhwa Apr 11, 2022

priyawadhwa left a comment

		err := fmt.Errorf("error creating EntryID from active treeID and uuid %v: %v", hexEncodedHash, err)
		return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))

Create EntryID for new artifacts and return EntryID to user #623

Create EntryID for new artifacts and return EntryID to user #623

Conversation

lkatalin commented Jan 26, 2022 • edited Loading

Summary

Ticket Link

Release Note

bobcallaway Jan 28, 2022

Choose a reason for hiding this comment

bobcallaway Jan 28, 2022

Choose a reason for hiding this comment

dlorenc commented Feb 1, 2022

dlorenc commented Feb 1, 2022

cpanato commented Feb 1, 2022

dlorenc commented Feb 1, 2022

lkatalin commented Feb 1, 2022

cpanato commented Feb 4, 2022

lukehinds left a comment

Choose a reason for hiding this comment

bobcallaway left a comment

Choose a reason for hiding this comment

bobcallaway Feb 18, 2022

Choose a reason for hiding this comment

lkatalin commented Feb 22, 2022

lkatalin commented Mar 1, 2022

dlorenc commented Mar 26, 2022

lkatalin commented Mar 28, 2022

priyawadhwa left a comment

Choose a reason for hiding this comment

priyawadhwa Apr 11, 2022

Choose a reason for hiding this comment

lkatalin Apr 11, 2022

Choose a reason for hiding this comment

priyawadhwa Apr 11, 2022

Choose a reason for hiding this comment

priyawadhwa Apr 11, 2022

Choose a reason for hiding this comment

priyawadhwa left a comment

Choose a reason for hiding this comment

lkatalin commented Jan 26, 2022 •

edited

Loading