Deprecate NoPendingAuthzReuse flag (#8458)

Author: jsha

features/features.go

@@ -28,6 +28,7 @@ type Config struct {
 	ExpirationMailerUsesJoin    bool
 	DOH                         bool
 	IgnoreAccountContacts       bool
+	NoPendingAuthzReuse         bool
 
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
@@ -71,12 +72,6 @@ type Config struct {
 	// fails validation.
 	AutomaticallyPauseZombieClients bool
 
-	// NoPendingAuthzReuse causes the RA to only select already-validated authzs
-	// to attach to a newly created order. This preserves important client-facing
-	// functionality (valid authz reuse) while letting us simplify our code by
-	// removing pending authz reuse.
-	NoPendingAuthzReuse bool
-
 	// StoreARIReplacesInOrders causes the SA to store and retrieve the optional
 	// ARI replaces field in the orders table.
 	StoreARIReplacesInOrders bool

mocks/sa.go

@@ -466,10 +466,6 @@ func (sa *StorageAuthorityReadOnly) GetValidAuthorizations2(ctx context.Context,
 	return auths, nil
 }
 
-func (sa *StorageAuthorityReadOnly) GetAuthorizations2(ctx context.Context, req *sapb.GetAuthorizationsRequest, _ ...grpc.CallOption) (*sapb.Authorizations, error) {
-	return &sapb.Authorizations{}, nil
-}
-
 // GetAuthorization2 is a mock
 func (sa *StorageAuthorityReadOnly) GetAuthorization2(ctx context.Context, id *sapb.AuthorizationID2, _ ...grpc.CallOption) (*corepb.Authorization, error) {
 	return &corepb.Authorization{}, nil

ra/ra.go

@@ -2211,24 +2211,12 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 	}
 	authzExpiryCutoff := ra.clk.Now().Add(minTimeToExpiry)
 
-	var existingAuthz *sapb.Authorizations
-	if features.Get().NoPendingAuthzReuse {
-		getAuthReq := &sapb.GetValidAuthorizationsRequest{
-			RegistrationID: req.RegistrationID,
-			ValidUntil:     timestamppb.New(authzExpiryCutoff),
-			Identifiers:    idents.ToProtoSlice(),
-			Profile:        req.CertificateProfileName,
-		}
-		existingAuthz, err = ra.SA.GetValidAuthorizations2(ctx, getAuthReq)
-	} else {
-		getAuthReq := &sapb.GetAuthorizationsRequest{
-			RegistrationID: req.RegistrationID,
-			ValidUntil:     timestamppb.New(authzExpiryCutoff),
-			Identifiers:    idents.ToProtoSlice(),
-			Profile:        req.CertificateProfileName,
-		}
-		existingAuthz, err = ra.SA.GetAuthorizations2(ctx, getAuthReq)
-	}
+	existingAuthz, err := ra.SA.GetValidAuthorizations2(ctx, &sapb.GetValidAuthorizationsRequest{
+		RegistrationID: req.RegistrationID,
+		ValidUntil:     timestamppb.New(authzExpiryCutoff),
+		Identifiers:    idents.ToProtoSlice(),
+		Profile:        req.CertificateProfileName,
+	})
 	if err != nil {
 		return nil, err
 	}

ra/ra_test.go

@@ -1718,7 +1718,7 @@ func TestNewOrder_AuthzReuse(t *testing.T) {
 			Name:           "Reuse pending authz",
 			RegistrationID: Registration.Id,
 			Identifier:     identifier.NewDNS(pending),
-			ExpectReuse:    true, // TODO(#7715): Invert this.
+			ExpectReuse:    false,
 		},
 		{
 			Name:           "Reuse valid authz",
@@ -1766,40 +1766,6 @@ func TestNewOrder_AuthzReuse(t *testing.T) {
 	}
 }
 
-// TestNewOrder_AuthzReuse_NoPending tests that authz reuse doesn't reuse
-// pending authzs when a feature flag is set.
-// This is not simply a test case in TestNewOrder_OrderReuse because it relies
-// on feature-flag gated behavior. It should be unified with that function when
-// the feature flag is removed.
-func TestNewOrder_AuthzReuse_NoPending(t *testing.T) {
-	// TODO(#7715): Integrate these cases into TestNewOrder_AuthzReuse.
-	_, _, ra, _, _, cleanUp := initAuthorities(t)
-	defer cleanUp()
-
-	features.Set(features.Config{NoPendingAuthzReuse: true})
-	defer features.Reset()
-
-	// Create an initial order and two pending authzs.
-	extant, err := ra.NewOrder(context.Background(), &rapb.NewOrderRequest{
-		RegistrationID: Registration.Id,
-		Identifiers: []*corepb.Identifier{
-			identifier.NewDNS("a.com").ToProto(),
-			identifier.NewDNS("b.com").ToProto(),
-		},
-	})
-	test.AssertNotError(t, err, "creating test order")
-
-	// With the feature flag enabled, creating a new order for one of these names
-	// should not reuse the existing pending authz.
-	new, err := ra.NewOrder(context.Background(), &rapb.NewOrderRequest{
-		RegistrationID: Registration.Id,
-		Identifiers:    []*corepb.Identifier{identifier.NewDNS("a.com").ToProto()},
-	})
-	test.AssertNotError(t, err, "creating test order")
-	test.AssertNotEquals(t, new.Id, extant.Id)
-	test.AssertNotEquals(t, new.V2Authorizations[0], extant.V2Authorizations[0])
-}
-
 func TestNewOrder_ValidationProfiles(t *testing.T) {
 	_, _, ra, _, _, cleanUp := initAuthorities(t)
 	defer cleanUp()
@@ -2012,7 +1978,7 @@ func TestNewOrder_ProfileIdentifierTypes(t *testing.T) {
 	}
 }
 
-// mockSAWithAuthzs has a GetAuthorizations2 method that returns the protobuf
+// mockSAWithAuthzs has a GetValidAuthorizations2 method that returns the protobuf
 // version of its authzs struct member. It also has a fake GetOrderForNames
 // which always fails, and a fake NewOrderAndAuthzs which always succeeds, to
 // facilitate the full execution of RA.NewOrder.
@@ -2043,14 +2009,6 @@ func (msa *mockSAWithAuthzs) GetValidAuthorizations2(ctx context.Context, req *s
 	return resp, nil
 }
 
-func (msa *mockSAWithAuthzs) GetAuthorizations2(ctx context.Context, req *sapb.GetAuthorizationsRequest, _ ...grpc.CallOption) (*sapb.Authorizations, error) {
-	return msa.GetValidAuthorizations2(ctx, &sapb.GetValidAuthorizationsRequest{
-		RegistrationID: req.RegistrationID,
-		Identifiers:    req.Identifiers,
-		ValidUntil:     req.ValidUntil,
-	})
-}
-
 func (msa *mockSAWithAuthzs) GetAuthorization2(ctx context.Context, req *sapb.AuthorizationID2, _ ...grpc.CallOption) (*corepb.Authorization, error) {
 	for _, authz := range msa.authzs {
 		if authz.ID == fmt.Sprintf("%d", req.Id) {