Backport of 'Log a line for requests the rack middleware blocks'

Hardik Galarequested to merge
19-4-stable-ee-backport-258338 into 19-4-stable-ee

What does this MR do and why?

Backports !258338 (merged), !265941 (merged), and !266584 (merged) to 19.4.

On 19.4, when every LabKit cohort is enforced, Gitlab::Middleware::LabkitRackRateLimit returns the 429 itself. Rack::Attack safelists the request, so it writes no Rack_Attack line to auth_json.log. Grape and lograge never run, so api_json.log has no entry either. All six rate_limiter_use_labkit_rack_cohort_* flags are default_enabled: true on 19.4, so every 19.4 instance is affected. Administrators and Support have no log line that shows which rule blocked a request, or which client it blocked.

This MR adds the Rate_Limit_Block line to auth_json.log:

  • !258338 (merged) adds the line, with matched, rule, limit, period_s, count, counted_by, and applies_to.
  • !265941 (merged) adds remote_ip, path, and method, with the same key names as the Rack_Attack line.
  • !266584 (merged) removes applies_to again. The field has a different shape for different rules, and Elasticsearch drops a line whose shape does not match the index mapping.

The line is sampled: it is written when a client first goes past a rule's limit, then at 2, 4, 8 (and so on) times the limit. The trigger uses the count that LabKit already returned, so a blocked request makes no extra Redis call.

Why this is a bug fix

On master, !258338 (merged) is ~"type::feature" and !265941 (merged) is ~"type::maintenance". On 19.4, they fix a functional regression, tracked as a bug in #634755. Before LabKit enforcement, Rack::Attack wrote a Rack_Attack line to auth_json.log for every blocked request. LabKit enforcement removed that line and put nothing in its place.

Conflict resolution

!258338 (merged) did not apply cleanly. !265941 (merged) applied cleanly on top of it. !266584 (merged) did not apply cleanly in the middleware, for the same RateLimitState reason. I kept the 19.4 closing } of the hash and removed only applies_to. Its spec change applied cleanly.

  • Gitlab::Instrumentation::RateLimitState does not exist on 19.4. I removed the RateLimitState.track(checks) call and the .merge(RateLimitState.payload) call, so the line has no rate_limit_state field on 19.4.
  • run keeps the 19.4 shape (limiters.all.values.map). The checks hash on master exists only for RateLimitState.track, so I did not add it.
  • log_block runs in its own guard, as on master, so a logging error never drops the 429.
  • In the spec, I removed the RateLimitState::STATE expectation and the unused counted_evaluation helper.

Apart from these items, the middleware is the same as the 19.5 backport of !265941 (merged).

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

  • This MR is backporting a bug fix, documentation update, or spec fix, previously merged in the default branch.
  • The MR that fixed the bug on the default branch has been deployed to GitLab.com (not applicable for documentation or spec changes).
  • The MR title is descriptive (e.g. "Backport of 'title of default branch MR'"). This is important, since the title will be copied to the patch blog post.
  • Required labels have been applied to this merge request
  • This MR has been approved by a maintainer (only one approval is required).
  • Ensure the e2e:test-on-omnibus-ee job has succeeded, or if it has failed, investigate the failures. If you determine the failures are unrelated, you may proceed. If you need assistance investigating, request help in the #s_developer_experience Slack channel to confirm the failures are unrelated to the merge request.

Note to the merge request author and maintainer

If you have questions about the patch release process, please:

🤖 ctrl+alt+AI

Edited by Hardik Gala

Merge request reports