Log key when rate limit has been exceeded.

[popcorn]

Why?

I'd like to know the key for which the rate limit has been exceeded.

Test

I tested both for distributed: {} and without it.

Caddy JSON rate limit setup

{
  "handler": "rate_limit",
  "rate_limits": {
    "dynamic_example": {
      "key": "{http.request.host}",
      "window": "5m",
      "max_events": 2
    }
  },
  "storage": {
    "module": "redis",
    "address": [
      "localhost:6379"
    ],
    "username": "",
    "password": "",
    "db": 0,
    "timeout": "5",
    "key_prefix": "caddy-rate-limit",
    "encryption_key": "",
    "compression": false,
    "tls_enabled": false,
    "tls_insecure": true
  },
  "distributed": {}
}

Resulting logs (prettified)

{
  "level": "info",
  "ts": "2024-06-28T12:54:58.067Z",
  "logger": "http.handlers.rate_limit",
  "msg": "rate limit exceeded",
  "zone": "dynamic_example",
  "key": "localhost",
  "wait": 297.70859879,
  "remote_ip": "172.17.0.1"
}

I'm not an expert in Go so please let me know if this is good enough to merge.

Cheers!

[mholt]

The change looks good, nice work. Although, my only concern is that it's possible for the key to contain sensitive information.

Caddy does have config options to redact certain fields, but its HTTP server, for example, redacts certain values by default (sensitive header fields for instance). I wonder if we should make this an opt-in thing. :thinking:

[popcorn]

Good thinking @mholt.

I updated the code, squashed it into a single commit and pushed.

Also I run the tests with log_key not set, set to false and true. Works as expected.

Let me know what you think.

[popcorn]

Cool! Do I need to do anything else in order for this to get merged?

[mholt]

Nope. :)

[popcorn]

Amazing, my first code contribution to Caddy 🤠

Cheers!

[mholt]

Congrats!