Skip to content

Consistent http.Server timeout configurations #30248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jentfoo merged 3 commits into from
Aug 11, 2023
Merged

Consistent http.Server timeout configurations #30248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fa9231
Consistent `http.Server` timeout configurations
jentfoo Aug 9, 2023
e972c12
defaults.go: Update ReadHeadersTimeout to 10 seconds
jentfoo Aug 10, 2023
addcb6a
alpnproxy/local_proxy.go: Move http timeouts to the top
jentfoo Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/auth/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,9 @@ func NewTLSServer(cfg TLSServerConfig) (*TLSServer, error) {
cfg: cfg,
httpServer: &http.Server{
Handler: tracingHandler,
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the more aggressive timeout causing test flakiness, or worse performance issues at scale. 1 second is almost always too short for our CI environment where many test cases are running in parallel.

What do you think about starting with 2 seconds, and leaving this in master for the v14 performance tests before backporting?

Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. From experience I'd much rather start larger and err on the side of caution and slowly reduce the timeout over time.

Copy link
Copy Markdown
Contributor Author

@jentfoo jentfoo Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about starting with 2 seconds, and leaving this in master for the v14 performance tests before backporting?

Sounds like a great plan to me. In commit ff22401ecd6b5c95793e8c03cba3ea77e1c642e2 I updated the configured timeout to 10 seconds. I feel like 2 seconds is likely fine, but we are coming from 30 seconds so even at 10, I feel like this is a notable net gain.

Maybe in Teleport 15 we go to 2 seconds (no concerns with moving slow to make sure we don't cause impacts)

Copy link
Copy Markdown
Contributor Author

@jentfoo jentfoo Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen two test failures, I worry this could be increasing the flakiness.

The timeouts seem pretty reasonable though, does it make sense to reduce test concurrency so that tests can complete faster? @zmb3 what are your thoughts?

Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tests? It's probably Go 1.21 and not your change

Copy link
Copy Markdown
Contributor Author

@jentfoo jentfoo Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seen twice:

 === Failed
=== FAIL: lib/auth TestAutoRotation (1.75s)
    tls_test.go:418: 
        	Error Trace:	/__w/teleport/teleport/lib/auth/tls_test.go:418
        	Error:      	Error "write tcp 127.0.0.1:47596->127.0.0.1:32991: write: broken pipe" does not contain "certificate"
        	Test:       	TestAutoRotation

I assumed it might be the write timeout causing the connection to be closed.

Copy link
Copy Markdown
Contributor

@greedy52 greedy52 Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#30253
may or may not be your change (most likely not) but i don't think we have found the root cause for the above flaky test yet

WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
},
log: logrus.WithFields(logrus.Fields{
Expand Down
5 changes: 4 additions & 1 deletion lib/client/redirect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/secret"
"github.com/gravitational/teleport/lib/utils"
)
Expand Down Expand Up @@ -151,7 +152,9 @@ func (rd *Redirector) Start() error {
Listener: listener,
Config: &http.Server{
Handler: rd.mux,
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
},
}
Expand Down
2 changes: 1 addition & 1 deletion lib/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ const (

// ReadHeadersTimeout is a default TCP timeout when we wait
// for the response headers to arrive
ReadHeadersTimeout = time.Second
ReadHeadersTimeout = 10 * time.Second

// DatabaseConnectTimeout is a timeout for connecting to a database via
// database access.
Expand Down
2 changes: 2 additions & 0 deletions lib/kube/proxy/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ func NewTLSServer(cfg TLSServerConfig) (*TLSServer, error) {
Server: &http.Server{
Handler: httplib.MakeTracingHandler(limiter, teleport.ComponentKube),
ReadHeaderTimeout: apidefaults.DefaultIOTimeout * 2,
ReadTimeout: apidefaults.DefaultIOTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
TLSConfig: cfg.TLS,
ConnState: ingress.HTTPConnStateReporter(ingress.Kube, cfg.IngressReporter),
Expand Down
5 changes: 4 additions & 1 deletion lib/observability/tracing/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"google.golang.org/protobuf/proto"

apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/lib/defaults"
)

// Collector is a simple in memory implementation of an OpenTelemetry Collector
Expand Down Expand Up @@ -84,7 +85,9 @@ func NewCollector(cfg CollectorConfig) (*Collector, error) {

c.httpServer = &http.Server{
Handler: c,
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
TLSConfig: tlsConfig.Clone(),
}
Expand Down
14 changes: 11 additions & 3 deletions lib/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2875,7 +2875,9 @@ func (process *TeleportProcess) initMetricsService() error {

server := &http.Server{
Handler: mux,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
ErrorLog: utils.NewStdlogger(log.Error, teleport.ComponentMetrics),
TLSConfig: tlsConfig,
Expand Down Expand Up @@ -2996,7 +2998,9 @@ func (process *TeleportProcess) initDiagnosticService() error {

server := &http.Server{
Handler: mux,
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
ErrorLog: utils.NewStdlogger(log.Error, teleport.ComponentDiagnostic),
}
Expand Down Expand Up @@ -3926,7 +3930,9 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error {
limiter.MakeMiddleware(proxyLimiter),
httplib.MakeTracingMiddleware(teleport.ComponentProxy),
),
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
ErrorLog: utils.NewStdlogger(log.Error, teleport.ComponentProxy),
ConnState: ingress.HTTPConnStateReporter(ingress.Web, ingressReporter),
Expand Down Expand Up @@ -4625,7 +4631,9 @@ func (process *TeleportProcess) initMinimalReverseTunnel(listeners *proxyListene
minimalWebServer, err := web.NewServer(web.ServerConfig{
Server: &http.Server{
Handler: httplib.MakeTracingHandler(minimalProxyLimiter, teleport.ComponentProxy),
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
ErrorLog: utils.NewStdlogger(log.Error, teleport.ComponentReverseTunnelServer),
},
Expand Down
11 changes: 10 additions & 1 deletion lib/srv/alpnproxy/forward_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ import (
log "github.com/sirupsen/logrus"
"golang.org/x/net/http/httpproxy"

apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/types"
awsapiutils "github.com/gravitational/teleport/api/utils/aws"
"github.com/gravitational/teleport/api/utils/azure"
"github.com/gravitational/teleport/api/utils/gcp"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/utils"
)

Expand Down Expand Up @@ -91,7 +93,14 @@ func NewForwardProxy(cfg ForwardProxyConfig) (*ForwardProxy, error) {

// Start starts serving on the listener.
func (p *ForwardProxy) Start() error {
err := http.Serve(p.cfg.Listener, p)
server := &http.Server{
Handler: p,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
}
err := server.Serve(p.cfg.Listener)
if err != nil && !utils.IsUseOfClosedNetworkError(err) {
return trace.Wrap(err)
}
Expand Down
54 changes: 36 additions & 18 deletions lib/srv/alpnproxy/local_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ import (
"golang.org/x/exp/slices"

"github.com/gravitational/teleport/api/client"
apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/utils/pingconn"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/srv/alpnproxy/common"
commonApp "github.com/gravitational/teleport/lib/srv/app/common"
"github.com/gravitational/teleport/lib/tlsca"
Expand Down Expand Up @@ -316,27 +318,43 @@ func (l *LocalProxy) StartHTTPAccessProxy(ctx context.Context) error {
l.cfg.Log.Info("Starting HTTP access proxy")
defer l.cfg.Log.Info("HTTP access proxy stopped")
defaultProxy := l.makeHTTPReverseProxy(l.getCerts())
err := http.Serve(l.cfg.Listener, http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
if l.cfg.HTTPMiddleware.HandleRequest(rw, req) {
return
}

// Requests from forward proxy have original hostnames instead of
// localhost. Set appropriate header to keep this information.
if addr, err := utils.ParseAddr(req.Host); err == nil && !addr.IsLocal() {
req.Header.Set("X-Forwarded-Host", req.Host)
}
server := &http.Server{
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
Handler: http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
Comment thread
codingllama marked this conversation as resolved.
if l.cfg.HTTPMiddleware.HandleRequest(rw, req) {
return
}

proxy, err := l.getHTTPReverseProxyForReq(req, defaultProxy)
if err != nil {
l.cfg.Log.Warnf("Failed to get reverse proxy: %v.", err)
trace.WriteError(rw, trace.Wrap(err))
return
}
// Requests from forward proxy have original hostnames instead of
// localhost. Set appropriate header to keep this information.
if addr, err := utils.ParseAddr(req.Host); err == nil && !addr.IsLocal() {
req.Header.Set("X-Forwarded-Host", req.Host)
}

proxy, err := l.getHTTPReverseProxyForReq(req, defaultProxy)
if err != nil {
l.cfg.Log.Warnf("Failed to get reverse proxy: %v.", err)
trace.WriteError(rw, trace.Wrap(err))
return
}

proxy.ServeHTTP(rw, req)
}),
}

// Shut down the server when the context is done
go func() {
<-ctx.Done()
server.Shutdown(context.Background())
}()

proxy.ServeHTTP(rw, req)
}))
if err != nil && !utils.IsUseOfClosedNetworkError(err) {
// Use the custom server to listen and serve
err := server.Serve(l.cfg.Listener)
if err != nil && err != http.ErrServerClosed && !utils.IsUseOfClosedNetworkError(err) {
return trace.Wrap(err)
}
return nil
Expand Down
4 changes: 3 additions & 1 deletion lib/srv/app/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,9 @@ func (s *Server) newHTTPServer(clusterName string) *http.Server {

return &http.Server{
Handler: httplib.MakeTracingHandler(s.authMiddleware, teleport.ComponentApp),
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
ErrorLog: utils.NewStdlogger(s.log.Error, teleport.ComponentApp),
ConnContext: func(ctx context.Context, c net.Conn) context.Context {
Expand Down
5 changes: 4 additions & 1 deletion lib/tbot/tbot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/auth/authclient"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/modules"
"github.com/gravitational/teleport/lib/tbot/bot"
"github.com/gravitational/teleport/lib/tbot/config"
Expand Down Expand Up @@ -188,7 +189,9 @@ func (b *Bot) Run(ctx context.Context) error {
srv := http.Server{
Addr: b.cfg.DiagAddr,
Handler: mux,
ReadHeaderTimeout: apidefaults.DefaultIOTimeout,
ReadTimeout: apidefaults.DefaultIOTimeout,
ReadHeaderTimeout: defaults.ReadHeadersTimeout,
WriteTimeout: apidefaults.DefaultIOTimeout,
IdleTimeout: apidefaults.DefaultIdleTimeout,
}
go func() {
Expand Down