Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ctpolicy/ctpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ loop:
if res.err != nil {
errs = append(errs, res.err.Error())
ctp.winnerCounter.WithLabelValues(res.log.Url, failed).Inc()

// Replace the errored submission with another attempt
if nextLog < len(logs) {
go ctp.getOne(subCtx, cert, logs[nextLog], resChan)
nextLog++
// Reset the ticker so we wait the correct stagger interval
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only half-convinced we should even touch the ticker here, and worrying about a buffered tick seems like a pretty minor issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It would be just as reasonable to not reset the ticker; doing so is a best-effort attempt to avoid submitting to more logs than necessary. No need to tie ourselves into knots guaranteeing that it doesn't fire again "too soon".

staggerTicker.Reset(ctp.stagger)
}
} else {
results = append(results, res)
ctp.winnerCounter.WithLabelValues(res.log.Url, succeeded).Inc()
Expand Down
30 changes: 28 additions & 2 deletions ctpolicy/ctpolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"strings"
"testing"
"testing/synctest"
"time"

"github.com/jmhodges/clock"
Expand Down Expand Up @@ -94,8 +95,10 @@ func TestGetSCTs(t *testing.T) {
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctp := New(tc.mock, tc.logs, nil, nil, 0, blog.NewMock(), metrics.NoopRegisterer)
synctest.Test(t, func(t *testing.T) {
mockLog := blog.NewMock()
defer mockLog.Close()
ctp := New(tc.mock, tc.logs, nil, nil, time.Second, mockLog, metrics.NoopRegisterer)
Comment thread
mcpherrinm marked this conversation as resolved.
Outdated
ret, err := ctp.GetSCTs(tc.ctx, []byte{0}, time.Time{})
if tc.result != nil {
test.AssertDeepEquals(t, ret, tc.result)
Expand All @@ -122,6 +125,29 @@ func (mp *mockFailOnePub) SubmitToSingleCTWithResult(_ context.Context, req *pub
return &pubpb.Result{Sct: []byte{0}}, nil
}

func TestGetSCTsNoStaggerOnError(t *testing.T) {
// We shouldn't wait for the stagger (1h, here) if
Comment thread
mcpherrinm marked this conversation as resolved.
Outdated
synctest.Test(t, func(t *testing.T) {
mockLog := blog.NewMock()
defer mockLog.Close()
ctp := New(&mockFailPub{}, loglist.List{
{Name: "LogA1", Operator: "OperA", Url: "UrlA1", Key: []byte("KeyA1")},
{Name: "LogA2", Operator: "OperA", Url: "UrlA2", Key: []byte("KeyA2")},
{Name: "LogB1", Operator: "OperB", Url: "UrlB1", Key: []byte("KeyB1")},
{Name: "LogC1", Operator: "OperC", Url: "UrlC1", Key: []byte("KeyC1")},
}, nil, nil, time.Hour, mockLog, metrics.NoopRegisterer)

start := time.Now()
_, err := ctp.GetSCTs(context.Background(), []byte{0}, time.Time{})
elapsed := time.Since(start)

test.AssertError(t, err, "GetSCTs should have failed")
if elapsed > 5*time.Second {
t.Errorf("GetSCTs took %s with a 1h stagger, instead of failing quickly", elapsed)
}
})
}

func TestGetSCTsMetrics(t *testing.T) {
ctp := New(&mockFailOnePub{badURL: "UrlA1"}, loglist.List{
{Name: "LogA1", Operator: "OperA", Url: "UrlA1", Key: []byte("KeyA1")},
Expand Down
6 changes: 6 additions & 0 deletions log/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,9 @@ func (m *Mock) Clear() {
w := m.w.(*mockWriter)
w.clearChan <- struct{}{}
}

// Close shuts down the mock's background goroutine.
func (m *Mock) Close() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this becomes unnecessary under #8606, because that PR's blog.Mock no longer uses channels or goroutines.

w := m.w.(*mockWriter)
close(w.closeChan)
}
Loading