sa: add GetRevokedCertsByShard #7946
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Which queries for revoked certificates by explicit shard only. Also, remove explicit shard support from GetRevokedCerts.
jsha
force-pushed
the
split-getrevokedcerts
branch
from
a087fb6 to
909464d
Compare
jsha
changed the title
beautifulentropy
requested changes
Jan 17, 2025
jsha
requested review from
a team and
aarongable
and removed request for
a team
aarongable
approved these changes
Jan 18, 2025
| @@ -271,6 +271,16 @@ func (sa *StorageAuthority) GetRevokedCerts(ctx context.Context, _ *sapb.GetRevo | |||
| return &ServerStreamClient[corepb.CRLEntry]{}, nil | |||
| } | |||
|
|
|||
| // GetRevokedCertsByShard is a mock | |||
| func (sa *StorageAuthorityReadOnly) GetRevokedCertsByShard(ctx context.Context, _ *sapb.GetRevokedCertsByShardRequest, _ ...grpc.CallOption) (sapb.StorageAuthorityReadOnly_GetRevokedCertsClient, error) { | |||
There was a problem hiding this comment.
Thanks to my upstream grpc stream generics work, you shouldn't need to create two different versions of this method that differ only in their receiver and return type! Instead a single mock with this signature from the autogenerated code should work:
Suggested change
| func (sa *StorageAuthorityReadOnly) GetRevokedCertsByShard(ctx context.Context, _ *sapb.GetRevokedCertsByShardRequest, _ ...grpc.CallOption) (sapb.StorageAuthorityReadOnly_GetRevokedCertsClient, error) { | |
| func (sa *StorageAuthorityReadOnly) GetRevokedCertsByShard(ctx context.Context, _ *sapb.GetRevokedCertsByShardRequest, _ ...grpc.CallOption) (grpc.ServerStreamingClient[corepb.CRLEntry], error) { |
And once you're using that generic return type, both methods can be collapsed into a single method with a StorageAuthorityReadOnly receiver because the mock StorageAuthority embeds the former.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The SA had some logic (not yet in use) to return revoked certificates either by temporal sharding (if
req.ShardIdxis zero) or by explicit sharding (ifreq.ShardIdxis nonzero).This PR splits the function into two. The existing
GetRevokedCertsalways does temporal sharding. The newGetRevokedCertsByShardalways does explicit sharding. Eventually onlyGetRevokedCertsByShardwill be necessary. This change was discussed in #7094 (comment) and is a precursor to having the crl-updater call both methods, so we can merge the results when generating CRLs.