search

envoyproxy / ratelimit

Go/gRPC service designed to enable generic rate limit scenarios from different types of applications.
Apache License 2.0
2.27k stars 443 forks source link

Add unit multiplier #552

Open harpunius opened 6 months ago

harpunius commented 6 months ago

Implements the unit multiplier idea suggested in https://github.com/envoyproxy/ratelimit/issues/190.

rate_limit:
  unit: <second, minute, hour, day>
  requests_per_unit: <uint>
  unit_multiplier: <uint | optional>

I chose to force unit multiplier to 1 if the value is unset to avoid if/else checks whenever we rely on the value or log anything. This means the PR got slightly bigger than intended, but I think it's nicer.

The config parser panics if unit_multiplier: 0 is set. One RFC is the TODO in UnitToDividerWithMultiplier where I added a redundant if-zero runtime check to make sure we don't multiply by zero and effectively disable the rate limiting.

Note, this is a draft and doesn't build due to the dependencies on both https://github.com/envoyproxy/go-control-plane/blob/main/envoy/service/ratelimit/v3/rls.pb.go#L357-L367 and https://github.com/envoyproxy/go-control-plane/blob/main/ratelimit/config/ratelimit/v3/rls_conf.pb.go#L246-L263. I opened an issue to track this: https://github.com/envoyproxy/envoy/issues/33277.

I couldn't run the integration tests locally (WSL setup) so let me know if we need to extend those tests as well.

Happy to hear your thoughts.

github-actions[bot] commented 5 months ago

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

harpunius commented 4 months ago

Not stale. Still waiting for input from e.g. @mattklein123

github-actions[bot] commented 3 months ago

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

harpunius commented 3 months ago

Still waiting for input.

dnbn commented 3 months ago

Hello @harpunius thanks It appears that one check were not successful. Thank you for your implementation proposal. Do you know when it can be merged?

ramaraochavali commented 2 months ago

@harpunius are you actively working on this? If not we would like to contribute this as this is needed for us

github-actions[bot] commented 1 month ago

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

harpunius commented 1 month ago

Not stale, still waiting for input https://github.com/envoyproxy/envoy/issues/33277.

github-actions[bot] commented 1 week ago

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

ramaraochavali commented 1 week ago

Not stale

Pawan-Bishnoi commented 1 week ago

@harpunius is draft still the correct status of this PR? (asking because that's probably the reason it isn't seeing much traction)

harpunius commented 1 week ago

Hey, thanks for your input and interest in this PR.

As mentioned in the PR description:

Note, this is a draft and doesn't build due to the dependencies on both https://github.com/envoyproxy/go-control-plane/blob/main/envoy/service/ratelimit/v3/rls.pb.go#L357-L367 and https://github.com/envoyproxy/go-control-plane/blob/main/ratelimit/config/ratelimit/v3/rls_conf.pb.go#L246-L263. I opened an issue to track this: https://github.com/envoyproxy/envoy/issues/33277.

For my implementation we need to extend the ratelimit protobuf definitions linked above. Tests run locally by statically linking the fixed protobuf files (with the new field added, see the issue in envoyproxy/envoy). Unfortunately, I don't see a way around that due to how the ratelimit envoy filter communicates with the ratelimit service.

I opened an issue to track this, but I still haven't gotten a response from the maintainers. I'm not very skilled with c++, so I'm having a hard time figuring out what additional work is needed in envoyproxy/envoy, for instance https://github.com/envoyproxy/envoy/blob/f580ed067db0f2a48085aa33e45a3a9cf5e36bfe/source/extensions/filters/http/ratelimit/ratelimit_headers.cc#L32 also needs to reflect the unit multiplier.

I did rebase so there at least aren't merge conflicts any more :)

Pawan-Bishnoi commented 5 days ago

ah, I see. So you are waiting to someone to update the rls proto in envoy. And only after that you will be able to merge your changes to this repo.

Makes sense. Let me see if I can move the envoy piece.