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. 🤔

[popcorn]

I think this is in the wrong place.

[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.

[mholt]

Awesome, looks great! I just have one nit to reduce repetition. Let me know if you have questions/thoughts about it.

[mholt]

What you can do here instead to avoid duplication is logger := h.logger.With(...) and put all the basic fields in there. Then, do if h.LogKey { ... } where you can add the key like logger = h.With(zap.String("key")).

You could alternatively build a slice of zap fields instead of using logger.With().

[popcorn]

Didn't know about that. Should be fixed now. Let me know if there's anything else.

[mholt]

Excellent. Nice work! Thank you

[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!