Add unit multiplier

[harpunius]

Implements the unit multiplier idea suggested in #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: envoyproxy/envoy#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]

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]

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

[github-actions]

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]

Still waiting for input.

[dnbn]

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]

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

[github-actions]

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]

Not stale, still waiting for input envoyproxy/envoy#33277.

[github-actions]

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]

Not stale

[Pawan-Bishnoi]

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

[harpunius]

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: envoyproxy/envoy#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]

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.

[github-actions]

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]

Still waiting for input for envoyproxy/envoy#33277.

[github-actions]

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]

Still waiting for input.

[github-actions]

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!

[Pawan-Bishnoi]

not stale.
Planning to work on this envoyproxy/envoy#37841

[github-actions]

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!

[taer]

@Pawan-Bishnoi what happened w/ the envoy proto change? This would be a very nice feature to have in the "reference" implementation.

[Pawan-Bishnoi]

I spent some time last weekend but the devContainer build was failing and ended up spending the whole day on that.
Will try again this weekend.

[github-actions]

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!

[Pawan-Bishnoi]

not stale: envoyproxy/envoy#38529

[github-actions]

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]

Not stale.

[github-actions]

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!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[taer]

This seemed like a nice feature. Let's me say per 30 minutes or similar. Is there work needed on this to get it in?

[Pawan-Bishnoi]

yes. This work: envoyproxy/envoy#38529

I am struggling to find time for this 😄