From 580df031031b33c3d3ccd89fb12eeea34af12296 Mon Sep 17 00:00:00 2001 From: David Weitzman Date: Fri, 12 Jun 2020 11:48:34 -0700 Subject: [PATCH] json handler: return full ratelimit service response as json (#148) Previously an HTTP POST to /json would only return an HTTP status code, not all the other details supported by grpc ratelimit responses. With this change an HTTP POST to /json receives the full proto3 response encoded as json by jsonpb. It seems unlikely that anyone would be parsing the text "over limit" from the HTTP body instead of just reading the 429 response code, but for anyone doing that this would be a breaking change. Signed-off-by: David Weitzman --- README.md | 27 ++++++++- src/server/server_impl.go | 38 ++++++++---- test/integration/integration_test.go | 5 ++ test/mocks/mocks.go | 1 + test/mocks/rls/rls.go | 50 ++++++++++++++++ test/server/server_impl_test.go | 86 ++++++++++++++++++++++++++++ 6 files changed, 196 insertions(+), 11 deletions(-) create mode 100644 test/mocks/rls/rls.go create mode 100644 test/server/server_impl_test.go diff --git a/README.md b/README.md index 0640cb3c1..eca0d0028 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/server/server_impl.go b/src/server/server_impl.go index da92df840..9a1239a96 100644 --- a/src/server/server_impl.go +++ b/src/server/server_impl.go @@ -1,7 +1,7 @@ package server import ( - "encoding/json" + "bytes" "expvar" "fmt" "io" @@ -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" @@ -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 @@ -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 { + 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 { diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index 60a25257b..6e1b9efd6 100644 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -5,6 +5,7 @@ package integration_test import ( "bytes" "fmt" + "io/ioutil" "math/rand" "net/http" "os" @@ -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)) diff --git a/test/mocks/mocks.go b/test/mocks/mocks.go index 490d20977..703865af0 100644 --- a/test/mocks/mocks.go +++ b/test/mocks/mocks.go @@ -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 diff --git a/test/mocks/rls/rls.go b/test/mocks/rls/rls.go new file mode 100644 index 000000000..77cd49ae9 --- /dev/null +++ b/test/mocks/rls/rls.go @@ -0,0 +1,50 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2 (interfaces: RateLimitServiceServer) + +// Package mock_v2 is a generated GoMock package. +package mock_v2 + +import ( + context "context" + v2 "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v2" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockRateLimitServiceServer is a mock of RateLimitServiceServer interface +type MockRateLimitServiceServer struct { + ctrl *gomock.Controller + recorder *MockRateLimitServiceServerMockRecorder +} + +// MockRateLimitServiceServerMockRecorder is the mock recorder for MockRateLimitServiceServer +type MockRateLimitServiceServerMockRecorder struct { + mock *MockRateLimitServiceServer +} + +// NewMockRateLimitServiceServer creates a new mock instance +func NewMockRateLimitServiceServer(ctrl *gomock.Controller) *MockRateLimitServiceServer { + mock := &MockRateLimitServiceServer{ctrl: ctrl} + mock.recorder = &MockRateLimitServiceServerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockRateLimitServiceServer) EXPECT() *MockRateLimitServiceServerMockRecorder { + return m.recorder +} + +// ShouldRateLimit mocks base method +func (m *MockRateLimitServiceServer) ShouldRateLimit(arg0 context.Context, arg1 *v2.RateLimitRequest) (*v2.RateLimitResponse, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ShouldRateLimit", arg0, arg1) + ret0, _ := ret[0].(*v2.RateLimitResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ShouldRateLimit indicates an expected call of ShouldRateLimit +func (mr *MockRateLimitServiceServerMockRecorder) ShouldRateLimit(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ShouldRateLimit", reflect.TypeOf((*MockRateLimitServiceServer)(nil).ShouldRateLimit), arg0, arg1) +} diff --git a/test/server/server_impl_test.go b/test/server/server_impl_test.go new file mode 100644 index 000000000..de058ecbc --- /dev/null +++ b/test/server/server_impl_test.go @@ -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") + + // 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"}`) +}