letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SA: Fix two bugs in UpdateCRLShard (#7052) 

The NextUpdate field should not be required, as it is not necessary for
tracking and preventing duplicate work between multiple crl-updater
instances.

The ThisUpdate conditional needs explicit handling for NULL to ensure
that it updates correctly.
This commit is contained in:
Aaron Gable 2023-08-31 09:06:33 -07:00 committed by GitHub
parent 1c22713813
commit 7bed24a401
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 39 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
sa/sa.go
View File

@ -1112,20 +1112,27 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req *
// rows are created if necessary when leased). It also sets the leasedUntil time
// to be equal to thisUpdate, to indicate that the shard is no longer leased.
func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.UpdateCRLShardRequest) (*emptypb.Empty, error) {
if core.IsAnyNilOrZero(req.IssuerNameID, req.ThisUpdate, req.NextUpdate) {
if core.IsAnyNilOrZero(req.IssuerNameID, req.ThisUpdate) {
return nil, errIncompleteRequest
}
// Only set the nextUpdate if it's actually present in the request message.
var nextUpdate *time.Time
if req.NextUpdate != nil {
nut := req.NextUpdate.AsTime()
nextUpdate = &nut
}
_, err := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
res, err := tx.ExecContext(ctx,
`UPDATE crlShards
SET thisUpdate = ?, nextUpdate = ?, leasedUntil = ?
WHERE issuerID = ?
AND idx = ?
AND thisUpdate < ?
AND (thisUpdate is NULL OR thisUpdate < ?)
LIMIT 1`,
req.ThisUpdate.AsTime(),
req.NextUpdate.AsTime(),
nextUpdate,
req.ThisUpdate.AsTime(),
req.IssuerNameID,
req.ShardIdx,

46
sa/sa_test.go
View File

@ -3492,10 +3492,6 @@ func TestLeaseSpecificCRLShard(t *testing.T) {
}
func TestUpdateCRLShard(t *testing.T) {
if os.Getenv("BOULDER_CONFIG_DIR") != "test/config-next" {
t.Skip("Test requires crlShards database table")
}
sa, clk, cleanUp := initSA(t)
defer cleanUp()
@ -3523,8 +3519,9 @@ func TestUpdateCRLShard(t *testing.T) {
test.AssertNotError(t, err, "setting up test shards")
thisUpdate := clk.Now().Truncate(time.Second).UTC()
var thisUpdateModel struct {
ThisUpdate time.Time `db:"thisUpdate"`
var crlModel struct {
ThisUpdate *time.Time
NextUpdate *time.Time
}
// Updating a leased shard should work.
@ -3541,13 +3538,11 @@ func TestUpdateCRLShard(t *testing.T) {
err = sa.dbMap.SelectOne(
ctx,
&thisUpdateModel,
`SELECT thisUpdate FROM crlShards WHERE issuerID = ? AND idx = ? LIMIT 1`,
1,
0,
&crlModel,
`SELECT thisUpdate FROM crlShards WHERE issuerID = 1 AND idx = 0 LIMIT 1`,
)
test.AssertNotError(t, err, "getting updated thisUpdate timestamp")
test.Assert(t, thisUpdateModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp")
test.Assert(t, crlModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp")
// Updating an unleased shard should work.
_, err = sa.UpdateCRLShard(
@ -3563,13 +3558,30 @@ func TestUpdateCRLShard(t *testing.T) {
err = sa.dbMap.SelectOne(
ctx,
&thisUpdateModel,
`SELECT thisUpdate FROM crlShards WHERE issuerID = ? AND idx = ? LIMIT 1`,
1,
1,
&crlModel,
`SELECT thisUpdate FROM crlShards WHERE issuerID = 1 AND idx = 1 LIMIT 1`,
)
test.AssertNotError(t, err, "getting updated thisUpdate timestamp")
test.Assert(t, thisUpdateModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp")
test.Assert(t, crlModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp")
// Updating without supplying a NextUpdate should work.
_, err = sa.UpdateCRLShard(
context.Background(),
&sapb.UpdateCRLShardRequest{
IssuerNameID: 1,
ShardIdx: 3,
ThisUpdate: timestamppb.New(thisUpdate.Add(time.Second)),
},
)
test.AssertNotError(t, err, "updating shard without NextUpdate")
err = sa.dbMap.SelectOne(
ctx,
&crlModel,
`SELECT nextUpdate FROM crlShards WHERE issuerID = 1 AND idx = 3 LIMIT 1`,
)
test.AssertNotError(t, err, "getting updated nextUpdate timestamp")
test.AssertBoxedNil(t, crlModel.NextUpdate, "checking updated nextUpdate timestamp")
// Updating a shard to an earlier time should fail.
_, err = sa.UpdateCRLShard(
@ -3593,5 +3605,5 @@ func TestUpdateCRLShard(t *testing.T) {
NextUpdate: timestamppb.New(thisUpdate.Add(10 * 24 * time.Hour)),
},
)
test.AssertError(t, err, "updating shard to an earlier time")
test.AssertError(t, err, "updating an unknown shard")
}