SA.GetOrder: conduct queries inside a transaction (#6541)

The SA's GetOrder method issues four separate database queries:
- get the Order object itself
- get the list of Names associated with it
- get the list of Authorization IDs associated with it
- get the contents of those Authorizations themselves

These four queries can hit different database replicas with different
amounts of replication lag, and therefore different views of the
universe. This can result in inconsistent results, such as the Order
existing, but not having any Authorization IDs associated with it.

Conduct these four queries all within a single transaction, to ensure
they all hit a single read-replica with a consistent view of the world.

Fixes #6540

Author: aarongable

sa/model.go

@@ -960,3 +960,32 @@ func getAuthorizationStatuses(s db.Selector, ids []int64) ([]authzValidity, erro
 	}
 	return allAuthzValidity, nil
 }
+
+// authzForOrder retrieves the authorization IDs for an order. It assumes that
+// the provided database selector already has a context associated with it.
+func authzForOrder(s db.Selector, orderID int64) ([]int64, error) {
+	var v2IDs []int64
+	_, err := s.Select(
+		&v2IDs,
+		"SELECT authzID FROM orderToAuthz2 WHERE orderID = ?",
+		orderID,
+	)
+	return v2IDs, err
+}
+
+// namesForOrder finds all of the requested names associated with an order. The
+// names are returned in their reversed form (see `sa.ReverseName`). It assumes
+// that the provided database selector already has a context associated with it.
+func namesForOrder(s db.Selector, orderID int64) ([]string, error) {
+	var reversedNames []string
+	_, err := s.Select(
+		&reversedNames,
+		`SELECT reversedName
+	   FROM requestedNames
+	   WHERE orderID = ?`,
+		orderID)
+	if err != nil {
+		return nil, err
+	}
+	return reversedNames, nil
+}

sa/sa_test.go

@@ -986,7 +986,7 @@ func TestNewOrderAndAuthzs(t *testing.T) {
 	test.AssertEquals(t, len(authzIDs), 4)
 	test.AssertDeepEquals(t, authzIDs, []int64{1, 2, 3, 4})
 
-	names, err := sa.namesForOrder(context.Background(), order.Id)
+	names, err := namesForOrder(sa.dbReadOnlyMap, order.Id)
 	test.AssertNotError(t, err, "namesForOrder errored")
 	test.AssertEquals(t, len(names), 4)
 	test.AssertDeepEquals(t, names, []string{"com.a", "com.b", "com.c", "com.d"})

sa/saro.go

@@ -573,83 +573,70 @@ func (ssa *SQLStorageAuthority) PreviousCertificateExists(ctx context.Context, r
 	return ssa.SQLStorageAuthorityRO.PreviousCertificateExists(ctx, req)
 }
 
-// authzForOrder retrieves the authorization IDs for an order.
-func (ssa *SQLStorageAuthorityRO) authzForOrder(ctx context.Context, orderID int64) ([]int64, error) {
-	var v2IDs []int64
-	_, err := ssa.dbReadOnlyMap.WithContext(ctx).Select(
-		&v2IDs,
-		"SELECT authzID FROM orderToAuthz2 WHERE orderID = ?",
-		orderID,
-	)
-	return v2IDs, err
-}
-
-// namesForOrder finds all of the requested names associated with an order. The
-// names are returned in their reversed form (see `sa.ReverseName`).
-func (ssa *SQLStorageAuthorityRO) namesForOrder(ctx context.Context, orderID int64) ([]string, error) {
-	var reversedNames []string
-	_, err := ssa.dbReadOnlyMap.WithContext(ctx).Select(
-		&reversedNames,
-		`SELECT reversedName
-	   FROM requestedNames
-	   WHERE orderID = ?`,
-		orderID)
-	if err != nil {
-		return nil, err
-	}
-	return reversedNames, nil
-}
-
 // GetOrder is used to retrieve an already existing order object
 func (ssa *SQLStorageAuthorityRO) GetOrder(ctx context.Context, req *sapb.OrderRequest) (*corepb.Order, error) {
 	if req == nil || req.Id == 0 {
 		return nil, errIncompleteRequest
 	}
 
-	omObj, err := ssa.dbReadOnlyMap.WithContext(ctx).Get(orderModel{}, req.Id)
-	if err != nil {
-		if db.IsNoRows(err) {
+	output, err := db.WithTransaction(ctx, ssa.dbReadOnlyMap, func(txWithCtx db.Executor) (interface{}, error) {
+		omObj, err := txWithCtx.Get(orderModel{}, req.Id)
+		if err != nil {
+			if db.IsNoRows(err) {
+				return nil, berrors.NotFoundError("no order found for ID %d", req.Id)
+			}
+			return nil, err
+		}
+		if omObj == nil {
 			return nil, berrors.NotFoundError("no order found for ID %d", req.Id)
 		}
-		return nil, err
-	}
-	if omObj == nil {
-		return nil, berrors.NotFoundError("no order found for ID %d", req.Id)
-	}
-	order, err := modelToOrder(omObj.(*orderModel))
-	if err != nil {
-		return nil, err
-	}
-	orderExp := time.Unix(0, order.Expires)
-	if orderExp.Before(ssa.clk.Now()) {
-		return nil, berrors.NotFoundError("no order found for ID %d", req.Id)
-	}
 
-	v2AuthzIDs, err := ssa.authzForOrder(ctx, order.Id)
-	if err != nil {
-		return nil, err
-	}
-	order.V2Authorizations = v2AuthzIDs
+		order, err := modelToOrder(omObj.(*orderModel))
+		if err != nil {
+			return nil, err
+		}
+
+		orderExp := time.Unix(0, order.Expires)
+		if orderExp.Before(ssa.clk.Now()) {
+			return nil, berrors.NotFoundError("no order found for ID %d", req.Id)
+		}
+
+		v2AuthzIDs, err := authzForOrder(txWithCtx, order.Id)
+		if err != nil {
+			return nil, err
+		}
+		order.V2Authorizations = v2AuthzIDs
 
-	names, err := ssa.namesForOrder(ctx, order.Id)
+		names, err := namesForOrder(txWithCtx, order.Id)
+		if err != nil {
+			return nil, err
+		}
+		// The requested names are stored reversed to improve indexing performance. We
+		// need to reverse the reversed names here before giving them back to the
+		// caller.
+		reversedNames := make([]string, len(names))
+		for i, n := range names {
+			reversedNames[i] = ReverseName(n)
+		}
+		order.Names = reversedNames
+
+		// Calculate the status for the order
+		status, err := statusForOrder(txWithCtx, order, ssa.clk.Now())
+		if err != nil {
+			return nil, err
+		}
+		order.Status = status
+
+		return order, nil
+	})
 	if err != nil {
 		return nil, err
 	}
-	// The requested names are stored reversed to improve indexing performance. We
-	// need to reverse the reversed names here before giving them back to the
-	// caller.
-	reversedNames := make([]string, len(names))
-	for i, n := range names {
-		reversedNames[i] = ReverseName(n)
-	}
-	order.Names = reversedNames
 
-	// Calculate the status for the order
-	status, err := statusForOrder(ssa.dbReadOnlyMap.WithContext(ctx), order, ssa.clk.Now())
-	if err != nil {
-		return nil, err
+	order, ok := output.(*corepb.Order)
+	if !ok {
+		return nil, fmt.Errorf("casting error in GetOrder")
 	}
-	order.Status = status
 
 	return order, nil
 }