From 419298e1c09b5c01b64b4d316a1d6b24090e8b98 Mon Sep 17 00:00:00 2001 From: Tolga Ceylan Date: Fri, 15 Dec 2017 14:32:25 -0800 Subject: [PATCH] Async hot hdr fix (#604) * fn: for async hot requests ensure/fix content-length/type * fn: added tests for FromModel for content type/length * fn: restrict the content-length fix to async in FromModel() --- api/agent/agent_test.go | 126 +++++++++++++++++++++++++++++++------- api/agent/call.go | 23 ++++++- api/server/runner_test.go | 7 +-- 3 files changed, 129 insertions(+), 27 deletions(-) diff --git a/api/agent/agent_test.go b/api/agent/agent_test.go index b075bb141..1a24244c8 100644 --- a/api/agent/agent_test.go +++ b/api/agent/agent_test.go @@ -17,6 +17,33 @@ import ( "github.com/sirupsen/logrus" ) +func checkExpectedHeaders(t *testing.T, expectedHeaders http.Header, receivedHeaders http.Header) { + + checkMap := make([]string, 0, len(expectedHeaders)) + for k, _ := range expectedHeaders { + checkMap = append(checkMap, k) + } + + for k, vs := range receivedHeaders { + for i, v := range expectedHeaders[k] { + if i >= len(vs) || vs[i] != v { + t.Fatal("header mismatch", k, vs) + } + } + + for i, _ := range checkMap { + if checkMap[i] == k { + checkMap = append(checkMap[:i], checkMap[i+1:]...) + break + } + } + } + + if len(checkMap) > 0 { + t.Fatalf("expected headers not found=%v", checkMap) + } +} + func TestCallConfigurationRequest(t *testing.T) { appName := "myapp" path := "/sleeper" @@ -182,18 +209,7 @@ func TestCallConfigurationRequest(t *testing.T) { expectedHeaders.Add("MYREALHEADER", "FOOPEASANT") expectedHeaders.Add("Content-Length", contentLength) - for k, vs := range req.Header { - for i, v := range expectedHeaders[k] { - if i >= len(vs) || vs[i] != v { - t.Fatal("header mismatch", k, vs) - } - } - delete(expectedHeaders, k) - } - - if len(expectedHeaders) > 0 { - t.Fatal("got extra headers, bad") - } + checkExpectedHeaders(t, expectedHeaders, req.Header) if w.Header()["Fn_call_id"][0] != model.ID { t.Fatal("response writer should have the call id, or else") @@ -265,19 +281,87 @@ func TestCallConfigurationModel(t *testing.T) { expectedHeaders.Add(k, v) } - for k, vs := range req.Header { - for i, v := range expectedHeaders[k] { - if i >= len(vs) || vs[i] != v { - t.Fatal("header mismatch", k, vs) - } - } - delete(expectedHeaders, k) + checkExpectedHeaders(t, expectedHeaders, req.Header) + + var b bytes.Buffer + io.Copy(&b, req.Body) + + if b.String() != payload { + t.Fatal("expected payload to match, but it was a lie") + } +} + +func TestAsyncCallHeaders(t *testing.T) { + appName := "myapp" + path := "/sleeper" + image := "fnproject/sleeper" + const timeout = 1 + const idleTimeout = 20 + const memory = 256 + method := "GET" + url := "http://127.0.0.1:8080/r/" + appName + path + payload := "payload" + typ := "async" + format := "http" + contentType := "suberb_type" + contentLength := strconv.FormatInt(int64(len(payload)), 10) + env := map[string]string{ + "FN_FORMAT": format, + "FN_APP_NAME": appName, + "FN_PATH": path, + "FN_MEMORY": strconv.Itoa(memory), + "FN_TYPE": typ, + "APP_VAR": "FOO", + "ROUTE_VAR": "BAR", + "DOUBLE_VAR": "BIZ, BAZ", + // FromRequest would insert these from original HTTP request + "Fn_header_content_type": contentType, + "Fn_header_content_length": contentLength, } - if len(expectedHeaders) > 0 { - t.Fatal("got extra headers, bad") + cm := &models.Call{ + BaseEnv: env, + EnvVars: env, + AppName: appName, + Path: path, + Image: image, + Type: typ, + Format: format, + Timeout: timeout, + IdleTimeout: idleTimeout, + Memory: memory, + Payload: payload, + URL: url, + Method: method, } + // FromModel doesn't need a datastore, for now... + ds := datastore.NewMockInit(nil, nil, nil) + + a := New(NewCachedDataAccess(NewDirectDataAccess(ds, ds, new(mqs.Mock)))) + defer a.Close() + + callI, err := a.GetCall(FromModel(cm)) + if err != nil { + t.Fatal(err) + } + + // make sure headers seem reasonable + req := callI.(*call).req + + // NOTE these are added as is atm, and if the env vars were comma joined + // they are not again here comma separated. + expectedHeaders := make(http.Header) + for k, v := range env { + expectedHeaders.Add(k, v) + } + + // These should be here based on payload length and/or fn_header_* original headers + expectedHeaders.Set("Content-Type", contentType) + expectedHeaders.Set("Content-Length", strconv.FormatInt(int64(len(payload)), 10)) + + checkExpectedHeaders(t, expectedHeaders, req.Header) + var b bytes.Buffer io.Copy(&b, req.Body) diff --git a/api/agent/call.go b/api/agent/call.go index ee3374ced..a64513b3b 100644 --- a/api/agent/call.go +++ b/api/agent/call.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strconv" "strings" "time" @@ -205,11 +206,31 @@ func FromModel(mCall *models.Call) CallOpt { return func(a *agent, c *call) error { c.Call = mCall - // NOTE this adds content length based on payload length req, err := http.NewRequest(c.Method, c.URL, strings.NewReader(c.Payload)) if err != nil { return err } + + // HACK: only applies to async below, for async we need to restore + // content-length and content-type of the original request, which are + // derived from Payload and original content-type which now is in + // Fn_header_content_type + if c.Type == models.TypeAsync { + // Hoist original request content type into async request + if req.Header.Get("Content-Type") == "" { + content_type, ok := c.EnvVars["Fn_header_content_type"] + if ok { + req.Header.Set("Content-Type", content_type) + } + } + + // Ensure content-length in async requests for protocol/http DumpRequestTo() + if req.ContentLength == -1 || req.Header.Get("Content-Length") == "" { + req.ContentLength = int64(len(c.Payload)) + req.Header.Set("Content-Length", strconv.FormatInt(int64(len(c.Payload)), 10)) + } + } + for k, v := range c.EnvVars { // TODO if we don't store env as []string headers are messed up req.Header.Set(k, v) diff --git a/api/server/runner_test.go b/api/server/runner_test.go index a5cd6dd66..f7f65c85d 100644 --- a/api/server/runner_test.go +++ b/api/server/runner_test.go @@ -243,8 +243,7 @@ func TestRouteRunnerTimeout(t *testing.T) { {Name: "myapp", Config: models.Config{}}, }, []*models.Route{ - {Path: "/pull", AppName: "myapp", Image: "fnproject/sleeper", Type: "sync", Memory: 128, Timeout: 30, IdleTimeout: 30}, - {Path: "/sleeper", AppName: "myapp", Image: "fnproject/sleeper", Type: "sync", Memory: 128, Timeout: 2, IdleTimeout: 30}, + {Path: "/sleeper", AppName: "myapp", Image: "fnproject/sleeper", Type: "sync", Memory: 128, Timeout: 4, IdleTimeout: 30}, {Path: "/waitmemory", AppName: "myapp", Image: "fnproject/sleeper", Type: "sync", Memory: hugeMem, Timeout: 1, IdleTimeout: 30}, }, nil, ) @@ -262,10 +261,8 @@ func TestRouteRunnerTimeout(t *testing.T) { expectedCode int expectedHeaders map[string][]string }{ - // first request with large timeout, we let the docker pull go through... - {"/r/myapp/pull", `{"sleep": 0}`, "POST", http.StatusOK, nil}, {"/r/myapp/sleeper", `{"sleep": 0}`, "POST", http.StatusOK, nil}, - {"/r/myapp/sleeper", `{"sleep": 4}`, "POST", http.StatusGatewayTimeout, nil}, + {"/r/myapp/sleeper", `{"sleep": 5}`, "POST", http.StatusGatewayTimeout, nil}, {"/r/myapp/waitmemory", `{"sleep": 0}`, "POST", http.StatusServiceUnavailable, map[string][]string{"Retry-After": {"15"}}}, } { body := strings.NewReader(test.body)