From 70602125de4a3d6d23c9c78a685cc4114dcb431f Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:48:48 +0200 Subject: [PATCH 1/7] feat(san-cert): keep evicted certificates until their own expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A certificate whose domains were all evicted was garbage-collected (files and key deleted) on the next hourly reconcile. Eviction can be a lying domain source — one empty or truncated poll from the application, and the entire dynamic certificate estate was destroyed, forcing a full re-issuance against ACME rate limits when the source recovered. Now only a superseded certificate (no domain maps through it anymore, so a loaded replacement answers every handshake) is removed immediately. An evicted certificate that still serves a mapped domain is kept — never renewed — until its own NotAfter passes. The per-domain expiry gauge is reported only by the certificate a domain currently maps to, so lingering certs cannot clobber their successor's metric. ## Test Coverage - TestCertRenewer_KeepsFullyEvictedCertificateUntilExpiry - TestCertRenewer_RemovesFullyEvictedCertificateAfterExpiry - TestCertRenewer_EvictedCertificateSurvivesSourceRecovery: bad poll + reconcile + recovery costs zero orders - TestCertRenewer_RemovesSupersededCertificateImmediately - TestCertRenewer_SkipsCertificatesNoLongerReferenced: partially superseded cert lingers for its evicted member Refs #96 --- internal/server/domain_renewal.go | 49 +++++++++++++---- internal/server/domain_renewal_test.go | 75 +++++++++++++++++++++++--- 2 files changed, 106 insertions(+), 18 deletions(-) diff --git a/internal/server/domain_renewal.go b/internal/server/domain_renewal.go index 9d7896fe..21e22831 100644 --- a/internal/server/domain_renewal.go +++ b/internal/server/domain_renewal.go @@ -140,12 +140,10 @@ func (r *certRenewer) reconcile() { } // A certificate with no domain that is both still allowed and still - // mapped to it has been superseded or fully evicted: garbage-collect - // it instead of renewing it forever. + // mapped to it has been superseded or fully evicted: retire it + // instead of renewing it forever. if len(r.renewableDomains(cert)) == 0 { - slog.Info("Removing certificate with no remaining domains", "certificate", cert.Identifier) - r.manager.removeCertificate(cert.Identifier) - r.notifyChange() + r.retireCertificate(cert) continue } @@ -185,13 +183,8 @@ func (r *certRenewer) shouldRenew(cert *ManagedCert) bool { // exemption; ARI `replaces` exempts the order entirely where supported. func (r *certRenewer) renew(cert *ManagedCert) { allowed := r.renewableDomains(cert) - - // Only eviction drops a certificate. Quarantine just defers renewal: the - // certificate keeps serving until the quarantine lifts or it expires. if len(allowed) == 0 { - slog.Info("Dropping certificate with no remaining domains", "certificate", cert.Identifier) - r.manager.removeCertificate(cert.Identifier) - r.notifyChange() + // reconcile retires zero-renewable certificates before calling renew. return } @@ -294,6 +287,34 @@ func (r *certRenewer) renewPartition(cert *ManagedCert, domains []string, replac return renewed, true } +// retireCertificate disposes of a certificate with no renewable domains. A +// superseded certificate — nothing maps through it anymore — goes immediately. +// An evicted certificate that still serves a mapped domain is kept until its +// own expiry: eviction can be a lying domain source, and deleting the key +// would turn one bad poll into a certificate outage for every member. +func (r *certRenewer) retireCertificate(cert *ManagedCert) { + if r.certStillServes(cert) && r.now().Before(cert.NotAfter) { + slog.Debug("Keeping evicted certificate until expiry", + "certificate", cert.Identifier, "expires", cert.NotAfter) + return + } + + slog.Info("Removing certificate with no remaining domains", "certificate", cert.Identifier) + r.manager.removeCertificate(cert.Identifier) + r.notifyChange() +} + +// certStillServes reports whether any of a certificate's domains still map to +// it — i.e. a handshake for that name would be answered with this certificate. +func (r *certRenewer) certStillServes(cert *ManagedCert) bool { + for _, domain := range cert.Domains { + if r.manager.certIDForDomain(domain) == cert.Identifier { + return true + } + } + return false +} + // renewableDomains filters a certificate's set down to domains that are still // allowed (deploy-registered or dynamic) AND still mapped to this certificate // — a domain that moved to a newer certificate no longer renews through this @@ -398,6 +419,12 @@ func (r *certRenewer) reportMetrics() { for _, cert := range certs { isWildcard := containsWildcard(cert.Domains) for _, domain := range cert.Domains { + // Evicted certificates linger until expiry; only the certificate a + // domain currently maps to may report that domain's expiry, or the + // zombie would clobber the gauge of its successor. + if r.manager.certIDForDomain(domain) != cert.Identifier { + continue + } metrics.Tracker.SetCertificateExpiry(domain, isWildcard, cert.NotAfter) } } diff --git a/internal/server/domain_renewal_test.go b/internal/server/domain_renewal_test.go index d3e8ddb2..fe0dc15f 100644 --- a/internal/server/domain_renewal_test.go +++ b/internal/server/domain_renewal_test.go @@ -130,13 +130,30 @@ func TestCertRenewer_DropsEvictedDomainsAtRenewal(t *testing.T) { assert.NotEqual(t, old.Identifier, certs[0].Identifier) } -func TestCertRenewer_DropsCertificateWhenAllDomainsEvicted(t *testing.T) { +func TestCertRenewer_KeepsFullyEvictedCertificateUntilExpiry(t *testing.T) { obtainer := successfulObtainer(t) manager := testSANCertManager(t) adoptTestCert(t, manager, []string{"gone.example.com"}, time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) - // Not registered, not dynamic: fully evicted + // Not registered, not dynamic: fully evicted. Eviction can be a lying + // domain source — the certificate must survive until its own expiry, not + // be renewed, and not be deleted. + + renewer := newCertRenewer(manager, newDomainQuarantine(), certRenewerConfig{Obtainer: obtainer}) + renewer.reconcile() + + assert.Empty(t, obtainer.Calls()) + require.Len(t, manager.ManagedCertificates(), 1) + assert.True(t, manager.HasValidCertificate("gone.example.com")) +} + +func TestCertRenewer_RemovesFullyEvictedCertificateAfterExpiry(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + + adoptTestCert(t, manager, []string{"gone.example.com"}, + time.Now().Add(-91*24*time.Hour), time.Now().Add(-time.Hour)) renewer := newCertRenewer(manager, newDomainQuarantine(), certRenewerConfig{Obtainer: obtainer}) renewer.reconcile() @@ -146,6 +163,49 @@ func TestCertRenewer_DropsCertificateWhenAllDomainsEvicted(t *testing.T) { assert.False(t, manager.HasCertificate("gone.example.com")) } +func TestCertRenewer_EvictedCertificateSurvivesSourceRecovery(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + + adoptTestCert(t, manager, []string{"tenant.example.com"}, + time.Now().Add(-time.Hour), time.Now().Add(89*24*time.Hour)) + + // A bad poll evicts everything; the reconcile in between must not delete + // the certificate, so recovery costs zero new orders. + manager.SetDynamicDomains("service1", nil) + renewer := newCertRenewer(manager, newDomainQuarantine(), certRenewerConfig{Obtainer: obtainer}) + renewer.reconcile() + + manager.SetDynamicDomains("service1", []string{"tenant.example.com"}) + renewer.reconcile() + + assert.Empty(t, obtainer.Calls()) + assert.True(t, manager.HasValidCertificate("tenant.example.com")) +} + +func TestCertRenewer_RemovesSupersededCertificateImmediately(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + + manager.SetDynamicDomains("service1", []string{"a.example.com", "b.example.com", "c.example.com"}) + + old := adoptTestCert(t, manager, []string{"a.example.com", "b.example.com"}, + time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) + // Every domain of the old certificate now maps to the newer, wider one: + // nothing serves through the old cert, so it goes immediately, unexpired. + current := adoptTestCert(t, manager, []string{"a.example.com", "b.example.com", "c.example.com"}, + time.Now().Add(-time.Hour), time.Now().Add(89*24*time.Hour)) + + renewer := newCertRenewer(manager, newDomainQuarantine(), certRenewerConfig{Obtainer: obtainer}) + renewer.reconcile() + + assert.Empty(t, obtainer.Calls()) + certs := manager.ManagedCertificates() + require.Len(t, certs, 1) + assert.Equal(t, current.Identifier, certs[0].Identifier) + assert.NotEqual(t, old.Identifier, certs[0].Identifier) +} + func TestCertRenewer_DefersPartiallyQuarantinedBatchWhenTimeAllows(t *testing.T) { obtainer := successfulObtainer(t) manager := testSANCertManager(t) @@ -254,8 +314,9 @@ func TestCertRenewer_SkipsCertificatesNoLongerReferenced(t *testing.T) { manager.SetDynamicDomains("service1", []string{"tenant.example.com"}) - // An older, superseded certificate still covers the domain, but the - // domain now maps to a newer one: the old cert must be GC'd, not renewed. + // tenant.example.com moved to a newer certificate, but the evicted + // gone.example.com still maps to (and is served by) the old one: the old + // cert must not be renewed, and must survive until its own expiry. old := adoptTestCert(t, manager, []string{"gone.example.com", "tenant.example.com"}, time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) current := adoptTestCert(t, manager, []string{"tenant.example.com"}, @@ -267,9 +328,9 @@ func TestCertRenewer_SkipsCertificatesNoLongerReferenced(t *testing.T) { assert.Empty(t, obtainer.Calls()) certs := manager.ManagedCertificates() - require.Len(t, certs, 1) - assert.Equal(t, current.Identifier, certs[0].Identifier) - assert.NotEqual(t, old.Identifier, certs[0].Identifier) + require.Len(t, certs, 2) + assert.Equal(t, current.Identifier, manager.certIDForDomain("tenant.example.com")) + assert.Equal(t, old.Identifier, manager.certIDForDomain("gone.example.com")) } func TestCertRenewer_SkipsRenewalWhenAllDomainsQuarantined(t *testing.T) { From 9505676795d04e8d464d5b9da6306b9f9d3c1923 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:52:57 +0200 Subject: [PATCH 2/7] feat(san-cert): hold suspicious mass-removals from a domain source applyDomains replaced a service's domain set wholesale, so one empty or truncated response from the application evicted every dynamic domain at once. Combined with certificate GC this made a single bad poll capable of tearing down TLS for an entire tenant fleet. A poll that removes more than 30% of the applied set now has its removals held: the previous set stays allowed (plus any additions from the poll), and only three consecutive over-threshold polls confirm and apply the shrink. A recovering poll clears the hold and resets the count. Held domains keep serving existing certificates but earn no new ACME orders. Holds are visible in `kamal-proxy domains list` (Removal held column) and `domains stats`, and every held poll logs at Warn. The hold lives in memory only: a restart reloads the last applied (unshrunk) set, which restarts the count in the conservative direction. ## Test Coverage - TestDynamicDomainManager_ShrinkGuardHoldsMassRemovals: 60% removal held, addition applies - TestDynamicDomainManager_ShrinkGuardHoldsEmptyPoll - TestDynamicDomainManager_ShrinkGuardAppliesSmallRemovals: 10% removal applies immediately - TestDynamicDomainManager_ShrinkGuardConfirmsAfterConsecutivePolls - TestDynamicDomainManager_ShrinkGuardCancelsOnRecovery Refs #96 --- internal/cmd/domains.go | 12 ++- internal/server/commands.go | 4 + internal/server/dynamic_domains.go | 89 ++++++++++++++++++--- internal/server/dynamic_domains_test.go | 102 ++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 13 deletions(-) diff --git a/internal/cmd/domains.go b/internal/cmd/domains.go index 418afd28..1584f6ea 100644 --- a/internal/cmd/domains.go +++ b/internal/cmd/domains.go @@ -64,7 +64,7 @@ func newDomainsListCommand() *domainsListCommand { func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { return fetchDomainsStatus(func(response server.DomainsStatusResponse) { table := NewTable() - table.AddRow([]string{"Service", "Domain", "Certified", "Quarantined until"}) + table.AddRow([]string{"Service", "Domain", "Certified", "Quarantined until", "Removal held"}) for _, name := range slices.Sorted(maps.Keys(response.Services)) { service := response.Services[name] @@ -83,7 +83,12 @@ func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { quarantined = entry.Until.Format("2006-01-02 15:04:05") } - table.AddRow([]string{name, domain.Domain, certified, quarantined}) + held := "" + if slices.Contains(service.HeldRemovals, domain.Domain) { + held = "yes" + } + + table.AddRow([]string{name, domain.Domain, certified, quarantined, held}) } } @@ -111,8 +116,10 @@ func (c *domainsStatsCommand) run(cmd *cobra.Command, args []string) error { return fetchDomainsStatus(func(response server.DomainsStatusResponse) { domains := 0 certified := 0 + held := 0 for _, service := range response.Services { domains += len(service.Domains) + held += len(service.HeldRemovals) for _, domain := range service.Domains { if domain.Certified { certified++ @@ -125,6 +132,7 @@ func (c *domainsStatsCommand) run(cmd *cobra.Command, args []string) error { fmt.Printf("Certified: %d\n", certified) fmt.Printf("Queued for issuance: %d\n", response.QueueLength) fmt.Printf("Quarantined: %d\n", len(response.Quarantine)) + fmt.Printf("Held removals: %d\n", held) fmt.Printf("Managed certificates: %d\n", response.Certificates) }) } diff --git a/internal/server/commands.go b/internal/server/commands.go index b8fbee40..1fb9fbc6 100644 --- a/internal/server/commands.go +++ b/internal/server/commands.go @@ -98,6 +98,10 @@ type DomainsServiceStatus struct { Source string `json:"source"` Domains []DomainStatus `json:"domains"` FetchedAt time.Time `json:"fetched_at"` + + // HeldRemovals lists domains the source stopped reporting but whose + // removal is held by the shrink guard, pending confirmation. + HeldRemovals []string `json:"held_removals,omitempty"` } type QuarantineStatus struct { diff --git a/internal/server/dynamic_domains.go b/internal/server/dynamic_domains.go index 3d6d664c..7f6ce0b3 100644 --- a/internal/server/dynamic_domains.go +++ b/internal/server/dynamic_domains.go @@ -27,6 +27,16 @@ const ( // preflightTimeout bounds the pre-issuance self-probe. preflightTimeout = 5 * time.Second + + // shrinkGuardThreshold is the fraction of a service's applied domain set + // that must disappear in a single poll before the removals are held for + // confirmation instead of applied. One truncated or empty response from + // the app must not evict the certificate estate. + shrinkGuardThreshold = 0.30 + + // shrinkGuardConfirmations is how many consecutive over-threshold polls + // it takes before a held mass-removal is trusted and applied. + shrinkGuardConfirmations = 3 ) // validDomainSource reports whether a tls-domains-source value is usable: a @@ -82,6 +92,7 @@ type DynamicDomainManager struct { sources map[string]*domainSource settings map[string]serviceSettings states map[string]*serviceDomainState + holds map[string]*shrinkHold lastRefresh time.Time preflightNonce string @@ -97,6 +108,7 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager sources: make(map[string]*domainSource), settings: make(map[string]serviceSettings), states: make(map[string]*serviceDomainState), + holds: make(map[string]*shrinkHold), preflightNonce: generateNonce(), probeClient: &http.Client{ Timeout: preflightTimeout, @@ -227,6 +239,7 @@ func (dm *DynamicDomainManager) ServiceRemoved(name string) { delete(dm.sources, name) delete(dm.settings, name) delete(dm.states, name) + delete(dm.holds, name) dm.mu.Unlock() if source == nil && state == nil { @@ -278,9 +291,13 @@ func (dm *DynamicDomainManager) Status() DomainsStatusResponse { dm.mu.Lock() states := make(map[string]*serviceDomainState, len(dm.states)) settings := make(map[string]serviceSettings, len(dm.settings)) + heldRemovals := make(map[string][]string, len(dm.holds)) for name, state := range dm.states { states[name] = state settings[name] = dm.settings[name] + if hold := dm.holds[name]; hold != nil { + heldRemovals[name] = append([]string{}, hold.removals...) + } } dm.mu.Unlock() @@ -295,9 +312,10 @@ func (dm *DynamicDomainManager) Status() DomainsStatusResponse { } services[name] = DomainsServiceStatus{ - Source: settings[name].source, - Domains: domains, - FetchedAt: state.FetchedAt, + Source: settings[name].source, + Domains: domains, + FetchedAt: state.FetchedAt, + HeldRemovals: heldRemovals[name], } } @@ -318,7 +336,9 @@ func (dm *DynamicDomainManager) Status() DomainsStatusResponse { // applyDomains installs a freshly fetched domain set for a service: updates // the allowlist, clears quarantine history for removed domains, requests -// issuance for uncovered ones, and persists. +// issuance for uncovered ones, and persists. A poll that removes more than +// shrinkGuardThreshold of the applied set has its removals held until +// consecutive polls confirm them; its additions still apply. func (dm *DynamicDomainManager) applyDomains(service string, domains []string) { // A deploy/remove race can leave an orphaned poller behind; never apply // domains for a service the router no longer knows. @@ -341,33 +361,50 @@ func (dm *DynamicDomainManager) applyDomains(service string, domains []string) { previous = state.Domains } + added, removed := diffDomains(previous, domains) + + applying := domains + held := dm.evaluateShrinkHold(service, previous, removed) + if held { + // Keep the previous set alive alongside whatever the poll added; the + // polled set replaces it only once the shrink is confirmed. + applying = append(append([]string{}, previous...), added...) + } + etag := "" if source := dm.sources[service]; source != nil { etag = source.ETag() } dm.states[service] = &serviceDomainState{ - Domains: domains, + Domains: applying, ETag: etag, FetchedAt: time.Now(), } dm.mu.Unlock() - added, removed := diffDomains(previous, domains) + dm.manager.SetDynamicDomains(service, applying) - dm.manager.SetDynamicDomains(service, domains) - - for _, domain := range removed { - dm.quarantine.Clear(domain) + if held { + slog.Warn("Holding suspicious mass-removal from domain source", + "service", service, "previous", len(previous), "polled", len(domains), + "held_removals", len(removed), "confirmations_needed", shrinkGuardConfirmations) + } else { + for _, domain := range removed { + dm.quarantine.Clear(domain) + } } + // Issuance follows the polled list, not the held union: a name the source + // stopped reporting keeps its allowlist entry and any live certificate, + // but earns no new ACME orders while its removal is in question. for _, domain := range domains { if !dm.manager.HasValidCertificate(domain) { dm.issuer.Request(domain, service) } } - if len(added) > 0 || len(removed) > 0 { + if !held && (len(added) > 0 || len(removed) > 0) { slog.Info("Domain source updated", "service", service, "domains", len(domains), "added", len(added), "removed", len(removed)) } @@ -375,6 +412,36 @@ func (dm *DynamicDomainManager) applyDomains(service string, domains []string) { dm.saveState() } +// shrinkHold tracks a suspicious mass-removal awaiting confirmation. +type shrinkHold struct { + removals []string + polls int +} + +// evaluateShrinkHold decides whether a poll's removals are applied or held. +// It must be called with dm.mu held. A poll below the threshold — including +// one that brings the domains back — clears any hold and applies normally. +func (dm *DynamicDomainManager) evaluateShrinkHold(service string, previous, removed []string) bool { + if float64(len(removed)) <= shrinkGuardThreshold*float64(len(previous)) { + delete(dm.holds, service) + return false + } + + hold := dm.holds[service] + if hold == nil { + hold = &shrinkHold{} + dm.holds[service] = hold + } + hold.polls++ + hold.removals = removed + + if hold.polls >= shrinkGuardConfirmations { + delete(dm.holds, service) + return false + } + return true +} + // endpointFor resolves a healthy target for path-mode sources at poll time. func (dm *DynamicDomainManager) endpointFor(service string) func() (string, string, error) { return func() (string, string, error) { diff --git a/internal/server/dynamic_domains_test.go b/internal/server/dynamic_domains_test.go index b6238584..f62f1343 100644 --- a/internal/server/dynamic_domains_test.go +++ b/internal/server/dynamic_domains_test.go @@ -239,6 +239,108 @@ func TestDynamicDomainManager_Status(t *testing.T) { assert.Equal(t, 1, status.Certificates) } +// deployWithDomains deploys a service whose source serves the given domains +// and waits for the initial poll to land, so later applyDomains calls diff +// against a known baseline. +func deployWithDomains(t testing.TB, dm *DynamicDomainManager, manager *SANCertManager, service string, domains []string) { + t.Helper() + + backend, _ := testDomainsBackend(t, domains...) + dm.ServiceDeployed(service, ServiceOptions{TLSEnabled: true, TLSDomainsSource: backend.URL}) + require.Eventually(t, func() bool { + return len(manager.DynamicDomains(service)) == len(domains) + }, 5*time.Second, 10*time.Millisecond) +} + +func testTenantDomains(n int) []string { + domains := make([]string, 0, n) + for i := 0; i < n; i++ { + domains = append(domains, fmt.Sprintf("tenant-%d.example.com", i)) + } + return domains +} + +func TestDynamicDomainManager_ShrinkGuardHoldsMassRemovals(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + // One poll drops 6 of 10 domains and adds one: the removals are held, the + // addition applies immediately. + shrunk := append(append([]string{}, full[:4]...), "new.example.com") + dm.applyDomains("service1", shrunk) + + assert.Len(t, manager.DynamicDomains("service1"), 11) + assert.True(t, manager.DomainAllowed("tenant-9.example.com")) + assert.True(t, manager.DomainAllowed("new.example.com")) + + status := dm.Status() + assert.Len(t, status.Services["service1"].HeldRemovals, 6) +} + +func TestDynamicDomainManager_ShrinkGuardHoldsEmptyPoll(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + dm.applyDomains("service1", []string{}) + + assert.Len(t, manager.DynamicDomains("service1"), 10) + assert.Len(t, dm.Status().Services["service1"].HeldRemovals, 10) +} + +func TestDynamicDomainManager_ShrinkGuardAppliesSmallRemovals(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + // Removing 1 of 10 is below the threshold: applied immediately. + dm.applyDomains("service1", full[:9]) + + assert.Len(t, manager.DynamicDomains("service1"), 9) + assert.False(t, manager.DomainAllowed("tenant-9.example.com")) + assert.Empty(t, dm.Status().Services["service1"].HeldRemovals) +} + +func TestDynamicDomainManager_ShrinkGuardConfirmsAfterConsecutivePolls(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + shrunk := full[:4] + + // Two consecutive over-threshold polls are held... + dm.applyDomains("service1", shrunk) + dm.applyDomains("service1", shrunk) + assert.Len(t, manager.DynamicDomains("service1"), 10) + + // ...the third confirms the shrink and applies it. + dm.applyDomains("service1", shrunk) + assert.ElementsMatch(t, shrunk, manager.DynamicDomains("service1")) + assert.Empty(t, dm.Status().Services["service1"].HeldRemovals) +} + +func TestDynamicDomainManager_ShrinkGuardCancelsOnRecovery(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + shrunk := full[:4] + + // A held shrink followed by a recovering poll clears the hold — and the + // confirmation count starts over for the next shrink. + dm.applyDomains("service1", shrunk) + dm.applyDomains("service1", full) + assert.Empty(t, dm.Status().Services["service1"].HeldRemovals) + + dm.applyDomains("service1", shrunk) + dm.applyDomains("service1", shrunk) + assert.Len(t, manager.DynamicDomains("service1"), 10) + + dm.applyDomains("service1", shrunk) + assert.ElementsMatch(t, shrunk, manager.DynamicDomains("service1")) +} + func TestDynamicDomainManager_PreflightProbe(t *testing.T) { dm, _ := testDynamicDomainManager(t, DynamicDomainConfig{}) From a07e4c7dde644b6d271251482290f557d2da9edc Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:56:33 +0200 Subject: [PATCH 3/7] feat(san-cert): probe dynamic members before every renewal order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-flight probe only guarded never-issued domains, so a tenant whose DNS moved away after issuance sat in every renewal order for its batch, failing it at the ACME server — and when the failure was not attributable to one domain, nothing was quarantined and the doomed batch retried every reconcile until the whole certificate expired. renew now probes each dynamic member before spending an order and treats unreachable ones exactly like quarantined ones: defer while there is time, compact them out inside the compaction window. Deploy-registered hosts are not probed — a DNS-01-only deployment may be unreachable over HTTP by design — and wildcards have no name to answer on. ## Test Coverage - TestCertRenewer_ProbesDynamicMembersBeforeRenewal: unreachable member compacted out near expiry - TestCertRenewer_DefersRenewalWhenMemberFailsProbeFarFromExpiry - TestCertRenewer_DefersRenewalWhenAllMembersFailProbe - TestCertRenewer_SkipsProbeForRegisteredMembers: DNS-01-only deploys still renew Refs #96 --- internal/server/domain_renewal.go | 50 ++++++++++++ internal/server/domain_renewal_test.go | 104 ++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/internal/server/domain_renewal.go b/internal/server/domain_renewal.go index 21e22831..82a2e5b2 100644 --- a/internal/server/domain_renewal.go +++ b/internal/server/domain_renewal.go @@ -6,6 +6,7 @@ import ( "hash/fnv" "log/slog" "slices" + "strings" "sync" "time" @@ -205,6 +206,27 @@ func (r *certRenewer) renew(cert *ManagedCert) { return } + // Probe the remaining members before spending an order: a tenant whose + // DNS moved away would fail validation and could sink the whole batch — + // or worse, fail in a way ACME does not attribute to any one domain. + // Unreachable members follow the same policy as quarantined ones. + if unreachable := r.preflightMembers(domains); len(unreachable) > 0 { + if time.Until(cert.NotAfter) > quarantineCompactionWindow { + slog.Info("Deferring renewal until unreachable members recover", + "certificate", cert.Identifier, "unreachable", unreachable) + return + } + + domains = slices.DeleteFunc(domains, func(domain string) bool { + return slices.Contains(unreachable, domain) + }) + if len(domains) == 0 { + slog.Info("Deferring renewal; every member failed the pre-flight probe", + "certificate", cert.Identifier) + return + } + } + domains, toppedUp := r.topUpBatch(domains) defer func() { if r.config.ReleasePending != nil && len(toppedUp) > 0 { @@ -368,6 +390,34 @@ func (r *certRenewer) topUpBatch(domains []string) (batch, taken []string) { return append(domains, kept...), kept } +// preflightMembers probes a renewal batch's dynamic members and quarantines +// the unreachable ones. Only dynamic (tenant-supplied) domains are probed: +// they must route back to this proxy to be validated or served at all. A +// deploy-registered host may be reachable over DNS-01 only, and a wildcard +// has no name to answer on, so neither is probed. +func (r *certRenewer) preflightMembers(domains []string) []string { + if r.config.Preflight == nil { + return nil + } + + unreachable := []string{} + for _, domain := range domains { + if strings.HasPrefix(domain, "*.") { + continue + } + if _, dynamic := r.manager.dynamicOwner(domain); !dynamic { + continue + } + if err := r.config.Preflight(domain); err != nil { + backoff := r.quarantine.RecordFailure(domain, quarantinePreflight) + slog.Warn("Renewal member failed pre-flight probe; holding back", + "domain", domain, "backoff", backoff, "error", err) + unreachable = append(unreachable, domain) + } + } + return unreachable +} + func (r *certRenewer) dynamicServiceFor(domains []string) (string, bool) { for _, domain := range domains { if service, ok := r.manager.dynamicOwner(domain); ok { diff --git a/internal/server/domain_renewal_test.go b/internal/server/domain_renewal_test.go index fe0dc15f..d9e93dd7 100644 --- a/internal/server/domain_renewal_test.go +++ b/internal/server/domain_renewal_test.go @@ -297,7 +297,12 @@ func TestCertRenewer_TopUpPreflightsNewDomains(t *testing.T) { BatchSize: func(service string) int { return 3 }, TakePending: issuer.takePending, ReleasePending: issuer.releasePending, - Preflight: func(domain string) error { return errors.New("does not route here") }, + Preflight: func(domain string) error { + if domain == "unreachable.example.com" { + return errors.New("does not route here") + } + return nil + }, }) renewer.reconcile() @@ -308,6 +313,103 @@ func TestCertRenewer_TopUpPreflightsNewDomains(t *testing.T) { assert.True(t, quarantine.IsQuarantined("unreachable.example.com")) } +func TestCertRenewer_ProbesDynamicMembersBeforeRenewal(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + quarantine := newDomainQuarantine() + + manager.SetDynamicDomains("service1", []string{"ok.example.com", "dead.example.com"}) + // Within the compaction window: the unreachable member is dropped from + // the order instead of sinking it at the ACME server. + adoptTestCert(t, manager, []string{"dead.example.com", "ok.example.com"}, + time.Now().Add(-87*24*time.Hour), time.Now().Add(3*24*time.Hour)) + + renewer := newCertRenewer(manager, quarantine, certRenewerConfig{ + Obtainer: obtainer, + Preflight: func(domain string) error { + if domain == "dead.example.com" { + return errors.New("does not route here") + } + return nil + }, + }) + renewer.reconcile() + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"ok.example.com"}, calls[0].Domains) + assert.True(t, quarantine.IsQuarantined("dead.example.com")) +} + +func TestCertRenewer_DefersRenewalWhenMemberFailsProbeFarFromExpiry(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + quarantine := newDomainQuarantine() + + manager.SetDynamicDomains("service1", []string{"ok.example.com", "dead.example.com"}) + // 20 days left: wait for the unreachable member rather than unmapping it + // from a still-valid certificate. + adoptTestCert(t, manager, []string{"dead.example.com", "ok.example.com"}, + time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) + + renewer := newCertRenewer(manager, quarantine, certRenewerConfig{ + Obtainer: obtainer, + Preflight: func(domain string) error { + if domain == "dead.example.com" { + return errors.New("does not route here") + } + return nil + }, + }) + renewer.reconcile() + + assert.Empty(t, obtainer.Calls()) + assert.True(t, quarantine.IsQuarantined("dead.example.com")) + require.Len(t, manager.ManagedCertificates(), 1) +} + +func TestCertRenewer_DefersRenewalWhenAllMembersFailProbe(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + quarantine := newDomainQuarantine() + + manager.SetDynamicDomains("service1", []string{"dead.example.com"}) + adoptTestCert(t, manager, []string{"dead.example.com"}, + time.Now().Add(-87*24*time.Hour), time.Now().Add(3*24*time.Hour)) + + renewer := newCertRenewer(manager, quarantine, certRenewerConfig{ + Obtainer: obtainer, + Preflight: func(domain string) error { return errors.New("does not route here") }, + }) + renewer.reconcile() + + assert.Empty(t, obtainer.Calls()) + require.Len(t, manager.ManagedCertificates(), 1) +} + +func TestCertRenewer_SkipsProbeForRegisteredMembers(t *testing.T) { + obtainer := successfulObtainer(t) + manager := testSANCertManager(t) + quarantine := newDomainQuarantine() + + // Deploy-registered hosts are not probed: a DNS-01-only deployment may + // be unreachable over HTTP by design, and must still renew. + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + adoptTestCert(t, manager, []string{"app.example.com"}, + time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) + + renewer := newCertRenewer(manager, quarantine, certRenewerConfig{ + Obtainer: obtainer, + Preflight: func(domain string) error { return errors.New("unreachable over HTTP") }, + }) + renewer.reconcile() + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) + assert.False(t, quarantine.IsQuarantined("app.example.com")) +} + func TestCertRenewer_SkipsCertificatesNoLongerReferenced(t *testing.T) { obtainer := successfulObtainer(t) manager := testSANCertManager(t) From 4993c8bf7e9ab7c2722b2cce75191ef480f6040b Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 08:03:30 +0200 Subject: [PATCH 4/7] feat(san-cert): probe for culprits when an ACME failure names no domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an order failed without lego's per-domain error lines, the issuer quarantined the whole batch blindly and the renewer quarantined nobody — so a failing renewal batch retried every reconcile with no backoff and no compaction, riding unchanged until the certificate expired. identifyFailedDomains (new internal/server/domain_failure.go, absorbing failedDomainsFromError) now attributes failures in three steps: parse the error's per-domain lines; failing that, pre-flight-probe the members and blame the unreachable ones; failing that, hold the entire batch on the quarantine ladder. Both the issuer and the renewer use it — the issuer gains probe-based attribution (survivors keep their retry), the renewer gains the quarantine-nobody fix. The renewer probes only dynamic members, since a registered host may be DNS-01-only and unreachable over HTTP by design. ## Test Coverage - TestIdentifyFailedDomains: table-driven precedence (parse > probe > all), wildcard skip - TestDomainIssuer_Issue_ProbesForCulpritsOnUnattributableFailure - TestCertRenewer_QuarantinesWholeBatchOnUnattributableFailure Refs #96 --- internal/server/domain_failure.go | 52 +++++++++++++++++++ internal/server/domain_failure_test.go | 70 ++++++++++++++++++++++++++ internal/server/domain_issuer.go | 24 +-------- internal/server/domain_issuer_test.go | 36 +++++++++++++ internal/server/domain_renewal.go | 15 +++++- internal/server/domain_renewal_test.go | 25 +++++++++ 6 files changed, 198 insertions(+), 24 deletions(-) create mode 100644 internal/server/domain_failure.go create mode 100644 internal/server/domain_failure_test.go diff --git a/internal/server/domain_failure.go b/internal/server/domain_failure.go new file mode 100644 index 00000000..37f916c5 --- /dev/null +++ b/internal/server/domain_failure.go @@ -0,0 +1,52 @@ +package server + +import "strings" + +// Attribution of failed ACME orders to the domains that caused them, shared by +// the dynamic issuer and the background renewer. + +// identifyFailedDomains names the domains responsible for a failed order: +// lego's per-domain error lines when present, a pre-flight probe of each +// member otherwise, and the whole set when neither can tell — an +// unattributable failure holds the entire batch on the quarantine ladder so +// retries back off instead of looping against ACME rate limits. +func identifyFailedDomains(err error, domains []string, preflight func(string) error) []string { + if failed := failedDomainsFromError(err, domains); len(failed) > 0 { + return failed + } + + if preflight != nil { + failed := []string{} + for _, domain := range domains { + if strings.HasPrefix(domain, "*.") { + continue + } + if probeErr := preflight(domain); probeErr != nil { + failed = append(failed, domain) + } + } + if len(failed) > 0 { + return failed + } + } + + return domains +} + +// failedDomainsFromError matches lego's per-domain error lines +// (": ") against the attempted domains. +func failedDomainsFromError(err error, domains []string) []string { + lines := strings.Split(err.Error(), "\n") + + failed := []string{} + for _, domain := range domains { + prefix := domain + ": " + for _, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), prefix) { + failed = append(failed, domain) + break + } + } + } + return failed +} diff --git a/internal/server/domain_failure_test.go b/internal/server/domain_failure_test.go new file mode 100644 index 00000000..5282863f --- /dev/null +++ b/internal/server/domain_failure_test.go @@ -0,0 +1,70 @@ +package server + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIdentifyFailedDomains(t *testing.T) { + domains := []string{"a.example.com", "b.example.com"} + failB := func(domain string) error { + if domain == "b.example.com" { + return errors.New("does not route here") + } + return nil + } + failAll := func(domain string) error { return errors.New("does not route here") } + passAll := func(domain string) error { return nil } + + tests := []struct { + name string + err error + domains []string + preflight func(string) error + expected []string + }{ + { + name: "per-domain error lines take precedence over probing", + err: fmt.Errorf("error: one or more domains had a problem:\na.example.com: acme: dns problem"), + domains: domains, + preflight: failB, + expected: []string{"a.example.com"}, + }, + { + name: "probe names the culprit when the error does not", + err: errors.New("acme: internal error"), + domains: domains, + preflight: failB, + expected: []string{"b.example.com"}, + }, + { + name: "everything passes probing: the whole batch is held", + err: errors.New("acme: internal error"), + domains: domains, + preflight: passAll, + expected: domains, + }, + { + name: "no probe available: the whole batch is held", + err: errors.New("acme: internal error"), + domains: domains, + expected: domains, + }, + { + name: "wildcard members are never probed", + err: errors.New("acme: internal error"), + domains: []string{"*.example.com", "a.example.com"}, + preflight: failAll, + expected: []string{"a.example.com"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, identifyFailedDomains(tt.err, tt.domains, tt.preflight)) + }) + } +} diff --git a/internal/server/domain_issuer.go b/internal/server/domain_issuer.go index 6741b34f..b808b7e6 100644 --- a/internal/server/domain_issuer.go +++ b/internal/server/domain_issuer.go @@ -4,7 +4,6 @@ import ( "context" "log/slog" "slices" - "strings" "sync" "time" @@ -376,10 +375,7 @@ func (i *domainIssuer) issue(batch []*issueRequest) { // quarantined too, so a failing batch cannot loop against ACME rate limits; // the poller re-requests them after the backoff expires. func (i *domainIssuer) handleObtainFailure(batch []*issueRequest, domains []string, requests map[string]*issueRequest, err error) { - failed := failedDomainsFromError(err, domains) - if len(failed) == 0 { - failed = domains - } + failed := identifyFailedDomains(err, domains, i.config.Preflight) slog.Warn("Certificate order failed", "domains", domains, "failed", failed, "error", err) @@ -417,24 +413,6 @@ func (i *domainIssuer) handleObtainFailure(batch []*issueRequest, domains []stri i.notify() } -// failedDomainsFromError matches lego's per-domain error lines -// (": ") against the attempted domains. -func failedDomainsFromError(err error, domains []string) []string { - lines := strings.Split(err.Error(), "\n") - - failed := []string{} - for _, domain := range domains { - prefix := domain + ": " - for _, line := range lines { - if strings.HasPrefix(strings.TrimSpace(line), prefix) { - failed = append(failed, domain) - break - } - } - } - return failed -} - func (i *domainIssuer) notify() { select { case i.wake <- struct{}{}: diff --git a/internal/server/domain_issuer_test.go b/internal/server/domain_issuer_test.go index 1c57c13e..76cc3f41 100644 --- a/internal/server/domain_issuer_test.go +++ b/internal/server/domain_issuer_test.go @@ -3,6 +3,7 @@ package server import ( "errors" "fmt" + "sync/atomic" "testing" "time" @@ -158,6 +159,41 @@ func TestDomainIssuer_Issue_UnidentifiableFailureQuarantinesWholeBatch(t *testin require.Len(t, obtainer.Calls(), 1) } +func TestDomainIssuer_Issue_ProbesForCulpritsOnUnattributableFailure(t *testing.T) { + var ordered atomic.Bool + obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + ordered.Store(true) + return nil, errors.New("acme: internal error") + }} + + issuer, manager, quarantine := testIssuer(t, obtainer, domainIssuerConfig{ + BatchSize: func(service string) int { return 2 }, + // Both domains route here when the order is placed; gone.example.com + // stops routing by the time the failure is investigated. + Preflight: func(domain string) error { + if ordered.Load() && domain == "gone.example.com" { + return errors.New("no longer routes here") + } + return nil + }, + }) + manager.SetDynamicDomains("service1", []string{"good.example.com", "gone.example.com"}) + + issuer.Request("good.example.com", "service1") + issuer.Request("gone.example.com", "service1") + issuer.issue(issuer.nextBatch()) + + // The probe identified the culprit: quarantined, while the survivor is + // re-enqueued for its retry instead of being quarantined with it. + assert.True(t, quarantine.IsQuarantined("gone.example.com")) + assert.False(t, quarantine.IsQuarantined("good.example.com")) + + batch := issuer.nextBatch() + require.Len(t, batch, 1) + assert.Equal(t, "good.example.com", batch[0].domain) + assert.True(t, batch[0].retried) +} + func TestDomainIssuer_Issue_RetriedSurvivorsAreNotReenqueuedAgain(t *testing.T) { obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { return nil, fmt.Errorf("error: one or more domains had a problem:\n%s: acme: failed", request.Domains[0]) diff --git a/internal/server/domain_renewal.go b/internal/server/domain_renewal.go index 82a2e5b2..30a3913f 100644 --- a/internal/server/domain_renewal.go +++ b/internal/server/domain_renewal.go @@ -434,7 +434,20 @@ func (r *certRenewer) handleRenewalFailure(cert *ManagedCert, domains []string, return } - failed := failedDomainsFromError(err, domains) + // Probe only dynamic members when attributing the failure: a registered + // host may be DNS-01-only and unreachable over HTTP by design. + probe := r.config.Preflight + if probe != nil { + preflight := probe + probe = func(domain string) error { + if _, dynamic := r.manager.dynamicOwner(domain); !dynamic { + return nil + } + return preflight(domain) + } + } + + failed := identifyFailedDomains(err, domains, probe) slog.Warn("Certificate renewal failed", "certificate", cert.Identifier, "domains", domains, "failed", failed, "error", err) diff --git a/internal/server/domain_renewal_test.go b/internal/server/domain_renewal_test.go index d9e93dd7..0bc2b79f 100644 --- a/internal/server/domain_renewal_test.go +++ b/internal/server/domain_renewal_test.go @@ -410,6 +410,31 @@ func TestCertRenewer_SkipsProbeForRegisteredMembers(t *testing.T) { assert.False(t, quarantine.IsQuarantined("app.example.com")) } +func TestCertRenewer_QuarantinesWholeBatchOnUnattributableFailure(t *testing.T) { + obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + return nil, errors.New("acme: internal error") + }} + manager := testSANCertManager(t) + quarantine := newDomainQuarantine() + + manager.SetDynamicDomains("service1", []string{"a.example.com", "b.example.com"}) + adoptTestCert(t, manager, []string{"a.example.com", "b.example.com"}, + time.Now().Add(-70*24*time.Hour), time.Now().Add(20*24*time.Hour)) + + renewer := newCertRenewer(manager, quarantine, certRenewerConfig{ + Obtainer: obtainer, + Preflight: func(domain string) error { return nil }, + }) + renewer.reconcile() + + // The failure names no domain and every member probes clean: hold the + // whole batch on the quarantine ladder so retries back off instead of + // looping hourly until the certificate expires. + assert.True(t, quarantine.IsQuarantined("a.example.com")) + assert.True(t, quarantine.IsQuarantined("b.example.com")) + require.Len(t, manager.ManagedCertificates(), 1) +} + func TestCertRenewer_SkipsCertificatesNoLongerReferenced(t *testing.T) { obtainer := successfulObtainer(t) manager := testSANCertManager(t) From 4955983996b3e07cd5c56b9a209babcc05b9f58c Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 08:08:19 +0200 Subject: [PATCH 5/7] feat(san-cert): guard the handshake batch with preflight and quarantine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit provisionCertificate batched every pending deploy-registered host into the triggering handshake's order with no protection: one typo'd --host failed the whole order, everything returned to pending, and the same doomed batch retried on every handshake, starving the healthy hosts of certificates. A new issuance guard (san_cert_batch_guard.go), wired to the dynamic subsystem's probe and quarantine at boot, now filters batch-mates before the order — quarantined or unreachable mates keep their pending slot but stay out of the order — and attributes failures afterwards: identified culprits are quarantined while survivors return to pending. The triggering domain is never dropped, and an unattributable failure (a generic ACME outage) restores everything unquarantined, exactly as an unguarded batch would. The provisioning slot is claimed before the probes run, so concurrent handshakes wait instead of racing into duplicate orders. ## Test Coverage - TestBatchGuard_QuarantinedBatchMateIsSkippedButStaysPending - TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder - TestBatchGuard_TriggerDomainIsNeverDropped - TestBatchGuard_QuarantinesCulpritsAndRestoresSurvivorsOnFailure - TestBatchGuard_UnattributableFailureRestoresEverythingUnquarantined - TestBatchGuard_NoGuardInstalledPreservesBehavior Refs #96 --- internal/server/dynamic_domains.go | 1 + internal/server/san_cert_batch_guard.go | 114 ++++++++++++++ internal/server/san_cert_batch_guard_test.go | 155 +++++++++++++++++++ internal/server/san_cert_manager.go | 40 +++-- 4 files changed, 296 insertions(+), 14 deletions(-) create mode 100644 internal/server/san_cert_batch_guard.go create mode 100644 internal/server/san_cert_batch_guard_test.go diff --git a/internal/server/dynamic_domains.go b/internal/server/dynamic_domains.go index 7f6ce0b3..edc24253 100644 --- a/internal/server/dynamic_domains.go +++ b/internal/server/dynamic_domains.go @@ -138,6 +138,7 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager }) manager.SetDynamicCertRequester(dm.issuer.Request) + manager.SetIssuanceGuard(dm.preflightProbe, dm.quarantine) dm.loadState() diff --git a/internal/server/san_cert_batch_guard.go b/internal/server/san_cert_batch_guard.go new file mode 100644 index 00000000..0e71e55f --- /dev/null +++ b/internal/server/san_cert_batch_guard.go @@ -0,0 +1,114 @@ +package server + +import ( + "log/slog" +) + +// Guarding of the handshake-driven provisioning batch. +// +// provisionCertificate batches every pending deploy-registered host into the +// triggering handshake's order. Without a guard, one host whose DNS points +// elsewhere — a typo'd `deploy --host`, a domain surrendered after deploy — +// fails the whole order and starves its batch-mates of certificates forever. +// The guard reuses the dynamic subsystem's preflight probe and quarantine: +// batch-mates that are quarantined or unreachable stay out of the order, and +// a failed order quarantines its identified culprits only. + +// issuanceGuard carries the hooks installed by the dynamic domain manager. +type issuanceGuard struct { + preflight func(domain string) error + quarantine *domainQuarantine +} + +// SetIssuanceGuard installs the preflight probe and quarantine that guard +// handshake-driven batches. NewDynamicDomainManager installs it at boot; +// without it, provisioning batches behave as before. +func (m *SANCertManager) SetIssuanceGuard(preflight func(domain string) error, quarantine *domainQuarantine) { + m.mu.Lock() + defer m.mu.Unlock() + + m.guard = issuanceGuard{preflight: preflight, quarantine: quarantine} +} + +func (m *SANCertManager) issuanceGuardSnapshot() issuanceGuard { + m.mu.RLock() + defer m.mu.RUnlock() + + return m.guard +} + +// filterBatchMates drops quarantined or unreachable domains from a handshake +// batch, quarantining fresh probe failures. The triggering domain is never +// dropped: its handshake is why we are here, and if it is the problem, the +// order failure will be attributed to it. Callers must NOT hold m.mu — the +// probe does network I/O. +func (m *SANCertManager) filterBatchMates(trigger string, domains []string) []string { + guard := m.issuanceGuardSnapshot() + if guard.quarantine == nil { + return domains + } + + kept := make([]string, 0, len(domains)) + for _, domain := range domains { + if domain == trigger { + kept = append(kept, domain) + continue + } + if guard.quarantine.IsQuarantined(domain) { + slog.Debug("Leaving quarantined domain out of handshake batch", "domain", domain) + continue + } + if guard.preflight != nil && !m.HasCertificate(domain) { + if err := guard.preflight(domain); err != nil { + backoff := guard.quarantine.RecordFailure(domain, quarantinePreflight) + slog.Warn("Batch domain failed pre-flight probe; holding back", + "domain", domain, "backoff", backoff, "error", err) + continue + } + } + kept = append(kept, domain) + } + return kept +} + +// attributeBatchFailure quarantines the culprits of a failed handshake order +// and returns the requested domains that should go back to pending. Only a +// culprit narrower than the whole order is quarantined: a generic ACME outage +// must not push deploy-registered hosts onto the quarantine ladder, so an +// unattributable failure restores everything, exactly as an unguarded batch +// would. +func (m *SANCertManager) attributeBatchFailure(err error, ordered, requested []string) []string { + guard := m.issuanceGuardSnapshot() + if guard.quarantine == nil { + return requested + } + + culprits := failedDomainsFromError(err, ordered) + if len(culprits) == 0 && guard.preflight != nil { + for _, identifier := range ordered { + if identifier[0] == '*' { + continue + } + if probeErr := guard.preflight(identifier); probeErr != nil { + culprits = append(culprits, identifier) + } + } + } + if len(culprits) == 0 { + return requested + } + + // The order may contain planned identifiers (a wildcard collapsed from + // siblings); blame the requested hosts a culprit identifier covers. + survivors := []string{} + for _, domain := range requested { + if identifiersCover(culprits, domain) { + backoff := guard.quarantine.RecordFailure(domain, quarantineACME) + slog.Warn("Quarantining culprit of failed handshake batch", + "domain", domain, "backoff", backoff) + continue + } + survivors = append(survivors, domain) + } + return survivors +} diff --git a/internal/server/san_cert_batch_guard_test.go b/internal/server/san_cert_batch_guard_test.go new file mode 100644 index 00000000..234a6d7c --- /dev/null +++ b/internal/server/san_cert_batch_guard_test.go @@ -0,0 +1,155 @@ +package server + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/go-acme/lego/v4/certificate" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testGuardedManager(t testing.TB, obtainer certObtainer) (*SANCertManager, *domainQuarantine) { + t.Helper() + + manager := testSANCertManager(t) + manager.httpObtainer = obtainer + quarantine := newDomainQuarantine() + return manager, quarantine +} + +func pendingDomainsOf(manager *SANCertManager) []string { + manager.mu.RLock() + defer manager.mu.RUnlock() + + domains := []string{} + for domain := range manager.pendingDomains { + domains = append(domains, domain) + } + return domains +} + +func TestBatchGuard_QuarantinedBatchMateIsSkippedButStaysPending(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(nil, quarantine) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("bad.example.com", "service1")) + quarantine.RecordFailure("bad.example.com", quarantineACME) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) + + // The quarantined mate keeps its pending slot for a later batch. + assert.Contains(t, pendingDomainsOf(manager), "bad.example.com") +} + +func TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(func(domain string) error { + if domain == "dead.example.com" { + return errors.New("does not route here") + } + return nil + }, quarantine) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("dead.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) + + assert.True(t, quarantine.IsQuarantined("dead.example.com")) + assert.Contains(t, pendingDomainsOf(manager), "dead.example.com") +} + +func TestBatchGuard_TriggerDomainIsNeverDropped(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + // The probe fails everything, and the trigger is even quarantined — its + // handshake still gets its shot. + manager.SetIssuanceGuard(func(domain string) error { return errors.New("unreachable") }, quarantine) + quarantine.RecordFailure("app.example.com", quarantineACME) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) +} + +func TestBatchGuard_QuarantinesCulpritsAndRestoresSurvivorsOnFailure(t *testing.T) { + obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + return nil, fmt.Errorf("error: one or more domains had a problem:\nbad.example.com: acme: error presenting token") + }} + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(nil, quarantine) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("bad.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.Error(t, err) + + assert.True(t, quarantine.IsQuarantined("bad.example.com")) + assert.False(t, quarantine.IsQuarantined("app.example.com")) + + // The survivor returns to pending for the next handshake; the culprit + // waits out its quarantine instead. + pending := pendingDomainsOf(manager) + assert.Contains(t, pending, "app.example.com") + assert.NotContains(t, pending, "bad.example.com") +} + +func TestBatchGuard_UnattributableFailureRestoresEverythingUnquarantined(t *testing.T) { + obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + return nil, errors.New("acme: internal error") + }} + manager, quarantine := testGuardedManager(t, obtainer) + // A generic ACME outage must not push deploy-registered hosts onto the + // quarantine ladder; the probe passing everyone proves no culprit. + manager.SetIssuanceGuard(func(domain string) error { return nil }, quarantine) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("other.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.Error(t, err) + + assert.Equal(t, 0, quarantine.Len()) + pending := pendingDomainsOf(manager) + assert.Contains(t, pending, "app.example.com") + assert.Contains(t, pending, "other.example.com") +} + +func TestBatchGuard_NoGuardInstalledPreservesBehavior(t *testing.T) { + obtainer := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + return nil, errors.New("acme: internal error") + }} + manager, _ := testGuardedManager(t, obtainer) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("other.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.Error(t, err) + + pending := pendingDomainsOf(manager) + assert.Contains(t, pending, "app.example.com") + assert.Contains(t, pending, "other.example.com") +} diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index a7f7479b..793237f0 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -137,6 +137,10 @@ type SANCertManager struct { // Callback to request asynchronous issuance for a dynamic domain dynamicCertRequester func(domain, service string) + // guard filters handshake-driven batches through preflight and quarantine + // (see san_cert_batch_guard.go); zero value means unguarded. + guard issuanceGuard + // Currently provisioning: rootDomain -> done channel provisioning map[string]chan struct{} @@ -479,25 +483,22 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string } } + // Claim the provisioning slot before touching the batch, so concurrent + // handshakes wait on it instead of racing into a duplicate order while + // batch-mates are being probed. + done := make(chan struct{}) + m.provisioning[provisioningKey] = done + // Collect ALL pending domains (up to MaxSANsPerCertificate) - domainsToProvision := []string{domain} + candidates := []string{domain} for pendingDomain := range m.pendingDomains { if pendingDomain != domain { - domainsToProvision = append(domainsToProvision, pendingDomain) + candidates = append(candidates, pendingDomain) } - if len(domainsToProvision) >= MaxSANsPerCertificate { + if len(candidates) >= MaxSANsPerCertificate { break } } - - // Start provisioning - done := make(chan struct{}) - m.provisioning[provisioningKey] = done - - // Remove domains from pending - for _, d := range domainsToProvision { - delete(m.pendingDomains, d) - } m.mu.Unlock() defer func() { @@ -507,6 +508,16 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string m.mu.Unlock() }() + // Quarantined or unreachable batch-mates stay out of the order — and keep + // their pending slot for a later batch, once their quarantine lifts. + domainsToProvision := m.filterBatchMates(domain, candidates) + + m.mu.Lock() + for _, d := range domainsToProvision { + delete(m.pendingDomains, d) + } + m.mu.Unlock() + // Sort the planned identifier set for consistent certificate identifiers sortedDomains := m.planIssuanceDomains(domainsToProvision) slices.Sort(sortedDomains) @@ -556,8 +567,9 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string resource, err := m.obtainCertificate(request) if err != nil { - // Re-add domains to pending so they can be retried - m.restorePending(domainsToProvision) + // Quarantine the identified culprits; survivors return to pending so + // they can be retried without them. + m.restorePending(m.attributeBatchFailure(err, sortedDomains, domainsToProvision)) return nil, fmt.Errorf("failed to obtain certificate: %w", err) } From c42a426a50799ac87616c5f6bceb4f3a39eafdef Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 08:10:50 +0200 Subject: [PATCH 6/7] docs(san-cert): document the shrink guard and eviction grace period --- README.md | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 58e885fe..e0cec547 100644 --- a/README.md +++ b/README.md @@ -1044,7 +1044,21 @@ to verify DNS actually routes here — unreachable domains are quarantined (5m, then 15m → 1h → 4h → 24h backoff) without burning an order. Failing domains quarantine alone; the rest of a batch is retried once. Renewals reuse the exact same identifier set (exempt from most rate limits) and pass ARI -`replaces` where supported. +`replaces` where supported. Every renewal re-probes its dynamic members +first, so a tenant whose DNS moved away after issuance is quarantined out of +the order instead of failing it — and when an order fails without naming a +domain, the members are probed to find the culprit before anyone is blamed. + +**A bad poll cannot destroy certificates.** Two guards protect the estate +from the source itself. A poll that removes more than 30% of the applied +domain set has its removals *held*: the previous set stays allowed (additions +still apply), and only three consecutive shrunken polls confirm and apply the +removal — an empty or truncated response from the app evicts nothing. Held +removals are visible in `kamal-proxy domains list` (Removal held column) and +logged at Warn on every held poll. Independently, a certificate whose domains +were all evicted is never deleted before its own expiry: it stops renewing, +keeps serving, and if the domains return before it expires, recovery costs +zero new ACME orders. `--tls-domains-batch-size` (max 25) opts into stable SAN batching for dynamic domains: batches fill append-only, and membership only changes at renewal @@ -1059,8 +1073,8 @@ the app is down. **Inspecting:** ```bash -kamal-proxy domains list # every dynamic domain, cert + quarantine status -kamal-proxy domains stats # counters: domains, certified, queued, quarantined +kamal-proxy domains list # every dynamic domain, cert + quarantine + held status +kamal-proxy domains stats # counters: domains, certified, queued, quarantined, held kamal-proxy domains refresh # trigger an immediate re-poll of all sources ``` From 260ec165f35c10e75d62e80840d1ac0dc0418480 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 09:36:29 +0200 Subject: [PATCH 7/7] fix(san-cert): address PR #97 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Probe every non-trigger batch-mate, not just never-issued ones: an expiring host whose DNS moved away must not ride into the order on the strength of a certificate it once held. - Run pre-flight probes with bounded concurrency (16) everywhere — batch filtering, renewal member checks, and failure attribution — so a batch of unreachable domains costs a few probe timeouts, not minutes of a blocked handshake or issuance slot. - Clear the source's ETag while a shrink hold is active: an unchanged source would answer 304 forever and the confirmation count could never advance, leaving a legitimate mass removal held indefinitely. - Reset the shrink hold on redeploy so confirmations counted against a replaced source do not carry over to its successor. - Persist guard-driven quarantine mutations (new onChange hook wired to the dynamic-domains state save) and clear a batch's failure history on successful issuance, matching the dynamic issuer. - Skip quarantined domains during batch collection so they cannot consume the batch's slots away from eligible hosts. - CLI: constant-time held-removal lookup; README: correct the shrink-guard and eviction-grace claims (three confirmed polls do apply a removal; normal renewal rules still apply to recovered domains). Refs #97 --- README.md | 13 +-- internal/cmd/domains.go | 7 +- internal/server/domain_failure.go | 63 ++++++++++--- internal/server/domain_renewal.go | 16 ++-- internal/server/dynamic_domains.go | 13 ++- internal/server/dynamic_domains_test.go | 44 +++++++++ internal/server/san_cert_batch_guard.go | 83 +++++++++++------ internal/server/san_cert_batch_guard_test.go | 94 ++++++++++++++++++-- internal/server/san_cert_manager.go | 16 +++- 9 files changed, 286 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index e0cec547..7086d47b 100644 --- a/README.md +++ b/README.md @@ -1053,12 +1053,13 @@ domain, the members are probed to find the culprit before anyone is blamed. from the source itself. A poll that removes more than 30% of the applied domain set has its removals *held*: the previous set stays allowed (additions still apply), and only three consecutive shrunken polls confirm and apply the -removal — an empty or truncated response from the app evicts nothing. Held -removals are visible in `kamal-proxy domains list` (Removal held column) and -logged at Warn on every held poll. Independently, a certificate whose domains -were all evicted is never deleted before its own expiry: it stops renewing, -keeps serving, and if the domains return before it expires, recovery costs -zero new ACME orders. +removal — a single (or transient) empty or truncated response from the app +evicts nothing. Held removals are visible in `kamal-proxy domains list` +(Removal held column) and logged at Warn on every held poll. Independently, a +certificate whose domains were all evicted is never deleted before its own +expiry: it stops renewing but keeps serving, and can be reused immediately if +the domains return before it expires — normal replacement and renewal rules +still apply. `--tls-domains-batch-size` (max 25) opts into stable SAN batching for dynamic domains: batches fill append-only, and membership only changes at renewal diff --git a/internal/cmd/domains.go b/internal/cmd/domains.go index 1584f6ea..b89a64dd 100644 --- a/internal/cmd/domains.go +++ b/internal/cmd/domains.go @@ -72,6 +72,11 @@ func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { return strings.Compare(a.Domain, b.Domain) }) + heldRemovals := make(map[string]struct{}, len(service.HeldRemovals)) + for _, domain := range service.HeldRemovals { + heldRemovals[domain] = struct{}{} + } + for _, domain := range domains { certified := "no" if domain.Certified { @@ -84,7 +89,7 @@ func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { } held := "" - if slices.Contains(service.HeldRemovals, domain.Domain) { + if _, ok := heldRemovals[domain.Domain]; ok { held = "yes" } diff --git a/internal/server/domain_failure.go b/internal/server/domain_failure.go index 37f916c5..fa6bfba2 100644 --- a/internal/server/domain_failure.go +++ b/internal/server/domain_failure.go @@ -1,10 +1,19 @@ package server -import "strings" +import ( + "strings" + "sync" +) // Attribution of failed ACME orders to the domains that caused them, shared by // the dynamic issuer and the background renewer. +// maxConcurrentProbes bounds parallel pre-flight probes. Each probe can take +// up to preflightTimeout, so a serial sweep over a large batch would block an +// issuance slot (or a waiting handshake) for minutes; concurrency keeps the +// worst case to a few probe timeouts. +const maxConcurrentProbes = 16 + // identifyFailedDomains names the domains responsible for a failed order: // lego's per-domain error lines when present, a pre-flight probe of each // member otherwise, and the whole set when neither can tell — an @@ -15,24 +24,50 @@ func identifyFailedDomains(err error, domains []string, preflight func(string) e return failed } - if preflight != nil { - failed := []string{} - for _, domain := range domains { - if strings.HasPrefix(domain, "*.") { - continue - } - if probeErr := preflight(domain); probeErr != nil { - failed = append(failed, domain) - } - } - if len(failed) > 0 { - return failed - } + if failed, _ := probeDomains(domains, preflight); len(failed) > 0 { + return failed } return domains } +// probeDomains runs the pre-flight probe over a set of domains with bounded +// concurrency and returns the ones that failed, in input order, with each +// failure's error. Wildcards are skipped — there is no name to answer on one. +// A nil probe reports nothing. +func probeDomains(domains []string, preflight func(string) error) ([]string, map[string]error) { + if preflight == nil { + return nil, nil + } + + errs := make([]error, len(domains)) + sem := make(chan struct{}, maxConcurrentProbes) + var wg sync.WaitGroup + for idx, domain := range domains { + if strings.HasPrefix(domain, "*.") { + continue + } + wg.Add(1) + sem <- struct{}{} + go func(idx int, domain string) { + defer wg.Done() + defer func() { <-sem }() + errs[idx] = preflight(domain) + }(idx, domain) + } + wg.Wait() + + failed := []string{} + failures := map[string]error{} + for idx, domain := range domains { + if errs[idx] != nil { + failed = append(failed, domain) + failures[domain] = errs[idx] + } + } + return failed, failures +} + // failedDomainsFromError matches lego's per-domain error lines // (": ") against the attempted domains. func failedDomainsFromError(err error, domains []string) []string { diff --git a/internal/server/domain_renewal.go b/internal/server/domain_renewal.go index 30a3913f..4c13e98e 100644 --- a/internal/server/domain_renewal.go +++ b/internal/server/domain_renewal.go @@ -400,7 +400,7 @@ func (r *certRenewer) preflightMembers(domains []string) []string { return nil } - unreachable := []string{} + probeable := []string{} for _, domain := range domains { if strings.HasPrefix(domain, "*.") { continue @@ -408,12 +408,14 @@ func (r *certRenewer) preflightMembers(domains []string) []string { if _, dynamic := r.manager.dynamicOwner(domain); !dynamic { continue } - if err := r.config.Preflight(domain); err != nil { - backoff := r.quarantine.RecordFailure(domain, quarantinePreflight) - slog.Warn("Renewal member failed pre-flight probe; holding back", - "domain", domain, "backoff", backoff, "error", err) - unreachable = append(unreachable, domain) - } + probeable = append(probeable, domain) + } + + unreachable, failures := probeDomains(probeable, r.config.Preflight) + for _, domain := range unreachable { + backoff := r.quarantine.RecordFailure(domain, quarantinePreflight) + slog.Warn("Renewal member failed pre-flight probe; holding back", + "domain", domain, "backoff", backoff, "error", failures[domain]) } return unreachable } diff --git a/internal/server/dynamic_domains.go b/internal/server/dynamic_domains.go index edc24253..525263a4 100644 --- a/internal/server/dynamic_domains.go +++ b/internal/server/dynamic_domains.go @@ -138,7 +138,7 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager }) manager.SetDynamicCertRequester(dm.issuer.Request) - manager.SetIssuanceGuard(dm.preflightProbe, dm.quarantine) + manager.SetIssuanceGuard(dm.preflightProbe, dm.quarantine, dm.saveState) dm.loadState() @@ -194,6 +194,10 @@ func (dm *DynamicDomainManager) ServiceDeployed(name string, options ServiceOpti host: host, } + // Confirmations counted against the old source must not carry over to the + // new one: a redeploy's first shrunken poll starts the count fresh. + delete(dm.holds, name) + state := dm.states[name] if state == nil { state = &serviceDomainState{Domains: []string{}} @@ -370,6 +374,13 @@ func (dm *DynamicDomainManager) applyDomains(service string, domains []string) { // Keep the previous set alive alongside whatever the poll added; the // polled set replaces it only once the shrink is confirmed. applying = append(append([]string{}, previous...), added...) + + // A held response must not park behind its own ETag: an unchanged + // source would answer 304 forever, the body would never be re-fetched, + // and the confirmation count could never advance. + if source := dm.sources[service]; source != nil { + source.SeedETag("") + } } etag := "" diff --git a/internal/server/dynamic_domains_test.go b/internal/server/dynamic_domains_test.go index f62f1343..114b426b 100644 --- a/internal/server/dynamic_domains_test.go +++ b/internal/server/dynamic_domains_test.go @@ -320,6 +320,50 @@ func TestDynamicDomainManager_ShrinkGuardConfirmsAfterConsecutivePolls(t *testin assert.Empty(t, dm.Status().Services["service1"].HeldRemovals) } +func TestDynamicDomainManager_ShrinkGuardClearsETagWhileHolding(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + // An ETag-answering source would 304 every poll after the held response, + // and the confirmation count could never advance. Holding must drop the + // ETag so the body keeps being refetched until the shrink resolves. + dm.mu.Lock() + dm.sources["service1"].SeedETag(`"v2"`) + dm.mu.Unlock() + + dm.applyDomains("service1", full[:4]) + + dm.mu.Lock() + etag := dm.sources["service1"].ETag() + stored := dm.states["service1"].ETag + dm.mu.Unlock() + assert.Empty(t, etag) + assert.Empty(t, stored) +} + +func TestDynamicDomainManager_ShrinkGuardResetsOnRedeploy(t *testing.T) { + dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) + full := testTenantDomains(10) + deployWithDomains(t, dm, manager, "service1", full) + + dm.applyDomains("service1", full[:4]) + dm.mu.Lock() + _, held := dm.holds["service1"] + dm.mu.Unlock() + require.True(t, held) + + // A redeploy replaces the source; confirmations counted against the old + // source must not carry over to the new one. + backend, _ := testDomainsBackend(t, full...) + dm.ServiceDeployed("service1", ServiceOptions{TLSEnabled: true, TLSDomainsSource: backend.URL}) + + dm.mu.Lock() + _, held = dm.holds["service1"] + dm.mu.Unlock() + assert.False(t, held) +} + func TestDynamicDomainManager_ShrinkGuardCancelsOnRecovery(t *testing.T) { dm, manager := testDynamicDomainManager(t, DynamicDomainConfig{}) full := testTenantDomains(10) diff --git a/internal/server/san_cert_batch_guard.go b/internal/server/san_cert_batch_guard.go index 0e71e55f..8823c8b3 100644 --- a/internal/server/san_cert_batch_guard.go +++ b/internal/server/san_cert_batch_guard.go @@ -2,6 +2,7 @@ package server import ( "log/slog" + "slices" ) // Guarding of the handshake-driven provisioning batch. @@ -18,16 +19,23 @@ import ( type issuanceGuard struct { preflight func(domain string) error quarantine *domainQuarantine + onChange func() } -// SetIssuanceGuard installs the preflight probe and quarantine that guard -// handshake-driven batches. NewDynamicDomainManager installs it at boot; -// without it, provisioning batches behave as before. -func (m *SANCertManager) SetIssuanceGuard(preflight func(domain string) error, quarantine *domainQuarantine) { +func (g issuanceGuard) notifyChange() { + if g.onChange != nil { + g.onChange() + } +} + +// SetIssuanceGuard installs the preflight probe, quarantine, and persistence +// callback that guard handshake-driven batches. NewDynamicDomainManager +// installs it at boot; without it, provisioning batches behave as before. +func (m *SANCertManager) SetIssuanceGuard(preflight func(domain string) error, quarantine *domainQuarantine, onChange func()) { m.mu.Lock() defer m.mu.Unlock() - m.guard = issuanceGuard{preflight: preflight, quarantine: quarantine} + m.guard = issuanceGuard{preflight: preflight, quarantine: quarantine, onChange: onChange} } func (m *SANCertManager) issuanceGuardSnapshot() issuanceGuard { @@ -38,39 +46,64 @@ func (m *SANCertManager) issuanceGuardSnapshot() issuanceGuard { } // filterBatchMates drops quarantined or unreachable domains from a handshake -// batch, quarantining fresh probe failures. The triggering domain is never +// batch, quarantining fresh probe failures. Every mate is probed, even one +// that held a certificate before — an expiring host whose DNS moved away must +// not ride into the order on its history. The triggering domain is never // dropped: its handshake is why we are here, and if it is the problem, the // order failure will be attributed to it. Callers must NOT hold m.mu — the -// probe does network I/O. +// probes do network I/O. func (m *SANCertManager) filterBatchMates(trigger string, domains []string) []string { guard := m.issuanceGuardSnapshot() if guard.quarantine == nil { return domains } - kept := make([]string, 0, len(domains)) + mates := make([]string, 0, len(domains)) for _, domain := range domains { if domain == trigger { - kept = append(kept, domain) continue } if guard.quarantine.IsQuarantined(domain) { slog.Debug("Leaving quarantined domain out of handshake batch", "domain", domain) continue } - if guard.preflight != nil && !m.HasCertificate(domain) { - if err := guard.preflight(domain); err != nil { - backoff := guard.quarantine.RecordFailure(domain, quarantinePreflight) - slog.Warn("Batch domain failed pre-flight probe; holding back", - "domain", domain, "backoff", backoff, "error", err) - continue - } + mates = append(mates, domain) + } + + unreachable, failures := probeDomains(mates, guard.preflight) + for _, domain := range unreachable { + backoff := guard.quarantine.RecordFailure(domain, quarantinePreflight) + slog.Warn("Batch domain failed pre-flight probe; holding back", + "domain", domain, "backoff", backoff, "error", failures[domain]) + } + if len(unreachable) > 0 { + guard.notifyChange() + } + + kept := []string{trigger} + for _, domain := range mates { + if !slices.Contains(unreachable, domain) { + kept = append(kept, domain) } - kept = append(kept, domain) } return kept } +// clearBatchQuarantine wipes the failure history of a successfully issued +// batch, exactly as the dynamic issuer does — otherwise a domain's next +// failure would start higher up the backoff ladder than it deserves. +func (m *SANCertManager) clearBatchQuarantine(domains []string) { + guard := m.issuanceGuardSnapshot() + if guard.quarantine == nil { + return + } + + for _, domain := range domains { + guard.quarantine.Clear(domain) + } + guard.notifyChange() +} + // attributeBatchFailure quarantines the culprits of a failed handshake order // and returns the requested domains that should go back to pending. Only a // culprit narrower than the whole order is quarantined: a generic ACME outage @@ -84,15 +117,8 @@ func (m *SANCertManager) attributeBatchFailure(err error, ordered, requested []s } culprits := failedDomainsFromError(err, ordered) - if len(culprits) == 0 && guard.preflight != nil { - for _, identifier := range ordered { - if identifier[0] == '*' { - continue - } - if probeErr := guard.preflight(identifier); probeErr != nil { - culprits = append(culprits, identifier) - } - } + if len(culprits) == 0 { + culprits, _ = probeDomains(ordered, guard.preflight) } if len(culprits) == 0 { return requested @@ -101,14 +127,19 @@ func (m *SANCertManager) attributeBatchFailure(err error, ordered, requested []s // The order may contain planned identifiers (a wildcard collapsed from // siblings); blame the requested hosts a culprit identifier covers. survivors := []string{} + quarantined := false for _, domain := range requested { if identifiersCover(culprits, domain) { backoff := guard.quarantine.RecordFailure(domain, quarantineACME) slog.Warn("Quarantining culprit of failed handshake batch", "domain", domain, "backoff", backoff) + quarantined = true continue } survivors = append(survivors, domain) } + if quarantined { + guard.notifyChange() + } return survivors } diff --git a/internal/server/san_cert_batch_guard_test.go b/internal/server/san_cert_batch_guard_test.go index 234a6d7c..d6dd2f0a 100644 --- a/internal/server/san_cert_batch_guard_test.go +++ b/internal/server/san_cert_batch_guard_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/go-acme/lego/v4/certificate" "github.com/stretchr/testify/assert" @@ -34,7 +35,7 @@ func pendingDomainsOf(manager *SANCertManager) []string { func TestBatchGuard_QuarantinedBatchMateIsSkippedButStaysPending(t *testing.T) { obtainer := successfulObtainer(t) manager, quarantine := testGuardedManager(t, obtainer) - manager.SetIssuanceGuard(nil, quarantine) + manager.SetIssuanceGuard(nil, quarantine, nil) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) require.NoError(t, manager.RegisterDomain("bad.example.com", "service1")) @@ -59,7 +60,7 @@ func TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder(t *te return errors.New("does not route here") } return nil - }, quarantine) + }, quarantine, nil) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) require.NoError(t, manager.RegisterDomain("dead.example.com", "service1")) @@ -75,12 +76,95 @@ func TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder(t *te assert.Contains(t, pendingDomainsOf(manager), "dead.example.com") } +func TestBatchGuard_ExpiringBatchMateIsProbedAndExcludedWhenUnreachable(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(func(domain string) error { + if domain == "dead.example.com" { + return errors.New("does not route here") + } + return nil + }, quarantine, nil) + + // dead.example.com held a certificate once, but it is expiring and its + // DNS moved away — having been issued before must not exempt it from the + // probe, or it poisons the trigger's order. + adoptTestCert(t, manager, []string{"dead.example.com"}, + time.Now().Add(-89*24*time.Hour), time.Now().Add(time.Hour)) + require.NoError(t, manager.RegisterDomain("dead.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) + assert.True(t, quarantine.IsQuarantined("dead.example.com")) +} + +func TestBatchGuard_SuccessfulBatchClearsQuarantineHistory(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(nil, quarantine, nil) + + // The trigger carries failure history; a successful order must wipe it, + // or its next failure starts higher up the backoff ladder. + quarantine.RecordFailure("app.example.com", quarantineACME) + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + assert.Equal(t, 0, quarantine.Len()) +} + +func TestBatchGuard_MutationsNotifyChangeForPersistence(t *testing.T) { + failing := &fakeObtainer{respond: func(request certificate.ObtainRequest) (*certificate.Resource, error) { + return nil, fmt.Errorf("error: one or more domains had a problem:\nbad.example.com: acme: error presenting token") + }} + manager, quarantine := testGuardedManager(t, failing) + + changes := 0 + manager.SetIssuanceGuard(nil, quarantine, func() { changes++ }) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("bad.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.Error(t, err) + assert.Greater(t, changes, 0, "quarantining a culprit must notify for persistence") +} + +func TestBatchGuard_QuarantinedDomainsDoNotConsumeBatchSlots(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(nil, quarantine, nil) + + // More quarantined hosts than a batch holds: the eligible mate must still + // find a slot instead of the quarantined ones filling the batch first. + for i := 0; i < MaxSANsPerCertificate+10; i++ { + domain := fmt.Sprintf("quarantined-%d.example.com", i) + require.NoError(t, manager.RegisterDomain(domain, "service1")) + quarantine.RecordFailure(domain, quarantineACME) + } + require.NoError(t, manager.RegisterDomain("ok.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.ElementsMatch(t, []string{"app.example.com", "ok.example.com"}, calls[0].Domains) +} + func TestBatchGuard_TriggerDomainIsNeverDropped(t *testing.T) { obtainer := successfulObtainer(t) manager, quarantine := testGuardedManager(t, obtainer) // The probe fails everything, and the trigger is even quarantined — its // handshake still gets its shot. - manager.SetIssuanceGuard(func(domain string) error { return errors.New("unreachable") }, quarantine) + manager.SetIssuanceGuard(func(domain string) error { return errors.New("unreachable") }, quarantine, nil) quarantine.RecordFailure("app.example.com", quarantineACME) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) @@ -98,7 +182,7 @@ func TestBatchGuard_QuarantinesCulpritsAndRestoresSurvivorsOnFailure(t *testing. return nil, fmt.Errorf("error: one or more domains had a problem:\nbad.example.com: acme: error presenting token") }} manager, quarantine := testGuardedManager(t, obtainer) - manager.SetIssuanceGuard(nil, quarantine) + manager.SetIssuanceGuard(nil, quarantine, nil) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) require.NoError(t, manager.RegisterDomain("bad.example.com", "service1")) @@ -123,7 +207,7 @@ func TestBatchGuard_UnattributableFailureRestoresEverythingUnquarantined(t *test manager, quarantine := testGuardedManager(t, obtainer) // A generic ACME outage must not push deploy-registered hosts onto the // quarantine ladder; the probe passing everyone proves no culprit. - manager.SetIssuanceGuard(func(domain string) error { return nil }, quarantine) + manager.SetIssuanceGuard(func(domain string) error { return nil }, quarantine, nil) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) require.NoError(t, manager.RegisterDomain("other.example.com", "service1")) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 793237f0..2bfcb2df 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -489,12 +489,18 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string done := make(chan struct{}) m.provisioning[provisioningKey] = done - // Collect ALL pending domains (up to MaxSANsPerCertificate) + // Collect ALL pending domains (up to MaxSANsPerCertificate). Quarantined + // domains do not consume batch slots: with more quarantined hosts than a + // batch holds, the eligible ones must still fit. candidates := []string{domain} for pendingDomain := range m.pendingDomains { - if pendingDomain != domain { - candidates = append(candidates, pendingDomain) + if pendingDomain == domain { + continue + } + if m.guard.quarantine != nil && m.guard.quarantine.IsQuarantined(pendingDomain) { + continue } + candidates = append(candidates, pendingDomain) if len(candidates) >= MaxSANsPerCertificate { break } @@ -578,6 +584,10 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string return nil, err } + // A successful order wipes the batch's failure history, as the dynamic + // issuer does — the next failure must not start high on the ladder. + m.clearBatchQuarantine(domainsToProvision) + return managed.Certificate, nil }