From af94f3f8ac4e72796337cebf983f6e64b211bea3 Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Tue, 31 Jul 2018 08:58:47 -0700 Subject: [PATCH] move max_request_size from agent to server (#1145) moves the config option for max request size up to the front end, adds the env var for it there, adds a server test for it and removes it from agent. a request is either gonna come through the lb (before grpc) or to the server, we can handle limiting the request there at least now, which may be easier than having multiple layers of request body checking. this aligns with not making the agent as responsible for http behaviors (eventually, not at all once route is fully deprecated). --- api/agent/agent_test.go | 35 ------------------- api/agent/call.go | 15 -------- api/agent/config.go | 4 --- api/agent/lb_agent.go | 5 --- api/server/server.go | 4 +++ api/server/server_options.go | 4 ++- api/server/server_test.go | 68 +++++++++++++++++++++++++----------- 7 files changed, 54 insertions(+), 81 deletions(-) diff --git a/api/agent/agent_test.go b/api/agent/agent_test.go index 357bd7cc0..d6941afd7 100644 --- a/api/agent/agent_test.go +++ b/api/agent/agent_test.go @@ -406,41 +406,6 @@ func (l testListener) BeforeCall(context.Context, *models.Call) error { return nil } -func TestReqTooLarge(t *testing.T) { - app := &models.App{ID: "app_id", Name: "myapp"} - - cm := &models.Call{ - AppID: app.ID, - Config: map[string]string{}, - Path: "/", - Image: "fnproject/fn-test-utils", - Type: "sync", - Format: "json", - Timeout: 10, - IdleTimeout: 20, - Memory: 64, - CPUs: models.MilliCPUs(200), - Payload: `{"sleepTime": 0, "isDebug": true, "isCrash": true}`, - URL: "http://127.0.0.1:8080/r/" + app.Name + "/", - Method: "GET", - } - - cfg, err := NewConfig() - if err != nil { - t.Fatal(err) - } - - cfg.MaxRequestSize = 5 - ls := logs.NewMock() - - a := New(NewDirectCallDataAccess(ls, new(mqs.Mock)), WithConfig(cfg)) - defer checkClose(t, a) - - _, err = a.GetCall(FromModel(cm)) - if err != models.ErrRequestContentTooBig { - t.Fatal(err) - } -} func TestSubmitError(t *testing.T) { app := &models.App{Name: "myapp"} diff --git a/api/agent/call.go b/api/agent/call.go index 622ebcd0c..cec577afe 100644 --- a/api/agent/call.go +++ b/api/agent/call.go @@ -354,11 +354,6 @@ func (a *agent) GetCall(opts ...CallOpt) (Call, error) { return nil, models.ErrCallTimeoutServerBusy } - err := setMaxBodyLimit(&a.cfg, &c) - if err != nil { - return nil, err - } - setupCtx(&c) c.handler = a.da @@ -379,16 +374,6 @@ func setupCtx(c *call) { c.req = c.req.WithContext(ctx) } -func setMaxBodyLimit(cfg *Config, c *call) error { - if cfg.MaxRequestSize > 0 && c.req.ContentLength > 0 && uint64(c.req.ContentLength) > cfg.MaxRequestSize { - return models.ErrRequestContentTooBig - } - if c.req.Body != nil { - c.req.Body = common.NewClampReadCloser(c.req.Body, cfg.MaxRequestSize, models.ErrRequestContentTooBig) - } - return nil -} - type call struct { *models.Call diff --git a/api/agent/config.go b/api/agent/config.go index 00f01d68c..48e1cfc31 100644 --- a/api/agent/config.go +++ b/api/agent/config.go @@ -21,7 +21,6 @@ type Config struct { CallEndTimeout time.Duration `json:"call_end_timeout"` MaxCallEndStacking uint64 `json:"max_call_end_stacking"` MaxResponseSize uint64 `json:"max_response_size_bytes"` - MaxRequestSize uint64 `json:"max_request_size_bytes"` MaxLogSize uint64 `json:"max_log_size_bytes"` MaxTotalCPU uint64 `json:"max_total_cpu_mcpus"` MaxTotalMemory uint64 `json:"max_total_memory_bytes"` @@ -59,8 +58,6 @@ const ( EnvMaxCallEndStacking = "FN_MAX_CALL_END_STACKING" // EnvMaxResponseSize is the maximum number of bytes that a function may return from an invocation EnvMaxResponseSize = "FN_MAX_RESPONSE_SIZE" - // EnvMaxRequestSize is the maximum request size that may be passed to an agent TODO kill me from here - EnvMaxRequestSize = "FN_MAX_REQUEST_SIZE" // EnvMaxLogSize is the maximum size that a function's log may reach EnvMaxLogSize = "FN_MAX_LOG_SIZE_BYTES" // EnvMaxTotalCPU is the maximum CPU that will be reserved across all containers @@ -116,7 +113,6 @@ func NewConfig() (*Config, error) { err = setEnvMsecs(err, EnvAsyncChewPoll, &cfg.AsyncChewPoll, time.Duration(60)*time.Second) err = setEnvMsecs(err, EnvCallEndTimeout, &cfg.CallEndTimeout, time.Duration(10)*time.Minute) err = setEnvUint(err, EnvMaxResponseSize, &cfg.MaxResponseSize) - err = setEnvUint(err, EnvMaxRequestSize, &cfg.MaxRequestSize) err = setEnvUint(err, EnvMaxLogSize, &cfg.MaxLogSize) err = setEnvUint(err, EnvMaxTotalCPU, &cfg.MaxTotalCPU) err = setEnvUint(err, EnvMaxTotalMemory, &cfg.MaxTotalMemory) diff --git a/api/agent/lb_agent.go b/api/agent/lb_agent.go index 7b286bed0..d00ad5a48 100644 --- a/api/agent/lb_agent.go +++ b/api/agent/lb_agent.go @@ -119,11 +119,6 @@ func (a *lbAgent) GetCall(opts ...CallOpt) (Call, error) { c.extensions = ext } - err := setMaxBodyLimit(&a.cfg, &c) - if err != nil { - return nil, err - } - setupCtx(&c) c.isLB = true diff --git a/api/server/server.go b/api/server/server.go index 2b407103e..f0ac6b835 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -122,6 +122,9 @@ const ( // EnvLBPlacementAlg is the algorithm to place fn calls to fn runners in lb.[0w EnvLBPlacementAlg = "FN_PLACER" + // EnvMaxRequestSize sets the limit in bytes for any API request's length. + EnvMaxRequestSize = "FN_MAX_REQUEST_SIZE" + // DefaultLogLevel is info DefaultLogLevel = "info" @@ -249,6 +252,7 @@ func NewFromEnv(ctx context.Context, opts ...Option) *Server { opts = append(opts, WithNodeCert(getEnv(EnvCert, ""))) opts = append(opts, WithNodeCertKey(getEnv(EnvCertKey, ""))) opts = append(opts, WithNodeCertAuthority(getEnv(EnvCertAuth, ""))) + opts = append(opts, LimitRequestBody(int64(getEnvInt(EnvMaxRequestSize, 0)))) publicLBURL := getEnv(EnvPublicLoadBalancerURL, "") if publicLBURL != "" { diff --git a/api/server/server_options.go b/api/server/server_options.go index 5b21ee7b3..24feead40 100644 --- a/api/server/server_options.go +++ b/api/server/server_options.go @@ -51,7 +51,9 @@ func EnableShutdownEndpoint(ctx context.Context, halt context.CancelFunc) Option // LimitRequestBody wraps every http request to limit its size to the specified max bytes. func LimitRequestBody(max int64) Option { return func(ctx context.Context, s *Server) error { - s.Router.Use(limitRequestBody(max)) + if max > 0 { + s.Router.Use(limitRequestBody(max)) + } return nil } } diff --git a/api/server/server_test.go b/api/server/server_test.go index c78374f8e..a1cf3bbea 100644 --- a/api/server/server_test.go +++ b/api/server/server_test.go @@ -3,13 +3,17 @@ package server import ( "bytes" "context" + "crypto/rand" + "encoding/base64" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" "net/url" "os" "strconv" + "strings" "testing" "github.com/fnproject/fn/api/agent" @@ -25,8 +29,8 @@ import ( var tmpDatastoreTests = "/tmp/func_test_datastore.db" -func testServer(ds models.Datastore, mq models.MessageQueue, logDB models.LogStore, rnr agent.Agent, nodeType NodeType) *Server { - return New(context.Background(), +func testServer(ds models.Datastore, mq models.MessageQueue, logDB models.LogStore, rnr agent.Agent, nodeType NodeType, opts ...Option) *Server { + return New(context.Background(), append(opts, WithLogLevel(getEnv(EnvLogLevel, DefaultLogLevel)), WithDatastore(ds), WithMQ(mq), @@ -34,7 +38,7 @@ func testServer(ds models.Datastore, mq models.MessageQueue, logDB models.LogSto WithAgent(rnr), WithType(nodeType), WithTriggerAnnotator(NewRequestBasedTriggerAnnotator()), - ) + )...) } func createRequest(t *testing.T, method, path string, body io.Reader) *http.Request { @@ -131,7 +135,13 @@ func TestFullStack(t *testing.T) { rnr, rnrcancel := testRunner(t, ds) defer rnrcancel() - srv := testServer(ds, &mqs.Mock{}, logDB, rnr, ServerTypeFull) + srv := testServer(ds, &mqs.Mock{}, logDB, rnr, ServerTypeFull, LimitRequestBody(32256)) + + var bigbufa [32257]byte + rand.Read(bigbufa[:]) + bigbuf := base64.StdEncoding.EncodeToString(bigbufa[:]) // this will be > bigbufa, but json compatible + toobigerr := errors.New("Content-Length too large for this server") + gatewayerr := errors.New("container exit code") for _, test := range []struct { name string @@ -140,24 +150,30 @@ func TestFullStack(t *testing.T) { body string expectedCode int expectedCacheSize int // TODO kill me + expectedError error }{ - {"create my app", "POST", "/v1/apps", `{ "app": { "name": "myapp" } }`, http.StatusOK, 0}, - {"list apps", "GET", "/v1/apps", ``, http.StatusOK, 0}, - {"get app", "GET", "/v1/apps/myapp", ``, http.StatusOK, 0}, + {"create my app", "POST", "/v1/apps", `{ "app": { "name": "myapp" } }`, http.StatusOK, 0, nil}, + {"list apps", "GET", "/v1/apps", ``, http.StatusOK, 0, nil}, + {"get app", "GET", "/v1/apps/myapp", ``, http.StatusOK, 0, nil}, // NOTE: cache is lazy, loads when a request comes in for the route, not when added - {"add myroute", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute", "path": "/myroute", "image": "fnproject/fn-test-utils", "type": "sync" } }`, http.StatusOK, 0}, - {"add myroute2", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute2", "path": "/myroute2", "image": "fnproject/fn-test-utils", "type": "sync" } }`, http.StatusOK, 0}, - {"get myroute", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 0}, - {"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0}, - {"get all routes", "GET", "/v1/apps/myapp/routes", ``, http.StatusOK, 0}, - {"execute myroute", "POST", "/r/myapp/myroute", `{ "echoContent": "Teste" }`, http.StatusOK, 1}, - {"execute myroute2", "POST", "/r/myapp/myroute2", `{"sleepTime": 0, "isDebug": true, "isCrash": true}`, http.StatusBadGateway, 2}, - {"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 2}, - {"delete myroute", "DELETE", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 1}, - {"delete myroute2", "DELETE", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0}, - {"delete app (success)", "DELETE", "/v1/apps/myapp", ``, http.StatusOK, 0}, - {"get deleted app", "GET", "/v1/apps/myapp", ``, http.StatusNotFound, 0}, - {"get deleteds route on deleted app", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusNotFound, 0}, + {"add myroute", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute", "path": "/myroute", "image": "fnproject/fn-test-utils", "type": "sync" } }`, http.StatusOK, 0, nil}, + {"add myroute2", "POST", "/v1/apps/myapp/routes", `{ "route": { "name": "myroute2", "path": "/myroute2", "image": "fnproject/fn-test-utils", "type": "sync" } }`, http.StatusOK, 0, nil}, + {"get myroute", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 0, nil}, + {"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0, nil}, + {"get all routes", "GET", "/v1/apps/myapp/routes", ``, http.StatusOK, 0, nil}, + {"execute myroute", "POST", "/r/myapp/myroute", `{ "echoContent": "Teste" }`, http.StatusOK, 1, nil}, + + // fails + {"execute myroute2", "POST", "/r/myapp/myroute2", `{"sleepTime": 0, "isDebug": true, "isCrash": true}`, http.StatusBadGateway, 2, gatewayerr}, + {"request body too large", "POST", "/r/myapp/myroute", bigbuf, http.StatusRequestEntityTooLarge, 0, toobigerr}, + + {"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 2, nil}, + {"delete myroute", "DELETE", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 1, nil}, + {"delete myroute2", "DELETE", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0, nil}, + {"delete app (success)", "DELETE", "/v1/apps/myapp", ``, http.StatusOK, 0, nil}, + + {"get deleted app", "GET", "/v1/apps/myapp", ``, http.StatusNotFound, 0, models.ErrAppsNotFound}, + {"get deleteds route on deleted app", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusNotFound, 0, models.ErrAppsNotFound}, } { t.Run(test.name, func(t *testing.T) { _, rec := routerRequest(t, srv.Router, test.method, test.path, bytes.NewBuffer([]byte(test.body))) @@ -168,8 +184,18 @@ func TestFullStack(t *testing.T) { t.Errorf("Test \"%s\": Expected status code to be %d but was %d", test.name, test.expectedCode, rec.Code) } - }) + if rec.Code > 300 && test.expectedError == nil { + t.Log(buf.String()) + t.Error("got error when not expected error", rec.Body.String()) + } else if test.expectedError != nil { + if !strings.Contains(rec.Body.String(), test.expectedError.Error()) { + t.Log(buf.String()) + t.Errorf("Test %s: Expected error message to have `%s`, but got `%s`", + test.name, test.expectedError.Error(), rec.Body.String()) + } + } + }) } }