Instrument the git basic auth IP ban path

What

Adds gitlab_rate_limiter_git_basic_auth_ban_total, a counter on the Git and container registry authentication IP ban path, with an event label taking failure, ban, blocked, already_banned and reset.

Also pins the current behaviour of that path in its spec.

Why

First step of gitlab-com/gl-infra/production-engineering#29557, replacing Rack::Attack::Allow2Ban with a labkit rate limiter. @reprazent asked for metrics on the existing path first, so the old and new implementations can be compared rather than swapped blind.

The path has no Prometheus instrumentation at all today. gitlab_rack_attack_events_total cannot cover it: that comes from a subscriber on Rack::Attack middleware notifications, and Allow2Ban's class methods publish none when called directly from IpRateLimiter. So today nobody can say how often bans fire.

Notes for review

  • Nothing is emitted when the limiter is skipped. An allowlisted or disabled request is not a ban event at all, and on an instance with the feature off it would be the only thing this counter ever recorded.
  • reset is high volume by design. Gitlab::Auth.rate_limit! calls reset! after every successful git auth, not only when a ban existed, so this label will dwarf the others. It is kept because it is the only measure of how often the clear-on-success path runs, which is the labkit primitive being designed against. Query the other four labels on their own.
  • Detecting ban creation needs one extra read. Allow2Ban#fail! returns false even on the call that writes the ban, so the only way to see the transition is to check banned? straight after registering the failure. That read happens on failed auth only, which is rare.
  • already_banned is separate from blocked on purpose. It is the case where a concurrent request banned the IP between this request's pre-auth check and its failure registration, and it is the only path that reaches the "threshold exceeded" auth log today.

Spec changes

The existing spec asserted less than it appeared to. Nothing covered which attempt the ban lands on, the counter expiring after findtime, the ban expiring after bantime, or reset! clearing an existing ban. One assertion expected Allow2Ban.reset!, which production never calls, so it could not fail. All of that is now pinned, because it is the baseline the labkit implementation will be compared against.

Edited by Nidhey Indurkar

Merge request reports

Loading
Loading