From 904957aaa96eceb8c36ec3978ededcc35304c7ee Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 12 Jan 2022 17:25:58 -0800 Subject: [PATCH] rpc: rework how responses are written back via HTTP (#7575) Add writeRPCResponse and writeHTTPResponse helpers, that handle the way RPC responses are written to HTTP replies. These replace the exported helpers. Visible effects: - JSON results are now marshaled without indentation. - HTTP status codes are now normalized. - Cache control headers are no longer set. Details: - When writing a response to a URL (GET) request, do not marshal the whole JSON-RPC object into the body, only encode the result or the error object. This is a user-visible change. - Do not change the HTTP status code for RPC errors. The RPC error already reports what went wrong, the HTTP status should only report problems with the HTTP transaction itself. This is a user-visible change. - Encode JSON without indentation in POST response bodies. This is mainly cosmetic but saves quite a bit of response data. Indent is still applied to GET responses to make life easier for code examples. - Remove an obsolete TODO about reporting an HTTP error on websocket upgrade. Nothing needed to change; the upgrader already reports an error. - Report an HTTP error when starting the server loop fails. - Improve logging for encoding errors. - Log less aggressively. --- CHANGELOG_PENDING.md | 3 +- rpc/jsonrpc/server/http_json_handler.go | 21 ++---- rpc/jsonrpc/server/http_server.go | 99 +++++++++++-------------- rpc/jsonrpc/server/http_server_test.go | 55 ++++---------- rpc/jsonrpc/server/http_uri_handler.go | 49 +++--------- rpc/jsonrpc/server/ws_handler.go | 12 +-- test/app/kvstore_test.sh | 4 +- 7 files changed, 85 insertions(+), 158 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index b08af302e..1609e9755 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -12,7 +12,8 @@ Special thanks to external contributors on this release: - CLI/RPC/Config - - [rpc] Remove the deprecated gRPC interface to the RPC service. (@creachadair) + - [rpc] \#7575 Rework how RPC responses are written back via HTTP. (@creachadair) + - [rpc] \#7121 Remove the deprecated gRPC interface to the RPC service. (@creachadair) - [blocksync] \#7159 Remove support for disabling blocksync in any circumstance. (@tychoish) - [mempool] \#7171 Remove legacy mempool implementation. (@tychoish) diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index a66e61933..035793d93 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/rpc/jsonrpc/server/http_json_handler.go @@ -21,22 +21,18 @@ import ( // jsonrpc calls grab the given method's function info and runs reflect.Call func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.HandlerFunc { return func(w http.ResponseWriter, hreq *http.Request) { - fail := func(res rpctypes.RPCResponse) { - if err := WriteRPCResponseHTTPError(w, res); err != nil { - logger.Error("Failed writing error response", "res", res, "err", err) - } - } - // For POST requests, reject a non-root URL path. This should not happen // in the standard configuration, since the wrapper checks the path. if hreq.URL.Path != "/" { - fail(rpctypes.RPCInvalidRequestError(nil, fmt.Errorf("invalid path: %q", hreq.URL.Path))) + writeRPCResponse(w, logger, rpctypes.RPCInvalidRequestError( + nil, fmt.Errorf("invalid path: %q", hreq.URL.Path))) return } b, err := io.ReadAll(hreq.Body) if err != nil { - fail(rpctypes.RPCInvalidRequestError(nil, fmt.Errorf("reading request body: %w", err))) + writeRPCResponse(w, logger, rpctypes.RPCInvalidRequestError( + nil, fmt.Errorf("reading request body: %w", err))) return } @@ -49,7 +45,7 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han requests, err := parseRequests(b) if err != nil { - fail(rpctypes.RPCParseError(fmt.Errorf("decoding request: %w", err))) + writeRPCResponse(w, logger, rpctypes.RPCParseError(fmt.Errorf("decoding request: %w", err))) return } @@ -98,11 +94,10 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han } } - if len(responses) > 0 { - if wErr := WriteRPCResponseHTTP(w, responses...); wErr != nil { - logger.Error("failed to write responses", "err", wErr) - } + if len(responses) == 0 { + return } + writeRPCResponse(w, logger, responses...) } } diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go index a9a5ca0b8..c0c2a834f 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/rpc/jsonrpc/server/http_server.go @@ -124,67 +124,58 @@ func ServeTLS( return nil } -// WriteRPCResponseHTTPError marshals res as JSON (with indent) and writes it -// to w. -// -// Maps JSON RPC error codes to HTTP Status codes as follows: -// -// HTTP Status code message -// 500 -32700 Parse error. -// 400 -32600 Invalid Request. -// 404 -32601 Method not found. -// 500 -32602 Invalid params. -// 500 -32603 Internal error. -// 500 -32099..-32000 Server error. +// writeInternalError writes an internal server error (500) to w with the text +// of err in the body. This is a fallback used when a handler is unable to +// write the expected response. +func writeInternalError(w http.ResponseWriter, err error) { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintln(w, err.Error()) +} + +// writeHTTPResponse writes a JSON-RPC response to w. If rsp encodes an error, +// the response body is its error object; otherwise its responses is the result. // -// source: https://www.jsonrpc.org/historical/json-rpc-over-http.html -func WriteRPCResponseHTTPError( - w http.ResponseWriter, - res rpctypes.RPCResponse, -) error { - if res.Error == nil { - panic("tried to write http error response without RPC error") +// Unless there is an error encoding the response, the status is 200 OK. +func writeHTTPResponse(w http.ResponseWriter, log log.Logger, rsp rpctypes.RPCResponse) { + var body []byte + var err error + if rsp.Error != nil { + body, err = json.Marshal(rsp.Error) + } else { + body = rsp.Result } - - jsonBytes, err := json.MarshalIndent(res, "", " ") if err != nil { - return fmt.Errorf("json marshal: %w", err) - } - - var httpCode int - switch res.Error.Code { - case -32600: - httpCode = http.StatusBadRequest - case -32601: - httpCode = http.StatusNotFound - default: - httpCode = http.StatusInternalServerError + log.Error("Error encoding RPC response: %w", err) + writeInternalError(w, err) + return } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(httpCode) - _, err = w.Write(jsonBytes) - return err + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body) } -// WriteRPCResponseHTTP marshals res as JSON (with indent) and writes it to w. -// If the rpc response can be cached, add cache-control to the response header. -func WriteRPCResponseHTTP(w http.ResponseWriter, res ...rpctypes.RPCResponse) error { - var v interface{} - if len(res) == 1 { - v = res[0] +// writeRPCResponse writes one or more JSON-RPC responses to w. A single +// response is encoded as an object, otherwise the response is sent as a batch +// (array) of response objects. +// +// Unless there is an error encoding the responses, the status is 200 OK. +func writeRPCResponse(w http.ResponseWriter, log log.Logger, rsps ...rpctypes.RPCResponse) { + var body []byte + var err error + if len(rsps) == 1 { + body, err = json.Marshal(rsps[0]) } else { - v = res + body, err = json.Marshal(rsps) } - - jsonBytes, err := json.MarshalIndent(v, "", " ") if err != nil { - return fmt.Errorf("json marshal: %w", err) + log.Error("Error encoding RPC response: %w", err) + writeInternalError(w, err) + return } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, err = w.Write(jsonBytes) - return err + _, _ = w.Write(body) } //----------------------------------------------------------------------------- @@ -222,9 +213,7 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler // If RPCResponse if res, ok := e.(rpctypes.RPCResponse); ok { - if wErr := WriteRPCResponseHTTP(rww, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + writeRPCResponse(rww, logger, res) } else { // Panics can contain anything, attempt to normalize it as an error. var err error @@ -238,12 +227,8 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler default: } - logger.Error("panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack())) - - res := rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), err) - if wErr := WriteRPCResponseHTTPError(rww, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + logger.Error("Panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack())) + writeInternalError(rww, err) } } diff --git a/rpc/jsonrpc/server/http_server_test.go b/rpc/jsonrpc/server/http_server_test.go index 7c114e62a..9fd45c1e4 100644 --- a/rpc/jsonrpc/server/http_server_test.go +++ b/rpc/jsonrpc/server/http_server_test.go @@ -117,13 +117,14 @@ func TestServeTLS(t *testing.T) { assert.Equal(t, []byte("some body"), body) } -func TestWriteRPCResponseHTTP(t *testing.T) { +func TestWriteRPCResponse(t *testing.T) { id := rpctypes.JSONRPCIntID(-1) // one argument w := httptest.NewRecorder() - err := WriteRPCResponseHTTP(w, rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"})) - require.NoError(t, err) + logger := log.NewTestingLogger(t) + writeRPCResponse(w, logger, + rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"})) resp := w.Result() body, err := io.ReadAll(resp.Body) _ = resp.Body.Close() @@ -131,21 +132,14 @@ func TestWriteRPCResponseHTTP(t *testing.T) { assert.Equal(t, 200, resp.StatusCode) assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) assert.Equal(t, "", resp.Header.Get("Cache-control")) - assert.Equal(t, `{ - "jsonrpc": "2.0", - "id": -1, - "result": { - "value": "hello" - } -}`, string(body)) + assert.Equal(t, `{"jsonrpc":"2.0","id":-1,"result":{"value":"hello"}}`, string(body)) // multiple arguments w = httptest.NewRecorder() - err = WriteRPCResponseHTTP(w, + writeRPCResponse(w, logger, rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"}), rpctypes.NewRPCSuccessResponse(id, &sampleResult{"world"}), ) - require.NoError(t, err) resp = w.Result() body, err = io.ReadAll(resp.Body) _ = resp.Body.Close() @@ -153,41 +147,20 @@ func TestWriteRPCResponseHTTP(t *testing.T) { assert.Equal(t, 200, resp.StatusCode) assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) - assert.Equal(t, `[ - { - "jsonrpc": "2.0", - "id": -1, - "result": { - "value": "hello" - } - }, - { - "jsonrpc": "2.0", - "id": -1, - "result": { - "value": "world" - } - } -]`, string(body)) + assert.Equal(t, `[{"jsonrpc":"2.0","id":-1,"result":{"value":"hello"}},`+ + `{"jsonrpc":"2.0","id":-1,"result":{"value":"world"}}]`, string(body)) } -func TestWriteRPCResponseHTTPError(t *testing.T) { +func TestWriteHTTPResponse(t *testing.T) { w := httptest.NewRecorder() - err := WriteRPCResponseHTTPError(w, rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), errors.New("foo"))) - require.NoError(t, err) + logger := log.NewTestingLogger(t) + writeHTTPResponse(w, logger, + rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), errors.New("foo"))) resp := w.Result() body, err := io.ReadAll(resp.Body) _ = resp.Body.Close() require.NoError(t, err) - assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) - assert.Equal(t, `{ - "jsonrpc": "2.0", - "id": -1, - "error": { - "code": -32603, - "message": "Internal error", - "data": "foo" - } -}`, string(body)) + assert.Equal(t, `{"code":-32603,"message":"Internal error","data":"foo"}`, string(body)) } diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index fcdfc707a..808da16e2 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/http" - "net/http/httputil" "reflect" "regexp" "strings" @@ -26,30 +25,24 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit dummyID := rpctypes.JSONRPCIntID(-1) // URIClientRequestID // Exception for websocket endpoints + // + // TODO(creachadair): Rather than reporting errors for these, we should + // remove them from the routing list entirely on this endpoint. if rpcFunc.ws { return func(w http.ResponseWriter, r *http.Request) { - res := rpctypes.RPCMethodNotFoundError(dummyID) - if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + w.WriteHeader(http.StatusNotFound) } } // All other endpoints return func(w http.ResponseWriter, r *http.Request) { - logger.Debug("HTTP HANDLER", "req", dumpHTTPRequest(r)) - ctx := rpctypes.WithCallInfo(r.Context(), &rpctypes.CallInfo{HTTPRequest: r}) args := []reflect.Value{reflect.ValueOf(ctx)} fnArgs, err := httpParamsToArgs(rpcFunc, r) if err != nil { - res := rpctypes.RPCInvalidParamsError(dummyID, - fmt.Errorf("error converting http params to arguments: %w", err), - ) - if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + writeHTTPResponse(w, logger, rpctypes.RPCInvalidParamsError( + dummyID, fmt.Errorf("error converting http params to arguments: %w", err))) return } args = append(args, fnArgs...) @@ -61,36 +54,23 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit switch e := err.(type) { // if no error then return a success response case nil: - res := rpctypes.NewRPCSuccessResponse(dummyID, result) - if wErr := WriteRPCResponseHTTP(w, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + writeHTTPResponse(w, logger, rpctypes.NewRPCSuccessResponse(dummyID, result)) // if this already of type RPC error then forward that error. case *rpctypes.RPCError: - res := rpctypes.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data) - if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) - } + writeHTTPResponse(w, logger, rpctypes.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data)) default: // we need to unwrap the error and parse it accordingly - var res rpctypes.RPCResponse - switch errors.Unwrap(err) { case coretypes.ErrZeroOrNegativeHeight, coretypes.ErrZeroOrNegativePerPage, coretypes.ErrPageOutOfRange, coretypes.ErrInvalidRequest: - res = rpctypes.RPCInvalidRequestError(dummyID, err) + writeHTTPResponse(w, logger, rpctypes.RPCInvalidRequestError(dummyID, err)) default: // ctypes.ErrHeightNotAvailable, ctypes.ErrHeightExceedsChainHead: - res = rpctypes.RPCInternalError(dummyID, err) - } - - if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { - logger.Error("failed to write response", "res", res, "err", wErr) + writeHTTPResponse(w, logger, rpctypes.RPCInternalError(dummyID, err)) } } - } } @@ -233,12 +213,3 @@ func getParam(r *http.Request, param string) string { } return s } - -func dumpHTTPRequest(r *http.Request) string { - d, e := httputil.DumpRequest(r, true) - if e != nil { - return e.Error() - } - - return string(d) -} diff --git a/rpc/jsonrpc/server/ws_handler.go b/rpc/jsonrpc/server/ws_handler.go index 845180d62..818e66512 100644 --- a/rpc/jsonrpc/server/ws_handler.go +++ b/rpc/jsonrpc/server/ws_handler.go @@ -71,7 +71,8 @@ func NewWebsocketManager( func (wm *WebsocketManager) WebsocketHandler(w http.ResponseWriter, r *http.Request) { wsConn, err := wm.Upgrade(w, r, nil) if err != nil { - // TODO - return http error + // The upgrader has already reported an HTTP error to the client, so we + // need only log it. wm.logger.Error("Failed to upgrade connection", "err", err) return } @@ -89,6 +90,7 @@ func (wm *WebsocketManager) WebsocketHandler(w http.ResponseWriter, r *http.Requ // starting the conn is blocking if err = conn.Start(r.Context()); err != nil { wm.logger.Error("Failed to start connection", "err", err) + writeInternalError(w, err) return } @@ -453,13 +455,13 @@ func (wsc *wsConnection) writeRoutine(ctx context.Context) { return } case msg := <-wsc.writeChan: - jsonBytes, err := json.MarshalIndent(msg, "", " ") + data, err := json.Marshal(msg) if err != nil { - wsc.Logger.Error("Failed to marshal RPCResponse to JSON", "err", err) + wsc.Logger.Error("Failed to marshal RPCResponse to JSON", "msg", msg, "err", err) continue } - if err = wsc.writeMessageWithDeadline(websocket.TextMessage, jsonBytes); err != nil { - wsc.Logger.Error("Failed to write response", "err", err, "msg", msg) + if err = wsc.writeMessageWithDeadline(websocket.TextMessage, data); err != nil { + wsc.Logger.Error("Failed to write response", "msg", msg, "err", err) return } } diff --git a/test/app/kvstore_test.sh b/test/app/kvstore_test.sh index 034e28878..305a2926c 100755 --- a/test/app/kvstore_test.sh +++ b/test/app/kvstore_test.sh @@ -57,7 +57,7 @@ echo "... testing query with /abci_query 2" # we should be able to look up the key RESPONSE=`curl -s "127.0.0.1:26657/abci_query?path=\"\"&data=$(toHex $KEY)&prove=false"` -RESPONSE=`echo $RESPONSE | jq .result.response.log` +RESPONSE=`echo $RESPONSE | jq .response.log` set +e A=`echo $RESPONSE | grep 'exists'` @@ -70,7 +70,7 @@ set -e # we should not be able to look up the value RESPONSE=`curl -s "127.0.0.1:26657/abci_query?path=\"\"&data=$(toHex $VALUE)&prove=false"` -RESPONSE=`echo $RESPONSE | jq .result.response.log` +RESPONSE=`echo $RESPONSE | jq .response.log` set +e A=`echo $RESPONSE | grep 'exists'` if [[ $? == 0 ]]; then