Repository: sigstore/rekor
Pull request #1006 (2022-08-26 19:07:25)

api.SearchLogQueryHandler thread safety
Signed-off-by: Ceridwen Driskill <cdriskill@google.com>

<!--
Thanks for opening a pull request! Please do not just delete this text.  The three fields below are mandatory.

Please remember to:
- This PR requires an issue. If it is a new feature, the issue should proceed the PR and will have allowed sufficent time for discussions to take place. Pleases use
issue tags such as "Closes #XYZ" or "Resolves sigstore/repo-name#XYZ".
  [documentation](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
- ensure your commits are signed-off, as sigstore uses the [DCO](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin) using `git commit -s`, or `git commit -s --amend` if you want to amend already existing commits
- lastly, ensure there are no merge commits!

Thank you :)
-->

#### Summary
<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve? How can reviewers test this PR?
-->
Updated to use channels instead of appending (which is not thread safe) in goroutines.

#### Release Note
<!--
Add a release note for each of the following conditions:

* Config changes (additions, deletions, updates)
* API additions—new endpoint, new response fields, or newly accepted request parameters
* Database changes (any)
* Websocket additions or changes
* Anything noteworthy to an administrator running private sigstore instances (err on the side of over-communicating)
* New features and improvements, including behavioural changes, UI changes and CLI changes
* Bug fixes and fixes of previous known issues
* Deprecation warnings, breaking changes, or compatibility notes

If no release notes are required write NONE. Use past-tense.

-->

#### Documentation
<!--

Does this change require an update to documentation? How will users implement your new feature?

Please reference a PR within https://docs.sigstore.dev

-->

