feat(rate-limiting) support dynamic path filtering

[nbialostosky]

Summary

The current implementation of path based rate limiting only allows a single path identifier per route. However, there are situations where rate limiting by path would be helpful on a path that is dynamic, such as including a UUID in the path.

The changes included support this functionality by allowing the rate_limiting identifier to be automatically set to the path of the request. This means you don't need to specify which path to rate limit as it will be done automatically.

The existing test in 04-access_spec.lua:

https://github.com/Kong/kong/blob/master/spec/03-plugins/23-rate-limiting/04-access_spec.lua#L396

Shows this change working by simply removing the path value set in the config, the test continues to function as expected as the path gets automatically pulled in.

Full changelog

feat(rate-limiting) support dynamic path filtering

Issue reference

N/A

[CLAassistant]

All committers have signed the CLA.

[vm-001]

Hi, @nbialostosky . I'm not a reviewer though. Just wondering what kind of scenario need to limiting by the URL path. Because I think the client always can change the URL. Does it still make sense to use limiting by path in this case?

One alternative solution is to use limit_by header feature + custom plugin, which was described here: https://github.com/Kong/kong/pull/9965#issuecomment-1356081388

[kikito]

Hello @nbialostosky , apologies for not getting back to you on this PR sooner.

We are very grateful for your contribution, but we decided not to include it in Kong. There's a concern and a strong reason:

• Concern: The changes are not fully tested.

Your change essentially removes checks from the plugin. It is expected that the existing tests will simply "keep passing" if all we do is remove checks.

rate limiting by path would be helpful on a path that is dynamic, such as including a UUID in the path.

I would have expected at least one test that makes sure that the scenario described there is possible after the changes.

• Strong reason: We have doubts about the extra flexibility being used maliciously

Kong is used by all sorts of organizations - from individual users maintaining a personal API to big banks. Some of its users are very specific in what can and cannot be ratelimited, and in which manner.

A change like this (or along these lines) would be potentially breaking for some of those organizations. If we are going to introduce changes like what's proposed here, we would have to give ample warning, not make this behavior active by default, but instead an "opt-in", and probably release the change on a mayor release.

[dcarley]

Just wondering what kind of scenario need to limiting by the URL path. Because I think the client always can change the URL. Does it still make sense to use limiting by path in this case?

We have been using use this change in a fork of the plugin to provide individual rate limits to webhook endpoints. Each webhook is unique to an integration and shouldn't be called too frequently. There's low risk of a client using up the limit of another webhook or exceeding the capacity of the service because the webhook paths aren't trivially discoverable.

One alternative solution is to use limit_by header feature + custom plugin, which was described here: https://github.com/Kong/kong/pull/9965#issuecomment-1356081388

That looks like it would be possible but it feels like a slightly odd coupling between plugins and we had doubts about the existing behaviour of the feature (see below).

Your change essentially removes checks from the plugin. It is expected that the existing tests will simply "keep passing" if all we do is remove checks.

Rate limiting by path is still tested here. Would it help to add a third path to that test?

Kong is used by all sorts of organizations - from individual users maintaining a personal API to big banks. Some of its users are very specific in what can and cannot be ratelimited, and in which manner.

A change like this (or along these lines) would be potentially breaking for some of those organizations. If we are going to introduce changes like what's proposed here, we would have to give ample warning, not make this behavior active by default, but instead an "opt-in", and probably release the change on a mayor release.

You're right that this would be considered a breaking change. An alternative could be to introduce it as another parameter, although I think it would be difficult to name alongside the existing implementation, particularly as it's unclear to us how the current implementation is intended to be used.

It currently only matches one exact (not a prefix) path and all other paths that match the same plugin configuration will fallback to using the IP as the identifier. We couldn't find any further information in #6286, #6234, or #6202. Are you able to provide more information @kikito?

Would this change be accepted if the existing path parameter became optional to honour the existing behaviour?

Alternatively, would this change be accepted in a major release, and what is the process for doing that?