Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add feature flag to remove use of "INSERT RETURNING" in NewOrderAndAuthzs #7739

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Add feature flag to remove use of "INSERT RETURNING" in NewOrderAndAuthzs #7739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94e5556
Add feature flag to remove use of 'INSERT ... RETURNING'
aarongable Oct 2, 2024
b8e7d8f
Add benchmark
aarongable Oct 2, 2024
0442894
Update existing unit tests
aarongable Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions features/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ type Config struct {
//
// This flag should only be used in conjunction with UseKvLimitsForNewOrder.
DisableLegacyLimitWrites bool

// InsertAuthzsIndividually causes the SA's NewOrderAndAuthzs method to
// create each new authz one at a time, rather than using MultiInserter.
// Although this is expected to be a performance penalty, it is necessary to
// get the AUTO_INCREMENT ID of each new authz without relying on MariaDB's
// unique "INSERT ... RETURNING" functionality.
InsertAuthzsIndividually bool
}

var fMu = new(sync.RWMutex)
Expand Down
58 changes: 36 additions & 22 deletions sa/sa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,37 +474,51 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
output, err := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
// First, insert all of the new authorizations and record their IDs.
newAuthzIDs := make([]int64, 0)
if len(req.NewAuthzs) != 0 {
inserter, err := db.NewMultiInserter("authz2", strings.Split(authzFields, ", "), "id")
if err != nil {
return nil, err
}
if features.Get().InsertAuthzsIndividually {
for _, authz := range req.NewAuthzs {
am, err := newAuthzReqToModel(authz)
if err != nil {
return nil, err
}
err = inserter.Add([]interface{}{
am.ID,
am.IdentifierType,
am.IdentifierValue,
am.RegistrationID,
statusToUint[core.StatusPending],
am.Expires,
am.Challenges,
nil,
nil,
am.Token,
nil,
nil,
})
err = tx.Insert(ctx, am)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference between this codepath and the current codepath is that the MultiInserter code below hardcodes nil for four fields: Attempted, AttemptedAt, ValidationRecord, and ValidationError. These fields should never be set in a NewOrderAndAuthzs request from the RA. Is it worth adding some extra lines of code to this new codepath to retain the same defense-in-depth as the old codepath has? Or are we happy abandoning that teensy bit of extra safety?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like either way is OK; defense in depth is great and I wouldn't say no to it, but this bad case feels impossible.

if err != nil {
return nil, err
}
newAuthzIDs = append(newAuthzIDs, am.ID)
}
newAuthzIDs, err = inserter.Insert(ctx, tx)
if err != nil {
return nil, err
} else {
if len(req.NewAuthzs) != 0 {
inserter, err := db.NewMultiInserter("authz2", strings.Split(authzFields, ", "), "id")
if err != nil {
return nil, err
}
for _, authz := range req.NewAuthzs {
am, err := newAuthzReqToModel(authz)
if err != nil {
return nil, err
}
err = inserter.Add([]interface{}{
am.ID,
am.IdentifierType,
am.IdentifierValue,
am.RegistrationID,
statusToUint[core.StatusPending],
am.Expires,
am.Challenges,
nil,
nil,
am.Token,
nil,
nil,
})
if err != nil {
return nil, err
}
}
newAuthzIDs, err = inserter.Insert(ctx, tx)
if err != nil {
return nil, err
}
}
}

Expand Down
66 changes: 64 additions & 2 deletions sa/sa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"crypto/x509"
"database/sql"
"encoding/base64"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -77,7 +78,7 @@ func (s *fakeServerStream[T]) Context() context.Context {

// initSA constructs a SQLStorageAuthority and a clean up function that should
// be defer'ed to the end of the test.
func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {
func initSA(t testing.TB) (*SQLStorageAuthority, clock.FakeClock, func()) {
t.Helper()
features.Reset()

Expand Down Expand Up @@ -109,7 +110,7 @@ func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {

// CreateWorkingTestRegistration inserts a new, correct Registration into the
// given SA.
func createWorkingRegistration(t *testing.T, sa *SQLStorageAuthority) *corepb.Registration {
func createWorkingRegistration(t testing.TB, sa *SQLStorageAuthority) *corepb.Registration {
initialIP, _ := net.ParseIP("88.77.66.11").MarshalText()
reg, err := sa.NewRegistration(context.Background(), &corepb.Registration{
Key: []byte(theKey),
Expand Down Expand Up @@ -1231,6 +1232,9 @@ func TestNewOrderAndAuthzs(t *testing.T) {
sa, _, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)

// Insert two pre-existing authorizations to reference
Expand Down Expand Up @@ -1285,6 +1289,9 @@ func TestNewOrderAndAuthzs_NonNilInnerOrder(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)

expires := fc.Now().Add(2 * time.Hour)
Expand All @@ -1306,6 +1313,9 @@ func TestNewOrderAndAuthzs_MismatchedRegID(t *testing.T) {
sa, _, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

_, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: 1,
Expand All @@ -1324,6 +1334,9 @@ func TestNewOrderAndAuthzs_NewAuthzExpectedFields(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()

reg := createWorkingRegistration(t, sa)
expires := fc.Now().Add(time.Hour)
domain := "a.com"
Expand Down Expand Up @@ -1372,6 +1385,55 @@ func TestNewOrderAndAuthzs_NewAuthzExpectedFields(t *testing.T) {
test.AssertBoxedNil(t, am.ValidationRecord, "am.ValidationRecord should be nil")
}

func BenchmarkNewOrderAndAuthzs(b *testing.B) {
for _, flag := range []bool{false, true} {
for _, numIdents := range []int{1, 2, 5, 10, 20, 50, 100} {
b.Run(fmt.Sprintf("%t/%d", flag, numIdents), func(b *testing.B) {
sa, _, cleanup := initSA(b)
defer cleanup()

if flag {
features.Set(features.Config{InsertAuthzsIndividually: true})
defer features.Reset()
}

reg := createWorkingRegistration(b, sa)

dnsNames := make([]string, 0, numIdents)
newAuthzs := make([]*sapb.NewAuthzRequest, 0, numIdents)
for range numIdents {
var nameBytes [3]byte
_, _ = rand.Read(nameBytes[:])
name := fmt.Sprintf("%s.example.com", hex.EncodeToString(nameBytes[:]))

dnsNames = append(dnsNames, name)
newAuthzs = append(newAuthzs, &sapb.NewAuthzRequest{
RegistrationID: reg.Id,
Identifier: identifier.NewDNS(name).AsProto(),
ChallengeTypes: []string{string(core.ChallengeTypeDNS01)},
Token: core.NewToken(),
Expires: timestamppb.New(sa.clk.Now().Add(24 * time.Hour)),
})
}

b.ResetTimer()

_, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: reg.Id,
Expires: timestamppb.New(sa.clk.Now().Add(24 * time.Hour)),
DnsNames: dnsNames,
},
NewAuthzs: newAuthzs,
})
if err != nil {
b.Error(err)
}
})
}
}
}

func TestSetOrderProcessing(t *testing.T) {
sa, fc, cleanup := initSA(t)
defer cleanup()
Expand Down
3 changes: 2 additions & 1 deletion test/config-next/sa.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"features": {
"MultipleCertificateProfiles": true,
"TrackReplacementCertificatesARI": true,
"DisableLegacyLimitWrites": true
"DisableLegacyLimitWrites": true,
"InsertAuthzsIndividually": true
}
},
"syslog": {
Expand Down
Loading