fn: URL parsing updates to fix json request_url (#657)

*) Updated fn-test-utils to latest fdk-go *) Added hot-json to runner tests *) Removed anon function in FromRequest which had a side effect to set req.URL.Host. This is now more explicit and eliminates some corresponding logic in protocol http. *) in gin, http request RequestURI is not set, removed code that references this. (use Call.URL instead)
2022-10-28 21:29:17 +03:00 · 2018-01-08 10:28:50 -08:00
parent b34a31aacc
commit 6f1f5e365d
7 changed files with 52 additions and 58 deletions
--- a/api/agent/call.go
+++ b/api/agent/call.go
@@ -49,6 +49,20 @@ type Param struct {
 }
 type Params []Param

+func fixupRequestURL(req *http.Request) string {
+	if req.URL.Scheme == "" {
+		if req.TLS == nil {
+			req.URL.Scheme = "http"
+		} else {
+			req.URL.Scheme = "https"
+		}
+	}
+	if req.URL.Host == "" {
+		req.URL.Host = req.Host
+	}
+	return req.URL.String()
+}
+
 func FromRequest(appName, path string, req *http.Request, params Params) CallOpt {
 	return func(a *agent, c *call) error {
 		app, err := a.da.GetApp(req.Context(), appName)
@@ -62,9 +76,10 @@ func FromRequest(appName, path string, req *http.Request, params Params) CallOpt
 		}

 		if route.Format == "" {
-			route.Format = "default"
+			route.Format = models.FormatDefault
 		}

+		url := fixupRequestURL(req)
 		id := id.New().String()

 		// baseVars are the vars on the route & app, not on this specific request [for hot functions]
@@ -96,19 +111,7 @@ func FromRequest(appName, path string, req *http.Request, params Params) CallOpt

 		envVars["FN_CALL_ID"] = id
 		envVars["FN_METHOD"] = req.Method
-		envVars["FN_REQUEST_URL"] = func() string {
-			if req.URL.Scheme == "" {
-				if req.TLS == nil {
-					req.URL.Scheme = "http"
-				} else {
-					req.URL.Scheme = "https"
-				}
-			}
-			if req.URL.Host == "" {
-				req.URL.Host = req.Host
-			}
-			return req.URL.String()
-		}()
+		envVars["FN_REQUEST_URL"] = url

 		// params
 		for _, param := range params {
@@ -172,7 +175,7 @@ func FromRequest(appName, path string, req *http.Request, params Params) CallOpt
 			BaseEnv:     baseVars,
 			EnvVars:     envVars,
 			CreatedAt:   strfmt.DateTime(time.Now()),
-			URL:         req.URL.String(), // TODO we should probably strip host/port
+			URL:         url,
 			Method:      req.Method,
 		}

--- a/api/agent/protocol/factory.go
+++ b/api/agent/protocol/factory.go
@@ -72,7 +72,7 @@ func (ci callInfoImpl) Request() *http.Request {
 	return ci.req
 }
 func (ci callInfoImpl) RequestURL() string {
-	return ci.req.URL.RequestURI()
+	return ci.call.URL
 }

 func (ci callInfoImpl) Headers() map[string][]string {
--- a/api/agent/protocol/http.go
+++ b/api/agent/protocol/http.go
@@ -27,7 +27,7 @@ func (p *HTTPProtocol) IsStreamable() bool { return true }
 // TODO maybe we should take io.Writer, io.Reader but then we have to
 // dump the request to a buffer again :(
 func (h *HTTPProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) error {
-	err := DumpRequestTo(h.in, ci.Request()) // TODO timeout
+	err := DumpRequestTo(h.in, ci) // TODO timeout
 	if err != nil {
 		return err
 	}
@@ -70,32 +70,17 @@ func (h *HTTPProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) e
 // 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.
+func DumpRequestTo(w io.Writer, ci CallInfo) error {

-	reqURI := req.RequestURI
-	if reqURI == "" {
-		reqURI = req.URL.RequestURI()
-	}
+	req := ci.Request()
+	reqURI := ci.RequestURL()

 	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)
-		}
+	absRequestURI := strings.HasPrefix(reqURI, "http://") || strings.HasPrefix(reqURI, "https://")
+	if !absRequestURI && req.URL.Host != "" {
+		fmt.Fprintf(w, "Host: %s\r\n", req.URL.Host)
 	}

 	chunked := len(req.TransferEncoding) > 0 && req.TransferEncoding[0] == "chunked"
--- a/api/agent/protocol/json_test.go
+++ b/api/agent/protocol/json_test.go
@@ -16,7 +16,7 @@ type RequestData struct {
 	A string `json:"a"`
 }

-func setupRequest(data interface{}) *http.Request {
+func setupRequest(data interface{}) *callInfoImpl {
 	req := &http.Request{
 		Method: http.MethodPost,
 		URL: &url.URL{
@@ -40,15 +40,20 @@ func setupRequest(data interface{}) *http.Request {
 		_ = json.NewEncoder(&buf).Encode(data)
 	}
 	req.Body = ioutil.NopCloser(&buf)
-	return req
+
+	call := &models.Call{Type: "json"}
+
+	// fixup URL in models.Call
+	call.URL = req.URL.String()
+
+	ci := &callInfoImpl{call, req}
+	return ci
 }

 func TestJSONProtocolwriteJSONInputRequestWithData(t *testing.T) {
 	rDataBefore := RequestData{A: "a"}
-	req := setupRequest(rDataBefore)
+	ci := setupRequest(rDataBefore)
 	r, w := io.Pipe()
-	call := &models.Call{Type: "json"}
-	ci := &callInfoImpl{call, req}
 	proto := JSONProtocol{w, r}
 	go func() {
 		err := proto.writeJSONToContainer(ci)
@@ -77,18 +82,15 @@ func TestJSONProtocolwriteJSONInputRequestWithData(t *testing.T) {
 		t.Errorf("Request data assertion mismatch: expected: %s, got %s",
 			rDataBefore.A, rDataAfter.A)
 	}
-	if incomingReq.Protocol.Type != call.Type {
+	if incomingReq.Protocol.Type != ci.call.Type {
 		t.Errorf("Call protocol type assertion mismatch: expected: %s, got %s",
-			call.Type, incomingReq.Protocol.Type)
+			ci.call.Type, incomingReq.Protocol.Type)
 	}
 }

 func TestJSONProtocolwriteJSONInputRequestWithoutData(t *testing.T) {
-	req := setupRequest(nil)
-
-	call := &models.Call{Type: "json"}
+	ci := setupRequest(nil)
 	r, w := io.Pipe()
-	ci := &callInfoImpl{call, req}
 	proto := JSONProtocol{w, r}
 	go func() {
 		err := proto.writeJSONToContainer(ci)
@@ -112,18 +114,15 @@ func TestJSONProtocolwriteJSONInputRequestWithoutData(t *testing.T) {
 		t.Errorf("Request body assertion mismatch: expected: %s, got %s",
 			"<empty-string>", incomingReq.Body)
 	}
-	if !models.Headers(req.Header).Equals(models.Headers(incomingReq.Protocol.Headers)) {
+	if !models.Headers(ci.req.Header).Equals(models.Headers(incomingReq.Protocol.Headers)) {
 		t.Errorf("Request headers assertion mismatch: expected: %s, got %s",
-			req.Header, incomingReq.Protocol.Headers)
+			ci.req.Header, incomingReq.Protocol.Headers)
 	}
 }

 func TestJSONProtocolwriteJSONInputRequestWithQuery(t *testing.T) {
-	req := setupRequest(nil)
-
+	ci := setupRequest(nil)
 	r, w := io.Pipe()
-	call := &models.Call{Type: "json"}
-	ci := &callInfoImpl{call, req}
 	proto := JSONProtocol{w, r}
 	go func() {
 		err := proto.writeJSONToContainer(ci)
@@ -143,8 +142,8 @@ func TestJSONProtocolwriteJSONInputRequestWithQuery(t *testing.T) {
 	if err != nil {
 		t.Error(err.Error())
 	}
-	if incomingReq.Protocol.RequestURL != req.URL.RequestURI() {
+	if incomingReq.Protocol.RequestURL != ci.call.URL {
 		t.Errorf("Request URL does not match protocol URL: expected: %s, got %s",
-			req.URL.RequestURI(), incomingReq.Protocol.RequestURL)
+			ci.call.URL, incomingReq.Protocol.RequestURL)
 	}
 }
--- a/api/server/runner_async_test.go
+++ b/api/server/runner_async_test.go
@@ -40,6 +40,8 @@ func TestRouteRunnerAsyncExecution(t *testing.T) {
 			{Name: "myapp", Config: map[string]string{"app": "true"}},
 		},
 		[]*models.Route{
+			{Type: "async", Path: "/hot-http", AppName: "myapp", Image: "fnproject/fn-test-utils", Format: "http", Config: map[string]string{"test": "true"}, Memory: 128, Timeout: 4, IdleTimeout: 30},
+			{Type: "async", Path: "/hot-json", AppName: "myapp", Image: "fnproject/fn-test-utils", Format: "json", Config: map[string]string{"test": "true"}, Memory: 128, Timeout: 4, IdleTimeout: 30},
 			{Type: "async", Path: "/myroute", AppName: "myapp", Image: "fnproject/hello", Config: map[string]string{"test": "true"}, Memory: 128, Timeout: 30, IdleTimeout: 30},
 			{Type: "async", Path: "/myerror", AppName: "myapp", Image: "fnproject/error", Config: map[string]string{"test": "true"}, Memory: 128, Timeout: 30, IdleTimeout: 30},
 			{Type: "async", Path: "/myroute/:param", AppName: "myapp", Image: "fnproject/hello", Config: map[string]string{"test": "true"}, Memory: 128, Timeout: 30, IdleTimeout: 30},
@@ -55,6 +57,8 @@ func TestRouteRunnerAsyncExecution(t *testing.T) {
 		expectedEnv  map[string]string
 	}{
 		{"/r/myapp/myroute", ``, map[string][]string{}, http.StatusAccepted, map[string]string{"TEST": "true", "APP": "true"}},
+		{"/r/myapp/hot-http", `{"sleepTime": 0, "isDebug": true}`, map[string][]string{}, http.StatusAccepted, map[string]string{"TEST": "true", "APP": "true"}},
+		{"/r/myapp/hot-json", `{"sleepTime": 0, "isDebug": true}`, map[string][]string{}, http.StatusAccepted, map[string]string{"TEST": "true", "APP": "true"}},
 		// FIXME: this just hangs
 		//{"/r/myapp/myroute/1", ``, map[string][]string{}, http.StatusAccepted, map[string]string{"TEST": "true", "APP": "true"}},
 		{"/r/myapp/myerror", ``, map[string][]string{}, http.StatusAccepted, map[string]string{"TEST": "true", "APP": "true"}},
--- a/api/server/runner_test.go
+++ b/api/server/runner_test.go
@@ -245,6 +245,7 @@ func TestRouteRunnerTimeout(t *testing.T) {
 		[]*models.Route{
 			{Path: "/cold", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Memory: 128, Timeout: 4, IdleTimeout: 30},
 			{Path: "/hot", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Format: "http", Memory: 128, Timeout: 4, IdleTimeout: 30},
+			{Path: "/hot-json", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Format: "json", Memory: 128, Timeout: 4, IdleTimeout: 30},
 			{Path: "/bigmem-cold", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Memory: hugeMem, Timeout: 1, IdleTimeout: 30},
 			{Path: "/bigmem-hot", AppName: "myapp", Image: "fnproject/fn-test-utils", Type: "sync", Format: "http", Memory: hugeMem, Timeout: 1, IdleTimeout: 30},
 		}, nil,
@@ -267,6 +268,8 @@ func TestRouteRunnerTimeout(t *testing.T) {
 		{"/r/myapp/cold", `{"sleepTime": 5000, "isDebug": true}`, "POST", http.StatusGatewayTimeout, nil},
 		{"/r/myapp/hot", `{"sleepTime": 5000, "isDebug": true}`, "POST", http.StatusGatewayTimeout, nil},
 		{"/r/myapp/hot", `{"sleepTime": 0, "isDebug": true}`, "POST", http.StatusOK, nil},
+		{"/r/myapp/hot-json", `{"sleepTime": 5000, "isDebug": true}`, "POST", http.StatusGatewayTimeout, nil},
+		{"/r/myapp/hot-json", `{"sleepTime": 0, "isDebug": true}`, "POST", http.StatusOK, nil},
 		{"/r/myapp/bigmem-cold", `{"sleepTime": 0, "isDebug": true}`, "POST", http.StatusServiceUnavailable, map[string][]string{"Retry-After": {"15"}}},
 		{"/r/myapp/bigmem-hot", `{"sleepTime": 0, "isDebug": true}`, "POST", http.StatusServiceUnavailable, map[string][]string{"Retry-After": {"15"}}},
 	} {
--- a/images/fn-test-utils/Gopkg.lock
+++ b/images/fn-test-utils/Gopkg.lock
@@ -5,7 +5,7 @@
  branch = "master"
  name = "github.com/fnproject/fdk-go"
  packages = ["."]
-  revision = "ce12b15e559bb56980c4134cbeadb99db9cd563a"
+  revision = "2d62538bfa636d3135c957ec7b78f2bedeeffa3d"

 [solve-meta]
  analyzer-name = "dep"