Use chi/middleware.Throttle instead of custom conn limiter (#4402) (#…

…4427)

Use chi/middleware.Throttle to track in-flight requests and return a 429 response once the limit is reached.

This is different behaviour from the TCP listener based approach where the check occurs before the TLS handshake and if the limit is reached, the connection is silently closed.

(cherry picked from commit fbe3b2b)

Co-authored-by: Michel Laterman <[email protected]>

changelog/fragments/1738613859-Return-HTTP-429-when-conn-limit-is-reached.yaml

@@ -0,0 +1,33 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: Return HTTP 429 when conn limit is reached
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+description: |
+  fleet-server will return a 429 instead of silently close connections when the conn limit is reached.
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: fleet-server
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+#pr: https://github.com/owner/repo/1234
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/fleet-server/issues/4200

fleet-server.reference.yml

@@ -149,7 +149,7 @@ fleet:
 #       policy_throttle: 5ms # 1ms min is forced
 #       # max_header_byte_size is the request header size limit
 #       max_header_byte_size: 8192 # 8Kib
-#       # max_connections is the maximum number of connnections per API endpoint
+#       # max_connections is the maximum number of in-flight HTTP requests per API listener
 #       max_connections: 0
 #
 #       # action_limit is a limiter for the action dispatcher, it is added to control how fast the checkin endpoint writes responses when an action effecting multiple agents is detected.

internal/pkg/api/router.go

@@ -9,14 +9,15 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/elastic/fleet-server/v7/internal/pkg/config"
-	"github.com/elastic/fleet-server/v7/internal/pkg/limit"
-	"github.com/elastic/fleet-server/v7/internal/pkg/logger"
 	"github.com/go-chi/chi/v5"
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/rs/zerolog"
 	"go.elastic.co/apm/module/apmchiv5/v2"
 	"go.elastic.co/apm/v2"
+
+	"github.com/elastic/fleet-server/v7/internal/pkg/config"
+	"github.com/elastic/fleet-server/v7/internal/pkg/limit"
+	"github.com/elastic/fleet-server/v7/internal/pkg/logger"
 )
 
 func newRouter(cfg *config.ServerLimits, si ServerInterface, tracer *apm.Tracer) http.Handler {
@@ -26,6 +27,9 @@ func newRouter(cfg *config.ServerLimits, si ServerInterface, tracer *apm.Tracer)
 	}
 	r.Use(logger.Middleware) // Attach middlewares to router directly so the occur before any request parsing/validation
 	r.Use(middleware.Recoverer)
+	if cfg.MaxConnections > 0 {
+		r.Use(middleware.Throttle(cfg.MaxConnections))
+	}
 	r.Use(Limiter(cfg).middleware)
 	return HandlerWithOptions(si, ChiServerOptions{
 		BaseRouter:       r,

internal/pkg/api/server.go

@@ -15,7 +15,6 @@ import (
 
 	"github.com/elastic/elastic-agent-libs/transport/tlscommon"
 	"github.com/elastic/fleet-server/v7/internal/pkg/config"
-	"github.com/elastic/fleet-server/v7/internal/pkg/limit"
 	"github.com/elastic/fleet-server/v7/internal/pkg/logger"
 
 	"github.com/rs/zerolog"
@@ -29,7 +28,7 @@ type server struct {
 
 // NewServer creates a new HTTP api for the passed addr.
 //
-// The server has a listener specific conn limit and endpoint specific rate-limits.
+// The server has an http request limit and endpoint specific rate-limits.
 // The underlying API structs (such as *CheckinT) may be shared between servers.
 func NewServer(addr string, cfg *config.Server, opts ...APIOpt) *server {
 	a := &apiServer{}
@@ -76,13 +75,6 @@ func (s *server) Run(ctx context.Context) error {
 		}
 	}()
 
-	// Conn Limiter must be before the TLS handshake in the stack;
-	// The server should not eat the cost of the handshake if there
-	// is no capacity to service the connection.
-	// Also, it appears the HTTP2 implementation depends on the tls.Listener
-	// being at the top of the stack.
-	ln = wrapConnLimitter(ctx, ln, s.cfg)
-
 	if s.cfg.TLS != nil && s.cfg.TLS.IsEnabled() {
 		commonTLSCfg, err := tlscommon.LoadTLSServerConfig(s.cfg.TLS)
 		if err != nil {
@@ -151,22 +143,6 @@ func getDiagConnFunc(ctx context.Context) func(c net.Conn, s http.ConnState) {
 	}
 }
 
-func wrapConnLimitter(ctx context.Context, ln net.Listener, cfg *config.Server) net.Listener {
-	hardLimit := cfg.Limits.MaxConnections
-
-	if hardLimit != 0 {
-		zerolog.Ctx(ctx).Info().
-			Int("hardConnLimit", hardLimit).
-			Msg("server hard connection limiter installed")
-
-		ln = limit.Listener(ln, hardLimit, zerolog.Ctx(ctx))
-	} else {
-		zerolog.Ctx(ctx).Info().Msg("server hard connection limiter disabled")
-	}
-
-	return ln
-}
-
 type stubLogger struct {
 	log zerolog.Logger
 }