Skipper: Allow configuring the rate limit error response

[herojan]

Is your feature request related to a problem? Please describe.
Our services are supposed to return json for all responses, and problem json for error responses. The rate limit filter returns a plaintext response, which means services need to handle that possibility specifically.

Example of a current ratelimit-exceeded response.

```
curl -i -H "Authorization: Bearer ${TOKEN}" "${URL}"
HTTP/2 429
date: Mon, 08 Apr 2019 15:03:21 GMT
content-type: text/plain; charset=utf-8
content-length: 18
retry-after: 55
server: Skipper
x-content-type-options: nosniff
x-rate-limit: 300

Too Many Requests
```

Describe the solution you would like
We would like to be able to configure the clusterClientRatelimit filter so that it returns a problem json response, with Content-Type of application/problem+json.

The solutions we had thought of was either a new config flag for skipper startup, indicating it should return json error responses, or a new param for the clusterClientRatelimit filter. After discussing internally we decided to go with a new param for clusterClientRatelimit, but we're not sure if it should be one or two extra params atm, one for Content-Type, one for response body.

Current solution:
Add two new params, so now we would have e.g.

clusterClientRatelimit("groupA", 10, "1h", "X-Forwarded-For", "application/problem+json", "{\"title\":\"Rate limit exceeded\",\"detail\":\"You have exceeded your ratelimit. See the retry-after and x-rate-limit headers for details.\",\"status\":429}")

Describe alternatives you've considered (optional)
We've thus far allowed teams to deal with it, but now that we have some time to work on it we'd like to solve this issue, since using skipper as an API Gateway should still allow the service to be compliant with the API guidelines

Would you like to work on it?
Yes

[AlexanderYastrebov]

It seems possible to achieve this with existing filters:

bin/skipper -inline-routes='* -> inlineContentIfStatus(429, "{\"title\":\"Rate limit exceeded\",\"detail\":\"You have exceeded your ratelimit. See the retry-after and x-rate-limit headers for details.\",\"status\":429}", "application/problem+json") -> ratelimit(1, "1m") -> "https://example.com"' -enable-ratelimits

[herojan]

Nice find, I tried it out and it doesn't appear to work though

inlineContentIfStatus(429, "{\"title\":\"Rate limit exceeded\",\"status\":429}", "application/problem+json") -> ratelimit(1, "1m")

Gives me

HTTP/2 429 
date: Fri, 04 Dec 2020 09:26:10 GMT
content-type: text/plain; charset=utf-8
content-length: 18
retry-after: 49
server: Skipper
x-content-type-options: nosniff
x-rate-limit: 300

Too Many Requests

I tried it with clusterClientRateLimit and tried putting the rateLimit filter first too. I believe this filter only works on the response from the end service, not from other filters.

[szuecs]

You would need to set the Response and not to write it to the ResponseWriter, I guess.
Something like this should work:

// ratelimit filters
ctx.Response().StatusCode = 429

// inlineContentIfStatus
if ctx.Response().StatusCode == f.status {
    ctx.Response().Body = ...
}

You also need to signal from inlineContentIfStatus Request() via stateBag that ratelimit filter does not write to ResponseWriter but sets the code

[herojan]

That makes sense, the only problem is it's a breaking change... We can document it in release notes I guess, and try to be careful with the rollout internally

[herojan]

Actually, I've just checked that if I run skipper locally (from master branch), then the command @AlexanderYastrebov provided works, but when I tested it via an ingress it didn't work.

Works Locally:
inlineContentIfStatus(429, "{\"status\":429}", "application/problem+json") -> ratelimit(1, "1m")

Fails locally:
ratelimit(1, "1m") -> inlineContentIfStatus(429, "{\"status\":429}", "application/problem+json")

It fails in either order when I use zalando.org/skipper-filter annotation in an ingress, is there some sorting done?

Also

You also need to signal from inlineContentIfStatus Request() via stateBag that ratelimit filter does not write to ResponseWriter but sets the code

I'm not sure what this means, I expected to see it in a different filter but can't find an example

[AlexanderYastrebov]

@herojan Hi, please check which skipper version is used in the ingress. I believe my solution (as well as you PR changes actually) are only possible after https://github.com/zalando/skipper/commit/79b6e240f40771704ed4da52754607d32c83fc01 which has landed into v0.11.190

inlineContentIfStatus should be before ratelimitbecause it works on response filterchain path which invokes filters in reverse order

[herojan]

inlineContentIfStatus should be before ratelimitbecause it works on response filterchain path which invokes filters in reverse order

Thanks for the info, that makes sense :+1:

please check which skipper version is used in the ingress

The version used is v0.11.188-30, so yeah it's before your changes. Ok great, then I think I can close this issue and just wait for rollout of your changes, then the combo of inlineContentIfStatus and clusterClientRateLimit should do what we want :partying_face: