From 5cdee5579d9629c93f442ecab9b319b25ca9f3b4 Mon Sep 17 00:00:00 2001 From: Travis Reeder Date: Mon, 8 Jan 2018 10:03:33 -0800 Subject: [PATCH] Fixes 404 responses from functions that go through NoRoute path. (#651) * Fixes 404 responses from functions that go through NoRoute path. * cleanup * cleanupp * fix link * Rollback a bad change. --- api/server/runner.go | 40 +++++++++++++++++++++++---------------- api/server/runner_test.go | 8 ++++---- api/server/server.go | 4 ++-- docs/developers/apps.md | 4 ++-- 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/api/server/runner.go b/api/server/runner.go index 21231a09e..f4c649a21 100644 --- a/api/server/runner.go +++ b/api/server/runner.go @@ -2,7 +2,6 @@ package server import ( "bytes" - "errors" "net/http" "path" "time" @@ -15,11 +14,19 @@ import ( "github.com/sirupsen/logrus" ) -// handleFunctionCall executes the function. +// handleFunctionCall executes the function, for router handlers +func (s *Server) handleFunctionCall(c *gin.Context) { + err := s.handleFunctionCall2(c) + if err != nil { + handleErrorResponse(c, err) + } +} + +// handleFunctionCall2 executes the function and returns an error // Requires the following in the context: // * "app_name" // * "path" -func (s *Server) handleFunctionCall(c *gin.Context) { +func (s *Server) handleFunctionCall2(c *gin.Context) error { ctx := c.Request.Context() var p string r := ctx.Value(api.Path) @@ -32,12 +39,15 @@ func (s *Server) handleFunctionCall(c *gin.Context) { var a string ai := ctx.Value(api.AppName) if ai == nil { - handleErrorResponse(c, errors.New("app name not set")) - return + err := models.ErrAppsMissingName + return err } a = ai.(string) - s.serve(c, a, path.Clean(p)) + // gin sets this to 404 on NoRoute, so we'll just ensure it's 200 by default. + c.Status(200) // this doesn't write the header yet + + return s.serve(c, a, path.Clean(p)) } // convert gin.Params to agent.Params to avoid introducing gin @@ -52,7 +62,7 @@ func parseParams(params gin.Params) agent.Params { // TODO it would be nice if we could make this have nothing to do with the gin.Context but meh // TODO make async store an *http.Request? would be sexy until we have different api format... -func (s *Server) serve(c *gin.Context, appName, path string) { +func (s *Server) serve(c *gin.Context, appName, path string) error { // GetCall can mod headers, assign an id, look up the route/app (cached), // strip params, etc. call, err := s.agent.GetCall( @@ -60,8 +70,7 @@ func (s *Server) serve(c *gin.Context, appName, path string) { agent.FromRequest(appName, path, c.Request, parseParams(c.Params)), ) if err != nil { - handleErrorResponse(c, err) - return + return err } model := call.Model() @@ -79,20 +88,18 @@ func (s *Server) serve(c *gin.Context, appName, path string) { buf := bytes.NewBuffer(make([]byte, int(contentLength))[:0]) // TODO sync.Pool me _, err := buf.ReadFrom(c.Request.Body) if err != nil { - handleErrorResponse(c, models.ErrInvalidPayload) - return + return models.ErrInvalidPayload } model.Payload = buf.String() // TODO idk where to put this, but agent is all runner really has... err = s.agent.Enqueue(c.Request.Context(), model) if err != nil { - handleErrorResponse(c, err) - return + return err } c.JSON(http.StatusAccepted, map[string]string{"call_id": model.ID}) - return + return nil } err = s.agent.Submit(call) @@ -106,8 +113,7 @@ func (s *Server) serve(c *gin.Context, appName, path string) { } // NOTE: if the task wrote the headers already then this will fail to write // a 5xx (and log about it to us) -- that's fine (nice, even!) - handleErrorResponse(c, err) - return + return err } // TODO plumb FXLB-WAIT somehow (api?) @@ -115,4 +121,6 @@ func (s *Server) serve(c *gin.Context, appName, path string) { // TODO we need to watch the response writer and if no bytes written // then write a 200 at this point? // c.Data(http.StatusOK) + + return nil } diff --git a/api/server/runner_test.go b/api/server/runner_test.go index 4fa9c0e74..3cbbfa80c 100644 --- a/api/server/runner_test.go +++ b/api/server/runner_test.go @@ -58,8 +58,8 @@ func TestRouteRunnerGet(t *testing.T) { if rec.Code != test.expectedCode { t.Log(buf.String()) - t.Errorf("Test %d: Expected status code to be %d but was %d", - i, test.expectedCode, rec.Code) + t.Errorf("Test %d: Expected status code for path %s to be %d but was %d", + i, test.path, test.expectedCode, rec.Code) } if test.expectedError != nil { @@ -104,8 +104,8 @@ func TestRouteRunnerPost(t *testing.T) { if rec.Code != test.expectedCode { t.Log(buf.String()) - t.Errorf("Test %d: Expected status code to be %d but was %d", - i, test.expectedCode, rec.Code) + t.Errorf("Test %d: Expected status code for path %s to be %d but was %d", + i, test.path, test.expectedCode, rec.Code) } if test.expectedError != nil { diff --git a/api/server/server.go b/api/server/server.go index ade55e8a9..134d2f2ca 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -24,8 +24,8 @@ import ( "github.com/fnproject/fn/api/version" "github.com/fnproject/fn/fnext" "github.com/gin-gonic/gin" - "github.com/opentracing/opentracing-go" - "github.com/openzipkin/zipkin-go-opentracing" + opentracing "github.com/opentracing/opentracing-go" + zipkintracer "github.com/openzipkin/zipkin-go-opentracing" "github.com/sirupsen/logrus" ) diff --git a/docs/developers/apps.md b/docs/developers/apps.md index adb8438e8..082a8458a 100644 --- a/docs/developers/apps.md +++ b/docs/developers/apps.md @@ -48,7 +48,7 @@ http://abc.io/users -> function in users/ directory fn deploy --all ``` -If you're just testing locally, you can speed it up with the `--local` flag. +If you're just testing locally, you can speed it up with the `--local` flag. Or if you want to deploy to a different app, use the `--app APPNAME` flag. ## Deploying a single function in the app @@ -60,4 +60,4 @@ fn deploy hello ## Example app -See https://github.com/fnproject/fn/tree/master/examples/apps/hellos for a simple example. Just clone it and run `fn deploy --all` to see it in action. +See https://github.com/treeder/fn-app-example for a simple example. Just clone it and run `fn deploy --all` to see it in action.