search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.59k stars 4.74k forks source link

network rate limit: allow limiting by client IP #19685

Closed rapatchi closed 2 years ago

rapatchi commented 2 years ago

Allow the network rate limit filter to have dynamic descriptors, such as downstream IP.

mattklein123 commented 2 years ago

This is not possible today. The network ratelimit filter just allows fixed keys/values: https://github.com/envoyproxy/envoy/blob/450631b8e19a0bf8121a7338c4ecc0612731e68e/source/extensions/filters/network/ratelimit/ratelimit.cc#L22-L27

This is a reasonable feature request if you want to open a feature request issue?

rapatchi commented 2 years ago

Thanks @mattklein123 for confirming. Not sure if the existing issue can be converted to a feature request. If not I will open a new one.

mattklein123 commented 2 years ago

@rapatchi it's fine to convert his issue but please update the title/description/etc. or just make a new one and close this one. Whatever you think is better.

rapatchi commented 2 years ago

@mattklein123 updated the description and title. Let me know if i missed something.

llu94 commented 2 years ago

If you don't mind, I'd be willing to take a look at this.

rapatchi commented 2 years ago

Thanks @llu94 , for taking a look into this.

llu94 commented 2 years ago

@mattklein123 @rapatchi hey guys, apologies for the delay. I had some crazy stuff going on in my day to day. I've sketched out a proof of concept PR for this issue here: https://github.com/envoyproxy/envoy/pull/19875/files

llu94 commented 2 years ago

I left some of my concerns and questions as TODO comments (which will be removed before merge) in the PR. Can you guys take a look and maybe answer some of those?

simha5009 commented 2 years ago

Can we close this?

Do we need the dependent PRs to be closed?

llu94 commented 2 years ago

The PR was merged months ago. Don't know why the issue is still open

On Wed, Jul 27, 2022, 09:45 Narasimha Karumanchi @.***> wrote:

Can I know the status of this?

Going to merge this sooner?

— Reply to this email directly, view it on GitHub https://github.com/envoyproxy/envoy/issues/19685#issuecomment-1196783622, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQJ37DP6ZP2MSRUVJP4ALE3VWE4RFANCNFSM5MY2STQA . You are receiving this because you were mentioned.Message ID: @.***>

phlax commented 2 years ago

thanks for headsup - ill close it now