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

@@ -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()
 

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:

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.StatusBadGateway, expHeaders, "invalid http response"},
-		{"/r/myapp/myhot", badHttp, "GET", http.StatusInternalServerError, expHeaders, "internal server error"},
 		// 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)