From 97194b3d8b7216fe53e3086ffe7513eb8759d916 Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Mon, 12 Feb 2018 17:51:45 -0800 Subject: [PATCH] return bad function http resp error (#728) * return bad function http resp error this was being thrown into the fn server logs but it's relatively easy to get this to crop up if a function user forgets that they left a `println` laying around that gets written to stdout, it garbles the http (or json, in its case) output and they just see 'internal server error'. for certain clients i could see that we really do want to keep this as 'internal server error' but for things like e.g. docker image not authorized we're showing that in the response, so this seems apt. json likely needs the same treatment, will file a bug. as always, my error messages are rarely helpful enough, help me please :) closes #355 * add formatting directive * fix up http error * output bad jasons to user closes #729 woo --- api/agent/protocol/http.go | 5 ++++- api/agent/protocol/json.go | 4 +++- api/server/runner_test.go | 14 ++++++-------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/api/agent/protocol/http.go b/api/agent/protocol/http.go index ea2761e0c..fdc6c6646 100644 --- a/api/agent/protocol/http.go +++ b/api/agent/protocol/http.go @@ -3,8 +3,11 @@ package protocol import ( "bufio" "context" + "fmt" "io" "net/http" + + "github.com/fnproject/fn/api/models" ) // HTTPProtocol converts stdin/stdout streams into HTTP/1.1 compliant @@ -37,7 +40,7 @@ func (h *HTTPProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) e resp, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) if err != nil { - return err + return models.NewAPIError(http.StatusBadGateway, fmt.Errorf("invalid http response from function err: %v", err)) } defer resp.Body.Close() diff --git a/api/agent/protocol/json.go b/api/agent/protocol/json.go index e601c9675..8bf1bdf6d 100644 --- a/api/agent/protocol/json.go +++ b/api/agent/protocol/json.go @@ -7,6 +7,8 @@ import ( "fmt" "io" "net/http" + + "github.com/fnproject/fn/api/models" ) // This is sent into the function @@ -197,7 +199,7 @@ func (h *JSONProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) e jout := new(jsonOut) dec := json.NewDecoder(h.out) if err := dec.Decode(jout); err != nil { - return fmt.Errorf("error decoding JSON from user function: %v", err) + return models.NewAPIError(http.StatusBadGateway, fmt.Errorf("invalid json response from function err: %v", err)) } if rw, ok := w.(http.ResponseWriter); ok { // this has to be done for pulling out: diff --git a/api/server/runner_test.go b/api/server/runner_test.go index 7ecba9a4b..63c4c5bf1 100644 --- a/api/server/runner_test.go +++ b/api/server/runner_test.go @@ -132,6 +132,7 @@ func TestRouteRunnerExecution(t *testing.T) { []*models.Route{ {Path: "/", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Memory: 128, Timeout: 30, IdleTimeout: 30, Headers: map[string][]string{"X-Function": {"Test"}}}, {Path: "/myhot", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Format: "http", Memory: 128, Timeout: 30, IdleTimeout: 30, Headers: map[string][]string{"X-Function": {"Test"}}}, + {Path: "/myhotjason", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Format: "json", Memory: 128, Timeout: 30, IdleTimeout: 30, Headers: map[string][]string{"X-Function": {"Test"}}}, {Path: "/myroute", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Memory: 128, Timeout: 30, IdleTimeout: 30, Headers: map[string][]string{"X-Function": {"Test"}}}, {Path: "/myerror", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Memory: 128, Timeout: 30, IdleTimeout: 30, Headers: map[string][]string{"X-Function": {"Test"}}}, {Path: "/mydne", AppName: "myapp", Image: "fnproject/imagethatdoesnotexist", Type: "sync", Memory: 128, Timeout: 30, IdleTimeout: 30}, @@ -152,6 +153,7 @@ func TestRouteRunnerExecution(t *testing.T) { crasher := `{"sleepTime": 0, "isDebug": true, "isCrash": true}` // crash container oomer := `{"sleepTime": 0, "isDebug": true, "allocateMemory": 12000000}` // ask for 12MB badHttp := `{"sleepTime": 0, "isDebug": true, "responseCode": -1}` // http status of -1 (invalid http) + badHot := `{"invalidResponse": true, "isDebug": true}` // write a not json/http as output ok := `{"sleepTime": 0, "isDebug": true}` // good response / ok for i, test := range []struct { @@ -164,21 +166,17 @@ func TestRouteRunnerExecution(t *testing.T) { }{ {"/r/myapp/", ok, "GET", http.StatusOK, expHeaders, ""}, - // TODO: this should return 502 and description about container misbehaving - {"/r/myapp/myhot", badHttp, "GET", http.StatusInternalServerError, expHeaders, "internal server error"}, + {"/r/myapp/myhot", badHttp, "GET", http.StatusBadGateway, expHeaders, "invalid http response"}, // hot container now back to normal, we should get OK {"/r/myapp/myhot", ok, "GET", http.StatusOK, expHeaders, ""}, + {"/r/myapp/myhot", badHot, "GET", http.StatusBadGateway, expHeaders, "invalid http response"}, + {"/r/myapp/myhotjason", badHot, "GET", http.StatusBadGateway, expHeaders, "invalid json response"}, + {"/r/myapp/myroute", ok, "GET", http.StatusOK, expHeaders, ""}, {"/r/myapp/myerror", crasher, "GET", http.StatusBadGateway, expHeaders, "container exit code 2"}, {"/r/myapp/mydne", ``, "GET", http.StatusNotFound, nil, "pull access denied"}, {"/r/myapp/mydnehot", ``, "GET", http.StatusNotFound, nil, "pull access denied"}, - - // Added same tests again to check if time is reduced by the auth cache - {"/r/myapp/", ok, "GET", http.StatusOK, expHeaders, ""}, - {"/r/myapp/myroute", ok, "GET", http.StatusOK, expHeaders, ""}, - {"/r/myapp/myerror", crasher, "GET", http.StatusBadGateway, expHeaders, "container exit code 2"}, - {"/r/myapp/myerror", crasher, "GET", http.StatusBadGateway, expHeaders, "container exit code 2"}, {"/r/myapp/myoom", oomer, "GET", http.StatusBadGateway, nil, "container out of memory"}, } { body := strings.NewReader(test.body)