Skip to content

Commit ff31292

Browse files
jshaRoland Bracewell Shoemaker
jsha
authored and
Roland Bracewell Shoemaker
committed
Put features.Reset in unitest setup functions. (letsencrypt#4129)
Previously we relied on each instance of `features.Set` to have a corresponding `defer features.Reset()`. If we forget that, we can wind up with unexpected behavior where features set in one test case leak into another test case. This led to the bug in letsencrypt#4118 going undetected. Fix letsencrypt#4120
1 parent bc200cb commit ff31292

File tree

7 files changed

+16
-23
lines changed

7 files changed

+16
-23
lines changed

‎cmd/ocsp-updater/main_test.go‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
caPB "github.com/letsencrypt/boulder/ca/proto"
1212
"github.com/letsencrypt/boulder/cmd"
1313
"github.com/letsencrypt/boulder/core"
14-
"github.com/letsencrypt/boulder/features"
1514
blog "github.com/letsencrypt/boulder/log"
1615
"github.com/letsencrypt/boulder/metrics"
1716
"github.com/letsencrypt/boulder/revocation"
@@ -312,12 +311,6 @@ func TestOldOCSPResponsesTick(t *testing.T) {
312311
// that are expired but whose certificate status rows do not have `IsExpired`
313312
// set.
314313
func TestOldOCSPResponsesTickIsExpired(t *testing.T) {
315-
// Explicitly enable the CertStatusOptimizationsMigrated feature so the OCSP
316-
// updater can use the `IsExpired` field. This must be done before `setup()`
317-
// so the correct dbMap associations are used
318-
_ = features.Set(map[string]bool{"CertStatusOptimizationsMigrated": true})
319-
defer features.Reset()
320-
321314
updater, sa, dbMap, fc, cleanUp := setup(t)
322315
defer cleanUp()
323316

‎ra/ra_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,8 @@ func (r *dummyRateLimitConfig) LoadPolicies(contents []byte) error {
218218
}
219219

220220
func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAuthority, *RegistrationAuthorityImpl, clock.FakeClock, func()) {
221+
features.Reset()
222+
221223
err := json.Unmarshal(AccountKeyJSONA, &AccountKeyA)
222224
test.AssertNotError(t, err, "Failed to unmarshal public JWK")
223225
err = json.Unmarshal(AccountKeyJSONB, &AccountKeyB)
@@ -1207,7 +1209,6 @@ func TestEarlyOrderRateLimiting(t *testing.T) {
12071209
// Start with the feature flag enabled.
12081210
err := features.Set(map[string]bool{"EarlyOrderRateLimit": true})
12091211
test.AssertNotError(t, err, "Failed to set EarlyOrderRateLimit feature flag")
1210-
defer features.Reset()
12111212

12121213
// Request an order for the test domain
12131214
newOrder := &rapb.NewOrderRequest{

‎sa/sa_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ var ctx = context.Background()
4141
// initSA constructs a SQLStorageAuthority and a clean up function
4242
// that should be defer'ed to the end of the test.
4343
func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {
44+
features.Reset()
45+
4446
dbMap, err := NewDbMap(vars.DBConnSA, 0)
4547
if err != nil {
4648
t.Fatalf("Failed to create dbMap: %s", err)
@@ -2313,7 +2315,6 @@ func TestAddCertificateRenewalBit(t *testing.T) {
23132315

23142316
err := features.Set(map[string]bool{"SetIssuedNamesRenewalBit": true})
23152317
test.AssertNotError(t, err, "Failed to enable SetIssuedNamesRenewalBit feature flag")
2316-
defer features.Reset()
23172318

23182319
reg := satest.CreateWorkingRegistration(t, sa)
23192320

@@ -2386,7 +2387,6 @@ func TestCountCertificatesRenewalBit(t *testing.T) {
23862387
"AllowRenewalFirstRL": true,
23872388
})
23882389
test.AssertNotError(t, err, "Failed to enable required features flag")
2389-
defer features.Reset()
23902390

23912391
// Create a test registration
23922392
reg := satest.CreateWorkingRegistration(t, sa)

‎va/caa_test.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -199,11 +199,6 @@ func TestCAATimeout(t *testing.T) {
199199
}
200200

201201
func TestCAAChecking(t *testing.T) {
202-
if err := features.Set(map[string]bool{"CAAValidationMethods": true, "CAAAccountURI": true}); err != nil {
203-
t.Fatalf("Failed to enable feature: %v", err)
204-
}
205-
defer features.Reset()
206-
207202
testCases := []struct {
208203
Name string
209204
Domain string
@@ -403,6 +398,10 @@ func TestCAAChecking(t *testing.T) {
403398
params := &caaParams{accountURIID: &accountURIID, validationMethod: &method}
404399

405400
va, _ := setup(nil, 0, "", nil)
401+
if err := features.Set(map[string]bool{"CAAValidationMethods": true, "CAAAccountURI": true}); err != nil {
402+
t.Fatalf("Failed to enable feature: %v", err)
403+
}
404+
406405
va.dnsClient = caaMockDNS{}
407406
va.accountURIPrefixes = []string{"https://letsencrypt.org/acct/reg/"}
408407
for _, caaTest := range testCases {

‎va/va_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1195,6 +1195,8 @@ func setup(
11951195
maxRemoteFailures int,
11961196
userAgent string,
11971197
remoteVAs []RemoteVA) (*ValidationAuthorityImpl, *blog.Mock) {
1198+
features.Reset()
1199+
11981200
logger := blog.NewMock()
11991201

12001202
if userAgent == "" {

‎wfe/wfe_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,8 @@ var testKeyPolicy = goodkey.KeyPolicy{
374374
var ctx = context.Background()
375375

376376
func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock) {
377+
features.Reset()
378+
377379
fc := clock.NewFake()
378380
stats := metrics.NewNoopScope()
379381

@@ -1748,7 +1750,6 @@ func TestAuthorization(t *testing.T) {
17481750
mux := wfe.Handler()
17491751

17501752
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
1751-
defer features.Reset()
17521753

17531754
responseWriter := httptest.NewRecorder()
17541755

@@ -2558,7 +2559,6 @@ func TestPrepChallengeForDisplay(t *testing.T) {
25582559
test.AssertEquals(t, chall.URI, "http://example.com/acme/challenge/eyup/0")
25592560

25602561
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
2561-
defer features.Reset()
25622562
authz.V2 = true
25632563
wfe.prepChallengeForDisplay(req, authz, chall)
25642564
test.AssertEquals(t, chall.URI, "http://example.com/acme/challenge/v2/eyup/iFVMwA==")
@@ -2630,9 +2630,8 @@ func TestNewRegistrationGetKeyBroken(t *testing.T) {
26302630
}
26312631

26322632
func TestChallengeNewIDScheme(t *testing.T) {
2633-
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
2634-
defer features.Reset()
26352633
wfe, _ := setupWFE(t)
2634+
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
26362635

26372636
for _, tc := range []struct {
26382637
path string

‎wfe2/wfe_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,8 @@ var testKeyPolicy = goodkey.KeyPolicy{
342342
var ctx = context.Background()
343343

344344
func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock) {
345+
features.Reset()
346+
345347
fc := clock.NewFake()
346348
stats := metrics.NewNoopScope()
347349

@@ -1417,7 +1419,6 @@ func TestGetAuthorization(t *testing.T) {
14171419
wfe, _ := setupWFE(t)
14181420

14191421
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
1420-
defer features.Reset()
14211422

14221423
// Expired authorizations should be inaccessible
14231424
authzURL := "expired"
@@ -2842,7 +2843,6 @@ func TestPrepAuthzForDisplay(t *testing.T) {
28422843
test.AssertEquals(t, chal.ProvidedKeyAuthorization, "")
28432844

28442845
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
2845-
defer features.Reset()
28462846
authz.ID = "12345"
28472847
authz.V2 = true
28482848
wfe.prepAuthorizationForDisplay(&http.Request{Host: "localhost"}, authz)
@@ -2891,9 +2891,8 @@ func TestFinalizeSCTError(t *testing.T) {
28912891
}
28922892

28932893
func TestChallengeNewIDScheme(t *testing.T) {
2894-
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
2895-
defer features.Reset()
28962894
wfe, _ := setupWFE(t)
2895+
_ = features.Set(map[string]bool{"NewAuthorizationSchema": true})
28972896

28982897
for _, tc := range []struct {
28992898
path string

0 commit comments

Comments
 (0)