diff --git a/bouncer.go b/bouncer.go index 5f02e27..d3837b4 100644 --- a/bouncer.go +++ b/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,11 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { return nil } defer func() { + // net/http returns a conn to the idle pool once its body has been read to EOF, closing early discards it. + // Drain the body from the 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 +823,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 } diff --git a/bouncer_test.go b/bouncer_test.go index 8eb03ed..00b2f10 100644 --- a/bouncer_test.go +++ b/bouncer_test.go @@ -8,6 +8,7 @@ import ( "net/url" "reflect" "strings" + "sync" "testing" "text/template" "time" @@ -555,3 +556,47 @@ 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 for non-200 status response so net/http can return the conn to the idle pool. +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) + if _, errWrite := rw.Write([]byte(`{"action":"allow"}`)); errWrite != nil { + t.Errorf("appsec stub write: %v", errWrite) + } + })) + 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 i := 0; i < calls; i++ { //nolint:intrange + 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) + } + }) + } +}