Optimize storage call for secondary unused issuer in loadCertResourceAnyIssuer

config.go

@@ -108,6 +108,34 @@ type Config struct {
 	// turn until one succeeds.
 	Issuers []Issuer
 
+	// By default, loading a certificate reads the certificate resource of
+	// every configured issuer and serves the newest of them, since any issuer
+	// may hold the most recent certificate for a name. That costs a storage
+	// round-trip per issuer on every load, including for issuers that have
+	// never issued for that name -- the usual case for an issuer configured
+	// only for failover.
+	//
+	// If LoadFirstUsableCert is true, a load stops reading issuers once one
+	// of them has a usable certificate, meaning one that does not need
+	// renewal. The newest of the resources it did read is still the one
+	// returned; only issuers that could not have supplied a certificate to
+	// serve are skipped.
+	//
+	// Nothing is skipped before a usable certificate is found: issuers that
+	// hold no certificate, or hold one that is due for renewal, are read and
+	// compared as usual, so a certificate obtained during failover is still
+	// found, and the newest still wins when none of them is usable.
+	//
+	// Issuers are documented to be in order of preference, which is what
+	// makes the certificate found first the right one to settle for. Note
+	// that an IssuerPolicy of UseFirstRandomIssuer gives up that order, so
+	// which issuer a load settles on is then arbitrary.
+	//
+	// This is worth setting when Storage is remote and the in-memory cache
+	// cannot hold every certificate being served, so that the round-trip
+	// skipped is one a TLS handshake would have waited for.
+	LoadFirstUsableCert bool
+
 	// How to select which issuer to use.
 	// Default: UseFirstIssuer (subject to change).
 	IssuerPolicy IssuerPolicy

crypto.go

@@ -34,6 +34,7 @@ import (
 	"strings"
 
 	"github.com/klauspost/cpuid/v2"
+	"github.com/mholt/acmez/v3/acme"
 	"github.com/zeebo/blake3"
 	"go.uber.org/zap"
 	"golang.org/x/net/idna"
@@ -173,10 +174,11 @@ func (cfg *Config) saveCertResource(ctx context.Context, issuer Issuer, cert Cer
 // loadCertResourceAnyIssuer loads and returns the certificate resource from any
 // of the configured issuers. If multiple are found (e.g. if there are 3 issuers
 // configured, and all 3 have a resource matching certNamesKey), then the newest
-// (latest NotBefore date) resource will be chosen. Callers pass the storage
-// to load from: see cachedStorage and groundTruthStorage.
-func (cfg *Config) loadCertResourceAnyIssuer(ctx context.Context, certNamesKey string, storage Storage) (CertificateResource, error) {
-	// we can save some extra decoding steps if there's only one issuer, since
+// (latest NotBefore date) resource will be chosen. If LoadFirstUsableCert is
+// set, the issuers after the first one holding a resource that does not need
+// renewal are not read. Callers pass the storage to load from: see
+// cachedStorage and groundTruthStorage.
+func (cfg *Config) loadCertResourceAnyIssuer(ctx context.Context, certNamesKey string, storage Storage) (CertificateResource, error) {	// we can save some extra decoding steps if there's only one issuer, since
 	// we don't need to compare potentially multiple available resources to
 	// select the best one, when there's only one choice anyway
 	if len(cfg.Issuers) == 1 {
@@ -214,6 +216,21 @@ func (cfg *Config) loadCertResourceAnyIssuer(ctx context.Context, certNamesKey s
 			issuer:              issuer,
 			decoded:             certs[0],
 		})
+
+		// this issuer is preferred over the ones after it, so if its
+		// certificate is one we would serve as-is, reading theirs can only
+		// cost round-trips to find certificates we would not use
+		if cfg.LoadFirstUsableCert {
+			var ari acme.RenewalInfo
+			if !cfg.DisableARI {
+				if ariPtr, err := certRes.getARI(); err == nil && ariPtr != nil {
+					ari = *ariPtr
+				}
+			}
+			if !cfg.certNeedsRenewal(certs[0], ari, false) {
+				break
+			}
+		}
 	}
 	if len(certResources) == 0 {
 		if lastErr == nil {

crypto_test.go

@@ -17,14 +17,17 @@ package certmagic
 
 import (
 	"bytes"
+	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/rsa"
 	"crypto/x509"
+	"strings"
 	"testing"
+	"time"
 )
 
 func TestEncodeDecodeRSAPrivateKey(t *testing.T) {
@@ -99,3 +102,143 @@ func privateKeyBytes(key crypto.PrivateKey) []byte {
 	}
 	return keyBytes
 }
+
+// testTwoIssuerConfig returns a config with a preferred and a fallback issuer,
+// as one would have for failover between two CAs.
+func testTwoIssuerConfig(t *testing.T) (*Config, *ACMEIssuer, *ACMEIssuer, *recordingStorage) {
+	t.Helper()
+
+	preferred := &ACMEIssuer{CA: "https://preferred.example.com/acme/directory"}
+	fallback := &ACMEIssuer{CA: "https://fallback.example.com/acme/directory"}
+	storage := &recordingStorage{Storage: &FileStorage{Path: t.TempDir()}}
+	cfg := &Config{
+		Issuers:            []Issuer{preferred, fallback},
+		Storage:            storage,
+		RenewalWindowRatio: DefaultRenewalWindowRatio,
+		Logger:             defaultTestLogger,
+		certCache: &Cache{
+			cache:      make(map[string]Certificate),
+			cacheIndex: make(map[string][]string),
+			logger:     defaultTestLogger,
+		},
+	}
+	preferred.config = cfg
+	fallback.config = cfg
+
+	return cfg, preferred, fallback, storage
+}
+
+// saveTestCertResource stores a certificate for domain from issuer, valid for
+// 90 days from notBefore.
+func saveTestCertResource(t *testing.T, cfg *Config, issuer Issuer, domain string, notBefore time.Time) {
+	t.Helper()
+
+	err := cfg.saveCertResource(context.Background(), issuer, testIssuedCertResource(t, issuer, domain, notBefore))
+	if err != nil {
+		t.Fatalf("Expected no error saving cert resource, got: %v", err)
+	}
+}
+
+// readAssetsOf reports whether storage was asked for any of the certificate
+// assets belonging to issuer.
+func readAssetsOf(storage *recordingStorage, issuer Issuer, domain string) bool {
+	prefix := StorageKeys.CertsSitePrefix(issuer.IssuerKey(), domain)
+	for _, call := range storage.calls {
+		if call.name != "Load" || len(call.args) == 0 {
+			continue
+		}
+		if key, ok := call.args[0].(string); ok && strings.HasPrefix(key, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
+func TestLoadCertResourceAnyIssuerPrefersNewestByDefault(t *testing.T) {
+	cfg, preferred, fallback, storage := testTwoIssuerConfig(t)
+	const domain = "example.com"
+	now := time.Now()
+
+	saveTestCertResource(t, cfg, preferred, domain, now.Add(-24*time.Hour))
+	saveTestCertResource(t, cfg, fallback, domain, now)
+
+	storage.calls = nil
+	certRes, err := cfg.loadCertResourceAnyIssuer(context.Background(), domain, cfg.Storage)
+	if err != nil {
+		t.Fatalf("Expected no error loading cert resource, got: %v", err)
+	}
+
+	if certRes.issuerKey != fallback.IssuerKey() {
+		t.Errorf("Expected the newest certificate, from %s, got one from %s", fallback.IssuerKey(), certRes.issuerKey)
+	}
+	if !readAssetsOf(storage, fallback, domain) {
+		t.Error("Expected all issuers to be read when LoadFirstUsableCert is not set")
+	}
+}
+
+func TestLoadFirstUsableCertSkipsLaterIssuers(t *testing.T) {
+	cfg, preferred, fallback, storage := testTwoIssuerConfig(t)
+	cfg.LoadFirstUsableCert = true
+	const domain = "example.com"
+	now := time.Now()
+
+	// the fallback's certificate is newer, so it is the one that would win
+	// without LoadFirstUsableCert
+	saveTestCertResource(t, cfg, preferred, domain, now.Add(-24*time.Hour))
+	saveTestCertResource(t, cfg, fallback, domain, now)
+
+	storage.calls = nil
+	certRes, err := cfg.loadCertResourceAnyIssuer(context.Background(), domain, cfg.Storage)
+	if err != nil {
+		t.Fatalf("Expected no error loading cert resource, got: %v", err)
+	}
+
+	if certRes.issuerKey != preferred.IssuerKey() {
+		t.Errorf("Expected the certificate from %s, got one from %s", preferred.IssuerKey(), certRes.issuerKey)
+	}
+	if readAssetsOf(storage, fallback, domain) {
+		t.Error("Expected the fallback issuer's assets not to be read")
+	}
+}
+
+// The preferred certificate has to be one we would serve as-is for its issuer
+// to end the search; otherwise a newer certificate from a later issuer is
+// still the better one to serve.
+func TestLoadFirstUsableCertLooksPastCertNeedingRenewal(t *testing.T) {
+	cfg, preferred, fallback, storage := testTwoIssuerConfig(t)
+	cfg.LoadFirstUsableCert = true
+	const domain = "example.com"
+	now := time.Now()
+
+	saveTestCertResource(t, cfg, preferred, domain, now.Add(-85*24*time.Hour))
+	saveTestCertResource(t, cfg, fallback, domain, now)
+
+	storage.calls = nil
+	certRes, err := cfg.loadCertResourceAnyIssuer(context.Background(), domain, cfg.Storage)
+	if err != nil {
+		t.Fatalf("Expected no error loading cert resource, got: %v", err)
+	}
+
+	if certRes.issuerKey != fallback.IssuerKey() {
+		t.Errorf("Expected the newest certificate, from %s, got one from %s", fallback.IssuerKey(), certRes.issuerKey)
+	}
+}
+
+// A certificate obtained during failover lives under an issuer that is not the
+// preferred one, and still has to be found.
+func TestLoadFirstUsableCertFindsFailoverCert(t *testing.T) {
+	cfg, _, fallback, _ := testTwoIssuerConfig(t)
+	cfg.LoadFirstUsableCert = true
+	const domain = "example.com"
+
+	saveTestCertResource(t, cfg, fallback, domain, time.Now())
+
+	certRes, err := cfg.loadCertResourceAnyIssuer(context.Background(), domain, cfg.Storage)
+	if err != nil {
+		t.Fatalf("Expected no error loading cert resource, got: %v", err)
+	}
+
+	if certRes.issuerKey != fallback.IssuerKey() {
+		t.Errorf("Expected the certificate from %s, got one from %s", fallback.IssuerKey(), certRes.issuerKey)
+	}
+}