From f402c743671563570293991e2e38fb3fb2351a68 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 21 Nov 2024 17:48:59 -0800 Subject: [PATCH] Clean up MultipleCertificateProfiles feature flag --- features/features.go | 11 +-- sa/database.go | 7 +- .../20240304000000_CertificateProfiles.sql | 10 +-- .../20240304000000_CertificateProfiles.sql | 9 +++ sa/model.go | 67 +---------------- sa/model_test.go | 23 +----- sa/sa.go | 38 +++------- sa/sa_test.go | 73 +------------------ sa/saro.go | 13 +--- test/config-next/sa.json | 1 - test/config/sa.json | 3 + 11 files changed, 41 insertions(+), 214 deletions(-) mode change 100644 => 120000 sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql create mode 100644 sa/db/boulder_sa/20240304000000_CertificateProfiles.sql diff --git a/features/features.go b/features/features.go index 36ca9db76e1..682dd03f2b1 100644 --- a/features/features.go +++ b/features/features.go @@ -15,6 +15,9 @@ import ( // then call features.Set(parsedConfig) to load the parsed struct into this // package's global Config. type Config struct { + // Deprecated features. Delete once removed from all production configs. + MultipleCertificateProfiles bool + // ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for // GET requests. WARNING: This feature is a draft and highly unstable. ServeRenewalInfo bool @@ -57,14 +60,6 @@ type Config struct { // Only used when EnforceMultiCAA is true. MultiCAAFullResults bool - // MultipleCertificateProfiles, when enabled, triggers the following - // behavior: - // - SA.NewOrderAndAuthzs: upon receiving a NewOrderRequest with a - // `certificateProfileName` value, will add that value to the database's - // `orders.certificateProfileName` column. Values in this column are - // allowed to be empty. - MultipleCertificateProfiles bool - // CheckIdentifiersPaused checks if any of the identifiers in the order are // currently paused at NewOrder time. If any are paused, an error is // returned to the Subscriber indicating that the order cannot be processed diff --git a/sa/database.go b/sa/database.go index 9d83875802e..5216f169aab 100644 --- a/sa/database.go +++ b/sa/database.go @@ -13,7 +13,6 @@ import ( "github.com/letsencrypt/boulder/cmd" "github.com/letsencrypt/boulder/core" boulderDB "github.com/letsencrypt/boulder/db" - "github.com/letsencrypt/boulder/features" blog "github.com/letsencrypt/boulder/log" ) @@ -273,11 +272,7 @@ func initTables(dbMap *borp.DbMap) { dbMap.AddTableWithName(core.Certificate{}, "certificates").SetKeys(true, "ID") dbMap.AddTableWithName(core.CertificateStatus{}, "certificateStatus").SetKeys(true, "ID") dbMap.AddTableWithName(core.FQDNSet{}, "fqdnSets").SetKeys(true, "ID") - if features.Get().MultipleCertificateProfiles { - dbMap.AddTableWithName(orderModelv2{}, "orders").SetKeys(true, "ID") - } else { - dbMap.AddTableWithName(orderModelv1{}, "orders").SetKeys(true, "ID") - } + dbMap.AddTableWithName(orderModel{}, "orders").SetKeys(true, "ID") dbMap.AddTableWithName(orderToAuthzModel{}, "orderToAuthz").SetKeys(false, "OrderID", "AuthzID") dbMap.AddTableWithName(orderFQDNSet{}, "orderFqdnSets").SetKeys(true, "ID") dbMap.AddTableWithName(authzModel{}, "authz2").SetKeys(true, "ID") diff --git a/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql b/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql deleted file mode 100644 index 583a106d6b8..00000000000 --- a/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql +++ /dev/null @@ -1,9 +0,0 @@ --- +migrate Up --- SQL in section 'Up' is executed when this migration is applied - -ALTER TABLE `orders` ADD COLUMN `certificateProfileName` varchar(32) DEFAULT NULL; - --- +migrate Down --- SQL section 'Down' is executed when this migration is rolled back - -ALTER TABLE `orders` DROP COLUMN `certificateProfileName`; diff --git a/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql b/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql new file mode 120000 index 00000000000..f837842abdd --- /dev/null +++ b/sa/db-next/boulder_sa/20240304000000_CertificateProfiles.sql @@ -0,0 +1 @@ +../../db/boulder_sa/20240304000000_CertificateProfiles.sql \ No newline at end of file diff --git a/sa/db/boulder_sa/20240304000000_CertificateProfiles.sql b/sa/db/boulder_sa/20240304000000_CertificateProfiles.sql new file mode 100644 index 00000000000..583a106d6b8 --- /dev/null +++ b/sa/db/boulder_sa/20240304000000_CertificateProfiles.sql @@ -0,0 +1,9 @@ +-- +migrate Up +-- SQL in section 'Up' is executed when this migration is applied + +ALTER TABLE `orders` ADD COLUMN `certificateProfileName` varchar(32) DEFAULT NULL; + +-- +migrate Down +-- SQL section 'Down' is executed when this migration is rolled back + +ALTER TABLE `orders` DROP COLUMN `certificateProfileName`; diff --git a/sa/model.go b/sa/model.go index 974fd6360dd..6f6a26d0970 100644 --- a/sa/model.go +++ b/sa/model.go @@ -381,18 +381,7 @@ type lintingCertModel struct { Expires time.Time } -// TODO(#7324) orderModelv1 is deprecated, use orderModelv2 moving forward. -type orderModelv1 struct { - ID int64 - RegistrationID int64 - Expires time.Time - Created time.Time - Error []byte - CertificateSerial string - BeganProcessing bool -} - -type orderModelv2 struct { +type orderModel struct { ID int64 RegistrationID int64 Expires time.Time @@ -408,56 +397,8 @@ type orderToAuthzModel struct { AuthzID int64 } -// TODO(#7324) orderToModelv1 is deprecated, use orderModelv2 moving forward. -func orderToModelv1(order *corepb.Order) (*orderModelv1, error) { - om := &orderModelv1{ - ID: order.Id, - RegistrationID: order.RegistrationID, - Expires: order.Expires.AsTime(), - Created: order.Created.AsTime(), - BeganProcessing: order.BeganProcessing, - CertificateSerial: order.CertificateSerial, - } - - if order.Error != nil { - errJSON, err := json.Marshal(order.Error) - if err != nil { - return nil, err - } - if len(errJSON) > mediumBlobSize { - return nil, fmt.Errorf("Error object is too large to store in the database") - } - om.Error = errJSON - } - return om, nil -} - -// TODO(#7324) modelToOrderv1 is deprecated, use orderModelv2 moving forward. -func modelToOrderv1(om *orderModelv1) (*corepb.Order, error) { - order := &corepb.Order{ - Id: om.ID, - RegistrationID: om.RegistrationID, - Expires: timestamppb.New(om.Expires), - Created: timestamppb.New(om.Created), - CertificateSerial: om.CertificateSerial, - BeganProcessing: om.BeganProcessing, - } - if len(om.Error) > 0 { - var problem corepb.ProblemDetails - err := json.Unmarshal(om.Error, &problem) - if err != nil { - return &corepb.Order{}, badJSONError( - "failed to unmarshal order model's error", - om.Error, - err) - } - order.Error = &problem - } - return order, nil -} - -func orderToModelv2(order *corepb.Order) (*orderModelv2, error) { - om := &orderModelv2{ +func orderToModel(order *corepb.Order) (*orderModel, error) { + om := &orderModel{ ID: order.Id, RegistrationID: order.RegistrationID, Expires: order.Expires.AsTime(), @@ -480,7 +421,7 @@ func orderToModelv2(order *corepb.Order) (*orderModelv2, error) { return om, nil } -func modelToOrderv2(om *orderModelv2) (*corepb.Order, error) { +func modelToOrder(om *orderModel) (*corepb.Order, error) { order := &corepb.Order{ Id: om.ID, RegistrationID: om.RegistrationID, diff --git a/sa/model_test.go b/sa/model_test.go index b2ba57a3766..0a4a97be398 100644 --- a/sa/model_test.go +++ b/sa/model_test.go @@ -237,7 +237,7 @@ func TestAuthzModel(t *testing.T) { // validation error JSON field to an Order produces the expected bad JSON error. func TestModelToOrderBadJSON(t *testing.T) { badJSON := []byte(`{`) - _, err := modelToOrderv2(&orderModelv2{ + _, err := modelToOrder(&orderModel{ Error: badJSON, }) test.AssertError(t, err, "expected error from modelToOrderv2") @@ -250,21 +250,6 @@ func TestOrderModelThereAndBackAgain(t *testing.T) { clk := clock.New() now := clk.Now() order := &corepb.Order{ - Id: 0, - RegistrationID: 2016, - Expires: timestamppb.New(now.Add(24 * time.Hour)), - Created: timestamppb.New(now), - Error: nil, - CertificateSerial: "1", - BeganProcessing: true, - } - model1, err := orderToModelv1(order) - test.AssertNotError(t, err, "orderToModelv1 should not have errored") - returnOrder, err := modelToOrderv1(model1) - test.AssertNotError(t, err, "modelToOrderv1 should not have errored") - test.AssertDeepEquals(t, order, returnOrder) - - anotherOrder := &corepb.Order{ Id: 1, RegistrationID: 2024, Expires: timestamppb.New(now.Add(24 * time.Hour)), @@ -274,11 +259,11 @@ func TestOrderModelThereAndBackAgain(t *testing.T) { BeganProcessing: true, CertificateProfileName: "phljny", } - model2, err := orderToModelv2(anotherOrder) + model, err := orderToModel(order) test.AssertNotError(t, err, "orderToModelv2 should not have errored") - returnOrder, err = modelToOrderv2(model2) + returnOrder, err := modelToOrder(model) test.AssertNotError(t, err, "modelToOrderv2 should not have errored") - test.AssertDeepEquals(t, anotherOrder, returnOrder) + test.AssertDeepEquals(t, order, returnOrder) } // TestPopulateAttemptedFieldsBadJSON tests that populating a challenge from an diff --git a/sa/sa.go b/sa/sa.go index 7e706149651..519a206e3ad 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -644,27 +644,13 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb } // Second, insert the new order. - var orderID int64 - var err error - created := ssa.clk.Now() - if features.Get().MultipleCertificateProfiles { - omv2 := orderModelv2{ - RegistrationID: req.NewOrder.RegistrationID, - Expires: req.NewOrder.Expires.AsTime(), - Created: created, - CertificateProfileName: req.NewOrder.CertificateProfileName, - } - err = tx.Insert(ctx, &omv2) - orderID = omv2.ID - } else { - omv1 := orderModelv1{ - RegistrationID: req.NewOrder.RegistrationID, - Expires: req.NewOrder.Expires.AsTime(), - Created: created, - } - err = tx.Insert(ctx, &omv1) - orderID = omv1.ID + om := orderModel{ + RegistrationID: req.NewOrder.RegistrationID, + Expires: req.NewOrder.Expires.AsTime(), + Created: ssa.clk.Now(), + CertificateProfileName: req.NewOrder.CertificateProfileName, } + err := tx.Insert(ctx, &om) if err != nil { return nil, err } @@ -677,7 +663,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb return nil, err } for _, id := range allAuthzIds { - err := inserter.Add([]interface{}{orderID, id}) + err := inserter.Add([]interface{}{om.ID, id}) if err != nil { return nil, err } @@ -688,7 +674,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb } // Fourth, insert the FQDNSet entry for the order. - err = addOrderFQDNSet(ctx, tx, req.NewOrder.DnsNames, orderID, req.NewOrder.RegistrationID, req.NewOrder.Expires.AsTime()) + err = addOrderFQDNSet(ctx, tx, req.NewOrder.DnsNames, om.ID, req.NewOrder.RegistrationID, req.NewOrder.Expires.AsTime()) if err != nil { return nil, err } @@ -696,7 +682,7 @@ 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, om.ID, req.NewOrder.Expires.AsTime()) if err != nil { return nil, err } @@ -712,8 +698,8 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb // Finally, build the overall Order PB. res := &corepb.Order{ // ID and Created were auto-populated on the order model when it was inserted. - Id: orderID, - Created: timestamppb.New(created), + Id: om.ID, + Created: timestamppb.New(om.Created), // These are carried over from the original request unchanged. RegistrationID: req.NewOrder.RegistrationID, Expires: req.NewOrder.Expires, @@ -797,7 +783,7 @@ func (ssa *SQLStorageAuthority) SetOrderError(ctx context.Context, req *sapb.Set return nil, errIncompleteRequest } _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) { - om, err := orderToModelv2(&corepb.Order{ + om, err := orderToModel(&corepb.Order{ Id: req.Id, Error: req.Error, }) diff --git a/sa/sa_test.go b/sa/sa_test.go index a6a39808d67..afd8fba0ddf 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -1359,72 +1359,7 @@ func TestFinalizeOrder(t *testing.T) { test.AssertEquals(t, updatedOrder.Status, string(core.StatusValid)) } -func TestOrderWithOrderModelv1(t *testing.T) { - sa, fc, cleanup := initSA(t) - defer cleanup() - - reg := createWorkingRegistration(t, sa) - authzExpires := fc.Now().Add(time.Hour) - authzID := createPendingAuthorization(t, sa, "example.com", authzExpires) - - // Set the order to expire in two hours - expires := fc.Now().Add(2 * time.Hour) - - inputOrder := &corepb.Order{ - RegistrationID: reg.Id, - Expires: timestamppb.New(expires), - DnsNames: []string{"example.com"}, - V2Authorizations: []int64{authzID}, - } - - // Create the order - order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{ - NewOrder: &sapb.NewOrderRequest{ - RegistrationID: inputOrder.RegistrationID, - Expires: inputOrder.Expires, - DnsNames: inputOrder.DnsNames, - V2Authorizations: inputOrder.V2Authorizations, - }, - }) - test.AssertNotError(t, err, "sa.NewOrderAndAuthzs failed") - - // The Order from GetOrder should match the following expected order - created := sa.clk.Now() - expectedOrder := &corepb.Order{ - // The registration ID, authorizations, expiry, and names should match the - // input to NewOrderAndAuthzs - RegistrationID: inputOrder.RegistrationID, - V2Authorizations: inputOrder.V2Authorizations, - DnsNames: inputOrder.DnsNames, - Expires: inputOrder.Expires, - // The ID should have been set to 1 by the SA - Id: 1, - // The status should be pending - Status: string(core.StatusPending), - // The serial should be empty since this is a pending order - CertificateSerial: "", - // We should not be processing it - BeganProcessing: false, - // The created timestamp should have been set to the current time - Created: timestamppb.New(created), - } - - // Fetch the order by its ID and make sure it matches the expected - storedOrder, err := sa.GetOrder(context.Background(), &sapb.OrderRequest{Id: order.Id}) - test.AssertNotError(t, err, "sa.GetOrder failed") - test.AssertDeepEquals(t, storedOrder, expectedOrder) -} - -func TestOrderWithOrderModelv2(t *testing.T) { - if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { - t.Skip() - } - - // The feature must be set before the SA is constructed because of a - // conditional on this feature in //sa/database.go. - features.Set(features.Config{MultipleCertificateProfiles: true}) - defer features.Reset() - +func TestOrder(t *testing.T) { fc := clock.NewFake() fc.Set(time.Date(2015, 3, 4, 5, 0, 0, 0, time.UTC)) @@ -1493,11 +1428,7 @@ func TestOrderWithOrderModelv2(t *testing.T) { test.AssertNotError(t, err, "sa.GetOrder failed") test.AssertDeepEquals(t, storedOrder, expectedOrder) - // - // Test that an order without a certificate profile name, but with the - // MultipleCertificateProfiles feature flag enabled works as expected. - // - + // Test that an order without a certificate profile name works as expected. inputOrderNoName := &corepb.Order{ RegistrationID: reg.Id, Expires: timestamppb.New(expires), diff --git a/sa/saro.go b/sa/saro.go index d3605322368..5a443d8817e 100644 --- a/sa/saro.go +++ b/sa/saro.go @@ -22,7 +22,6 @@ import ( corepb "github.com/letsencrypt/boulder/core/proto" "github.com/letsencrypt/boulder/db" berrors "github.com/letsencrypt/boulder/errors" - "github.com/letsencrypt/boulder/features" bgrpc "github.com/letsencrypt/boulder/grpc" "github.com/letsencrypt/boulder/identifier" blog "github.com/letsencrypt/boulder/log" @@ -523,11 +522,7 @@ func (ssa *SQLStorageAuthorityRO) GetOrder(ctx context.Context, req *sapb.OrderR txn := func(tx db.Executor) (interface{}, error) { var omObj interface{} var err error - if features.Get().MultipleCertificateProfiles { - omObj, err = tx.Get(ctx, orderModelv2{}, req.Id) - } else { - omObj, err = tx.Get(ctx, orderModelv1{}, req.Id) - } + omObj, err = tx.Get(ctx, orderModel{}, req.Id) if err != nil { if db.IsNoRows(err) { return nil, berrors.NotFoundError("no order found for ID %d", req.Id) @@ -539,11 +534,7 @@ func (ssa *SQLStorageAuthorityRO) GetOrder(ctx context.Context, req *sapb.OrderR } var order *corepb.Order - if features.Get().MultipleCertificateProfiles { - order, err = modelToOrderv2(omObj.(*orderModelv2)) - } else { - order, err = modelToOrderv1(omObj.(*orderModelv1)) - } + order, err = modelToOrder(omObj.(*orderModel)) if err != nil { return nil, err } diff --git a/test/config-next/sa.json b/test/config-next/sa.json index 04c8b1b6dcd..4951eaf16f1 100644 --- a/test/config-next/sa.json +++ b/test/config-next/sa.json @@ -49,7 +49,6 @@ }, "healthCheckInterval": "4s", "features": { - "MultipleCertificateProfiles": true, "DisableLegacyLimitWrites": true, "InsertAuthzsIndividually": true } diff --git a/test/config/sa.json b/test/config/sa.json index d8a19fea04c..73605bd3c9d 100644 --- a/test/config/sa.json +++ b/test/config/sa.json @@ -46,6 +46,9 @@ ] } } + }, + "features": { + "MultipleCertificateProfiles": true } }, "syslog": {