🐛 drain the appsec response body so connections are reused

Fix #384.

appsecQuery never read the appsec response body. net/http only returns a
connection to the idle pool once its body has reached EOF, so closing early
discarded it: every request opened a fresh TCP connection to the appsec host and
left it in TIME_WAIT. crowdsecQuery, two functions above, does call io.ReadAll,
which is why the LAPI path in the same process pools correctly.

Measured with an httptest appsec server over 10 calls:

  empty response body      -> 1 connection   (already at EOF, reuse worked)
  non-empty response body  -> 10 connections (one per request)

The drain goes in the defer rather than before the final return, because the 500
and non-200 paths return earlier. Draining only at the end fixes 200 and leaves
the other two leaking:

  status 200 -> 1 connection    status 403 -> 10    status 500 -> 10

which is the wrong half to fix: 403 is what appsec produces for a site under
attack, and 500 is what a wedged appsec produces.

Also removes the unreachable `if err != nil { ... appsecQuery:readBody }` left
over from a version that did read the body -- err is always nil there -- and sets
MaxIdleConnsPerHost on the three transports. They each talk to a single host, so
the unset default of 2 (DefaultMaxIdleConnsPerHost) capped the pool well below
the configured MaxIdleConns of 10.

Adds Test_appsecQuery_reusesConnection, which asserts one connection for ten
calls across 200/403/500 and fails on the previous code.

bouncer.go

@@ -243,17 +243,19 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam
 		},
 		httpClient: &http.Client{
 			Transport: &http.Transport{
-				MaxIdleConns:    10,
-				IdleConnTimeout: 30 * time.Second,
-				TLSClientConfig: tlsConfig,
+				MaxIdleConns:        10,
+				MaxIdleConnsPerHost: 10,
+				IdleConnTimeout:     30 * time.Second,
+				TLSClientConfig:     tlsConfig,
 			},
 			Timeout: time.Duration(config.HTTPTimeoutSeconds) * time.Second,
 		},
 		httpAppsecClient: &http.Client{
 			Transport: &http.Transport{
-				MaxIdleConns:    10,
-				IdleConnTimeout: 30 * time.Second,
-				TLSClientConfig: tlsAppsecConfig,
+				MaxIdleConns:        10,
+				MaxIdleConnsPerHost: 10,
+				IdleConnTimeout:     30 * time.Second,
+				TLSClientConfig:     tlsAppsecConfig,
 			},
 			Timeout: time.Duration(config.HTTPTimeoutSeconds) * time.Second,
 		},
@@ -278,7 +280,7 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam
 		log,
 		bouncer.cacheClient,
 		&http.Client{
-			Transport: &http.Transport{MaxIdleConns: 10, IdleConnTimeout: 30 * time.Second},
+			Transport: &http.Transport{MaxIdleConns: 10, MaxIdleConnsPerHost: 10, IdleConnTimeout: 30 * time.Second},
 			Timeout:   time.Duration(config.HTTPTimeoutSeconds) * time.Second,
 		},
 		config.CaptchaProvider,
@@ -801,6 +803,13 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error {
 		return nil
 	}
 	defer func() {
+		// net/http only returns a connection to the idle pool once its body has
+		// been read to EOF; closing early discards it. Drain here rather than at
+		// the end of the function so the 500 and non-200 paths, which return
+		// earlier, keep their connections too.
+		if _, errDrain := io.Copy(io.Discard, res.Body); errDrain != nil {
+			bouncer.log.Debug("appsecQuery:drainBody " + errDrain.Error())
+		}
 		if err = res.Body.Close(); err != nil {
 			bouncer.log.Error("appsecQuery:closeBody " + err.Error())
 		}
@@ -816,9 +825,6 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error {
 		return fmt.Errorf("appsecQuery statusCode:%d", res.StatusCode)
 	}
 
-	if err != nil {
-		return fmt.Errorf("appsecQuery:readBody %w", err)
-	}
 	return nil
 }
 

bouncer_test.go

@@ -2,12 +2,14 @@ package crowdsec_bouncer_traefik_plugin //nolint:revive,stylecheck
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"reflect"
 	"strings"
+	"sync"
 	"testing"
 	"text/template"
 	"time"
@@ -555,3 +557,51 @@ func Test_appsecQuery_unreadableBodyGetNotDropped(t *testing.T) {
 		t.Fatal("appsecQuery() blocked on an HTTP/3 GET request body (issue #351 regression)")
 	}
 }
+
+// Test_appsecQuery_reusesConnection is a regression test for issue #384: the
+// appsec response body must be drained so net/http can return the connection to
+// the idle pool. Without it every request opened a fresh TCP connection and left
+// it in TIME_WAIT. The 403 and 500 cases matter as much as 200: they return
+// before the end of the function, and they are what a site under attack or a
+// wedged appsec produce in volume.
+func Test_appsecQuery_reusesConnection(t *testing.T) {
+	for _, status := range []int{http.StatusOK, http.StatusForbidden, http.StatusInternalServerError} {
+		t.Run(http.StatusText(status), func(t *testing.T) {
+			var mu sync.Mutex
+			conns := map[string]bool{}
+			appsecServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+				mu.Lock()
+				conns[r.RemoteAddr] = true
+				mu.Unlock()
+				rw.WriteHeader(status)
+				// a non-empty body is what makes the connection unusable when it
+				// is not drained; an empty one is already at EOF
+				fmt.Fprint(rw, `{"action":"allow"}`)
+			}))
+			defer appsecServer.Close()
+
+			appsecURL, _ := url.Parse(appsecServer.URL)
+			bouncer := &Bouncer{
+				appsecScheme:       appsecURL.Scheme,
+				appsecHost:         appsecURL.Host,
+				appsecPath:         "/",
+				appsecBodyLimit:    10485760,
+				appsecFailureBlock: false,
+				httpAppsecClient:   appsecServer.Client(),
+				log:                logger.New("INFO", ""),
+			}
+
+			const calls = 10
+			for range calls {
+				req, _ := http.NewRequest(http.MethodGet, "http://localhost/", nil)
+				_ = appsecQuery(bouncer, "1.2.3.4", req)
+			}
+
+			mu.Lock()
+			defer mu.Unlock()
+			if len(conns) != 1 {
+				t.Errorf("appsecQuery() opened %d connections for %d calls, want 1 (response body not drained?)", len(conns), calls)
+			}
+		})
+	}
+}