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()) + } + } + }) } }