-
Notifications
You must be signed in to change notification settings - Fork 26
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
Rework pkg/tlog/entry.go to not depend on sigstore/rekor/pkg/types #28
Conversation
See #23 This lets us reduce the size of the sigstore-go binary by over 4 MB, since sigstore/rekor/pkg/types has many dependencies. Signed-off-by: Zach Steindler <[email protected]>
Long-term, we'd likely want all rekor types supported in sigstore-go. Do you think it'd be better to figure out how to minimize dependencies in rekor/pkg/types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not have duplicated logic here for parsing, and this would skip correctness checks that Rekor jas implemented. I would rather we refactor Rekor to trim down dependencies for each rekor type, or at least these critical ones.
@@ -66,21 +67,24 @@ func NewEntry(body []byte, integratedTime int64, logIndex int64, logID []byte, s | |||
if err != nil { | |||
return nil, err | |||
} | |||
rekorEntry, err := types.UnmarshalEntry(pe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One downside to removing this is there's certain correctness checks that occur during unmarshalling that will now be skipped.
logEntryAnon: models.LogEntryAnon{ | ||
Body: base64.StdEncoding.EncodeToString(body), | ||
IntegratedTime: swag.Int64(integratedTime), | ||
LogIndex: swag.Int64(logIndex), | ||
LogID: swag.String(string(logID)), | ||
}, | ||
kind: pe.Kind(), | ||
version: rekorEntry.APIVersion(), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is kind
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We retain the pe
object on rekorEntry
, so we just call rekorEntry.Kind()
when we need it (see below).
I'm not as familiar with the Rekor codebase, but this could be investigated further. It's possible this could be as simple as removing |
It might be due to some of the Rekor packages like https://github.com/sigstore/rekor/blob/main/pkg/types/hashedrekord/v0.0.1/entry.go#L36-L38 that should be removed. I'm not sure if there's tooling for this, but it'd be nice to see what packages are causing the large size. |
I took a quick look at Rekor, but wasn't able to come up with a simple proposal to reduce dependencies. I think a deeper looks is probably warranted before giving up completely on a Rekor refactor. I hesitate to call it "tooling", but here's the approach I'm taking:
|
Another idea for an iterative approach could be for each package we import, look at its dependencies and order by most minimal usage. Copy-paste the functions used and see if the size decreases significantly. If so, then we'll note that dependency needs refactoring if it's under our control, or if it's a third-party dependency, we could just copy in the function. |
I think this also warrants a larger conversation about what libraries are authoritative for verification logic throughout the Sigstore stack. I've thought about sigstore-go as glue for services. Ideally, I would like the following (and this is just my thoughts! I'll file an issue to discuss more):
I'd like for maintainers to align on this, as it'll drive what the dependency tree should look like and how we can simplify it. Edit: Also previous discussion around this: sigstore/sigstore#678 |
I would love to see if we can slim down Rekor, as that would benefit multiple clients, not just sigstore-go package. |
I 100% agree that the Rekor libraries should own type construction and parsing, and that ideally sigstore-go would depend on those libraries. ... but I've spent some time over the past few days trying to see if there was an easy way to reduce the size of But as it stands, we have a bit of a trade-off. On one hand, we have the more thorough correctness checks in |
Personally, I would prefer larger size over duplication. We impact readability and testability with duplication. When I had suggested minimizing dependencies, I was thinking more about minimizing dependencies focusing on unmaintained or minimally maintained dependencies. I’m also surprised those packages are so large, is this the bson dependency again or something else? |
Yeah, the problem is the dependency has to be at the right level - something we depend on directly that we can change our usage of. If we depend on A, which depends on B, which has a bunch of dependencies in C that are big, it isn't straightforward to untangle that (let alone upstream a change to B).
Unfortunately for the time being I've run out of time to do additional research in Rekor's codebase. Like I mentioned, I was not able to identify something that we could easily swap out for a similar decrease in size. It's possible it exists, and I would love to see someone post a PR with that alternative.
I think it depends on the size! @kommendorkapten came up with a simplified version of the problem statement:
21 MB!! Compared to just 11 MB using the generated model:
|
Okay, so it sounds like the next step is to try to upstream slimming into Rekor. I don't have any near-term plans to do so, but whoever picks that up should feel free to refer back to this PR. |
Summary
See #23
This lets us reduce the size of the sigstore-go binary by over 4 MB, since sigstore/rekor/pkg/types has many dependencies.
Release Note
NONE
Documentation
N/A