Set the request IP before rate limiting resolves the user
What does this MR do and why?
Gitlab::Middleware::IpAddress was mounted with use inside the Grape app at lib/api/api.rb, so it ran after Gitlab::Middleware::LabkitRackRateLimit had already resolved the authenticated user. That resolution calls PersonalAccessTokens::LastUsedService, which reads Gitlab::IpAddressState.current and finds it nil, so the token's last-used IP is never recorded.
This mounts it in the outer Rails stack instead, directly above the rate limiter, and drops the now-redundant Grape-level mount. Resulting order: Warden::Manager 53, IpAddress 54, LabkitRackRateLimit 55, Rack::Attack 56.
config/application.rb loads before Zeitwerk, so the insert needs the accompanying require_dependency. Without it the application fails to boot with uninitialized constant Gitlab::Middleware::IpAddress.
Two bugs, one of them visible today
Feed paths, reproducible on master. authenticate_sessionless_user!(:rss) is a prepend_before_action, so it runs ahead of ApplicationController's around_action :set_current_ip_address. A feed request authenticated with a personal access token therefore records no IP at all. spec/requests/rack_middlewares/ip_address_spec.rb covers this: it fails on master and passes here.
API paths, only visible once the cohort flags are gone. The rate limiter currently runs behind the six labkit rack cohort flags, which the test suite stubs off, so this cannot be reproduced in CI on master. On the flag removal branch in !253596 (merged), spec/requests/api/personal_access_tokens_spec.rb:420 and :632 fail with actual collection contained: [] without this change and pass with it, leaving PersonalAccessTokens::LastUsedService untouched.
Scope of the change
Gitlab::IpAddressState is now set from the middleware rather than from inside Grape and ApplicationController, so it is available earlier. To check what that actually affects, every read of IpAddressState.current during one authenticated API request was instrumented across four configurations:
| Base | Change | Readers that saw a value |
|---|---|---|
master |
none | LastUsedService only, IP present |
master |
this MR | identical |
| flag removal branch | none | LastUsedService only, some reads nil |
| flag removal branch | this MR | LastUsedService only, IP present |
PersonalAccessTokens::LastUsedService is the only reader that appears. The other readers of IpAddressState.current are unaffected on this path: lib/api/internal/base.rb:133 because the middleware skips /api/v\d+/internal/ and internal API sets its own IP explicitly, and the EE readers because they run inside a controller, policy or service that already had one.
Test results
Run locally against a real Postgres and Redis.
- 716 examples, 0 failures, 8 pending across
rack_middlewares/ip_address_spec.rb,api/internal/base_spec.rb,api/personal_access_tokens_spec.rb,auth/auth_finders_spec.rb,personal_access_tokens/last_used_service_spec.rb,application_controller_spec.rbandapi/helpers/authentication_spec.rb. - The
IpAddressStatereaders and writers,sidekiq_middleware/set_ip_address,action_cable/ip_address_state_callback,application_workerandee/gitlab/workhorse: 125 examples, 0 failures. - On the flag removal branch with this change: 422 examples, 0 failures, 8 pending, and the two failing token examples go from 2 failures to 0.
- Middleware order and boot confirmed with
rails runner. Combined with !254069 (merged) the order isWarden::Manager53,IpAddress54,LoadBalancing::RackMiddleware55,LabkitRackRateLimit56, so the two changes compose. - RuboCop on all three files: no offences.
Not verified: production behaviour, and the reader census covers GET /projects rather than every request shape.
For the authentication group
@ifarkas this is the change @reprazent raised with you. The question was whether widening where Gitlab::IpAddressState is set is acceptable. The census above is the evidence on that: on the API path it changes exactly one consumer, the one being fixed.
References
- Issue: gitlab-com/gl-infra/production-engineering#29728
- Flag removal this unblocks: !253596 (merged)
- Sibling fix from the same root cause: !254069 (merged)
- Supersedes the service-level fix in !254147 (closed), which is held open as a fallback
Screenshots or screen recordings
Not applicable, no user interface change.
How to set up and validate locally
-
On
master, confirm the feed bug:bundle exec rspec spec/requests/rack_middlewares/ip_address_spec.rbfails. -
On this branch, the same command passes.
-
The API side needs the flag removal branch, since the suite stubs the cohort flags off:
git switch hgala/29663-remove-labkit-rack-cohort-flags bundle exec rspec spec/requests/api/personal_access_tokens_spec.rb:420 spec/requests/api/personal_access_tokens_spec.rb:632Both fail. Apply this branch's
config/application.rbandlib/api/api.rbon top and both pass.
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.