Skip to content

Commit

Permalink
sa: wrap transactions in a function for commits/rollbacks (#4373)
Browse files Browse the repository at this point in the history 
In the current SA code, we need to remember to call Rollback on any error.
If we don't, we'll leave dangling transactions, which are hard to spot but eventually
clog up the database and cause availability problems.

This change attempts to deal with rollbacks more rigorously, by implementing a
withTransaction function that takes a closure as input. withTransaction opens
a transaction, applies a context.Context to it, and then runs the closure. If the
closure returns an error, withTransaction rolls back and return the error; otherwise
it commits and returns nil.

One of the quirks of this implementation is that it relies on the closure modifying
variables from its parent scope in order to return values. An alternate implementation
could define the return value of the closure as interface{}, nil, and have the calling
function do a type assertion. I'm seeking feedback on that; not sure yet which is cleaner.

This is a subset of the functions that need this treatment. I've got more coming, but
some of the changes break tests so I'm checking into why.

Updates #4337
  • Loading branch information
@jsha @rolandshoemaker
jsha authored and rolandshoemaker committed Jul 31, 2019
1 parent eb20b2a commit 16235b6
Show file tree
Hide file tree
Showing 2 changed files with 196 additions and 167 deletions.
324 changes: 157 additions & 167 deletions sa/sa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,41 +223,41 @@ func (ssa *SQLStorageAuthority) GetRegistrationByKey(ctx context.Context, key *j

// GetAuthorization obtains an Authorization by ID
func (ssa *SQLStorageAuthority) GetAuthorization(ctx context.Context, id string) (core.Authorization, error) {
authz := core.Authorization{}
tx, err := ssa.dbMap.Begin()
if err != nil {
return authz, err
}
txWithCtx := tx.WithContext(ctx)

pa, err := selectPendingAuthz(txWithCtx, "WHERE id = ?", id)
if err != nil && err != sql.ErrNoRows {
return authz, Rollback(tx, err)
}
if err == sql.ErrNoRows {
var fa authzModel
err := txWithCtx.SelectOne(&fa, fmt.Sprintf("SELECT %s FROM authz WHERE id = ?", authzFields), id)
authz, overallError := withTransaction(ctx, ssa.dbMap, func(txWithCtx transaction) (interface{}, error) {
pa, err := selectPendingAuthz(txWithCtx, "WHERE id = ?", id)
if err != nil && err != sql.ErrNoRows {
return authz, Rollback(tx, err)
} else if err == sql.ErrNoRows {
// If there was no result in either the pending authz table or the authz
// table then return a `berrors.NotFound` instance (or a rollback error if
// the transaction rollback fails)
return authz, Rollback(
tx,
berrors.NotFoundError("no authorization found with id %q", id))
}
authz = fa.Authorization
} else {
authz = pa.Authorization
}
return nil, err
}
var authz core.Authorization
if err == sql.ErrNoRows {
var fa authzModel
err := txWithCtx.SelectOne(&fa, fmt.Sprintf("SELECT %s FROM authz WHERE id = ?", authzFields), id)
if err != nil && err != sql.ErrNoRows {
return nil, err
} else if err == sql.ErrNoRows {
// If there was no result in either the pending authz table or the authz
// table then return a `berrors.NotFound` instance
return nil, berrors.NotFoundError("no authorization found with id %q", id)
}
authz = fa.Authorization
} else {
authz = pa.Authorization
}

authz.Challenges, err = ssa.getChallenges(txWithCtx, authz.ID)
if err != nil {
return authz, Rollback(tx, err)
}
authz.Challenges, err = ssa.getChallenges(txWithCtx, authz.ID)
if err != nil {
return nil, err
}

return authz, tx.Commit()
return authz, nil
})
if overallError != nil {
return core.Authorization{}, overallError
}
if authz, ok := authz.(core.Authorization); ok {
return authz, nil
}
return core.Authorization{}, fmt.Errorf("shouldn't happen: casting error in GetAuthorization")
}

// GetValidAuthorizations returns the latest authorization object for all
Expand Down Expand Up @@ -555,57 +555,52 @@ func (ssa *SQLStorageAuthority) UpdateRegistration(ctx context.Context, reg core
// NewPendingAuthorization retrieves a pending authorization for
// authz.Identifier if one exists, or creates a new one otherwise.
func (ssa *SQLStorageAuthority) NewPendingAuthorization(ctx context.Context, authz core.Authorization) (core.Authorization, error) {
var output core.Authorization

tx, err := ssa.dbMap.Begin()
if err != nil {
return output, err
}
txWithCtx := tx.WithContext(ctx)

// Create a random ID and check that it doesn't exist already
authz.ID = core.NewToken()
for existingPending(txWithCtx, authz.ID) ||
existingFinal(txWithCtx, authz.ID) {
output, overallError := withTransaction(ctx, ssa.dbMap, func(txWithCtx transaction) (interface{}, error) {
// Create a random ID and check that it doesn't exist already
authz.ID = core.NewToken()
}

// Insert a stub row in pending
pendingAuthz := pendingauthzModel{Authorization: authz}
err = txWithCtx.Insert(&pendingAuthz)
if err != nil {
err = Rollback(tx, err)
return output, err
}
for existingPending(txWithCtx, authz.ID) ||
existingFinal(txWithCtx, authz.ID) {
authz.ID = core.NewToken()
}

for i, c := range authz.Challenges {
challModel, err := challengeToModel(&c, pendingAuthz.ID)
if err != nil {
err = Rollback(tx, err)
return output, err
}
// Magic happens here: Gorp will modify challModel, setting challModel.ID
// to the auto-increment primary key. This is important because we want
// the challenge objects inside the Authorization we return to know their
// IDs, so they can have proper URLs.
// See https://godoc.org/github.com/coopernurse/gorp#DbMap.Insert
err = txWithCtx.Insert(challModel)
// Insert a stub row in pending
pendingAuthz := pendingauthzModel{Authorization: authz}
err := txWithCtx.Insert(&pendingAuthz)
if err != nil {
err = Rollback(tx, err)
return output, err
return nil, err
}
challenge, err := modelToChallenge(challModel)
if err != nil {
err = Rollback(tx, err)
return output, err

for i, c := range authz.Challenges {
challModel, err := challengeToModel(&c, pendingAuthz.ID)
if err != nil {
return nil, err
}
// Magic happens here: Gorp will modify challModel, setting challModel.ID
// to the auto-increment primary key. This is important because we want
// the challenge objects inside the Authorization we return to know their
// IDs, so they can have proper URLs.
// See https://godoc.org/github.com/coopernurse/gorp#DbMap.Insert
err = txWithCtx.Insert(challModel)
if err != nil {
return nil, err
}
challenge, err := modelToChallenge(challModel)
if err != nil {
return nil, err
}
authz.Challenges[i] = challenge
}
authz.Challenges[i] = challenge
}

err = tx.Commit()
output = pendingAuthz.Authorization
output.Challenges = authz.Challenges
return output, err
pendingAuthz.Authorization.Challenges = authz.Challenges
return pendingAuthz.Authorization, nil
})
if overallError != nil {
return core.Authorization{}, overallError
}
if output, ok := output.(core.Authorization); ok {
return output, nil
}
return core.Authorization{}, fmt.Errorf("shouldn't happen: casting error in NewPendingAuthorization")
}

// GetPendingAuthorization returns the most recent Pending authorization
Expand Down Expand Up @@ -657,47 +652,42 @@ func (ssa *SQLStorageAuthority) GetPendingAuthorization(
// Authorization is not found a berrors.NotFound result is returned. If the
// Authorization is status pending a berrors.InternalServer error is returned.
func (ssa *SQLStorageAuthority) FinalizeAuthorization(ctx context.Context, authz core.Authorization) error {
tx, err := ssa.dbMap.Begin()
if err != nil {
return err
}
txWithCtx := tx.WithContext(ctx)

// Check that a pending authz exists
if !existingPending(txWithCtx, authz.ID) {
err = berrors.NotFoundError("authorization with ID %q not found", authz.ID)
return Rollback(tx, err)
}
if statusIsPending(authz.Status) {
err = berrors.InternalServerError("authorization to finalize is pending (ID %q)", authz.ID)
return Rollback(tx, err)
}
_, overallError := withTransaction(ctx, ssa.dbMap, func(txWithCtx transaction) (interface{}, error) {
// Check that a pending authz exists
if !existingPending(txWithCtx, authz.ID) {
return nil, berrors.NotFoundError("authorization with ID %q not found", authz.ID)
}
if statusIsPending(authz.Status) {
return nil, berrors.InternalServerError("authorization to finalize is pending (ID %q)", authz.ID)
}

auth := &authzModel{authz}
pa, err := selectPendingAuthz(txWithCtx, "WHERE id = ?", authz.ID)
if err == sql.ErrNoRows {
return Rollback(tx, berrors.NotFoundError("authorization with ID %q not found", authz.ID))
}
if err != nil {
return Rollback(tx, err)
}
auth := &authzModel{authz}
pa, err := selectPendingAuthz(txWithCtx, "WHERE id = ?", authz.ID)
if err == sql.ErrNoRows {
return nil, berrors.NotFoundError("authorization with ID %q not found", authz.ID)
}
if err != nil {
return nil, err
}

err = txWithCtx.Insert(auth)
if err != nil {
return Rollback(tx, err)
}
err = txWithCtx.Insert(auth)
if err != nil {
return nil, err
}

_, err = txWithCtx.Delete(pa)
if err != nil {
return Rollback(tx, err)
}
_, err = txWithCtx.Delete(pa)
if err != nil {
return nil, err
}

err = updateChallenges(txWithCtx, authz.ID, authz.Challenges)
if err != nil {
return Rollback(tx, err)
}
err = updateChallenges(txWithCtx, authz.ID, authz.Challenges)
if err != nil {
return nil, err
}

return tx.Commit()
return nil, nil
})
return overallError
}

func (ssa *SQLStorageAuthority) getAuthorizationIDsByDomain2(ctx context.Context, domain string) ([]int64, error) {
Expand Down Expand Up @@ -757,71 +747,71 @@ func (ssa *SQLStorageAuthority) AddCertificate(
certStatus.OCSPLastUpdated = ssa.clk.Now()
}

tx, err := ssa.dbMap.Begin()
if err != nil {
return "", err
}
txWithCtx := tx.WithContext(ctx)
_, overallError := withTransaction(ctx, ssa.dbMap, func(txWithCtx transaction) (interface{}, error) {
// Note: will fail on duplicate serials. Extremely unlikely to happen and soon
// to be fixed by redesign. Reference issue
// https://github.com/letsencrypt/boulder/issues/2265 for more
err = txWithCtx.Insert(cert)
if err != nil {
if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
return nil, berrors.DuplicateError("cannot add a duplicate cert")
}
return nil, err
}

// Note: will fail on duplicate serials. Extremely unlikely to happen and soon
// to be fixed by redesign. Reference issue
// https://github.com/letsencrypt/boulder/issues/2265 for more
err = txWithCtx.Insert(cert)
if err != nil {
if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
err = berrors.DuplicateError("cannot add a duplicate cert")
err = txWithCtx.Insert(certStatus)
if err != nil {
if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
return nil, berrors.DuplicateError("cannot add a duplicate cert status")
}
return nil, err
}
return "", Rollback(tx, err)
}

err = txWithCtx.Insert(certStatus)
if err != nil {
if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
err = berrors.DuplicateError("cannot add a duplicate cert status")
// NOTE(@cpu): When we collect up names to check if an FQDN set exists (e.g.
// that it is a renewal) we use just the DNSNames from the certificate and
// ignore the Subject Common Name (if any). This is a safe assumption because
// if a certificate we issued were to have a Subj. CN not present as a SAN it
// would be a misissuance and miscalculating whether the cert is a renewal or
// not for the purpose of rate limiting is the least of our troubles.
isRenewal, err := ssa.checkFQDNSetExists(
txWithCtx.SelectOne,
parsedCertificate.DNSNames)
if err != nil {
return nil, err
}
return "", Rollback(tx, err)
}

// NOTE(@cpu): When we collect up names to check if an FQDN set exists (e.g.
// that it is a renewal) we use just the DNSNames from the certificate and
// ignore the Subject Common Name (if any). This is a safe assumption because
// if a certificate we issued were to have a Subj. CN not present as a SAN it
// would be a misissuance and miscalculating whether the cert is a renewal or
// not for the purpose of rate limiting is the least of our troubles.
isRenewal, err := ssa.checkFQDNSetExists(
txWithCtx.SelectOne,
parsedCertificate.DNSNames)
if err != nil {
return "", Rollback(tx, err)
}
err = addIssuedNames(txWithCtx, parsedCertificate, isRenewal)
if err != nil {
return nil, err
}

err = addIssuedNames(txWithCtx, parsedCertificate, isRenewal)
if err != nil {
return "", Rollback(tx, err)
}
// Add to the rate limit table, but only for new certificates. Renewals
// don't count against the certificatesPerName limit.
if !isRenewal {
timeToTheHour := parsedCertificate.NotBefore.Round(time.Hour)
err = ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour)
if err != nil {
return nil, err
}
}

// Add to the rate limit table, but only for new certificates. Renewals
// don't count against the certificatesPerName limit.
if !isRenewal {
timeToTheHour := parsedCertificate.NotBefore.Round(time.Hour)
err = ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour)
err = addFQDNSet(
txWithCtx,
parsedCertificate.DNSNames,
serial,
parsedCertificate.NotBefore,
parsedCertificate.NotAfter,
)
if err != nil {
return "", Rollback(tx, err)
return nil, err
}
}

err = addFQDNSet(
txWithCtx,
parsedCertificate.DNSNames,
serial,
parsedCertificate.NotBefore,
parsedCertificate.NotAfter,
)
if err != nil {
return "", Rollback(tx, err)
return digest, nil
})
if overallError != nil {
return "", overallError
}

return digest, tx.Commit()
return digest, nil
}

// CountPendingAuthorizations returns the number of pending, unexpired
Expand Down
Loading

0 comments on commit 16235b6

Please sign in to comment.