Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json handler: return full ratelimit service response as json #148

Merged
merged 3 commits into from
Jun 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,32 @@ Takes an HTTP POST with a JSON body of the form e.g.
}
```
The service will return an http 200 if this request is allowed (if no ratelimits exceeded) or 429 if one or more
ratelimits were exceeded. Endpoint does not currently return detailed information on which limits were exceeded.
ratelimits were exceeded.

The response is a RateLimitResponse encoded with
[proto3-to-json mapping](https://developers.google.com/protocol-buffers/docs/proto3#json):
```json
{
"overallCode": "OVER_LIMIT",
"statuses": [
{
"code": "OVER_LIMIT",
"currentLimit": {
"requestsPerUnit": 1,
"unit": "MINUTE"
}
},
{
"code": "OK",
"currentLimit": {
"requestsPerUnit": 2,
"unit": "MINUTE"
},
"limitRemaining": 1
}
]
}
```

# Debug Port

Expand Down
38 changes: 28 additions & 10 deletions src/server/server_impl.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package server

import (
"encoding/json"
"bytes"
"expvar"
"fmt"
"io"
Expand All @@ -19,6 +19,7 @@ import (
pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2"
"github.com/envoyproxy/ratelimit/src/limiter"
"github.com/envoyproxy/ratelimit/src/settings"
"github.com/golang/protobuf/jsonpb"
"github.com/gorilla/mux"
reuseport "github.com/kavu/go_reuseport"
"github.com/lyft/goruntime/loader"
Expand Down Expand Up @@ -53,15 +54,18 @@ func (server *server) AddDebugHttpEndpoint(path string, help string, handler htt
server.debugListener.endpoints[path] = help
}

// add an http/1 handler at the /json endpoint which allows this ratelimit service to work with
// create an http/1 handler at the /json endpoint which allows this ratelimit service to work with
// clients that cannot use the gRPC interface (e.g. lua)
// example usage from cURL with domain "dummy" and descriptor "perday":
// echo '{"domain": "dummy", "descriptors": [{"entries": [{"key": "perday"}]}]}' | curl -vvvXPOST --data @/dev/stdin localhost:8080/json
func (server *server) AddJsonHandler(svc pb.RateLimitServiceServer) {
handler := func(writer http.ResponseWriter, request *http.Request) {
func NewJsonHandler(svc pb.RateLimitServiceServer) func(http.ResponseWriter, *http.Request) {
// Default options include enums as strings and no identation.
m := &jsonpb.Marshaler{}

return func(writer http.ResponseWriter, request *http.Request) {
var req pb.RateLimitRequest

if err := json.NewDecoder(request.Body).Decode(&req); err != nil {
if err := jsonpb.Unmarshal(request.Body, &req); err != nil {
logger.Warnf("error: %s", err.Error())
http.Error(writer, err.Error(), http.StatusBadRequest)
return
Expand All @@ -73,15 +77,29 @@ func (server *server) AddJsonHandler(svc pb.RateLimitServiceServer) {
http.Error(writer, err.Error(), http.StatusBadRequest)
return
}

logger.Debugf("resp:%s", resp)
if resp.OverallCode == pb.RateLimitResponse_OVER_LIMIT {
http.Error(writer, "over limit", http.StatusTooManyRequests)
} else if resp.OverallCode == pb.RateLimitResponse_UNKNOWN {
http.Error(writer, "unknown", http.StatusInternalServerError)

buf := bytes.NewBuffer(nil)
err = m.Marshal(buf, resp)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this actually happen? If not can you assert it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that there probably couldn't be a serialization error but probably could be an I/O error if the remote peer closes the TCP connection, in which case the fmt.Fprintf() would fail anyway.

I could split the serialization from the writing to the socket and do a assert/panic and recover dance to print the stack trace if a serialization error happens. If that's the route, I'd kind of rather just do a normal log.Error() instead of an assert because I don't think the extra code to deal with panic()s to get a stack trace will provide much value.

Up to you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine either way, I mostly just want to avoid error handling and just crash if there are things that should never happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My sense is:

  • handling the error differently than how it's handled here is probably more code
  • I'd be comfortable leaving the code here as-is and waiting until someone has evidence that a json serialization error can actually happen before writing additional logic to handle it in a more nuanced way

More generally on the topic of when to crash the process, it seems like crashing a process makes sense when:

  • The process is permanently severely degraded or broken
  • Restarting has a decent chance of un-wedging it

My sense is that a single unknown json serialization error in isolation probably doesn't justify a crash, vs something like a resource allocation issue potentially could justify a crash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guiding principle is whether you can test this or not. I don't think you can test this branch, as such I would use https://github.com/envoyproxy/ratelimit/blob/master/src/assert/assert.go and not handle the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added unit tests for all the cases. For the serialization failure I used a 'nil' return value from ShouldRateLimit(). A non-nil serialization problem would theoretically be possible if the ShouldRateLimit() API ever evolves to include Any types, but I don't think there are Any types in the spec today

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks. Will take a look.

logger.Errorf("error marshaling proto3 to json: %s", err.Error())
http.Error(writer, "error marshaling proto3 to json: "+err.Error(), http.StatusInternalServerError)
return
}

writer.Header().Set("Content-Type", "application/json")
if resp == nil || resp.OverallCode == pb.RateLimitResponse_UNKNOWN {
writer.WriteHeader(http.StatusInternalServerError)
} else if resp.OverallCode == pb.RateLimitResponse_OVER_LIMIT {
writer.WriteHeader(http.StatusTooManyRequests)
}
writer.Write(buf.Bytes())
}
server.router.HandleFunc("/json", handler)
}

func (server *server) AddJsonHandler(svc pb.RateLimitServiceServer) {
server.router.HandleFunc("/json", NewJsonHandler(svc))
}

func (server *server) GrpcServer() *grpc.Server {
Expand Down
5 changes: 5 additions & 0 deletions test/integration/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package integration_test
import (
"bytes"
"fmt"
"io/ioutil"
"math/rand"
"net/http"
"os"
Expand Down Expand Up @@ -376,11 +377,15 @@ func TestBasicConfigLegacy(t *testing.T) {
}`)
http_resp, _ := http.Post("http://localhost:8082/json", "application/json", bytes.NewBuffer(json_body))
assert.Equal(http_resp.StatusCode, 200)
body, _ := ioutil.ReadAll(http_resp.Body)
http_resp.Body.Close()
assert.Equal(`{"overallCode":"OK","statuses":[{"code":"OK","currentLimit":{"requestsPerUnit":1,"unit":"MINUTE"}}]}`, string(body))

