diff --git a/README.md b/README.md index 58e885f..7086d47 100644 --- a/README.md +++ b/README.md @@ -1044,7 +1044,22 @@ 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 — 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 @@ -1059,8 +1074,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 ``` diff --git a/internal/cmd/domains.go b/internal/cmd/domains.go index 418afd2..b89a64d 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] @@ -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 { @@ -83,7 +88,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 _, ok := heldRemovals[domain.Domain]; ok { + held = "yes" + } + + table.AddRow([]string{name, domain.Domain, certified, quarantined, held}) } } @@ -111,8 +121,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 +137,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 b8fbee4..1fb9fbc 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/domain_failure.go b/internal/server/domain_failure.go new file mode 100644 index 0000000..fa6bfba --- /dev/null +++ b/internal/server/domain_failure.go @@ -0,0 +1,87 @@ +package server + +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 +// 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 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 { + 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 0000000..5282863 --- /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 6741b34..b808b7e 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 1c57c13..76cc3f4 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 9d7896f..4c13e98 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" @@ -140,12 +141,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 +184,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 } @@ -212,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 { @@ -294,6 +309,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 @@ -347,6 +390,36 @@ 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 + } + + probeable := []string{} + for _, domain := range domains { + if strings.HasPrefix(domain, "*.") { + continue + } + if _, dynamic := r.manager.dynamicOwner(domain); !dynamic { + continue + } + 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 +} + func (r *certRenewer) dynamicServiceFor(domains []string) (string, bool) { for _, domain := range domains { if service, ok := r.manager.dynamicOwner(domain); ok { @@ -363,7 +436,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) @@ -398,6 +484,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 d3e8ddb..0bc2b79 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) @@ -237,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() @@ -248,14 +313,137 @@ 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_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) 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 +455,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) { diff --git a/internal/server/dynamic_domains.go b/internal/server/dynamic_domains.go index 3d6d664..525263a 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, @@ -126,6 +138,7 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager }) manager.SetDynamicCertRequester(dm.issuer.Request) + manager.SetIssuanceGuard(dm.preflightProbe, dm.quarantine, dm.saveState) dm.loadState() @@ -181,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{}} @@ -227,6 +244,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 +296,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 +317,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 +341,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 +366,57 @@ 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...) + + // 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 := "" 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, domains) + dm.manager.SetDynamicDomains(service, applying) - 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 +424,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 b623858..114b426 100644 --- a/internal/server/dynamic_domains_test.go +++ b/internal/server/dynamic_domains_test.go @@ -239,6 +239,152 @@ 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_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) + 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{}) diff --git a/internal/server/san_cert_batch_guard.go b/internal/server/san_cert_batch_guard.go new file mode 100644 index 0000000..8823c8b --- /dev/null +++ b/internal/server/san_cert_batch_guard.go @@ -0,0 +1,145 @@ +package server + +import ( + "log/slog" + "slices" +) + +// 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 + onChange func() +} + +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, onChange: onChange} +} + +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. 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 +// probes do network I/O. +func (m *SANCertManager) filterBatchMates(trigger string, domains []string) []string { + guard := m.issuanceGuardSnapshot() + if guard.quarantine == nil { + return domains + } + + mates := make([]string, 0, len(domains)) + for _, domain := range domains { + if domain == trigger { + continue + } + if guard.quarantine.IsQuarantined(domain) { + slog.Debug("Leaving quarantined domain out of handshake batch", "domain", domain) + 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) + } + } + 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 +// 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 { + culprits, _ = probeDomains(ordered, guard.preflight) + } + 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{} + 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 new file mode 100644 index 0000000..d6dd2f0 --- /dev/null +++ b/internal/server/san_cert_batch_guard_test.go @@ -0,0 +1,239 @@ +package server + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "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, nil) + + 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, nil) + + 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_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, nil) + 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, nil) + + 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, nil) + + 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 a7f7479..2bfcb2d 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,28 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string } } - // Collect ALL pending domains (up to MaxSANsPerCertificate) - domainsToProvision := []string{domain} + // 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). 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 { - domainsToProvision = append(domainsToProvision, pendingDomain) + if pendingDomain == domain { + continue + } + if m.guard.quarantine != nil && m.guard.quarantine.IsQuarantined(pendingDomain) { + continue } - if len(domainsToProvision) >= MaxSANsPerCertificate { + candidates = append(candidates, pendingDomain) + 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 +514,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 +573,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) } @@ -566,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 }