From 7f30fa73b510dc39afc33d1e890a7ff8a8fa3dfa Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 08:46:53 -0800 Subject: [PATCH 1/6] Use SELECT ... FOR UPDATE for reads within transactions --- sa/sa.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index 4d867dbc606..84ef5f46bad 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -311,7 +311,7 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb var row struct { Count int64 } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM precertificates WHERE serial=?", serialHex) + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM precertificates WHERE serial=? FOR UPDATE", serialHex) if err != nil { return nil, err } @@ -404,7 +404,7 @@ func (ssa *SQLStorageAuthority) AddCertificate(ctx context.Context, req *sapb.Ad var row struct { Count int64 } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM certificates WHERE serial=?", serial) + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM certificates WHERE serial=? FOR UPDATE", serial) if err != nil { return nil, err } @@ -1082,7 +1082,8 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa `SELECT id, issuerID, idx, thisUpdate, nextUpdate, leasedUntil FROM crlShards WHERE issuerID = ? - AND idx BETWEEN ? AND ?`, + AND idx BETWEEN ? AND ? + FOR UPDATE`, req.IssuerNameID, req.MinShardIdx, req.MaxShardIdx, ) if err != nil { @@ -1184,7 +1185,8 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * FROM crlShards WHERE issuerID = ? AND idx = ? - LIMIT 1`, + LIMIT 1 + FOR UPDATE`, req.IssuerNameID, req.MinShardIdx, ) From 177907e03b39496c970944d5ee5d488097ca3ab5 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 09:06:03 -0800 Subject: [PATCH 2/6] Undo change to Add[Pre]Certificate; too unknown --- sa/sa.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index 84ef5f46bad..e735fa911ee 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -311,7 +311,7 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb var row struct { Count int64 } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM precertificates WHERE serial=? FOR UPDATE", serialHex) + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM precertificates WHERE serial=?", serialHex) if err != nil { return nil, err } @@ -404,7 +404,7 @@ func (ssa *SQLStorageAuthority) AddCertificate(ctx context.Context, req *sapb.Ad var row struct { Count int64 } - err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM certificates WHERE serial=? FOR UPDATE", serial) + err := tx.SelectOne(ctx, &row, "SELECT COUNT(*) as count FROM certificates WHERE serial=?", serial) if err != nil { return nil, err } From 2aeb0e2bc9890125e12ecd496c0a7c9c71fa1000 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 10:31:35 -0800 Subject: [PATCH 3/6] Rename test case for easier test filtering --- test/integration/crl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/crl_test.go b/test/integration/crl_test.go index 7cbdc19d4c0..575b295a158 100644 --- a/test/integration/crl_test.go +++ b/test/integration/crl_test.go @@ -102,7 +102,7 @@ func TestCRLPipeline(t *testing.T) { resp.Body.Close() } -func TestTemporalAndExplicitShardingCoexist(t *testing.T) { +func TestCRLTemporalAndExplicitShardingCoexist(t *testing.T) { db, err := sql.Open("mysql", vars.DBConnSAIntegrationFullPerms) if err != nil { t.Fatalf("sql.Open: %s", err) From 761bfb523365766be3685d25d07430f84828aab5 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 10:32:02 -0800 Subject: [PATCH 4/6] Undo SELECT...FOR UPDATE change --- sa/sa.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index e735fa911ee..4d867dbc606 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -1082,8 +1082,7 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa `SELECT id, issuerID, idx, thisUpdate, nextUpdate, leasedUntil FROM crlShards WHERE issuerID = ? - AND idx BETWEEN ? AND ? - FOR UPDATE`, + AND idx BETWEEN ? AND ?`, req.IssuerNameID, req.MinShardIdx, req.MaxShardIdx, ) if err != nil { @@ -1185,8 +1184,7 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * FROM crlShards WHERE issuerID = ? AND idx = ? - LIMIT 1 - FOR UPDATE`, + LIMIT 1`, req.IssuerNameID, req.MinShardIdx, ) From 7a18f315e1d28e1d6fe3a13b490d2c63c1c41ebd Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 10:32:34 -0800 Subject: [PATCH 5/6] Condition UPDATEs on WHERE LeasedUntil to prevent transaction interleaving --- sa/sa.go | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index 4d867dbc606..963d91f1005 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -1091,7 +1091,6 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa // Determine which shard index we want to lease. var shardIdx int - var needToInsert bool if len(shards) < (int(req.MaxShardIdx + 1 - req.MinShardIdx)) { // Some expected shards are missing (i.e. never-before-produced), so we // pick one at random. @@ -1107,7 +1106,17 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa shardIdx = idx break } - needToInsert = true + + _, err = tx.ExecContext(ctx, + `INSERT INTO crlShards (issuerID, idx, leasedUntil) + VALUES (?, ?, ?)`, + req.IssuerNameID, + shardIdx, + req.Until.AsTime(), + ) + if err != nil { + return -1, fmt.Errorf("inserting selected shard: %w", err) + } } else { // We got all the shards we expect, so we pick the oldest unleased shard. var oldest *crlShardModel @@ -1125,30 +1134,18 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa return -1, fmt.Errorf("issuer %d has no unleased shards in range %d-%d", req.IssuerNameID, req.MinShardIdx, req.MaxShardIdx) } shardIdx = oldest.Idx - needToInsert = false - } - if needToInsert { - _, err = tx.ExecContext(ctx, - `INSERT INTO crlShards (issuerID, idx, leasedUntil) - VALUES (?, ?, ?)`, - req.IssuerNameID, - shardIdx, - req.Until.AsTime(), - ) - if err != nil { - return -1, fmt.Errorf("inserting selected shard: %w", err) - } - } else { _, err = tx.ExecContext(ctx, `UPDATE crlShards SET leasedUntil = ? WHERE issuerID = ? AND idx = ? + AND leasedUntil = ? LIMIT 1`, req.Until.AsTime(), req.IssuerNameID, shardIdx, + oldest.LeasedUntil, ) if err != nil { return -1, fmt.Errorf("updating selected shard: %w", err) @@ -1213,10 +1210,12 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * SET leasedUntil = ? WHERE issuerID = ? AND idx = ? + AND leasedUntil = ? LIMIT 1`, req.Until.AsTime(), req.IssuerNameID, req.MinShardIdx, + shardModel.LeasedUntil, ) if err != nil { return nil, fmt.Errorf("updating selected shard: %w", err) From dfccb905de1f714a1a29bd8ad5a58534fbb10313 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 28 Feb 2025 13:35:36 -0800 Subject: [PATCH 6/6] add rows-affected checks --- sa/sa.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/sa/sa.go b/sa/sa.go index 963d91f1005..629d92534cd 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -1135,7 +1135,7 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa } shardIdx = oldest.Idx - _, err = tx.ExecContext(ctx, + res, err := tx.ExecContext(ctx, `UPDATE crlShards SET leasedUntil = ? WHERE issuerID = ? @@ -1150,6 +1150,13 @@ func (ssa *SQLStorageAuthority) leaseOldestCRLShard(ctx context.Context, req *sa if err != nil { return -1, fmt.Errorf("updating selected shard: %w", err) } + rowsAffected, err := res.RowsAffected() + if err != nil { + return -1, fmt.Errorf("confirming update of selected shard: %w", err) + } + if rowsAffected != 1 { + return -1, errors.New("failed to lease shard") + } } return shardIdx, err @@ -1205,7 +1212,7 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * return nil, fmt.Errorf("inserting selected shard: %w", err) } } else { - _, err = tx.ExecContext(ctx, + res, err := tx.ExecContext(ctx, `UPDATE crlShards SET leasedUntil = ? WHERE issuerID = ? @@ -1220,6 +1227,13 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * if err != nil { return nil, fmt.Errorf("updating selected shard: %w", err) } + rowsAffected, err := res.RowsAffected() + if err != nil { + return -1, fmt.Errorf("confirming update of selected shard: %w", err) + } + if rowsAffected != 1 { + return -1, errors.New("failed to lease shard") + } } return nil, nil