cdris: Fixes #997
codecov-commenter: # [Codecov](https://codecov.io/gh/sigstore/rekor/pull/1006?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) Report
> Merging [#1006](https://codecov.io/gh/sigstore/rekor/pull/1006?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (3b1f766) into [main](https://codecov.io/gh/sigstore/rekor/commit/cb5572dbe7c372d1f4e9f92af6317d4e22313b7f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (cb5572d) will **decrease** coverage by `7.19%`.
> The diff coverage is `0.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
- Coverage   48.20%   41.00%   -7.20%     
==========================================
  Files          62       69       +7     
  Lines        5398     6516    +1118     
==========================================
+ Hits         2602     2672      +70     
- Misses       2513     3562    +1049     
+ Partials      283      282       -1     
```


| [Impacted Files](https://codecov.io/gh/sigstore/rekor/pull/1006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Coverage Δ | |
|---|---|---|
| [pkg/api/entries.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL2FwaS9lbnRyaWVzLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/sharding/ranges.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3NoYXJkaW5nL3Jhbmdlcy5nbw==) | `46.01% <0.00%> (-1.26%)` | :arrow_down: |
| [pkg/types/rpm/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL3JwbS92MC4wLjEvZW50cnkuZ28=) | `55.59% <0.00%> (-0.66%)` | :arrow_down: |
| [pkg/types/helm/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL2hlbG0vdjAuMC4xL2VudHJ5Lmdv) | `48.29% <0.00%> (-0.63%)` | :arrow_down: |
| [pkg/types/jar/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL2phci92MC4wLjEvZW50cnkuZ28=) | `38.83% <0.00%> (-0.58%)` | :arrow_down: |
| [pkg/types/tuf/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL3R1Zi92MC4wLjEvZW50cnkuZ28=) | `41.86% <0.00%> (-0.52%)` | :arrow_down: |
| [pkg/types/rekord/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL3Jla29yZC92MC4wLjEvZW50cnkuZ28=) | `49.00% <0.00%> (-0.50%)` | :arrow_down: |
| [cmd/rekor-cli/app/verify.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-Y21kL3Jla29yLWNsaS9hcHAvdmVyaWZ5Lmdv) | `2.36% <0.00%> (-0.10%)` | :arrow_down: |
| [pkg/types/types.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL3R5cGVzLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/client/yaml.go](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL2NsaWVudC95YW1sLmdv) | | |
| ... and [13 more](https://codecov.io/gh/sigstore/rekor/pull/1006/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore)


Patch:
diff --git a/cmd/rekor-cli/app/get.go b/cmd/rekor-cli/app/get.go
index 440b10a..b75f8a5 100644
--- a/cmd/rekor-cli/app/get.go
+++ b/cmd/rekor-cli/app/get.go
@@ -159,7 +159,7 @@ func parseEntry(uuid string, e models.LogEntryAnon) (interface{}, error) {
 	if err != nil {
 		return nil, err
 	}
-	eimpl, err := types.NewEntry(pe)
+	eimpl, err := types.UnmarshalEntry(pe)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/api/entries.go b/pkg/api/entries.go
index 3225ed6..5be6a62 100644
--- a/pkg/api/entries.go
+++ b/pkg/api/entries.go
@@ -101,7 +101,7 @@ func logEntryFromLeaf(ctx context.Context, signer signature.Signer, tc TrillianC
 		if err != nil {
 			return nil, err
 		}
-		eimpl, err := types.NewEntry(pe)
+		eimpl, err := types.UnmarshalEntry(pe)
 		if err != nil {
 			return nil, err
 		}
@@ -161,7 +161,7 @@ func GetLogEntryByIndexHandler(params entries.GetLogEntryByIndexParams) middlewa
 
 func createLogEntry(params entries.CreateLogEntryParams) (models.LogEntry, middleware.Responder) {
 	ctx := params.HTTPRequest.Context()
-	entry, err := types.NewEntry(params.ProposedEntry)
+	entry, err := types.CreateVersionedEntry(params.ProposedEntry)
 	if err != nil {
 		return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))
 	}
@@ -338,10 +338,12 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 		}
 
 		code := http.StatusBadRequest
-		for _, e := range params.Entry.Entries() {
+		entries := params.Entry.Entries()
+		searchHashesChan := make(chan []byte, len(entries))
+		for _, e := range entries {
 			e := e // https://golang.org/doc/faq#closures_and_goroutines
 			g.Go(func() error {
-				entry, err := types.NewEntry(e)
+				entry, err := types.UnmarshalEntry(e)
 				if err != nil {
 					return err
 				}
@@ -353,7 +355,7 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 				}
 				hasher := rfc6962.DefaultHasher
 				leafHash := hasher.HashLeaf(leaf)
-				searchHashes = append(searchHashes, leafHash)
+				searchHashesChan <- leafHash
 				return nil
 			})
 		}
@@ -361,6 +363,10 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 		if err := g.Wait(); err != nil {
 			return handleRekorAPIError(params, code, err, err.Error())
 		}
+		close(searchHashesChan)
+		for hash := range searchHashesChan {
+			searchHashes = append(searchHashes, hash)
+		}
 
 		searchByHashResults := make([]*trillian.GetEntryAndProofResponse, len(searchHashes))
 		g, _ = errgroup.WithContext(httpReqCtx)
@@ -399,7 +405,7 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 
 	if len(params.Entry.LogIndexes) > 0 {
 		g, _ := errgroup.WithContext(httpReqCtx)
-
+		resultPayloadChan := make(chan models.LogEntry, len(params.Entry.LogIndexes))
 		for _, logIndex := range params.Entry.LogIndexes {
 			logIndex := logIndex // https://golang.org/doc/faq#closures_and_goroutines
 			g.Go(func() error {
@@ -407,7 +413,7 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 				if err != nil {
 					return err
 				}
-				resultPayload = append(resultPayload, logEntry)
+				resultPayloadChan <- logEntry
 				return nil
 			})
 		}
@@ -415,6 +421,10 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 		if err := g.Wait(); err != nil {
 			return handleRekorAPIError(params, http.StatusInternalServerError, fmt.Errorf("grpc error: %w", err), trillianUnexpectedResult)
 		}
+		close(resultPayloadChan)
+		for result := range resultPayloadChan {
+			resultPayload = append(resultPayload, result)
+		}
 	}
 
 	return entries.NewSearchLogQueryOK().WithPayload(resultPayload)
diff --git a/pkg/types/README.md b/pkg/types/README.md
index f77f240..81246c4 100644
--- a/pkg/types/README.md
+++ b/pkg/types/README.md
@@ -8,6 +8,10 @@ Rekor supports pluggable types (aka different schemas) for entries stored in the
 
 - Alpine Packages [schema](alpine/alpine_schema.json)
   - Versions: 0.0.1
+- COSE Envelopes [schema](cose/cose_schema.json)
+  - Versions: 0.0.1
+- HashedRekord [schema](hashedrekord/hashedrekord_schema.json)
+  - Versions: 0.0.1
 - Helm Provenance Files [schema](helm/helm_schema.json)
   - Versions: 0.0.1
 - In-Toto Attestations [schema](intoto/intoto_schema.json)
@@ -20,7 +24,7 @@ Rekor supports pluggable types (aka different schemas) for entries stored in the
   - Versions: 0.0.1
 - RPM Packages [schema](rpm/rpm_schema.json)
   - Versions: 0.0.1
-- COSE Envelopes [schema](cose/cose_schema.json)
+- TUF Metadata [schema](tuf/tuf_schema.json)
   - Versions: 0.0.1
 
 Refer to [Rekor docs](https://docs.sigstore.dev/rekor/pluggable-types) for adding support for new types.
diff --git a/pkg/types/entries.go b/pkg/types/entries.go
index 529ede0..cda7f27 100644
--- a/pkg/types/entries.go
+++ b/pkg/types/entries.go
@@ -59,8 +59,28 @@ func NewProposedEntry(ctx context.Context, kind, version string, props ArtifactP
 	return nil, fmt.Errorf("could not create entry for kind '%v'", kind)
 }
 
-// NewEntry returns the specific instance for the type and version specified in the doc
-func NewEntry(pe models.ProposedEntry) (EntryImpl, error) {
+// CreateVersionedEntry returns the specific instance for the type and version specified in the doc
+// This method should be used on the insertion flow, which validates that the specific version proposed
+// is permitted to be entered into the log.
+func CreateVersionedEntry(pe models.ProposedEntry) (EntryImpl, error) {
+	ei, err := UnmarshalEntry(pe)
+	if err != nil {
+		return nil, err
+	}
+	kind := pe.Kind()
+	if tf, found := TypeMap.Load(kind); found {
+		if !tf.(func() TypeImpl)().IsSupportedVersion(ei.APIVersion()) {
+			return nil, fmt.Errorf("entry kind '%v' does not support inserting entries of version '%v'", kind, ei.APIVersion())
+		}
+	}
+
+	return ei, nil
+}
+
+// UnmarshalEntry returns the specific instance for the type and version specified in the doc
+// This method does not check for whether the version of the entry could be currently inserted into the log,
+// and is useful when dealing with entries that have been persisted to the log.
+func UnmarshalEntry(pe models.ProposedEntry) (EntryImpl, error) {
 	if pe == nil {
 		return nil, errors.New("proposed entry cannot be nil")
 	}
@@ -73,7 +93,7 @@ func NewEntry(pe models.ProposedEntry) (EntryImpl, error) {
 		}
 		return t.UnmarshalEntry(pe)
 	}
-	return nil, fmt.Errorf("could not create entry for kind '%v'", kind)
+	return nil, fmt.Errorf("could not unmarshal entry for kind '%v'", kind)
 }
 
 // DecodeEntry maps the (abstract) input structure into the specific entry implementation class;


Test patch:
diff --git a/pkg/types/alpine/v0.0.1/entry_test.go b/pkg/types/alpine/v0.0.1/entry_test.go
index d53e0f3..153e748 100644
--- a/pkg/types/alpine/v0.0.1/entry_test.go
+++ b/pkg/types/alpine/v0.0.1/entry_test.go
@@ -157,7 +157,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/hashedrekord/v0.0.1/entry_test.go b/pkg/types/hashedrekord/v0.0.1/entry_test.go
index 99b1177..72a55d6 100644
--- a/pkg/types/hashedrekord/v0.0.1/entry_test.go
+++ b/pkg/types/hashedrekord/v0.0.1/entry_test.go
@@ -285,7 +285,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/helm/v0.0.1/entry_test.go b/pkg/types/helm/v0.0.1/entry_test.go
index 655e7b5..69eb38f 100644
--- a/pkg/types/helm/v0.0.1/entry_test.go
+++ b/pkg/types/helm/v0.0.1/entry_test.go
@@ -188,7 +188,7 @@ func TestCrossFieldValidation(t *testing.T) {
 				if err != nil {
 					t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 				}
-				if _, err := types.NewEntry(pe); err != nil {
+				if _, err := types.UnmarshalEntry(pe); err != nil {
 					t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 				}
 			}
diff --git a/pkg/types/intoto/v0.0.1/entry_test.go b/pkg/types/intoto/v0.0.1/entry_test.go
index 5d4151e..f01b854 100644
--- a/pkg/types/intoto/v0.0.1/entry_test.go
+++ b/pkg/types/intoto/v0.0.1/entry_test.go
@@ -270,7 +270,7 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				if err != nil {
 					t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tt.name, err)
 				}
-				canonicalEntry, err := types.NewEntry(pe)
+				canonicalEntry, err := types.UnmarshalEntry(pe)
 				if err != nil {
 					t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tt.name, err)
 				}
diff --git a/pkg/types/jar/v0.0.1/entry_test.go b/pkg/types/jar/v0.0.1/entry_test.go
index e56c62e..42ff533 100644
--- a/pkg/types/jar/v0.0.1/entry_test.go
+++ b/pkg/types/jar/v0.0.1/entry_test.go
@@ -108,7 +108,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/rekord/v0.0.1/entry_test.go b/pkg/types/rekord/v0.0.1/entry_test.go
index d0d7cee..0e79731 100644
--- a/pkg/types/rekord/v0.0.1/entry_test.go
+++ b/pkg/types/rekord/v0.0.1/entry_test.go
@@ -233,7 +233,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/rfc3161/v0.0.1/entry_test.go b/pkg/types/rfc3161/v0.0.1/entry_test.go
index 72f90d8..9f39f5b 100644
--- a/pkg/types/rfc3161/v0.0.1/entry_test.go
+++ b/pkg/types/rfc3161/v0.0.1/entry_test.go
@@ -182,7 +182,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/rpm/v0.0.1/entry_test.go b/pkg/types/rpm/v0.0.1/entry_test.go
index 6819ee4..3e50873 100644
--- a/pkg/types/rpm/v0.0.1/entry_test.go
+++ b/pkg/types/rpm/v0.0.1/entry_test.go
@@ -173,7 +173,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/tuf/v0.0.1/entry_test.go b/pkg/types/tuf/v0.0.1/entry_test.go
index 4dbc16c..f8d090f 100644
--- a/pkg/types/tuf/v0.0.1/entry_test.go
+++ b/pkg/types/tuf/v0.0.1/entry_test.go
@@ -208,7 +208,7 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
 			}
-			if _, err := types.NewEntry(pe); err != nil {
+			if _, err := types.UnmarshalEntry(pe); err != nil {
 				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
 			}
 		}
diff --git a/pkg/types/types_test.go b/pkg/types/types_test.go
index 9be029f..ea7c530 100644
--- a/pkg/types/types_test.go
+++ b/pkg/types/types_test.go
@@ -65,7 +65,7 @@ func (e UnmarshalErrorValidEntry) ContextValidate(context context.Context, forma
 	return nil
 }
 
-func TestNewEntry(t *testing.T) {
+func TestUnmarshalEntry(t *testing.T) {
 	type TestCase struct {
 		entry         models.ProposedEntry
 		expectSuccess bool
@@ -83,7 +83,7 @@ func TestNewEntry(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		if _, err := NewEntry(tc.entry); (err == nil) != tc.expectSuccess {
+		if _, err := UnmarshalEntry(tc.entry); (err == nil) != tc.expectSuccess {
 			t.Errorf("unexpected error creating entry of type '%v': %v", tc.entry.Kind(), err)
 		}
 	}