diff --git a/command/agent/http.go b/command/agent/http.go index 66fed38dfc5..bbf071cbe0e 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -40,10 +40,6 @@ const ( // MissingRequestID is a placeholder if we cannot retrieve a request // UUID from context MissingRequestID = "" - - // HTTPConnStateFuncWriteTimeout is how long to try to write conn state errors - // before closing the connection - HTTPConnStateFuncWriteTimeout = 10 * time.Millisecond ) var ( @@ -175,19 +171,17 @@ func makeConnState(isTLS bool, handshakeTimeout time.Duration, connLimit int) fu // Still return the connection limiter return connlimit.NewLimiter(connlimit.Config{ MaxConnsPerClientIP: connLimit, - }).HTTPConnStateFuncWithDefault429Handler(HTTPConnStateFuncWriteTimeout) + }).HTTPConnStateFunc() } - return nil } if connLimit > 0 { // Return conn state callback with connection limiting and a // handshake timeout. - connLimiter := connlimit.NewLimiter(connlimit.Config{ MaxConnsPerClientIP: connLimit, - }).HTTPConnStateFuncWithDefault429Handler(HTTPConnStateFuncWriteTimeout) + }).HTTPConnStateFunc() return func(conn net.Conn, state http.ConnState) { switch state { diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 68f2ac868d7..ee252ab97c5 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -886,8 +886,8 @@ func TestHTTPServer_Limits_OK(t *testing.T) { cafile = "../../helper/tlsutil/testdata/ca.pem" foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" - maxConns = 10 // limit must be < this for testing - bufSize = 1 * 1024 // enough for 429 error message + maxConns = 10 // limit must be < this for testing + bufSize = 1 // enough to know if something was written ) cases := []struct { @@ -1055,20 +1055,16 @@ func TestHTTPServer_Limits_OK(t *testing.T) { } } - dial := func(t *testing.T, addr string, useTLS bool) net.Conn { + dial := func(t *testing.T, addr string, useTLS bool) (net.Conn, error) { if useTLS { cert, err := tls.LoadX509KeyPair(foocert, fookey) require.NoError(t, err) - conn, err := tls.Dial("tcp", addr, &tls.Config{ + return tls.Dial("tcp", addr, &tls.Config{ Certificates: []tls.Certificate{cert}, InsecureSkipVerify: true, // good enough }) - require.NoError(t, err) - return conn } else { - conn, err := net.DialTimeout("tcp", addr, 1*time.Second) - require.NoError(t, err) - return conn + return net.DialTimeout("tcp", addr, 1*time.Second) } } @@ -1079,7 +1075,9 @@ func TestHTTPServer_Limits_OK(t *testing.T) { conns := make([]net.Conn, limit) errCh := make(chan error, limit) for i := range conns { - conns[i] = dial(t, addr, useTLS) + conn, err := dial(t, addr, useTLS) + require.NoError(t, err) + conns[i] = conn go func(i int) { buf := []byte{0} @@ -1098,22 +1096,44 @@ func TestHTTPServer_Limits_OK(t *testing.T) { case <-time.After(500 * time.Millisecond): } - // Assert a new connection is dropped - conn := dial(t, addr, useTLS) - - defer func() { - require.NoError(t, conn.Close()) - }() - - deadline := time.Now().Add(10 * time.Second) - require.NoError(t, conn.SetReadDeadline(deadline)) - - buf := make([]byte, bufSize) - n, err := conn.Read(buf) - - require.NoError(t, err) - require.NotZero(t, n) - require.True(t, strings.HasPrefix(string(buf), "HTTP/1.1 429 Too Many Requests")) + // Create a new connection that will go over the connection limit. + limitConn, err := dial(t, addr, useTLS) + + // At this point, the state of the connection + handshake are up in the + // air. + // + // 1) dial failed, handshake never started + // => Conn is nil + io.EOF + // 2) dial completed, handshake failed + // => Conn is malformed + (net.OpError OR io.EOF) + // 3) dial completed, handshake succeeded + // => Conn is not nil + no error, however using the Conn should + // result in io.EOF + // + // At no point should Nomad actually write through the limited Conn. + + if limitConn == nil || err != nil { + // Case 1 or Case 2 - returned Conn is useless and the error should + // be one of: + // "EOF" + // "closed network connection" + // "connection reset by peer" + msg := err.Error() + acceptable := strings.Contains(msg, "EOF") || + strings.Contains(msg, "closed network connection") || + strings.Contains(msg, "connection reset by peer") + require.True(t, acceptable) + } else { + // Case 3 - returned Conn is usable, but Nomad should not write + // anything before closing it. + buf := make([]byte, bufSize) + deadline := time.Now().Add(10 * time.Second) + require.NoError(t, limitConn.SetReadDeadline(deadline)) + n, err := limitConn.Read(buf) + require.Equal(t, io.EOF, err) + require.Zero(t, n) + require.NoError(t, limitConn.Close()) + } // Assert existing connections are ok require.Len(t, errCh, 0) @@ -1161,7 +1181,7 @@ func TestHTTPServer_Limits_OK(t *testing.T) { if tc.assertLimit { // There's a race between assertTimeout(false) closing // its connection and the HTTP server noticing and - // untracking it. Since there's no way to coordinate + // un-tracking it. Since there's no way to coordinate // when this occurs, sleeping is the only way to avoid // asserting limits before the timed out connection is // untracked.