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

@@ -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"}
 

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
 

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)

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

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 != "" {

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
 	}
 }

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 {
+func testServer(ds models.Datastore, mq models.MessageQueue, logDB models.LogStore, rnr agent.Agent, nodeType NodeType, opts ...Option) *Server {
-	return New(context.Background(),
+	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},
+		{"create my app", "POST", "/v1/apps", `{ "app": { "name": "myapp" } }`, http.StatusOK, 0, nil},
-		{"list apps", "GET", "/v1/apps", ``, http.StatusOK, 0},
+		{"list apps", "GET", "/v1/apps", ``, http.StatusOK, 0, nil},
-		{"get app", "GET", "/v1/apps/myapp", ``, http.StatusOK, 0},
+		{"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 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},
+		{"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},
+		{"get myroute", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 0, nil},
-		{"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0},
+		{"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0, nil},
-		{"get all routes", "GET", "/v1/apps/myapp/routes", ``, http.StatusOK, 0},
+		{"get all routes", "GET", "/v1/apps/myapp/routes", ``, http.StatusOK, 0, nil},
-		{"execute myroute", "POST", "/r/myapp/myroute", `{ "echoContent": "Teste" }`, http.StatusOK, 1},
+		{"execute myroute", "POST", "/r/myapp/myroute", `{ "echoContent": "Teste" }`, http.StatusOK, 1, nil},
-		{"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},
+		// fails
-		{"delete myroute", "DELETE", "/v1/apps/myapp/routes/myroute", ``, http.StatusOK, 1},
+		{"execute myroute2", "POST", "/r/myapp/myroute2", `{"sleepTime": 0, "isDebug": true, "isCrash": true}`, http.StatusBadGateway, 2, gatewayerr},
-		{"delete myroute2", "DELETE", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 0},
+		{"request body too large", "POST", "/r/myapp/myroute", bigbuf, http.StatusRequestEntityTooLarge, 0, toobigerr},
-		{"delete app (success)", "DELETE", "/v1/apps/myapp", ``, http.StatusOK, 0},
+
-		{"get deleted app", "GET", "/v1/apps/myapp", ``, http.StatusNotFound, 0},
+		{"get myroute2", "GET", "/v1/apps/myapp/routes/myroute2", ``, http.StatusOK, 2, nil},
-		{"get deleteds route on deleted app", "GET", "/v1/apps/myapp/routes/myroute", ``, http.StatusNotFound, 0},
+		{"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())
+				}
+			}
+		})
 	}
 }