Add X-RateLimit-* response headers as an opt-in feature #56
Conversation
armsnyder
force-pushed
the
feature/headers
branch
from
8c6692f to
08ae3b2
Compare
There was a problem hiding this comment.
Sorry for the delay here @armsnyder, I was not able to allocate space to maintenance in the last few months, but will be very responsive going forward.
I have a high level comment about proto maintenance. If you are ok with taking that on in this PR, I would appreciate it. If not, let me know and I can review the rest of the PR.
| @@ -7,6 +7,6 @@ $protoc --version | |||
| protoc --go_out=plugins=grpc:. proto/ratelimit/*.proto | |||
|
|
|||
| # Data-plane-api proto | |||
| data_plane_cmd="protoc --go_out=plugins=grpc,Menvoy/api/v2/ratelimit/ratelimit.proto=github.com/lyft/ratelimit/proto/envoy/api/v2/ratelimit:proto/. --proto_path=vendor/github.com/envoyproxy/data-plane-api --proto_path=vendor/github.com/lyft/protoc-gen-validate --proto_path=vendor/github.com/google/protobuf/src " | |||
| data_plane_cmd="protoc --go_out=plugins=grpc,Menvoy/api/v2/ratelimit/ratelimit.proto=github.com/lyft/ratelimit/proto/envoy/api/v2/ratelimit,Menvoy/api/v2/core/base.proto=github.com/envoyproxy/go-control-plane/envoy/api/v2/core:proto/. --proto_path=vendor/github.com/envoyproxy/data-plane-api --proto_path=vendor/github.com/lyft/protoc-gen-validate --proto_path=vendor/github.com/google/protobuf/src " | |||
There was a problem hiding this comment.
proposal: for ease of management of the envoy-api protos, I think we should just take go-control-plane as a glide dependency, and remove the proto compilation and the compiled protos. Leaving the legacy proto/ratelimit/ratelimit.pb.go in place of course.
|
@armsnyder I'm new this to project and evaluating it for rate-limiting outside of Envoy (for now). I was planning to build up these standard headers from the raw data in the response anyway, so this feature seems very useful. Could a standard grpc client receiving the response proxy these headers upstream to the caller? An example for the project readme could help give the feature some visibility. |
|
@junr03 @armsnyder I was just building out my own client library that similarly constructs these headers and noticed that the value required for It seems like I see Regardless, I am still a big fan of this PR! |
| rateLimitResetHeader(limitingDescriptor, now), | ||
| } | ||
| } else if limitCount > 1 { | ||
| // If there is more than one limit, then picking one of them for the "X-RateLimit-Limit" |
There was a problem hiding this comment.
@armsnyder I just ran into this same situation (two or more descriptors that match the request) while building my client library. My test case is contrived and might not represent real world usage, but do we have any sense as to whether most users of this project have multiple descriptors that match on a regular basis?
Like you did here, I have no obvious way to decide which set of limiting conditions to show. Maybe the closest to the limit on a literal or percentage basis - the worst case of them all?
The consuming app doesn't know which rate limiting condition they are hitting when given just the headers and I've never seen a provider that enumerated which rate limit I was hitting.
|
Sorry for the embarrassingly long delay in getting this reviewed. Let's get this reviewed and merged. I think we need to fix our CI also which is probably out of date. @junr03 are you able to review this? If not I will take a look. |
|
My earlier comments were not resolved. I can take ownership of the branch and you can review, once I merge master to bring in #73. I still think it is for the best (especially since what you had to do for #73), to defer proto compilation to go-control-plane like we have done in other places. I can do that in a subsequent PR to this. @mattklein123 We can land this together monday, I am ooo today. |
|
@junr03 sg sorry didn't realize you were still tracking. Thank you! |
|
@armsnyder (for obvious permissions reasons) I can't take over this PR. I have opened a new PR #77. PTAL if you would like to review, or let me know if you want to work on it. Closing this version for now. |
@lyft/network-team
Around August 2018, Envoy added a new field,
headersto theRateLimitResponseproto message, which can be used to give Envoy custom headers to attach to its response:envoyproxy/envoy#4015
With this PR the ratelimit service will populate this response field with three headers:
X-RateLimit-LimitCurrentLimit.RequestsPerUnitvalue for theRateLimitResponse_DescriptorStatusif there is one. If more than one descriptor from the request has an associated limit, then this field is NOT included in the response.X-RateLimit-RemainingLimitRemainingvalue of theRateLimitResponse_DescriptorStatuss returned by the Redis cache.X-RateLimit-ResetRateLimit.Unitand current time. If there are multiple descriptors, the one with the fewest remaining requests and longest time to reset is used.The behavior of populating these headers is disabled by default, so this change is perfectly backwards-compatible. The behavior must be enabled explicitly using the
RESPONSE_HEADERS_ENABLEDenvironment variable.The legacy rate limit API also still functions when
RESPONSE_HEADERS_ENABLEDistrue, but the headers are dropped from the response during the conversion step.