Add X-RateLimit-* response headers as an opt-in feature

[junr03]

Taking over #56

Original PR message:

Around August 2018, Envoy added a new field, headers to the RateLimitResponse proto 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:

Header	Description	How it maps to the descriptor statuses
X-RateLimit-Limit	The number of requests allowed in the configured unit of time	The CurrentLimit.RequestsPerUnit value for the RateLimitResponse_DescriptorStatus if 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-Remaining	The number of requests remaining before the subsequent request will be rate limited	The smallest LimitRemaining value of the RateLimitResponse_DescriptorStatuss returned by the Redis cache.
X-RateLimit-Reset	The number of seconds until the limit resets and requests will start to succeed	Calculated similarly to the way the Redis client generates descriptor keys, using modulo arithmetic with the RateLimit.Unit and 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_ENABLED environment variable.

The legacy rate limit API also still functions when RESPONSE_HEADERS_ENABLED is true, but the headers are dropped from the response during the conversion step.

[junr03]

cc @stevenscg

[junr03]

@mattklein123 @stevenscg

I did a little refactoring for ease of testing, and for separation of logic. However, the more I think about this functionality the more I want to discuss before we commit to adding it.

1. Should the reset data also be added to the RateLimit response on a per limit basis?
2. In the "more than one limit" scenario we reduce the information returned to the min limit remaining. Is this the behavior we want?

[junr03]

If we pick min for remaining, shouldn't we pick the accompanying Limit to fill the limitHeader?

[mattklein123]

@junr03 I have a high level comment. Why are we adding headers here at all vs. just populating values in the proto response, and then letting the caller of this service do what they want with the data?

[junr03]

@junr03 I have a high level comment. Why are we adding headers here at all vs. just populating values in the proto response, and then letting the caller of this service do what they want with the data?

@mattklein123 Yep that was my larger question re: Should the reset data also be added to the RateLimit response on a per limit basis?. The client already has the necessary data (with the exception of reset) in the RateLimitResponse, and with even higher resolution (hence my question 2 above).

The idea here was to leverage (https://github.com/envoyproxy/envoy/blob/4a5f85809cef13be47873483789a5a7bb64ded7c/api/envoy/service/ratelimit/v2/rls.proto#L94) to get the data via headers downstream from the Envoy making the call for free.

[mattklein123]

@junr03 what I'm saying is that I think the change to rls.proto to add headers is incorrect. We should remove that and explicitly add the data that we need in the proto response, and then let the caller do whatever they want with it.

[junr03]

@mattklein123 yeah, I think the two of us are on the same page. Seeing it here made me rethink the use of those headers in general i.e it is unclear to me if we should have taken the original Envoy PR. However, I see two levels here:

1. We could definitely add the reset filed in the Descriptor status as that might be useful for clients to have:

• I can make the data-plane PR to add the field.
• I can make the ratelimit PR to support it in Lyft's implementation.

2. It is unclear to me if we win anything removing the functionality from envoy for a couple reasons:

• The functionality in Envoy is not prescriptive about the headers that are supported, and there might be information that a different implementation of the ratelimit api might want to send via headers. It just happens that this particular PR is redundant.
• Us supporting this PR in this project is not necessarily attached to us supporting in Envoy. And given above, I am not sure that it buys us much to deprecate the Envoy functionality if there are others in the community that want to leverage the functionality.

wdyt?

Any additional comments @stevenscg

[mattklein123]

Yes it's fine to keep this feature, but for the things we are using it for here I don't think we should be using header, we should be sending the data explicitly and letting Envoy send headers if it wishes.

[junr03]

Same page. I am going to close this, and proceed per 1 above. Thanks for the discussion @mattklein123

[leosunmo]

Has there been any action on this within Envoy? I couldn't find anything by searching Envoy Issues and PRs and this seems like an extremely useful feature (requirement for my current project). Ideally I'd prefer to not fork the ratelimit project with this PR merged. Any other way of achieving this?

[junr03]

@leosunmo no, I did not have the cycles to implement as per 1 above. I am further away from this project now, and will not have time to do so. I have cycles to review and advise however. If you have some time to implement the alternative presented above I would be more than happy to review and help you get it in.

[leosunmo]

Thanks for that @junr03. Unfortunately the main Envoy project is a bit outside my comfort zone so probably won't be able to pick that up. I'll try to create an issue at some point soon.