From 2b5b6239a4c2d611c9d9fb7a23a79d0c5f9218a5 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 12 Jun 2024 15:00:24 -0700 Subject: [PATCH] sa: truncate all timestamps to seconds (#7519) As described in #7075, go-sql-driver/mysql v1.5.0 truncates timestamps to microseconds, while v1.6.0 and above does not. That means upon upgrading to v1.6.0, timestamps are written to the database with a resolution of nanoseconds, and SELECT statements also use a resolution of nanoseconds. We believe this is the cause of performance problems we observed when upgrading to v1.6.0 and above. To fix that, apply rounding in the application code. Rather than just rounding to microseconds, round to seconds since that is the resolution we care about. Using seconds rather than microseconds may also allow some of our indexes to grow more slowly over time. Note: this omits truncating some timestamps in CRL shard calculations, since truncating those resulted in test failures that I'll follow up on separately. --- cmd/bad-key-revoker/main.go | 2 +- docs/CONTRIBUTING.md | 7 ++++ sa/sa.go | 84 ++++++++++++++++++++++++------------- sa/sa_test.go | 2 +- sa/saro.go | 39 ++++++++--------- 5 files changed, 83 insertions(+), 51 deletions(-) diff --git a/cmd/bad-key-revoker/main.go b/cmd/bad-key-revoker/main.go index e7015e0c8..b234987f5 100644 --- a/cmd/bad-key-revoker/main.go +++ b/cmd/bad-key-revoker/main.go @@ -141,7 +141,7 @@ func (bkr *badKeyRevoker) findUnrevoked(ctx context.Context, unchecked unchecked "SELECT id, certSerial FROM keyHashToSerial WHERE keyHash = ? AND id > ? AND certNotAfter > ? ORDER BY id LIMIT ?", unchecked.KeyHash, initialID, - bkr.clk.Now(), + bkr.clk.Now().Truncate(time.Second), bkr.serialBatchSize, ) if err != nil { diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 7cefe6dde..7e311ae9e 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -324,6 +324,13 @@ value. The `core.IsAnyNilOrZero` function can check these cases. Senders must check that timestamps are non-zero before sending them. Receivers must check that timestamps are non-zero before accepting them. +# Rounding time in DB + +All times that we write to the database are truncated to one second's worth of +precision. This reduces the size of indexes that include timestamps, and makes +querying them more efficient. The Storage Authority (SA) is responsible for this +truncation, and performs it for SELECT queries as well as INSERT and UPDATE. + # Release Process The current Boulder release process is described in diff --git a/sa/sa.go b/sa/sa.go index b1af5118c..c1cb9ff85 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -108,7 +108,7 @@ func (ssa *SQLStorageAuthority) NewRegistration(ctx context.Context, req *corepb return nil, err } - reg.CreatedAt = ssa.clk.Now() + reg.CreatedAt = ssa.clk.Now().Truncate(time.Second) err = ssa.dbMap.Insert(ctx, reg) if err != nil { @@ -141,6 +141,10 @@ func (ssa *SQLStorageAuthority) UpdateRegistration(ctx context.Context, req *cor return nil, err } + // The CreatedAt field shouldn't change from the original, so we copy it straight through. + // This also ensures that it's already truncated to second (which happened on creation). + update.CreatedAt = curr.CreatedAt + // Copy the existing registration model's LockCol to the new updated // registration model's LockCol update.LockCol = curr.LockCol @@ -169,8 +173,8 @@ func (ssa *SQLStorageAuthority) AddSerial(ctx context.Context, req *sapb.AddSeri err := ssa.dbMap.Insert(ctx, &recordedSerialModel{ Serial: req.Serial, RegistrationID: req.RegID, - Created: req.Created.AsTime(), - Expires: req.Expires.AsTime(), + Created: req.Created.AsTime().Truncate(time.Second), + Expires: req.Expires.AsTime().Truncate(time.Second), }) if err != nil { return nil, err @@ -223,7 +227,7 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb Serial: serialHex, RegistrationID: req.RegID, DER: req.Der, - Issued: req.Issued.AsTime(), + Issued: req.Issued.AsTime().Truncate(time.Second), Expires: parsed.NotAfter, } @@ -252,13 +256,15 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb cs := &core.CertificateStatus{ Serial: serialHex, Status: status, - OCSPLastUpdated: ssa.clk.Now(), + OCSPLastUpdated: ssa.clk.Now().Truncate(time.Second), RevokedDate: time.Time{}, RevokedReason: 0, LastExpirationNagSent: time.Time{}, - NotAfter: parsed.NotAfter, - IsExpired: false, - IssuerNameID: req.IssuerNameID, + // No need to truncate because it's already truncated to encode + // per https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.5.1 + NotAfter: parsed.NotAfter, + IsExpired: false, + IssuerNameID: req.IssuerNameID, } err = ssa.dbMap.Insert(ctx, cs) if err != nil { @@ -317,7 +323,7 @@ func (ssa *SQLStorageAuthority) AddCertificate(ctx context.Context, req *sapb.Ad Serial: serial, Digest: digest, DER: req.Der, - Issued: req.Issued.AsTime(), + Issued: req.Issued.AsTime().Truncate(time.Second), Expires: parsedCertificate.NotAfter, } @@ -476,13 +482,15 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb if err != nil { return nil, err } + // These parameters correspond to the fields listed in `authzFields`, as used in the + // `db.NewMultiInserter` call above, and occur in the same order. err = inserter.Add([]interface{}{ am.ID, am.IdentifierType, am.IdentifierValue, am.RegistrationID, statusToUint[core.StatusPending], - am.Expires, + am.Expires.Truncate(time.Second), am.Challenges, nil, nil, @@ -503,11 +511,12 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb // Second, insert the new order. var orderID int64 var err error - created := ssa.clk.Now() + created := ssa.clk.Now().Truncate(time.Second) + expires := req.NewOrder.Expires.AsTime().Truncate(time.Second) if features.Get().MultipleCertificateProfiles { omv2 := orderModelv2{ RegistrationID: req.NewOrder.RegistrationID, - Expires: req.NewOrder.Expires.AsTime(), + Expires: expires, Created: created, CertificateProfileName: req.NewOrder.CertificateProfileName, } @@ -516,7 +525,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb } else { omv1 := orderModelv1{ RegistrationID: req.NewOrder.RegistrationID, - Expires: req.NewOrder.Expires.AsTime(), + Expires: expires, Created: created, } err = tx.Insert(ctx, &omv1) @@ -549,19 +558,25 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb } // Fourth, insert the FQDNSet entry for the order. - err = addOrderFQDNSet(ctx, tx, req.NewOrder.Names, orderID, req.NewOrder.RegistrationID, req.NewOrder.Expires.AsTime()) + err = addOrderFQDNSet(ctx, + tx, + req.NewOrder.Names, + orderID, + req.NewOrder.RegistrationID, + expires, + ) if err != nil { return nil, err } - // Finally, build the overall Order PB. + // Finally, build the overall Order PB to return. res := &corepb.Order{ // ID and Created were auto-populated on the order model when it was inserted. Id: orderID, Created: timestamppb.New(created), // These are carried over from the original request unchanged. RegistrationID: req.NewOrder.RegistrationID, - Expires: req.NewOrder.Expires, + Expires: timestamppb.New(expires), Names: req.NewOrder.Names, // Have to combine the already-associated and newly-reacted authzs. V2Authorizations: append(req.NewOrder.V2Authorizations, newAuthzIDs...), @@ -576,7 +591,12 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb if req.NewOrder.ReplacesSerial != "" { // Update the replacementOrders table to indicate that this order // replaces the provided certificate serial. - err := addReplacementOrder(ctx, tx, req.NewOrder.ReplacesSerial, orderID, req.NewOrder.Expires.AsTime()) + err := addReplacementOrder(ctx, + tx, + req.NewOrder.ReplacesSerial, + orderID, + req.NewOrder.Expires.AsTime().Truncate(time.Second), + ) if err != nil { return nil, err } @@ -789,7 +809,7 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization2(ctx context.Context, req // database attemptedAt field Null instead of 1970-01-01 00:00:00. var attemptedTime *time.Time if !core.IsAnyNilOrZero(req.AttemptedAt) { - val := req.AttemptedAt.AsTime() + val := req.AttemptedAt.AsTime().Truncate(time.Second) attemptedTime = &val } params := map[string]interface{}{ @@ -799,7 +819,7 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization2(ctx context.Context, req "validationRecord": vrJSON, "id": req.Id, "pending": statusUint(core.StatusPending), - "expires": req.Expires.AsTime(), + "expires": req.Expires.AsTime().Truncate(time.Second), // if req.ValidationError is nil veJSON should also be nil // which should result in a NULL field "validationError": veJSON, @@ -867,7 +887,7 @@ func (ssa *SQLStorageAuthority) RevokeCertificate(ctx context.Context, req *sapb } _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) { - revokedDate := req.Date.AsTime() + revokedDate := req.Date.AsTime().Truncate(time.Second) res, err := tx.ExecContext(ctx, `UPDATE certificateStatus SET @@ -924,8 +944,8 @@ func (ssa *SQLStorageAuthority) UpdateRevokedCertificate(ctx context.Context, re } _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) { - thisUpdate := req.Date.AsTime() - revokedDate := req.Backdate.AsTime() + thisUpdate := req.Date.AsTime().Truncate(time.Second) + revokedDate := req.Backdate.AsTime().Truncate(time.Second) res, err := tx.ExecContext(ctx, `UPDATE certificateStatus SET @@ -1007,7 +1027,7 @@ func (ssa *SQLStorageAuthority) AddBlockedKey(ctx context.Context, req *sapb.Add cols, qs := blockedKeysColumns, "?, ?, ?, ?" vals := []interface{}{ req.KeyHash, - req.Added.AsTime(), + req.Added.AsTime().Truncate(time.Second), sourceInt, req.Comment, } @@ -1232,7 +1252,10 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req * // UpdateCRLShard updates the thisUpdate and nextUpdate timestamps of a CRL // shard. It rejects the update if it would cause the thisUpdate timestamp to -// move backwards. It does *not* reject the update if the shard is no longer +// move backwards, but if thisUpdate would stay the same (for instance, multiple +// CRL generations within a single second), it will succeed. +// +// It does *not* reject the update if the shard is no longer // leased: although this would be unexpected (because the lease timestamp should // be the same as the crl-updater's context expiration), it's not inherently a // sign of an update that should be skipped. It does reject the update if the @@ -1247,24 +1270,25 @@ func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.Up // 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() + nut := req.NextUpdate.AsTime().Truncate(time.Second) nextUpdate = &nut } _, err := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) { + thisUpdate := req.ThisUpdate.AsTime().Truncate(time.Second) res, err := tx.ExecContext(ctx, `UPDATE crlShards SET thisUpdate = ?, nextUpdate = ?, leasedUntil = ? WHERE issuerID = ? AND idx = ? - AND (thisUpdate is NULL OR thisUpdate < ?) + AND (thisUpdate is NULL OR thisUpdate <= ?) LIMIT 1`, - req.ThisUpdate.AsTime(), + thisUpdate, nextUpdate, - req.ThisUpdate.AsTime(), + thisUpdate, req.IssuerNameID, req.ShardIdx, - req.ThisUpdate.AsTime(), + thisUpdate, ) if err != nil { return nil, err @@ -1275,7 +1299,7 @@ func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.Up return nil, err } if rowsAffected == 0 { - return nil, fmt.Errorf("unable to update shard %d for issuer %d", req.ShardIdx, req.IssuerNameID) + return nil, fmt.Errorf("unable to update shard %d for issuer %d; possibly because shard exists", req.ShardIdx, req.IssuerNameID) } if rowsAffected != 1 { return nil, errors.New("update affected unexpected number of rows") diff --git a/sa/sa_test.go b/sa/sa_test.go index d4bb066af..9686c03ec 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -4012,7 +4012,7 @@ func TestUpdateCRLShard(t *testing.T) { `SELECT thisUpdate FROM crlShards WHERE issuerID = 1 AND idx = 0 LIMIT 1`, ) test.AssertNotError(t, err, "getting updated thisUpdate timestamp") - test.Assert(t, crlModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp") + test.AssertEquals(t, *crlModel.ThisUpdate, thisUpdate) // Updating an unleased shard should work. _, err = sa.UpdateCRLShard( diff --git a/sa/saro.go b/sa/saro.go index 9bfb4b626..670e893da 100644 --- a/sa/saro.go +++ b/sa/saro.go @@ -230,8 +230,8 @@ func (ssa *SQLStorageAuthorityRO) CountRegistrationsByIP(ctx context.Context, re createdAt <= :latest`, map[string]interface{}{ "ip": req.Ip, - "earliest": req.Range.Earliest.AsTime(), - "latest": req.Range.Latest.AsTime(), + "earliest": req.Range.Earliest.AsTime().Truncate(time.Second), + "latest": req.Range.Latest.AsTime().Truncate(time.Second), }) if err != nil { return nil, err @@ -261,8 +261,8 @@ func (ssa *SQLStorageAuthorityRO) CountRegistrationsByIPRange(ctx context.Contex :earliest < createdAt AND createdAt <= :latest`, map[string]interface{}{ - "earliest": req.Range.Earliest.AsTime(), - "latest": req.Range.Latest.AsTime(), + "earliest": req.Range.Earliest.AsTime().Truncate(time.Second), + "latest": req.Range.Latest.AsTime().Truncate(time.Second), "beginIP": beginIP, "endIP": endIP, }) @@ -507,7 +507,7 @@ func (ssa *SQLStorageAuthorityRO) CountFQDNSets(ctx context.Context, req *sapb.C WHERE setHash = ? AND issued > ?`, core.HashNames(req.Domains), - ssa.clk.Now().Add(-req.Window.AsDuration()), + ssa.clk.Now().Add(-req.Window.AsDuration()).Truncate(time.Second), ) return &sapb.Count{Count: count}, err } @@ -531,7 +531,7 @@ func (ssa *SQLStorageAuthorityRO) FQDNSetTimestampsForWindow(ctx context.Context AND issued > ? ORDER BY issued DESC`, core.HashNames(req.Domains), - ssa.clk.Now().Add(-req.Window.AsDuration()), + ssa.clk.Now().Add(-req.Window.AsDuration()).Truncate(time.Second), ) if err != nil { return nil, err @@ -708,7 +708,8 @@ func (ssa *SQLStorageAuthorityRO) GetOrderForNames(ctx context.Context, req *sap AND expires > ? ORDER BY expires ASC LIMIT 1`, - fqdnHash, ssa.clk.Now()) + fqdnHash, + ssa.clk.Now().Truncate(time.Second)) if db.IsNoRows(err) { return nil, berrors.NotFoundError("no order matching request found") @@ -791,7 +792,7 @@ func (ssa *SQLStorageAuthorityRO) GetAuthorizations2(ctx context.Context, req *s req.RegistrationID, statusUint(core.StatusValid), statusUint(core.StatusPending), - req.Now.AsTime(), + req.Now.AsTime().Truncate(time.Second), identifierTypeToUint[string(identifier.DNS)], } @@ -859,7 +860,7 @@ func (ssa *SQLStorageAuthorityRO) GetPendingAuthorization2(ctx context.Context, map[string]interface{}{ "regID": req.RegistrationID, "status": statusUint(core.StatusPending), - "validUntil": req.ValidUntil.AsTime(), + "validUntil": req.ValidUntil.AsTime().Truncate(time.Second), "dnsType": identifierTypeToUint[string(identifier.DNS)], "ident": req.IdentifierValue, }, @@ -888,7 +889,7 @@ func (ssa *SQLStorageAuthorityRO) CountPendingAuthorizations2(ctx context.Contex status = :status`, map[string]interface{}{ "regID": req.Id, - "expires": ssa.clk.Now(), + "expires": ssa.clk.Now().Truncate(time.Second), "status": statusUint(core.StatusPending), }, ) @@ -929,7 +930,7 @@ func (ssa *SQLStorageAuthorityRO) GetValidOrderAuthorizations2(ctx context.Conte ), map[string]interface{}{ "regID": req.AcctID, - "expires": ssa.clk.Now(), + "expires": ssa.clk.Now().Truncate(time.Second), "status": statusUint(core.StatusValid), "orderID": req.Id, }, @@ -975,8 +976,8 @@ func (ssa *SQLStorageAuthorityRO) CountInvalidAuthorizations2(ctx context.Contex "regID": req.RegistrationID, "dnsType": identifierTypeToUint[string(identifier.DNS)], "ident": req.Hostname, - "expiresEarliest": req.Range.Earliest.AsTime(), - "expiresLatest": req.Range.Latest.AsTime(), + "expiresEarliest": req.Range.Earliest.AsTime().Truncate(time.Second), + "expiresLatest": req.Range.Latest.AsTime().Truncate(time.Second), "status": statusUint(core.StatusInvalid), }, ) @@ -1009,7 +1010,7 @@ func (ssa *SQLStorageAuthorityRO) GetValidAuthorizations2(ctx context.Context, r params := []interface{}{ req.RegistrationID, statusUint(core.StatusValid), - req.Now.AsTime(), + req.Now.AsTime().Truncate(time.Second), identifierTypeToUint[string(identifier.DNS)], } for _, domain := range req.Domains { @@ -1214,7 +1215,7 @@ func (ssa *SQLStorageAuthorityRO) getRevokedCertsFromRevokedCertificatesTable(re }) } -// getRevokedCertsFromCertificateStatusTable uses the new old certificateStatus +// getRevokedCertsFromCertificateStatusTable uses the old certificateStatus // table to implement GetRevokedCerts. func (ssa *SQLStorageAuthorityRO) getRevokedCertsFromCertificateStatusTable(req *sapb.GetRevokedCertsRequest, stream grpc.ServerStreamingServer[corepb.CRLEntry]) error { atTime := req.RevokedBefore.AsTime() @@ -1225,8 +1226,8 @@ func (ssa *SQLStorageAuthorityRO) getRevokedCertsFromCertificateStatusTable(req AND issuerID = ? AND status = ?` params := []interface{}{ - req.ExpiresAfter.AsTime(), - req.ExpiresBefore.AsTime(), + req.ExpiresAfter.AsTime().Truncate(time.Second), + req.ExpiresBefore.AsTime().Truncate(time.Second), req.IssuerNameID, core.OCSPStatusRevoked, } @@ -1357,7 +1358,7 @@ func (ssa *SQLStorageAuthorityRO) GetSerialsByKey(req *sapb.SPKIHash, stream grp AND certNotAfter > ?` params := []interface{}{ req.KeyHash, - ssa.clk.Now(), + ssa.clk.Now().Truncate(time.Second), } selector, err := db.NewMappedSelector[keyHashModel](ssa.dbReadOnlyMap) @@ -1384,7 +1385,7 @@ func (ssa *SQLStorageAuthorityRO) GetSerialsByAccount(req *sapb.RegistrationID, AND expires > ?` params := []interface{}{ req.Id, - ssa.clk.Now(), + ssa.clk.Now().Truncate(time.Second), } selector, err := db.NewMappedSelector[recordedSerialModel](ssa.dbReadOnlyMap)