http_resp, _ = http.Post("http://localhost:8082/json", "application/json", bytes.NewBuffer(json_body))
assert.Equal(http_resp.StatusCode, 429)
body, _ = ioutil.ReadAll(http_resp.Body)
http_resp.Body.Close()
assert.Equal(`{"overallCode":"OVER_LIMIT","statuses":[{"code":"OVER_LIMIT","currentLimit":{"requestsPerUnit":1,"unit":"MINUTE"}}]}`, string(body))

invalid_json := []byte(`{"unclosed quote: []}`)
http_resp, _ = http.Post("http://localhost:8082/json", "application/json", bytes.NewBuffer(invalid_json))
Expand Down
1 change: 1 addition & 0 deletions test/mocks/mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ package mocks
//go:generate go run github.com/golang/mock/mockgen -destination ./config/config.go github.com/envoyproxy/ratelimit/src/config RateLimitConfig,RateLimitConfigLoader
//go:generate go run github.com/golang/mock/mockgen -destination ./redis/redis.go github.com/envoyproxy/ratelimit/src/redis Client
//go:generate go run github.com/golang/mock/mockgen -destination ./limiter/limiter.go github.com/envoyproxy/ratelimit/src/limiter RateLimitCache,TimeSource,JitterRandSource
//go:generate go run github.com/golang/mock/mockgen -destination ./rls/rls.go github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2 RateLimitServiceServer
50 changes: 50 additions & 0 deletions test/mocks/rls/rls.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

86 changes: 86 additions & 0 deletions test/server/server_impl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package server_test

import (
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"strings"
"testing"

pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2"

"github.com/envoyproxy/ratelimit/src/server"
mock_v2 "github.com/envoyproxy/ratelimit/test/mocks/rls"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
)

func assertHttpResponse(t *testing.T,
handler http.HandlerFunc,
requestBody string,
expectedStatusCode int,
expectedContentType string,
expectedResponseBody string) {

t.Helper()
assert := assert.New(t)

req := httptest.NewRequest("METHOD_NOT_CHECKED", "/path_not_checked", strings.NewReader(requestBody))
w := httptest.NewRecorder()
handler(w, req)

resp := w.Result()
actualBody, _ := ioutil.ReadAll(resp.Body)
assert.Equal(expectedContentType, resp.Header.Get("Content-Type"))
assert.Equal(expectedStatusCode, resp.StatusCode)
assert.Equal(expectedResponseBody, string(actualBody))
}

func TestJsonHandler(t *testing.T) {
controller := gomock.NewController(t)
defer controller.Finish()

rls := mock_v2.NewMockRateLimitServiceServer(controller)
handler := server.NewJsonHandler(rls)

// Missing request body
assertHttpResponse(t, handler, "", 400, "text/plain; charset=utf-8", "EOF\n")

// Request body is not valid json
assertHttpResponse(t, handler, "}", 400, "text/plain; charset=utf-8", "invalid character '}' looking for beginning of value\n")

// Unknown response code
rls.EXPECT().ShouldRateLimit(nil, &pb.RateLimitRequest{
Domain: "foo",
}).Return(&pb.RateLimitResponse{}, nil)
assertHttpResponse(t, handler, `{"domain": "foo"}`, 500, "application/json", "{}")

// ratelimit service error
rls.EXPECT().ShouldRateLimit(nil, &pb.RateLimitRequest{
Domain: "foo",
}).Return(nil, fmt.Errorf("some error"))
assertHttpResponse(t, handler, `{"domain": "foo"}`, 400, "text/plain; charset=utf-8", "some error\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I kinda think this sort of error from ShouldRateLimit should result in a 500 rather than a 400, but changing that is not in scope for this PR.


// json unmarshaling error
rls.EXPECT().ShouldRateLimit(nil, &pb.RateLimitRequest{
Domain: "foo",
}).Return(nil, nil)
assertHttpResponse(t, handler, `{"domain": "foo"}`, 500, "text/plain; charset=utf-8", "error marshaling proto3 to json: Marshal called with nil\n")

// successful request, not rate limited
rls.EXPECT().ShouldRateLimit(nil, &pb.RateLimitRequest{
Domain: "foo",
}).Return(&pb.RateLimitResponse{
OverallCode: pb.RateLimitResponse_OK,
}, nil)
assertHttpResponse(t, handler, `{"domain": "foo"}`, 200, "application/json", `{"overallCode":"OK"}`)

// successful request, rate limited
rls.EXPECT().ShouldRateLimit(nil, &pb.RateLimitRequest{
Domain: "foo",
}).Return(&pb.RateLimitResponse{
OverallCode: pb.RateLimitResponse_OVER_LIMIT,
}, nil)
assertHttpResponse(t, handler, `{"domain": "foo"}`, 429, "application/json", `{"overallCode":"OVER_LIMIT"}`)
}