feat(san-cert): SAN batch resilience — eviction grace, shrink guard, renewal preflight, batch quarantine - #97
Merged
Conversation
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
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
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
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
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
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the three gaps that let a dead tenant domain or a bad domain-source poll degrade or destroy SAN certificate coverage (the WM3 scenario: ~1100 tenant domains, many dead). No new flags; no RPC verbs; behavior-only.
internal/server/domain_renewal.go) — a certificate whose domains were all evicted is never deleted before its ownNotAfter: it stops renewing and keeps serving. Only a superseded cert (no domain maps through it anymore — a loaded replacement answers every handshake) is removed immediately. A bad poll + reconcile + recovery now costs zero new ACME orders. The per-domain expiry gauge is reported only by the cert a domain currently maps to, so lingering certs can't clobber their successor's metric.internal/server/dynamic_domains.go) — a poll removing more than 30% of a service's applied domain set has its removals held (additions still apply) until 3 consecutive over-threshold polls confirm the shrink; a recovering poll clears the hold. Held domains keep serving existing certs but earn no new orders. Visible inkamal-proxy domains list(Removal held column) anddomains stats; every held poll logs at Warn.internal/server/domain_renewal.go) — every renewal probes its dynamic members before spending an order; unreachable ones follow the existing quarantine policy (defer while there's time, compact inside the 7-day window). Deploy-registered hosts are not probed (may be DNS-01-only by design); wildcards have nothing to answer on.internal/server/domain_failure.go) —identifyFailedDomains: parse lego's per-domain error lines; failing that, probe the members; failing that, hold the whole batch on the quarantine ladder. Fixes the renewer's quarantine-nobody hole (a doomed batch used to retry hourly, unbacked-off, until the whole cert expired) and gives the issuer culprit-aware survivor retries.internal/server/san_cert_batch_guard.go) —provisionCertificatefilters batch-mates through quarantine + preflight (they keep their pending slot), and attributes order failures: culprits quarantined, survivors restored. The triggering domain is never dropped. Wired viaSetIssuanceGuardfromNewDynamicDomainManagerat boot.Closes #96
Test plan
make test— 1768 server-package tests green (21 new)go test -race ./internal/server/— cleanmake lint— 0 issues (golangci-lint v2.11.3),gofmt -lempty,go vetcleanmake docker && docker run --rm kamal-proxy kamal-proxy -h— image smoke testTestCertRenewer_EvictedCertificateSurvivesSourceRecovery,TestDynamicDomainManager_ShrinkGuardHoldsEmptyPoll)--tls-domains-sourceat an endpoint, return an empty list once, confirmdomains listshows held removals and no cert files disappear from the storeDeviations & judgment calls
certRenewer.renewnow splits a renewal into one order per provider partition (renewPartition), andprovisionCertificatenarrows a handshake batch to one partition. The grace/probe logic wraps around that structure rather than the single-order flow the plan described.*.guard is kept as belt-and-braces and unit-tested inidentifyFailedDomainsinstead.provisionCertificateused to collect the batch and mark it non-pending under one lock. The guard's probes do network I/O, so the provisioning slot is now claimed first (concurrent handshakes wait on it), then probes run unlocked, then the kept domains leave pending. No new race: the slot was always the mutual exclusion.failedDomainsFromErrorfromdomain_issuer.gointo the newdomain_failure.goalongsideidentifyFailedDomains(shared by issuer + renewer + batch guard).Summary by cubic
Improves SAN certificate resilience: prevents bad domain polls and failing tenants from tearing down coverage, and reduces wasted ACME orders with preflight checks and smarter failure handling. Addresses #96.
New Features
kamal-proxy domains list(Removal held) anddomains stats. While holding, the source ETag is cleared to avoid 304 loops; the hold resets on redeploy.SetIssuanceGuard.Bug Fixes
domains listshows “Removal held”;domains statscounts held removals.Written for commit 260ec16. Summary will update on new commits.