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

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

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)

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)