Addressing issue #291 - Adding custom headers with the ratelimit trig… #292
Conversation
…limit triggered Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
232d4ee to
162fbfc
Compare
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
b87cf73 to
7d1c7e2
Compare
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
|
Just so I'm understanding correctly, is this PR trying to implement the response of the X-RateLimit-Limit, X-RateLimit-Reset, X-RateLimit-Remaining info as specified in https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ratelimit/v3/rate_limit.proto ? |
Yes, correct. |
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
src/settings/settings.go
Outdated
| HeaderRatelimitLimit string `envconfig:"LIMIT_LIMIT_HEADER" default:""` | ||
| // value: remaining count | ||
| HeaderRatelimitRemaining string `envconfig:"LIMIT_REMAINING_HEADER" default:""` | ||
| // value: remaining seconds | ||
| HeaderRatelimitReset string `envconfig:"LIMIT_RESET_HEADER" default:""` |
There was a problem hiding this comment.
IMO I think it would be better to have an additional flag, something like XRatelimitResponseHeadersEnabled rather than having the toggle logic in the service struct and have these 3 settings have a default value corresponding to the header names specified in the RFC, eg: "X-RateLimit-Limit" "X-RateLimit-Remaining" and "X-RateLimit-Reset"
There was a problem hiding this comment.
Sure, I can go with that
There was a problem hiding this comment.
To not have to read the settings every time, I still think it makes sense to read them at config reload and and put in service-struct
There was a problem hiding this comment.
I guess what I'm trying to say is -- most likely 99% of people will use the official header names as stated in the RFC/Envoy docs so I think we should set those as the default values in here. That means we would need an 'enabled' flag in the settings to determine if these responses are enabled rather than enabling them if the strings are not empty. I think that would be cleaner as well.
There was a problem hiding this comment.
Yes, I agree I'll do that change with an "Enable" flag, but the default values to be identical to the ones that are generated by default by Envoy might be a bit unecessary. In which case enabling the custom headers, will generate the same headers as Envoy does by default. I can do that if you think it makes sense, but perhaps it would make more sense to use the proper-headers from the draft spec, ie. without the "X-" (https://datatracker.ietf.org/doc/html/draft-polli-ratelimit-headers-03)
There was a problem hiding this comment.
Added the draft standard headers as the default values
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
…sting TimeSource Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
…imit into 291_custom_headers
Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com>
|
looks great, thank you! |
…limit trig… (envoyproxy#292) * Addressing issue envoyproxy#291 - Adding custom headers with the ratelimit triggered Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing doc format Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing data race condition during config-reload Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Changed settings approach. Refactored custom clock to use already existing TimeSource Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Cleanup after timesource refactoring Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixed review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> Co-authored-by: Jesper Söderlund <jesper.soderlund@klarna.com> Signed-off-by: Debby Ku <debbyku.2008@gmail.com>
…limit trig… (envoyproxy#292) * Addressing issue envoyproxy#291 - Adding custom headers with the ratelimit triggered Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing doc format Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing data race condition during config-reload Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Changed settings approach. Refactored custom clock to use already existing TimeSource Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Cleanup after timesource refactoring Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixed review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> Co-authored-by: Jesper Söderlund <jesper.soderlund@klarna.com> Signed-off-by: Debby Ku <debbyku.2008@gmail.com>
…limit trig… (envoyproxy#292) * Addressing issue envoyproxy#291 - Adding custom headers with the ratelimit triggered Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing doc format Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixing data race condition during config-reload Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Changed settings approach. Refactored custom clock to use already existing TimeSource Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Cleanup after timesource refactoring Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> * Fixed review comments Signed-off-by: Jesper Söderlund <jesper.soderlund@klarna.com> Co-authored-by: Jesper Söderlund <jesper.soderlund@klarna.com>
Issue #291
Adding custom headers with rate limit data so that there is the option to use the draft specification specified headers of "Ratelimit-" instead of "X-Ratelimit-" or other custom naming schemes to conform to corporate standards