Move load balancing middleware above the rate limiter

Hardik Galarequested to merge
hgala/29728-load-balancing-middleware-order into master

What does this MR do and why?

Gitlab::Middleware::LabkitRackRateLimit resolves the authenticated user on the way into the request, so it can apply the higher rate limit that authenticated requesters get.

config/initializers/load_balancing.rb installed the database load balancer middleware with config.middleware.use(...), and use appends to the end of the middleware stack. That put Gitlab::Database::LoadBalancing::RackMiddleware at position 66, below LabkitRackRateLimit at position 54. As a result, the user object was loaded before database sticking had been applied, and that read missed the replica catch-up guarantee the load balancer middleware exists to provide.

This MR swaps use for insert_before, so the load balancer runs above the rate limiter. The resulting order is: Warden::Manager at 53, Gitlab::Database::LoadBalancing::RackMiddleware at 54, LabkitRackRateLimit at 55, Rack::Attack at 56.

The load balancer moved rather than the rate limiter because the rate limiter needs to stay as early in the stack as possible, in particular ahead of middlewares that can render a response, such as EtagCaching and Go. Moving the rate limiter down instead would have put roughly ten middlewares ahead of a rejection. This ordering was agreed with @reprazent in the discussion on !253596 (merged).

This bug was surfaced by removing the six labkit rack cohort feature flags, which stopped the test suite from masking the middleware. This MR needs to land before that flag removal at !253596 (merged).

The bug is older than labkit. As @jon_jenkins points out, Rack::Attack has resolved the same authenticated_identifier from above the load balancer for years, so this reorder fixes more than the labkit path.

What now runs inside the load balancer session

Moving the load balancer from position 66 to 54 means roughly eleven middlewares that previously ran outside its database session now run inside it: Rack::Attack (which appears twice in the stack), RackAttackHeaders, Rack::Static, Multipart, ApolloUploadServer::Middleware, BatchLoader::Middleware, Ci::JobToken::Middleware, QueryAnalyzer, EtagCaching::Middleware, and Go.

@jon_jenkins reviewed this from the database side and narrowed it down. Only two of the eleven touch the database: Rack::Attack for user and token lookups, and Go for a project lookup. Both are read-only and both gain from running inside sticking. Rack::Static comes from lookbook, so it is not in the production stack at all. The rest issue no queries.

The real change in behaviour is not the reorder. It is that requests short-circuited by EtagCaching with a 304, and by the rate limiters with a 429, now run find_caught_up_replica, where they previously did no load balancer work. For anonymous requests that costs nothing, because there is no user. For signed-in users it is three Redis GETs on the db_load_balancing instance per poll, plus a pg_wal_lsn_diff probe on every replica across the three databases when the user has a write location from the last 30 seconds.

There is no feedback loop under replication lag. The probe is a shared-memory read on the replica rather than I/O, the key TTL bounds it to a 30 second write cohort, and polling runs on a fixed interval, so slow 304s do not generate more requests. The cost is puma thread time on sequential probes, not database load, and the same users already pay it on every real request, so this extends it to their polls rather than changing its shape.

Test results:

  • spec/requests/rack_middlewares/, plus spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb, plus spec/lib/gitlab/middleware/labkit_rack_rate_limit_spec.rb and ee/spec/requests/labkit_rack_rate_limit_spec.rb: 58 examples, 0 failures.
  • Correction: an earlier version of this section said spec/requests/rack_middlewares/load_balancing_spec.rb:10 fails without this change and therefore acts as a regression guard. It does not. On this branch it passes either way, because the cohort flags are stubbed off across the suite, so the middleware never resolves the user in specs and nothing bypasses sticking. This merge request has no regression guard, and its pipeline cannot demonstrate the fix. The spec only fails where the middleware runs unconditionally, which is the flag removal branch in !253596 (merged), and that is where it currently fails in CI.
  • The full spec/lib/gitlab/database/load_balancing/ directory: 463 examples, 1 failure, 12 pending. The single failure is host_spec.rb:666 and it is pre-existing, since it fails identically with this change reverted.
  • RuboCop on the changed file reports no offences.

Requesting the pipeline:run-all-rspec label: predictive spec selection keys off changed files, and config/initializers/load_balancing.rb maps to very few specs, while the real blast radius of a middleware reorder is every request in the application.

References

Screenshots or screen recordings

Not applicable, there is no user interface change.

How to set up and validate locally

  1. Confirm the ordering, by running this and checking that Gitlab::Database::LoadBalancing::RackMiddleware now appears immediately before Gitlab::Middleware::LabkitRackRateLimit: bundle exec rails runner 'Rails.application.middleware.map(&:name).each_with_index { |m, i| puts "#{i}\t#{m}" }'

  2. To see the spec actually fail, use the flag removal branch in !253596 (merged), not this one or master:

    git switch hgala/29663-remove-labkit-rack-cohort-flags
bundle exec rspec spec/requests/rack_middlewares/load_balancing_spec.rb

    Then apply this branch's one-line initializer change on top and re-run; it passes.

  3. Run the wider set for regressions: bundle exec rspec spec/requests/rack_middlewares/ spec/lib/gitlab/database/load_balancing/

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Hardik Gala

Merge request reports