fix(EVBL-47): fail fast on terminal pending-cert failures to avoid CA rate limit - #368
Closed
evan-datadog wants to merge 12 commits into
Closed
fix(EVBL-47): fail fast on terminal pending-cert failures to avoid CA rate limit#368evan-datadog wants to merge 12 commits into
evan-datadog wants to merge 12 commits into
Conversation
… rate limit A pending certificate that fails with a terminal error (broken DNS delegation, no DNS provider, or invalid ACME credentials) was re-queued by fetch_acme_cert up to ACME_ADDITIONAL_ATTEMPTS times, each retry creating a new ACME order and re-running the DNS-01 challenge against the CA. For deterministic config/delegation failures this can never succeed and only burns the CA's rate limit (Let's Encrypt: 5 duplicate certificates / failed validations per week per domain). Add pending_certificate_service.is_terminal_failure() and use it in fetch_acme_cert (celery) and the pending-cert CLI to mark such pending certificates resolved immediately instead of re-queueing, while keeping bounded retries for genuinely transient failures (e.g. DNS propagation). Workspace: local
…li cert.id, add e2e tests Review-swarm findings addressed: - is_terminal_failure now matches markers case-insensitively (e.g. UNAUTHORIZED). - cli.py used cert.id on a dict (AttributeError); switch to pending_cert.id in both the terminal and attempts-exhausted branches, consistent with celery.py. - Add end-to-end tests for fetch_acme_cert: terminal failure marks resolved and does not re-queue; transient failure increments attempts and re-queues. Workspace: local
Protocol: deploy to staging, create a pending cert that fails terminally (no DNS provider / broken delegation), and confirm in the logs it is attempted only once and marked resolved (no re-queue / no repeated ACME order), plus a negative control that transient failures still retry. Workspace: local
|
g.current_user can be a detached/expired SQLAlchemy User in some contexts (e.g. tests), so accessing .email raises DetachedInstanceError. Fall back to the anonymous LEMUR label instead of crashing. Fixes flaky CI failures in test_pending_certificates.py. Workspace: local
This reverts commit c8fbcf5. Workspace: local
… flakiness g is app-context scoped and persists across tests, so a User set by an authenticated request in one test leaks into later tests as a stale, detached (and expired) instance. Accessing its attributes then raises DetachedInstanceError in audit_log, certificate schema serialization, etc. Clear g.current_user in the session fixture teardown so each test starts clean. Workspace: local
maperu
reviewed
Aug 17, 2026
| # re-runs the ACME order/challenge and burns the CA's rate limit (Let's Encrypt: | ||
| # 5 duplicate certificates / failed validations per week per domain), so they must | ||
| # fail fast and mark the pending certificate resolved instead of retrying. | ||
| _TERMINAL_FAILURE_MARKERS = ( |
There was a problem hiding this comment.
Could we model terminal failures with explicit exception types raised at the ACME/DNS boundaries? get_ordered_certificates should preserve the exception object, and the retry code can use isinstance(error, PendingCertificateTerminalError). For upstream HTTP errors, we can translate structured 401/403 status codes into an ACMEAuthenticationError. This avoids treating mutable exception text as an API and makes adding new terminal cases intentional and testable.
maperu
reviewed
Aug 17, 2026
| # 5 duplicate certificates / failed validations per week per domain), so they must | ||
| # fail fast and mark the pending certificate resolved instead of retrying. | ||
| _TERMINAL_FAILURE_MARKERS = ( | ||
| "no dns providers found for domain", |
There was a problem hiding this comment.
are you sure that's always terminal, what about delegated CNAMES
Author
There was a problem hiding this comment.
If there is no delegated CNAME we still want to fail fast instead of retrying over and over and exausting the rate limit
…ion types Replace string-matching on mutable exception text with an explicit exception hierarchy raised at the ACME/DNS boundaries: - PendingCertificateTerminalError base + ACMEAuthenticationError, NoDNSProviderError, DNSChallengeSetupError subclasses (InvalidConfiguration re-parented under the base). - acme_handlers raises NoDNSProviderError / DNSChallengeSetupError instead of bare Exception at the DNS-01 boundary. - get_ordered_certificates preserves the exception object in last_error and classifies it via _classify_pending_error, translating upstream HTTP 401/403 into ACMEAuthenticationError (requests.HTError / botocore ClientError / acme.messages.Error status extraction). - fetch_acme_cert / cli use isinstance(last_error, PendingCertificateTerminalError) for the fail-fast decision. is_terminal_failure keeps the string-marker fallback only for pre-existing stored status text. Adding a new terminal case is now a subclass + raise at the boundary, intentional and unit-testable, instead of a brittle substring match. Workspace: local
maperu
reviewed
Aug 17, 2026
|
|
||
| import lemur.common.celery as _celery_module # noqa: E402 | ||
|
|
||
| _celery_module.current_app = MagicMock() |
There was a problem hiding this comment.
This globally replaces lemur.common.celery.current_app and never restores it
Author
There was a problem hiding this comment.
Fixed with _mock_celery_current_app
maperu
requested changes
Aug 17, 2026
…ore-notify, test fixture - get_authorizations: when ACME_ENABLE_DELEGATED_CNAME is on but the _acme-challenge CNAME isnt resolving yet, raise a transient ValueError instead of a terminal NoDNSProviderError, so DNS-propagation delays dont burn the CA rate limit. - fetch_acme_cert / cli: persist status=str(last_error) and resolved=True BEFORE sending the failure notification, so a notification-delivery error cant leave the pending cert unresolved (and retried). - test_pending_cert_retry: replace the module-scope current_app mock with an autouse fixture that restores it on teardown; add persist-before-notify and typed-error tests. - test_acme_dns: cover delegated-CNAME-pending (transient) vs no-provider (terminal). Workspace: local
…thing deployed) _TERMINAL_FAILURE_MARKERS was introduced by this PR, so there is no deployed state to be backward-compatible with. Remove the string-matching fallback and classify terminal failures purely by exception type: - is_terminal_failure is now a thin isinstance(error, PendingCertificateTerminalError). - fetch_acme_cert / cli use is_terminal_failure as the single source of truth (no isinstance-or-string fallback). - Update tests to use the typed exceptions (NoDNSProviderError, DNSChallengeSetupError, ACMEAuthenticationError) instead of terminal-looking plain Exception strings. - Remove the now-obsolete case-insensitive string-marker test. Workspace: local
…ted CNAME Revert the earlier transient ValueError for a not-y-resolving delegated CNAME. If there is no delegated DNS provider (the CNAME does not exist), retrying can never succeed and only re-exhausts the CA rate limit, so it must fail fast as a terminal NoDNSProviderError. The delegated-CNAME path already resolves the provider on the CNAME target when the CNAME exists; the no-provider case is terminal in all configurations. Update test_get_authorizations_delegated_cname_no_provider_terminal to assert NoDNSProviderError. Workspace: local
…tal attempts The bounded retry previously ran ACME_ADDITIONAL_ATTEMPTS + 1 times (4 total attempts with the default of 2), because the give-up condition was number_attempts > ACME_ADDITIONAL_ATTEMPTS. Each retry re-runs the ACME order/challenge, so a persistently-failing cert burned nearly the whole Let's Encrypt rate limit (5 duplicate certs / failed validations per week). Change the give-up condition to number_attempts >= ACME_ADDITIONAL_ATTEMPTS, so a cert is attempted ACME_ADDITIONAL_ATTEMPTS + 1 times total (3 with the default) — the initial attempt plus ACME_ADDITIONAL_ATTEMPTS retries. This also makes the constant's name match its behavior (additional attempts beyond the initial one). Workspace: local
evan-datadog
added a commit
that referenced
this pull request
Aug 18, 2026
… flakiness g is app-context scoped and persists across tests, so a User set by an authenticated request in one test leaks into later tests as a stale, detached (and expired) instance. Accessing its attributes then raises DetachedInstanceError in audit_log / certificate schema serialization. Clear g.current_user in the session fixture teardown so each test starts clean. This is needed for CI to pass reliably on master-based branches (same fix as PR #368). Workspace: local
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
2 participants
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.
Problem
When
dc bootissues a create-cert request, Lemur creates a PendingCertificate and auto-triggersfetch_acme_cert. On failure,fetch_acme_certre-queues itself (up toACME_ADDITIONAL_ATTEMPTStimes), and each retry creates a new ACME order and re-runs the DNS-01 challenge against the CA.For a deterministic failure — broken DNS delegation, no DNS provider configured, or invalid ACME credentials — retrying can never succeed. It just keeps creating orders / failed validations until it exhausts the CA's rate limit (Let's Encrypt: 5 duplicate certificates / failed validations per week per domain). What should be a single failure becomes repeated issuance attempts that block the DC from ever getting its certs.
Fix
Add
pending_certificate_service.is_terminal_failure(error)that classifies configuration / DNS-delegation / credential failures as terminal, and use it in both the celery task (fetch_acme_cert) and the pending-cert CLI to mark the pending certificate resolved immediately instead of re-queueing.InvalidConfiguration, "No DNS providers found for domain", "Unable to determine DNS challenges", auth/credential errors (unauthorized / authentication / invalid account / 401 / 403).Tests
test_pending_certificates.py—is_terminal_failurecovers:InvalidConfiguration, no-DNS-provider, no-DNS-challenges, auth (401/403/unauthorized) → True; transient (Failed verification / order not ready) → False.Notes