Skip to content

Commit 5deb7c2

Browse files
mholtmholt
committed
Make logger values required
Eliminates a bajillion nil checks and footguns (except in tests, which bypass exported APIs, but that is expected) Most recent #207 Logging can still be disabled via zap.NewNop(), if necessary. (But disabling logging in CertMagic is a really bad idea.)
1 parent 93fd493 commit 5deb7c2

18 files changed

+374
-485
lines changed

‎account_test.go

+7
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ func TestNewAccount(t *testing.T) {
3131
testConfig := &Config{
3232
Issuers: []Issuer{am},
3333
Storage: &FileStorage{Path: "./_testdata_tmp"},
34+
Logger: defaultTestLogger,
3435
certCache: new(Cache),
3536
}
3637
am.config = testConfig
@@ -58,6 +59,7 @@ func TestSaveAccount(t *testing.T) {
5859
testConfig := &Config{
5960
Issuers: []Issuer{am},
6061
Storage: &FileStorage{Path: "./_testdata1_tmp"},
62+
Logger: defaultTestLogger,
6163
certCache: new(Cache),
6264
}
6365
am.config = testConfig
@@ -93,6 +95,7 @@ func TestGetAccountDoesNotAlreadyExist(t *testing.T) {
9395
testConfig := &Config{
9496
Issuers: []Issuer{am},
9597
Storage: &FileStorage{Path: "./_testdata_tmp"},
98+
Logger: defaultTestLogger,
9699
certCache: new(Cache),
97100
}
98101
am.config = testConfig
@@ -114,6 +117,7 @@ func TestGetAccountAlreadyExists(t *testing.T) {
114117
testConfig := &Config{
115118
Issuers: []Issuer{am},
116119
Storage: &FileStorage{Path: "./_testdata2_tmp"},
120+
Logger: defaultTestLogger,
117121
certCache: new(Cache),
118122
}
119123
am.config = testConfig
@@ -168,6 +172,7 @@ func TestGetEmailFromPackageDefault(t *testing.T) {
168172
testConfig := &Config{
169173
Issuers: []Issuer{am},
170174
Storage: &FileStorage{Path: "./_testdata2_tmp"},
175+
Logger: defaultTestLogger,
171176
certCache: new(Cache),
172177
}
173178
am.config = testConfig
@@ -189,6 +194,7 @@ func TestGetEmailFromUserInput(t *testing.T) {
189194
testConfig := &Config{
190195
Issuers: []Issuer{am},
191196
Storage: &FileStorage{Path: "./_testdata3_tmp"},
197+
Logger: defaultTestLogger,
192198
certCache: new(Cache),
193199
}
194200
am.config = testConfig
@@ -223,6 +229,7 @@ func TestGetEmailFromRecent(t *testing.T) {
223229
testConfig := &Config{
224230
Issuers: []Issuer{am},
225231
Storage: &FileStorage{Path: "./_testdata4_tmp"},
232+
Logger: defaultTestLogger,
226233
certCache: new(Cache),
227234
}
228235
am.config = testConfig

‎acmeclient.go

+11-17
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,7 @@ func (iss *ACMEIssuer) newACMEClient(useTestCA bool) (*acmez.Client, error) {
175175
},
176176
ChallengeSolvers: make(map[string]acmez.Solver),
177177
}
178-
if iss.Logger != nil {
179-
client.Logger = iss.Logger.Named("acme_client")
180-
}
178+
client.Logger = iss.Logger.Named("acme_client")
181179

182180
// configure challenges (most of the time, DNS challenge is
183181
// exclusive of other ones because it is usually only used
@@ -260,24 +258,20 @@ func (c *acmeClient) throttle(ctx context.Context, names []string) error {
260258
// TODO: stop rate limiter when it is garbage-collected...
261259
}
262260
rateLimitersMu.Unlock()
263-
if c.iss.Logger != nil {
264-
c.iss.Logger.Info("waiting on internal rate limiter",
265-
zap.Strings("identifiers", names),
266-
zap.String("ca", c.acmeClient.Directory),
267-
zap.String("account", email),
268-
)
269-
}
261+
c.iss.Logger.Info("waiting on internal rate limiter",
262+
zap.Strings("identifiers", names),
263+
zap.String("ca", c.acmeClient.Directory),
264+
zap.String("account", email),
265+
)
270266
err := rl.Wait(ctx)
271267
if err != nil {
272268
return err
273269
}
274-
if c.iss.Logger != nil {
275-
c.iss.Logger.Info("done waiting on internal rate limiter",
276-
zap.Strings("identifiers", names),
277-
zap.String("ca", c.acmeClient.Directory),
278-
zap.String("account", email),
279-
)
280-
}
270+
c.iss.Logger.Info("done waiting on internal rate limiter",
271+
zap.Strings("identifiers", names),
272+
zap.String("ca", c.acmeClient.Directory),
273+
zap.String("account", email),
274+
)
281275
return nil
282276
}
283277

‎acmeissuer.go

+20-20
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,9 @@ type ACMEIssuer struct {
110110
// certificate chains
111111
PreferredChains ChainPreference
112112

113-
// Set a logger to enable logging
113+
// Set a logger to configure logging; a default
114+
// logger must always be set; if no logging is
115+
// desired, set this to zap.NewNop().
114116
Logger *zap.Logger
115117

116118
config *Config
@@ -197,6 +199,11 @@ func NewACMEIssuer(cfg *Config, template ACMEIssuer) *ACMEIssuer {
197199
template.Logger = DefaultACME.Logger
198200
}
199201

202+
// absolutely do not allow a nil logger; that would panic
203+
if template.Logger == nil {
204+
template.Logger = defaultLogger
205+
}
206+
200207
template.config = cfg
201208
template.mu = new(sync.Mutex)
202209

@@ -398,7 +405,7 @@ func (am *ACMEIssuer) doIssue(ctx context.Context, csr *x509.CertificateRequest,
398405
// processing. If there are no matches, the first chain is returned.
399406
func (am *ACMEIssuer) selectPreferredChain(certChains []acme.Certificate) acme.Certificate {
400407
if len(certChains) == 1 {
401-
if am.Logger != nil && (len(am.PreferredChains.AnyCommonName) > 0 || len(am.PreferredChains.RootCommonName) > 0) {
408+
if len(am.PreferredChains.AnyCommonName) > 0 || len(am.PreferredChains.RootCommonName) > 0 {
402409
am.Logger.Debug("there is only one chain offered; selecting it regardless of preferences",
403410
zap.String("chain_url", certChains[0].URL))
404411
}
@@ -423,11 +430,9 @@ func (am *ACMEIssuer) selectPreferredChain(certChains []acme.Certificate) acme.C
423430
for i, chain := range certChains {
424431
certs, err := parseCertsFromPEMBundle(chain.ChainPEM)
425432
if err != nil {
426-
if am.Logger != nil {
427-
am.Logger.Error("unable to parse PEM certificate chain",
428-
zap.Int("chain", i),
429-
zap.Error(err))
430-
}
433+
am.Logger.Error("unable to parse PEM certificate chain",
434+
zap.Int("chain", i),
435+
zap.Error(err))
431436
continue
432437
}
433438
decodedChains[i] = certs
@@ -438,11 +443,9 @@ func (am *ACMEIssuer) selectPreferredChain(certChains []acme.Certificate) acme.C
438443
for i, chain := range decodedChains {
439444
for _, cert := range chain {
440445
if cert.Issuer.CommonName == prefAnyCN {
441-
if am.Logger != nil {
442-
am.Logger.Debug("found preferred certificate chain by issuer common name",
443-
zap.String("preference", prefAnyCN),
444-
zap.Int("chain", i))
445-
}
446+
am.Logger.Debug("found preferred certificate chain by issuer common name",
447+
zap.String("preference", prefAnyCN),
448+
zap.Int("chain", i))
446449
return certChains[i]
447450
}
448451
}
@@ -454,20 +457,16 @@ func (am *ACMEIssuer) selectPreferredChain(certChains []acme.Certificate) acme.C
454457
for _, prefRootCN := range am.PreferredChains.RootCommonName {
455458
for i, chain := range decodedChains {
456459
if chain[len(chain)-1].Issuer.CommonName == prefRootCN {
457-
if am.Logger != nil {
458-
am.Logger.Debug("found preferred certificate chain by root common name",
459-
zap.String("preference", prefRootCN),
460-
zap.Int("chain", i))
461-
}
460+
am.Logger.Debug("found preferred certificate chain by root common name",
461+
zap.String("preference", prefRootCN),
462+
zap.Int("chain", i))
462463
return certChains[i]
463464
}
464465
}
465466
}
466467
}
467468

468-
if am.Logger != nil {
469-
am.Logger.Warn("did not find chain matching preferences; using first")
470-
}
469+
am.Logger.Warn("did not find chain matching preferences; using first")
471470
}
472471

473472
return certChains[0]
@@ -509,6 +508,7 @@ type ChainPreference struct {
509508
var DefaultACME = ACMEIssuer{
510509
CA: LetsEncryptProductionCA,
511510
TestCA: LetsEncryptStagingCA,
511+
Logger: defaultLogger,
512512
}
513513

514514
// Some well-known CA endpoints available to use.

‎async.go

+13-18
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,7 @@ func (jm *jobManager) worker() {
7171
jm.queue = jm.queue[1:]
7272
jm.mu.Unlock()
7373
if err := next.job(); err != nil {
74-
if next.logger != nil {
75-
next.logger.Error("job failed", zap.Error(err))
76-
}
74+
next.logger.Error("job failed", zap.Error(err))
7775
}
7876
if next.name != "" {
7977
jm.mu.Lock()
@@ -116,22 +114,19 @@ func doWithRetry(ctx context.Context, log *zap.Logger, f func(context.Context) e
116114
intervalIndex++
117115
}
118116
if time.Since(start) < maxRetryDuration {
119-
if log != nil {
120-
log.Error("will retry",
121-
zap.Error(err),
122-
zap.Int("attempt", attempts),
123-
zap.Duration("retrying_in", retryIntervals[intervalIndex]),
124-
zap.Duration("elapsed", time.Since(start)),
125-
zap.Duration("max_duration", maxRetryDuration))
126-
}
117+
log.Error("will retry",
118+
zap.Error(err),
119+
zap.Int("attempt", attempts),
120+
zap.Duration("retrying_in", retryIntervals[intervalIndex]),
121+
zap.Duration("elapsed", time.Since(start)),
122+
zap.Duration("max_duration", maxRetryDuration))
123+
127124
} else {
128-
if log != nil {
129-
log.Error("final attempt; giving up",
130-
zap.Error(err),
131-
zap.Int("attempt", attempts),
132-
zap.Duration("elapsed", time.Since(start)),
133-
zap.Duration("max_duration", maxRetryDuration))
134-
}
125+
log.Error("final attempt; giving up",
126+
zap.Error(err),
127+
zap.Int("attempt", attempts),
128+
zap.Duration("elapsed", time.Since(start)),
129+
zap.Duration("max_duration", maxRetryDuration))
135130
return nil
136131
}
137132
}

‎cache.go

+35-40
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,11 @@ func NewCache(opts CacheOptions) *Cache {
118118
logger: opts.Logger,
119119
}
120120

121+
// absolutely do not allow a nil logger; panics galore
122+
if c.logger == nil {
123+
c.logger = defaultLogger
124+
}
125+
121126
go c.maintainAssets(0)
122127

123128
return c
@@ -194,14 +199,12 @@ func (certCache *Cache) cacheCertificate(cert Certificate) {
194199
func (certCache *Cache) unsyncedCacheCertificate(cert Certificate) {
195200
// no-op if this certificate already exists in the cache
196201
if _, ok := certCache.cache[cert.hash]; ok {
197-
if certCache.logger != nil {
198-
certCache.logger.Debug("certificate already cached",
199-
zap.Strings("subjects", cert.Names),
200-
zap.Time("expiration", expiresAt(cert.Leaf)),
201-
zap.Bool("managed", cert.managed),
202-
zap.String("issuer_key", cert.issuerKey),
203-
zap.String("hash", cert.hash))
204-
}
202+
certCache.logger.Debug("certificate already cached",
203+
zap.Strings("subjects", cert.Names),
204+
zap.Time("expiration", expiresAt(cert.Leaf)),
205+
zap.Bool("managed", cert.managed),
206+
zap.String("issuer_key", cert.issuerKey),
207+
zap.String("hash", cert.hash))
205208
return
206209
}
207210

@@ -217,13 +220,11 @@ func (certCache *Cache) unsyncedCacheCertificate(cert Certificate) {
217220
i := 0
218221
for _, randomCert := range certCache.cache {
219222
if i == rnd {
220-
if certCache.logger != nil {
221-
certCache.logger.Debug("cache full; evicting random certificate",
222-
zap.Strings("removing_subjects", randomCert.Names),
223-
zap.String("removing_hash", randomCert.hash),
224-
zap.Strings("inserting_subjects", cert.Names),
225-
zap.String("inserting_hash", cert.hash))
226-
}
223+
certCache.logger.Debug("cache full; evicting random certificate",
224+
zap.Strings("removing_subjects", randomCert.Names),
225+
zap.String("removing_hash", randomCert.hash),
226+
zap.Strings("inserting_subjects", cert.Names),
227+
zap.String("inserting_hash", cert.hash))
227228
certCache.removeCertificate(randomCert)
228229
break
229230
}
@@ -239,16 +240,14 @@ func (certCache *Cache) unsyncedCacheCertificate(cert Certificate) {
239240
certCache.cacheIndex[name] = append(certCache.cacheIndex[name], cert.hash)
240241
}
241242

242-
if certCache.logger != nil {
243-
certCache.logger.Debug("added certificate to cache",
244-
zap.Strings("subjects", cert.Names),
245-
zap.Time("expiration", expiresAt(cert.Leaf)),
246-
zap.Bool("managed", cert.managed),
247-
zap.String("issuer_key", cert.issuerKey),
248-
zap.String("hash", cert.hash),
249-
zap.Int("cache_size", len(certCache.cache)),
250-
zap.Int("cache_capacity", certCache.options.Capacity))
251-
}
243+
certCache.logger.Debug("added certificate to cache",
244+
zap.Strings("subjects", cert.Names),
245+
zap.Time("expiration", expiresAt(cert.Leaf)),
246+
zap.Bool("managed", cert.managed),
247+
zap.String("issuer_key", cert.issuerKey),
248+
zap.String("hash", cert.hash),
249+
zap.Int("cache_size", len(certCache.cache)),
250+
zap.Int("cache_capacity", certCache.options.Capacity))
252251
}
253252

254253
// removeCertificate removes cert from the cache.
@@ -275,16 +274,14 @@ func (certCache *Cache) removeCertificate(cert Certificate) {
275274
// delete the actual cert from the cache
276275
delete(certCache.cache, cert.hash)
277276

278-
if certCache.logger != nil {
279-
certCache.logger.Debug("removed certificate from cache",
280-
zap.Strings("subjects", cert.Names),
281-
zap.Time("expiration", expiresAt(cert.Leaf)),
282-
zap.Bool("managed", cert.managed),
283-
zap.String("issuer_key", cert.issuerKey),
284-
zap.String("hash", cert.hash),
285-
zap.Int("cache_size", len(certCache.cache)),
286-
zap.Int("cache_capacity", certCache.options.Capacity))
287-
}
277+
certCache.logger.Debug("removed certificate from cache",
278+
zap.Strings("subjects", cert.Names),
279+
zap.Time("expiration", expiresAt(cert.Leaf)),
280+
zap.Bool("managed", cert.managed),
281+
zap.String("issuer_key", cert.issuerKey),
282+
zap.String("hash", cert.hash),
283+
zap.Int("cache_size", len(certCache.cache)),
284+
zap.Int("cache_capacity", certCache.options.Capacity))
288285
}
289286

290287
// replaceCertificate atomically replaces oldCert with newCert in
@@ -296,11 +293,9 @@ func (certCache *Cache) replaceCertificate(oldCert, newCert Certificate) {
296293
certCache.removeCertificate(oldCert)
297294
certCache.unsyncedCacheCertificate(newCert)
298295
certCache.mu.Unlock()
299-
if certCache.logger != nil {
300-
certCache.logger.Info("replaced certificate in cache",
301-
zap.Strings("subjects", newCert.Names),
302-
zap.Time("new_expiration", expiresAt(newCert.Leaf)))
303-
}
296+
certCache.logger.Info("replaced certificate in cache",
297+
zap.Strings("subjects", newCert.Names),
298+
zap.Time("new_expiration", expiresAt(newCert.Leaf)))
304299
}
305300

306301
func (certCache *Cache) getAllMatchingCerts(name string) []Certificate {

‎certificates.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func (cfg *Config) CacheUnmanagedTLSCertificate(ctx context.Context, tlsCert tls
177177
return err
178178
}
179179
err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, nil)
180-
if err != nil && cfg.Logger != nil {
180+
if err != nil {
181181
cfg.Logger.Warn("stapling OCSP", zap.Error(err))
182182
}
183183
cfg.emit(ctx, "cached_unmanaged_cert", map[string]any{"sans": cert.Names})
@@ -225,7 +225,7 @@ func (cfg Config) makeCertificateWithOCSP(ctx context.Context, certPEMBlock, key
225225
return cert, err
226226
}
227227
err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, certPEMBlock)
228-
if err != nil && cfg.Logger != nil {
228+
if err != nil {
229229
cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names))
230230
}
231231
return cert, nil
@@ -333,9 +333,7 @@ func (cfg *Config) managedCertInStorageExpiresSoon(ctx context.Context, cert Cer
333333
// to the new cert. It assumes that the new certificate for oldCert.Names[0] is
334334
// already in storage. It returns the newly-loaded certificate if successful.
335335
func (cfg *Config) reloadManagedCertificate(ctx context.Context, oldCert Certificate) (Certificate, error) {
336-
if cfg.Logger != nil {
337-
cfg.Logger.Info("reloading managed certificate", zap.Strings("identifiers", oldCert.Names))
338-
}
336+
cfg.Logger.Info("reloading managed certificate", zap.Strings("identifiers", oldCert.Names))
339337
newCert, err := cfg.loadManagedCertificate(ctx, oldCert.Names[0])
340338
if err != nil {
341339
return Certificate{}, fmt.Errorf("loading managed certificate for %v from storage: %v", oldCert.Names, err)

0 commit comments

Comments
 (0)