gloo icon indicating copy to clipboard operation
gloo copied to clipboard

X-RateLimit-Limit Header only showing the request that has the lower requestperUnit

Open edubonifs opened this issue 1 year ago • 6 comments

Version

1.13.x

Is your feature request related to a problem? Please describe.

Hi, currently the X-RateLimit-Limit header only logs one of the rules that we are defining in the RateLimitConfig object.

For example, if we have the following RateLimitConfig:

spec:
  raw:
    rateLimits:
    - setActions:
      - requestHeaders:
          descriptorKey: type
          headerName: x-type
      - requestHeaders:
          descriptorKey: number
          headerName: x-number
    setDescriptors:
    - alwaysApply: true
      rateLimit:
        requestsPerUnit: 3
        unit: MINUTE
      simpleDescriptors:
      - key: type
        value: a
    - alwaysApply: true
      rateLimit:
        requestsPerUnit: 5
        unit: MINUTE
      simpleDescriptors:
      - key: number
        value: one

In the logs we only see this:

{"limit":"3, 3;w=60;name=\"crd|generic_key^gloo-system.my-rate-limit-policy|generic_key^solo.setDescriptor.uniqueValue|type^a\""}

So we are only logging one rule but two rules are being impacted if we make for example this call:

curl http://a6516aedec8874df5bb964a389cb8bed-873684526.us-east-1.elb.amazonaws.com/get -v -i -H "x-type: a" -H "x-number: one"

Describe the solution you'd like

One of our clients would like that we log every rule that is being incremented in each call if more than one is being incremented in a time, not only one of them.

If the rule which was breached wasn't logged in any of the previous messages it's hard to explain to a customer that they sent faster than the rate limit if they can't show the relevant messages

Describe alternatives you've considered

No response

Additional Context

No response

edubonifs avatar May 11 '23 10:05 edubonifs

~~Can you tell from the testing if this behavior driven by the ordering of the setDescriptors definition, or if it is by the descriptor nearest being limited?~~

~~It's not really practical to log multiple descriptors since we are tapping into Envoy's API for X-RateLimit-Headers which only receives a single descriptor entry (link to API definition of response message).~~

~~However, if it is not the case that the descriptor nearest being limited is the one returned by the rate-limiter service (and thus logged by Envoy), that's something we could look into. We would have to assess the performance implications since that sorting/determination would have to be made synchronously on the data path.~~

See below comment

jbohanon avatar May 11 '23 11:05 jbohanon

Are we sure logs is even the right place for this? Looking at the backing storage is likely something that would allow for better visibility

nfuden avatar May 11 '23 11:05 nfuden

almost everything about my last comment was incorrect:

  • We return all of the descriptor statuses to Envoy
  • Envoy determines the descriptor that is nearest to being limited and uses that for the headers (ref)

jbohanon avatar May 11 '23 12:05 jbohanon

Hi @jbohanon that's right, in my tests I was also seeing the nearest the one who was logged.

So it is envoy itself which is returning only the nearest config

edubonifs avatar May 11 '23 13:05 edubonifs

RFC spec indicates comma-separated multi-value headers MUST NOT have their order changed, so we could feasibly propose an option to return all descriptors in the X-RateLimit headers if absolutely necessary, but it doesn't really seem relevant to parse the descriptors that are not the limiting ones except maybe in the edge case that multiple descriptors that are equally close to limiting exists, and the first's window expires while the second's window is still active. That may be what is happening to cause this:

If the rule which was breached wasn't logged in any of the previous messages it's hard to explain to a customer that they sent faster than the rate limit if they can't show the relevant messages

So with this config:

spec:
  raw:
    rateLimits:
    - setActions:
      - requestHeaders:
          descriptorKey: type
          headerName: x-type
      - requestHeaders:
          descriptorKey: number
          headerName: x-number
    setDescriptors:
    - alwaysApply: true
      rateLimit:
        requestsPerUnit: 2
        unit: MINUTE
      simpleDescriptors:
      - key: type
        value: a
    - alwaysApply: true
      rateLimit:
        requestsPerUnit: 2
        unit: HOUR
      simpleDescriptors:
      - key: number
        value: one

First request would indicate limit approaching on descriptor type Second request within a minute would indicate limit approaching on descriptor type Third request after the minute has elapsed would be limited on descriptor number

I don't think a machine would be able to do anything meaningful with the extra headers, so maybe some extra logging or just querying storage backend like Nathan suggested

jbohanon avatar May 11 '23 13:05 jbohanon

In our use case we log the rate limit headers which is useful for support to resolve any queries from customers about why their requests were limited. In some cases we have regulatory limits on our APIs in specific circumstances.

In the case where this was noted we have a large rate limit for a customer and a very small (1rps) limit for a specific circumstance. We see the high rate limit being logged in the requests but the low limit only once it has been breached.

That's reasonable but if we want to explain to the customer why that rate was breached we can't see which previous request increased that counter.

So exposing this in the header to be logged will allow our support team to better identify what's happening with the limits and requests.

I would imagine that the same would be true for returning that header to customers. In this example if the customer is looking at those rate limit headers they would see that a rate limit rule has blocked them but not the preceeding requests which contributed to it.

I would think that enhancing the header, or adding an additional header would make the most sense.

jonlane80 avatar May 12 '23 11:05 jonlane80