feat(rate_limit): add ban_for and Limiter#clear

Nidhey Indurkarrequested to merge
nindurkar/rate-limit-ban-action into master

What

Adds ban_for to Rule and #clear to Limiter.

A rule carrying ban_for counts as usual, and on crossing its limit writes a second key that keeps blocking after the counting window has expired. While that ban holds the rule stops counting, so a caller cannot extend its own ban. Limiter#clear(identifier) discards a limiter's counters and bans for one identifier.

Implements the semantics documented in handbook!20850, merged today.

Why

Groundwork for gitlab-com/gl-infra/production-engineering#29557, replacing Rack::Attack::Allow2Ban, which guards Git HTTP and container registry authentication. Counting alone cannot express it: a ban has to outlive its window, and a successful login has to discard the state early.

A ban is not an action

action decides who is blocked; ban_for decides the accounting. So the two compose:

  • action: :limit with ban_for enforces the ban
  • action: :log with ban_for does identical accounting, including writing the ban and suppressing counting while it holds, and only skips the blocking

That second line is the point of the shape @reprazent suggested during design review. A shadow measures what enforcement would have produced rather than approximating it, which is how :log already relates to :limit everywhere else.

Redis keys and cluster safety

BAN_SCRIPT is the first script here to touch two keys, and GitLab.com stores this data in a Redis Cluster, which rejects a script whose keys span slots. The limiter, rule and characteristics therefore sit inside a hash tag, with the suffix outside it:

labkit:rl:{git_basic_auth:failed_auth_ban_by_ip:ip:1.2.3.4}
labkit:rl:{git_basic_auth:failed_auth_ban_by_ip:ip:1.2.3.4}:ban

Both hash on the identical braced portion, so they always land on the same shard.

This changes the shape of every key, not only ban keys, because the counter has to carry the tag for the ban to share its slot. Two consequences:

  • in-flight counters are abandoned on deploy, which is one window of under-counting (60s for the git throttles)
  • anything matching labkit:rl:<limiter>:* needs labkit:rl:{<limiter>:*

Metrics

A blocking ban would otherwise be indistinguishable from an ordinary block, since the action is still limit. rule_evaluations_total now reports result="banned", which is where @reprazent preferred it over widening calls_total, and it fits the action/result split from labkit!343.

Design notes for review

  • An active ban short-circuits before the increment, matching Allow2Ban, whose filter returns early when banned.
  • One Lua script does the ban check, the increment and the ban write. Split across calls, a concurrent check can miss the threshold crossing. The ban check reads TTL alone: >= 0 means the key exists and gives the remaining time, so no separate EXISTS.
  • exceeded tracks the ban, not the count. Once the window expires the count can sit below the limit while the ban still holds.
  • reset_at is the ban expiry while banned, since that is when a retry is useful.
  • ban_for is rejected on :skip (nothing counted, so no limit to cross) and with count_distinct (the ban path counts with INCRBYFLOAT and never reaches the SADD branch, so the pairing silently counted the wrong thing).
  • clear is scoped to limiter plus identifier, not to a rule: a caller clearing after a success knows who succeeded, not which rules matched. It fails open like check.

Testing

bundle exec rake verify: 2002 examples, 0 failures, rubocop clean across 202 files.

Integration specs against real Redis cover the ban landing on crossing the limit, outliving the counter window, not counting while banned, leaving other identifiers alone, peek seeing the ban without writing, and clear removing both keys. The shadow has its own three: it reports the ban without blocking, suppresses counting exactly as enforcement would, and does not terminate evaluation. Both result="banned" cases are pinned.

Related

Edited by Nidhey Indurkar

Merge request reports