Add inital CTPolicy impl#3414
Conversation
|
(Oh also: this doesn't implement |
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Excellent turnaround on this, thanks! I think the code would gain some clarity by having types for SCTs and certs, i.e.:
type SCT []byte
type CertDER []byte
These could probably go in core/types.go and we could start using them in more places. Otherwise, the basic approach looks good.
| } | ||
| select { | ||
| case scts <- sct: | ||
| case <-subCtx.Done(): |
There was a problem hiding this comment.
It's a little weird to check for .Done() here, because there's no action to skip. AFAICT the idea is that all enqueues to scts after the first will block, so this provides a way out of the blocking, right? I think we actually don't need to check subCtx.Done() here, because the goal is to tell the deeper layers (like gRPC and HTTP) that we want to cancel.
Perhaps a more straightforward way to implement this function would be: Define a small struct containing {sct, err}, and make the goroutine very small: it calls SubmitToSingleCTWithResult and enqueues one of these structs onto a chan of size == len(group). Then run the for loop for exactly len(group) iterations, and return the first non-error result. If you fall off the end of the for loop, return the last error.
There was a problem hiding this comment.
Discussed OOB: while gRPC can cancel the RPC, without checking doing a parallel check of .Done() the goroutine we spawn here will live on forever until the write on scts is unblocked, which will never happen.
| func (ctp *CTPolicy) race(ctx context.Context, cert []byte, group []cmd.LogDescription) ([]byte, error) { | ||
| scts := make(chan []byte, 1) | ||
| errs := make(chan error, len(group)) | ||
| subCtx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Ideally you should use defer cancel() here; otherwise you can leak resources.
| errs := make(chan error, len(ctp.groups)) | ||
| subCtx, cancel := context.WithCancel(ctx) | ||
| for _, v := range ctp.groups { | ||
| wg.Add(1) |
There was a problem hiding this comment.
I think this function can also be simplified by the pattern I suggested above; you'd only need one chan.
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Seconding @jsha's ask for some concrete types. It's a WIP and this is probably on your radar already but I think the code would benefit from some more commenting throughout the bodies of the functions added.
Overall I think this is a solid approach! I think the intent is clear and the approach you took to managing the concurrency was very readable. Thanks!
| } | ||
|
|
||
| numErr := 0 | ||
| for { |
There was a problem hiding this comment.
Might as well make this a more traditional for loop with a loop variable, since that's what numErr acts as. Then instead of if numErr == len(group) we can just fall out the bottom of the loop and return the "all submissions for group failed" error there.
| } | ||
|
|
||
| go func() { | ||
| // The for loop below is blocked on the results channel being open, |
There was a problem hiding this comment.
Why not make the for loop below go from 0 to len(ctp.groups)? Then you don't need to worry about blocking on the channel close.
| close(results) | ||
| }() | ||
|
|
||
| ret := []core.SCTDER{} |
There was a problem hiding this comment.
We've been leaning towards what I think is slightly more idiomatic style:
var ret []core.SCTDER{}
(which starts as nil).
| sct, err := ctLog.client.AddChain(ctx, chain) | ||
| took := time.Since(start).Seconds() | ||
| status := "success" | ||
| defer func() { |
There was a problem hiding this comment.
Why defer here? If we only care about the success or failure of the submission function, it would be more straightforward to do one stat increment inside the if err != nil clause (error) and one outside of it (success). We could put in a little helper function if we wanted to DRY it up a bit, but I don't think that is strictly necessary.
Deferring the stat increment has a somewhat minor unexpected effect where the success stat doesn't get incremented until after the SA write is successful, which could be a second or two if SA is slow.
There was a problem hiding this comment.
Mainly just to avoid calling the method twice, I'm fine with just doing that though if people don't really care :/
There was a problem hiding this comment.
Yeah, I think this is a case where the defer seems more "special" and forces you to think a little about what's going on, when we really want something straightforward.
For instance, when I first read this defer, I though "Oh, this is intended to let any of the error cases set the status to error, while the main case sets it to success. But in that case, the sense is wrong: The status field should start as error, and be set to success only if none of the error cases apply. Oh, but the second two error cases don't really measure errors during submission, so we wouldn't actually want that." So I'm hoping to short-circuit that thought process for future readers. :-)
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Nice work @rolandshoemaker !
|
@rolandshoemaker Do you still think this needs additional test coverage? If not can you update the PR description to give a commit message for this that can be used with a squash merge. |
3 participants
Adds a package which implements group based SCT retrieval.
Fixes #3412.