X Tutup
Skip to content

Commit 4ef9fb1

Browse files
aarongableaarongable
authored
Add new SA.NewOrderAndAuthzs gRPC method (letsencrypt#5602)
Add a new method to the SA's gRPC interface which takes both an Order and a list of new Authorizations to insert into the database, and adds both (as well as the various ancillary rows) inside a transaction. To enable this, add a new abstraction layer inside the `db/` package that facilitates inserting many rows at once, as we do for the `authz2`, `orderToAuthz2`, and `requestedNames` tables in this operation. Finally, add a new codepath to the RA (and a feature flag to control it) which uses this new SA method instead of separately calling the `NewAuthorization` method multiple times. Enable this feature flag in the config-next integration tests. This should reduce the failure rate of the new-order flow by reducing the number of database operations by coalescing multiple inserts into a single multi-row insert. It should also reduce the incidence of new authorizations being created in the database but then never exposed to the subscriber because of a failure later in the new-order flow, both by reducing failures overall and by adding those authorizations in a transaction which will be rolled back if there is a later failure. Fixes letsencrypt#5577
1 parent d1d04c9 commit 4ef9fb1

File tree

16 files changed

+860
-336
lines changed

16 files changed

+860
-336
lines changed

‎core/interfaces.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ type StorageAdder interface {
9292
AddSerial(ctx context.Context, req *sapb.AddSerialRequest) (*emptypb.Empty, error)
9393
DeactivateRegistration(ctx context.Context, req *sapb.RegistrationID) (*emptypb.Empty, error)
9494
NewOrder(ctx context.Context, req *sapb.NewOrderRequest) (*corepb.Order, error)
95+
NewOrderAndAuthzs(ctx context.Context, req *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error)
9596
SetOrderProcessing(ctx context.Context, req *sapb.OrderRequest) (*emptypb.Empty, error)
9697
FinalizeOrder(ctx context.Context, req *sapb.FinalizeOrderRequest) (*emptypb.Empty, error)
9798
SetOrderError(ctx context.Context, req *sapb.SetOrderErrorRequest) (*emptypb.Empty, error)

‎db/mocks.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ type Executor interface {
5959
Delete(...interface{}) (int64, error)
6060
Get(interface{}, ...interface{}) (interface{}, error)
6161
Update(...interface{}) (int64, error)
62+
Query(string, ...interface{}) (*sql.Rows, error)
6263
}
6364

6465
// Transaction extends an Executor and adds Rollback, Commit, and WithContext.

‎db/multi.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
package db
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// MultiInserter makes it easy to construct a
9+
// `INSERT INTO table (...) VALUES ... RETURNING id;`
10+
// query which inserts multiple rows into the same table. It can also execute
11+
// the resulting query.
12+
type MultiInserter struct {
13+
table string
14+
fields string
15+
retCol string
16+
numFields int
17+
values [][]interface{}
18+
}
19+
20+
// NewMultiInserter creates a new MultiInserter, checking for reasonable table
21+
// name and list of fields.
22+
func NewMultiInserter(table string, fields string, retCol string) (*MultiInserter, error) {
23+
numFields := len(strings.Split(fields, ","))
24+
if len(table) == 0 || len(fields) == 0 || numFields == 0 {
25+
return nil, fmt.Errorf("empty table name or fields list")
26+
}
27+
if strings.Contains(retCol, ",") {
28+
return nil, fmt.Errorf("return column must be singular, but got %q", retCol)
29+
}
30+
31+
return &MultiInserter{
32+
table: table,
33+
fields: fields,
34+
retCol: retCol,
35+
numFields: numFields,
36+
values: make([][]interface{}, 0),
37+
}, nil
38+
}
39+
40+
// Add registers another row to be included in the Insert query.
41+
func (mi *MultiInserter) Add(row []interface{}) error {
42+
if len(row) != mi.numFields {
43+
return fmt.Errorf("field count mismatch, got %d, expected %d", len(row), mi.numFields)
44+
}
45+
mi.values = append(mi.values, row)
46+
return nil
47+
}
48+
49+
// query returns the formatted query string, and the slice of arguments for
50+
// for gorp to use in place of the query's question marks. Currently only
51+
// used by .Insert(), below.
52+
func (mi *MultiInserter) query() (string, []interface{}) {
53+
questionsRow := strings.TrimRight(strings.Repeat("?,", mi.numFields), ",")
54+
55+
var questionsBuf strings.Builder
56+
var queryArgs []interface{}
57+
for _, row := range mi.values {
58+
fmt.Fprintf(&questionsBuf, "(%s),", questionsRow)
59+
queryArgs = append(queryArgs, row...)
60+
}
61+
62+
questions := strings.TrimRight(questionsBuf.String(), ",")
63+
64+
returning := ""
65+
if mi.retCol != "" {
66+
returning = fmt.Sprintf(" RETURNING %s", mi.retCol)
67+
}
68+
query := fmt.Sprintf("INSERT INTO %s (%s) VALUES %s%s;", mi.table, mi.fields, questions, returning)
69+
70+
return query, queryArgs
71+
}
72+
73+
// Insert performs the action represented by .query() on the provided database,
74+
// which is assumed to already have a context attached. If a non-empty retCol
75+
// was provided, then it returns the list of values from that column returned
76+
// by the query.
77+
func (mi *MultiInserter) Insert(exec Executor) ([]int64, error) {
78+
query, queryArgs := mi.query()
79+
rows, err := exec.Query(query, queryArgs...)
80+
if err != nil {
81+
return nil, err
82+
}
83+
84+
ids := make([]int64, 0, len(mi.values))
85+
if mi.retCol != "" {
86+
for rows.Next() {
87+
var id int64
88+
err = rows.Scan(&id)
89+
if err != nil {
90+
rows.Close()
91+
return nil, err
92+
}
93+
ids = append(ids, id)
94+
}
95+
}
96+
97+
err = rows.Close()
98+
if err != nil {
99+
return nil, err
100+
}
101+
102+
return ids, nil
103+
}

‎db/multi_test.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package db
2+
3+
import (
4+
"testing"
5+
6+
"github.com/letsencrypt/boulder/test"
7+
)
8+
9+
func TestNewMulti(t *testing.T) {
10+
_, err := NewMultiInserter("", "colA", "")
11+
test.AssertError(t, err, "Empty table name should fail")
12+
13+
_, err = NewMultiInserter("myTable", "", "")
14+
test.AssertError(t, err, "Empty fields string should fail")
15+
16+
mi, err := NewMultiInserter("myTable", "colA", "")
17+
test.AssertNotError(t, err, "Single-column construction should not fail")
18+
test.AssertEquals(t, mi.numFields, 1)
19+
20+
mi, err = NewMultiInserter("myTable", "colA,colB, colC", "")
21+
test.AssertNotError(t, err, "Multi-column construction should not fail")
22+
test.AssertEquals(t, mi.numFields, 3)
23+
}
24+
25+
func TestMultiAdd(t *testing.T) {
26+
mi, err := NewMultiInserter("table", "a,b,c", "")
27+
test.AssertNotError(t, err, "Failed to create test MultiInserter")
28+
29+
err = mi.Add([]interface{}{})
30+
test.AssertError(t, err, "Adding empty row should fail")
31+
32+
err = mi.Add([]interface{}{"foo"})
33+
test.AssertError(t, err, "Adding short row should fail")
34+
35+
err = mi.Add([]interface{}{"foo", "bar", "baz", "bing", "boom"})
36+
test.AssertError(t, err, "Adding long row should fail")
37+
38+
err = mi.Add([]interface{}{"one", "two", "three"})
39+
test.AssertNotError(t, err, "Adding correct-length row shouldn't fail")
40+
test.AssertEquals(t, len(mi.values), 1)
41+
42+
err = mi.Add([]interface{}{1, "two", map[string]int{"three": 3}})
43+
test.AssertNotError(t, err, "Adding heterogeneous row shouldn't fail")
44+
test.AssertEquals(t, len(mi.values), 2)
45+
// Note that .Add does *not* enforce that each row is of the same types.
46+
}
47+
48+
func TestMultiQuery(t *testing.T) {
49+
mi, err := NewMultiInserter("table", "a,b,c", "")
50+
test.AssertNotError(t, err, "Failed to create test MultiInserter")
51+
err = mi.Add([]interface{}{"one", "two", "three"})
52+
test.AssertNotError(t, err, "Failed to insert test row")
53+
err = mi.Add([]interface{}{"egy", "kettö", "három"})
54+
test.AssertNotError(t, err, "Failed to insert test row")
55+
56+
query, queryArgs := mi.query()
57+
test.AssertEquals(t, query, "INSERT INTO table (a,b,c) VALUES (?,?,?),(?,?,?);")
58+
test.AssertDeepEquals(t, queryArgs, []interface{}{"one", "two", "three", "egy", "kettö", "három"})
59+
60+
mi, err = NewMultiInserter("table", "a,b,c", "id")
61+
test.AssertNotError(t, err, "Failed to create test MultiInserter")
62+
err = mi.Add([]interface{}{"one", "two", "three"})
63+
test.AssertNotError(t, err, "Failed to insert test row")
64+
err = mi.Add([]interface{}{"egy", "kettö", "három"})
65+
test.AssertNotError(t, err, "Failed to insert test row")
66+
67+
query, queryArgs = mi.query()
68+
test.AssertEquals(t, query, "INSERT INTO table (a,b,c) VALUES (?,?,?),(?,?,?) RETURNING id;")
69+
test.AssertDeepEquals(t, queryArgs, []interface{}{"one", "two", "three", "egy", "kettö", "három"})
70+
}

‎features/featureflag_string.go‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎features/features.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ const (
4848
// ECDSAForAll enables all accounts, regardless of their presence in the CA's
4949
// ecdsaAllowedAccounts config value, to get issuance from ECDSA issuers.
5050
ECDSAForAll
51+
// StreamlineOrderAndAuthzs enables the use of a new SA gRPC method that
52+
// combines creating new Authzs and the new Order into a single operations.
53+
StreamlineOrderAndAuthzs
5154
)
5255

5356
// List of features and their default value, protected by fMu
@@ -68,6 +71,7 @@ var features = map[FeatureFlag]bool{
6871
FasterNewOrdersRateLimit: false,
6972
NonCFSSLSigner: false,
7073
ECDSAForAll: false,
74+
StreamlineOrderAndAuthzs: false,
7175
}
7276

7377
var fMu = new(sync.RWMutex)

‎grpc/sa-wrappers.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ func (sas StorageAuthorityClientWrapper) NewOrder(ctx context.Context, request *
101101
return sas.inner.NewOrder(ctx, request)
102102
}
103103

104+
func (sas StorageAuthorityClientWrapper) NewOrderAndAuthzs(ctx context.Context, request *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error) {
105+
return sas.inner.NewOrderAndAuthzs(ctx, request)
106+
}
107+
104108
func (sac StorageAuthorityClientWrapper) SetOrderProcessing(ctx context.Context, req *sapb.OrderRequest) (*emptypb.Empty, error) {
105109
return sac.inner.SetOrderProcessing(ctx, req)
106110
}
@@ -261,6 +265,10 @@ func (sas StorageAuthorityServerWrapper) NewOrder(ctx context.Context, request *
261265
return sas.inner.NewOrder(ctx, request)
262266
}
263267

268+
func (sas StorageAuthorityServerWrapper) NewOrderAndAuthzs(ctx context.Context, request *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error) {
269+
return sas.inner.NewOrderAndAuthzs(ctx, request)
270+
}
271+
264272
func (sas StorageAuthorityServerWrapper) SetOrderProcessing(ctx context.Context, req *sapb.OrderRequest) (*emptypb.Empty, error) {
265273
return sas.inner.SetOrderProcessing(ctx, req)
266274
}

‎mocks/mocks.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,11 @@ func (sa *StorageAuthority) NewOrder(_ context.Context, req *sapb.NewOrderReques
421421
return response, nil
422422
}
423423

424+
// NewOrderAndAuthzs is a mock
425+
func (sa *StorageAuthority) NewOrderAndAuthzs(_ context.Context, req *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error) {
426+
return sa.NewOrder(context.TODO(), req.NewOrder)
427+
}
428+
424429
// SetOrderProcessing is a mock
425430
func (sa *StorageAuthority) SetOrderProcessing(_ context.Context, req *sapb.OrderRequest) (*emptypb.Empty, error) {
426431
return &emptypb.Empty{}, nil

0 commit comments

Comments
 (0)
X Tutup