diff --git a/api/agent/agent_test.go b/api/agent/agent_test.go index 7c5e6109e..6366ac140 100644 --- a/api/agent/agent_test.go +++ b/api/agent/agent_test.go @@ -1,7 +1,9 @@ package agent import ( + "bufio" "bytes" + "encoding/json" "fmt" "io" "net/http" @@ -407,3 +409,89 @@ func TestSubmitError(t *testing.T) { t.Fatal("expected error string to be set on call") } } + +// this implements io.Reader, but importantly, is not a strings.Reader or +// a type of reader than NewRequest can identify to set the content length +// (important, for some tests) +type dummyReader struct { + inner io.Reader +} + +func (d *dummyReader) Read(b []byte) (int, error) { + return d.inner.Read(b) +} + +func TestHTTPWithoutContentLengthWorks(t *testing.T) { + // TODO it may be a good idea to mock out the http server and use a real + // response writer with sync, and also test that this works with async + log + + appName := "myapp" + path := "/hello" + url := "http://127.0.0.1:8080/r/" + appName + path + + // we need to load in app & route so that FromRequest works + ds := datastore.NewMockInit( + []*models.App{ + {Name: appName}, + }, + []*models.Route{ + { + Path: path, + AppName: appName, + Image: "fnproject/fn-test-utils", + Type: "sync", + Format: "http", // this _is_ the test + Timeout: 5, + IdleTimeout: 10, + Memory: 128, + }, + }, nil, + ) + + a := New(NewDirectDataAccess(ds, ds, new(mqs.Mock))) + defer a.Close() + + bodOne := `{"echoContent":"yodawg"}` + + // get a req that uses the dummy reader, so that this can't determine + // the size of the body and set content length (user requests may also + // forget to do this, and we _should_ read it as chunked without issue). + req, err := http.NewRequest("GET", url, &dummyReader{strings.NewReader(bodOne)}) + if err != nil { + t.Fatal("unexpected error building request", err) + } + + // grab a buffer so we can read what gets written to this guy + var out bytes.Buffer + callI, err := a.GetCall(FromRequest(appName, path, req), WithWriter(&out)) + if err != nil { + t.Fatal(err) + } + + err = a.Submit(callI) + if err != nil { + t.Error("submit should not error:", err) + } + + // we're using http format so this will have written a whole http request + res, err := http.ReadResponse(bufio.NewReader(&out), nil) + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + + // {"request":{"echoContent":"yodawg"}} + var resp struct { + R struct { + Body string `json:"echoContent"` + } `json:"request"` + } + + json.NewDecoder(res.Body).Decode(&resp) + + if resp.R.Body != "yodawg" { + t.Fatal(`didn't get a yodawg in the body, http protocol may be fudged up + (to debug, recommend ensuring inside the function gets 'Transfer-Encoding: chunked' if + no Content-Length is set. also make sure the body makes it (and the image hasn't changed)); GLHF, got:`, resp.R.Body) + } +} diff --git a/api/agent/protocol/http.go b/api/agent/protocol/http.go index 795b3e0d5..635c76e3d 100644 --- a/api/agent/protocol/http.go +++ b/api/agent/protocol/http.go @@ -3,11 +3,8 @@ package protocol import ( "bufio" "context" - "fmt" "io" "net/http" - "net/http/httputil" - "strings" ) // HTTPProtocol converts stdin/stdout streams into HTTP/1.1 compliant @@ -26,123 +23,40 @@ func (p *HTTPProtocol) IsStreamable() bool { return true } func (h *HTTPProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) error { req := ci.Request() - req.RequestURI = ci.RequestURL() // force set to this, for DumpRequestTo to use + req.RequestURI = ci.RequestURL() // force set to this, for req.Write to use (TODO? still?) - err := DumpRequestTo(h.in, req) // TODO timeout + // req.Write handles if the user does not specify content length + err := req.Write(h.in) if err != nil { return err } - if rw, ok := w.(http.ResponseWriter); ok { - // if we're writing directly to the response writer, we need to set headers - // and status code first since calling res.Write will just write the http - // response as the body (headers and all) + resp, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout + if err != nil { + return err + } + defer resp.Body.Close() - res, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout - if err != nil { - return err - } - - for k, vs := range res.Header { - for _, v := range vs { - rw.Header().Add(k, v) // on top of any specified on the route - } - } - rw.WriteHeader(res.StatusCode) - // TODO should we TCP_CORK ? - - io.Copy(rw, res.Body) // TODO timeout - res.Body.Close() - } else { - // logs can just copy the full thing in there, headers and all. - - res, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout - if err != nil { - return err - } - res.Write(w) // TODO timeout + rw, ok := w.(http.ResponseWriter) + if !ok { + // async / [some] tests go through here. write a full http request to the writer + resp.Write(w) // TODO timeout + return nil } + // if we're writing directly to the response writer, we need to set headers + // and status code, and only copy the body. resp.Write would copy a full + // http request into the response body (not what we want). + + // add resp's on top of any specified on the route [on rw] + for k, vs := range resp.Header { + for _, v := range vs { + rw.Header().Add(k, v) + } + } + if resp.StatusCode > 0 { + rw.WriteHeader(resp.StatusCode) + } + io.Copy(rw, resp.Body) return nil } - -// DumpRequestTo is httputil.DumpRequest with some modifications. It will -// dump the request to the provided io.Writer with the body always, consuming -// the body in the process. -// -// TODO we should support h2! -func DumpRequestTo(w io.Writer, req *http.Request) error { - // By default, print out the unmodified req.RequestURI, which - // is always set for incoming server requests. But because we - // previously used req.URL.RequestURI and the docs weren't - // always so clear about when to use DumpRequest vs - // DumpRequestOut, fall back to the old way if the caller - // provides a non-server Request. - - reqURI := req.RequestURI - if reqURI == "" { - reqURI = req.URL.RequestURI() - } - - fmt.Fprintf(w, "%s %s HTTP/%d.%d\r\n", valueOrDefault(req.Method, "GET"), - reqURI, req.ProtoMajor, req.ProtoMinor) - - absRequestURI := strings.HasPrefix(req.RequestURI, "http://") || strings.HasPrefix(req.RequestURI, "https://") - if !absRequestURI { - host := req.Host - if host == "" && req.URL != nil { - host = req.URL.Host - } - - if host != "" { - fmt.Fprintf(w, "Host: %s\r\n", host) - } - } - - chunked := len(req.TransferEncoding) > 0 && req.TransferEncoding[0] == "chunked" - - if len(req.TransferEncoding) > 0 { - fmt.Fprintf(w, "Transfer-Encoding: %s\r\n", strings.Join(req.TransferEncoding, ",")) - } - - if req.Close { - fmt.Fprintf(w, "Connection: close\r\n") - } - - err := req.Header.WriteSubset(w, reqWriteExcludeHeaderDump) - if err != nil { - return err - } - - io.WriteString(w, "\r\n") - - if req.Body != nil { - var dest io.Writer = w - if chunked { - dest = httputil.NewChunkedWriter(dest) - } - - // TODO copy w/ ctx - _, err = io.Copy(dest, req.Body) - if chunked { - dest.(io.Closer).Close() - io.WriteString(w, "\r\n") - } - } - - return err -} - -var reqWriteExcludeHeaderDump = map[string]bool{ - "Host": true, // not in Header map anyway - "Transfer-Encoding": true, - "Trailer": true, -} - -// Return value if nonempty, def otherwise. -func valueOrDefault(value, def string) string { - if value != "" { - return value - } - return def -}