Skip to content

A 429 with no Retry-After teaches the limiter nothing #10

Description

@sshine

From docs/FINDINGS.md. Found by running the Go provider this replaces, and correlated
against this codebase rather than acted on. Sources: external-dns-desec-provider#26,
a year of production fixes, its review, and sshine/external-dns-desec-provider#2,
another operator's account of the same failure from a four-cluster deployment.
This is not a decision.

The mechanism

desec-rs, ratelimit.rs:

pub(crate) fn record_throttled(&self, scopes: &ScopeSet, retry_after: Option<Duration>) {
    let Some(retry_after) = retry_after else {
        return;
    };
    ...
}

No header, no penalty. The sliding windows are untouched too, because they record grants and a
throttled request was never granted. So a millisecond after a bare 429, the limiter's model of the
account is exactly what it was a millisecond before: admissible.

Our classify covers for this on the wire, but only there:

desec::Error::RateLimited { retry_after, .. } => WebhookError::unavailable(
    "deSEC returned 429 Too Many Requests",
    retry_after.unwrap_or(Duration::from_secs(60)),
),

That fills in the Retry-After we send external-dns. It does not reach the limiter, and per #1
external-dns retries on its own --interval regardless of what that header says. So the next
reconcile's POST /records goes straight to the wire and earns another 429, and so does the one
after it.

CodeRabbit found this in the Go provider's transport and proposed recording a conservative default
in the missing-header branch. The same one-line shape applies here, except it belongs upstream in
desec-rs.

How likely it is

Not very, and that is the interesting part. deSEC runs Django REST Framework throttles, which
always set Retry-After from wait(). The realistic sources of a bare 429 are all
intermediaries: a corporate proxy, a CDN in front of a self-hosted deSEC reached via
--api-url, a service-mesh limiter, or deSEC's own front end shedding load before the application
sees the request.

Which is to say: the case where the client most needs to back off unprompted is the one case where
it doesn't.

Where we stand

Unhandled. The remedy is upstream and small, and the only argument against it is that a penalty
invented from nothing is a guess. max_rate_limit_wait already bounds how wrong that guess can be
(#11), which makes it a cheap one.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingfindingRecorded in docs/FINDINGS.mdupstreamNeeds or would be better served by a change in desec-rs

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions