From 3d3beb6a9d95f0f1a7ca0c0a19979c290639f9b0 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 21 Mar 2020 22:10:13 -0700 Subject: [PATCH] Add response header timeouts (#9170) - Add conservative timeouts upto 3 minutes for internode communication - Add aggressive timeouts of 30 seconds for gateway communication Fixes #9105 Fixes #8732 Fixes #8881 Fixes #8376 Fixes #9028 --- cmd/admin-handlers.go | 2 +- cmd/config-current.go | 23 ++++++++-------- cmd/gateway/azure/gateway-azure.go | 4 +-- cmd/gateway/b2/gateway-b2.go | 8 +++--- cmd/gateway/gcs/gateway-gcs.go | 4 +-- cmd/gateway/oss/gateway-oss.go | 4 +-- cmd/gateway/s3/gateway-s3.go | 4 +-- cmd/generic-handlers.go | 4 +-- cmd/notification.go | 3 +++ cmd/object-handlers.go | 2 +- cmd/storage-rest-server.go | 42 ++++++++++++++++-------------- cmd/utils.go | 19 ++++++++------ cmd/web-handlers.go | 3 ++- staticcheck.conf | 2 +- 14 files changed, 68 insertions(+), 56 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index b87cf1640..a919dca36 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -1533,7 +1533,7 @@ func (a adminAPIHandlers) ServerInfoHandler(w http.ResponseWriter, r *http.Reque func fetchLambdaInfo(cfg config.Config) []map[string][]madmin.TargetIDStatus { // Fetch the targets - targetList, err := notify.RegisterNotificationTargets(cfg, GlobalServiceDoneCh, NewCustomHTTPTransport(), nil, true) + targetList, err := notify.RegisterNotificationTargets(cfg, GlobalServiceDoneCh, NewGatewayHTTPTransport(), nil, true) if err != nil { return nil } diff --git a/cmd/config-current.go b/cmd/config-current.go index 1bd9246f9..40b63d2ba 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -251,7 +251,7 @@ func validateConfig(s config.Config) error { } } { - kmsCfg, err := crypto.LookupConfig(s, globalCertsCADir.Get(), NewCustomHTTPTransport()) + kmsCfg, err := crypto.LookupConfig(s, globalCertsCADir.Get(), NewGatewayHTTPTransport()) if err != nil { return err } @@ -269,7 +269,7 @@ func validateConfig(s config.Config) error { } if _, err := openid.LookupConfig(s[config.IdentityOpenIDSubSys][config.Default], - NewCustomHTTPTransport(), xhttp.DrainBody); err != nil { + NewGatewayHTTPTransport(), xhttp.DrainBody); err != nil { return err } @@ -279,7 +279,7 @@ func validateConfig(s config.Config) error { } if _, err := opa.LookupConfig(s[config.PolicyOPASubSys][config.Default], - NewCustomHTTPTransport(), xhttp.DrainBody); err != nil { + NewGatewayHTTPTransport(), xhttp.DrainBody); err != nil { return err } @@ -287,7 +287,7 @@ func validateConfig(s config.Config) error { return err } - return notify.TestNotificationTargets(s, GlobalServiceDoneCh, NewCustomHTTPTransport(), + return notify.TestNotificationTargets(s, GlobalServiceDoneCh, NewGatewayHTTPTransport(), globalNotificationSys.ConfiguredTargetIDs()) } @@ -362,7 +362,7 @@ func lookupConfigs(s config.Config) { } } - kmsCfg, err := crypto.LookupConfig(s, globalCertsCADir.Get(), NewCustomHTTPTransport()) + kmsCfg, err := crypto.LookupConfig(s, globalCertsCADir.Get(), NewGatewayHTTPTransport()) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to setup KMS config: %w", err)) } @@ -381,13 +381,13 @@ func lookupConfigs(s config.Config) { } globalOpenIDConfig, err = openid.LookupConfig(s[config.IdentityOpenIDSubSys][config.Default], - NewCustomHTTPTransport(), xhttp.DrainBody) + NewGatewayHTTPTransport(), xhttp.DrainBody) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize OpenID: %w", err)) } opaCfg, err := opa.LookupConfig(s[config.PolicyOPASubSys][config.Default], - NewCustomHTTPTransport(), xhttp.DrainBody) + NewGatewayHTTPTransport(), xhttp.DrainBody) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize OPA: %w", err)) } @@ -412,22 +412,23 @@ func lookupConfigs(s config.Config) { for _, l := range loggerCfg.HTTP { if l.Enabled { // Enable http logging - logger.AddTarget(http.New(l.Endpoint, loggerUserAgent, string(logger.All), NewCustomHTTPTransport())) + logger.AddTarget(http.New(l.Endpoint, loggerUserAgent, string(logger.All), NewGatewayHTTPTransport())) } } for _, l := range loggerCfg.Audit { if l.Enabled { // Enable http audit logging - logger.AddAuditTarget(http.New(l.Endpoint, loggerUserAgent, string(logger.All), NewCustomHTTPTransport())) + logger.AddAuditTarget(http.New(l.Endpoint, loggerUserAgent, string(logger.All), NewGatewayHTTPTransport())) } } - globalConfigTargetList, err = notify.GetNotificationTargets(s, GlobalServiceDoneCh, NewCustomHTTPTransport()) + globalConfigTargetList, err = notify.GetNotificationTargets(s, GlobalServiceDoneCh, NewGatewayHTTPTransport()) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err)) } - globalEnvTargetList, err = notify.GetNotificationTargets(newServerConfig(), GlobalServiceDoneCh, NewCustomHTTPTransport()) + + globalEnvTargetList, err = notify.GetNotificationTargets(newServerConfig(), GlobalServiceDoneCh, NewGatewayHTTPTransport()) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err)) } diff --git a/cmd/gateway/azure/gateway-azure.go b/cmd/gateway/azure/gateway-azure.go index 578535971..f31219f09 100644 --- a/cmd/gateway/azure/gateway-azure.go +++ b/cmd/gateway/azure/gateway-azure.go @@ -1,5 +1,5 @@ /* - * MinIO Cloud Storage, (C) 2017, 2018 MinIO, Inc. + * MinIO Cloud Storage, (C) 2017-2020 MinIO, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -150,7 +150,7 @@ func (g *Azure) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, erro metrics := minio.NewMetrics() t := &minio.MetricsTransport{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), Metrics: metrics, } diff --git a/cmd/gateway/b2/gateway-b2.go b/cmd/gateway/b2/gateway-b2.go index fea79d673..397c67159 100644 --- a/cmd/gateway/b2/gateway-b2.go +++ b/cmd/gateway/b2/gateway-b2.go @@ -1,5 +1,5 @@ /* - * MinIO Cloud Storage, (C) 2017, 2018 MinIO, Inc. + * MinIO Cloud Storage, (C) 2017-2020 MinIO, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -102,7 +102,7 @@ func (g *B2) Name() string { // talk to B2 remote backend. func (g *B2) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, error) { ctx := context.Background() - client, err := b2.AuthorizeAccount(ctx, creds.AccessKey, creds.SecretKey, b2.Transport(minio.NewCustomHTTPTransport())) + client, err := b2.AuthorizeAccount(ctx, creds.AccessKey, creds.SecretKey, b2.Transport(minio.NewGatewayHTTPTransport())) if err != nil { return nil, err } @@ -111,7 +111,7 @@ func (g *B2) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, error) creds: creds, b2Client: client, httpClient: &http.Client{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), }, ctx: ctx, }, nil @@ -238,7 +238,7 @@ func (l *b2Objects) MakeBucketWithLocation(ctx context.Context, bucket, location } func (l *b2Objects) reAuthorizeAccount(ctx context.Context) error { - client, err := b2.AuthorizeAccount(l.ctx, l.creds.AccessKey, l.creds.SecretKey, b2.Transport(minio.NewCustomHTTPTransport())) + client, err := b2.AuthorizeAccount(l.ctx, l.creds.AccessKey, l.creds.SecretKey, b2.Transport(minio.NewGatewayHTTPTransport())) if err != nil { return err } diff --git a/cmd/gateway/gcs/gateway-gcs.go b/cmd/gateway/gcs/gateway-gcs.go index 4000bc194..9c6c12d79 100644 --- a/cmd/gateway/gcs/gateway-gcs.go +++ b/cmd/gateway/gcs/gateway-gcs.go @@ -1,5 +1,5 @@ /* - * MinIO Cloud Storage, (C) 2017, 2018 MinIO, Inc. + * MinIO Cloud Storage, (C) 2017-2020 MinIO, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -184,7 +184,7 @@ func (g *GCS) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, error) metrics := minio.NewMetrics() t := &minio.MetricsTransport{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), Metrics: metrics, } diff --git a/cmd/gateway/oss/gateway-oss.go b/cmd/gateway/oss/gateway-oss.go index 7283426e4..79b89f384 100644 --- a/cmd/gateway/oss/gateway-oss.go +++ b/cmd/gateway/oss/gateway-oss.go @@ -1,5 +1,5 @@ /* - * MinIO Cloud Storage, (C) 2017, 2018 MinIO, Inc. + * MinIO Cloud Storage, (C) 2017-2020 MinIO, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -126,7 +126,7 @@ func (g *OSS) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, error) return nil, err } client.HTTPClient = &http.Client{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), } return &ossObjects{ Client: client, diff --git a/cmd/gateway/s3/gateway-s3.go b/cmd/gateway/s3/gateway-s3.go index 86a37d04f..0209c0f7e 100644 --- a/cmd/gateway/s3/gateway-s3.go +++ b/cmd/gateway/s3/gateway-s3.go @@ -152,7 +152,7 @@ var defaultAWSCredProviders = []credentials.Provider{ &credentials.FileAWSCredentials{}, &credentials.IAM{ Client: &http.Client{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), }, }, &credentials.EnvMinio{}, @@ -212,7 +212,7 @@ func (g *S3) NewGatewayLayer(creds auth.Credentials) (minio.ObjectLayer, error) metrics := minio.NewMetrics() t := &minio.MetricsTransport{ - Transport: minio.NewCustomHTTPTransport(), + Transport: minio.NewGatewayHTTPTransport(), Metrics: metrics, } diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go index 44f38e608..ae9b2b5ec 100644 --- a/cmd/generic-handlers.go +++ b/cmd/generic-handlers.go @@ -1,5 +1,5 @@ /* - * MinIO Cloud Storage, (C) 2015, 2016, 2017 MinIO, Inc. + * MinIO Cloud Storage, (C) 2015-2020 MinIO, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -721,7 +721,7 @@ func (f bucketForwardingHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques func setBucketForwardingHandler(h http.Handler) http.Handler { fwd := handlers.NewForwarder(&handlers.Forwarder{ PassHost: true, - RoundTripper: NewCustomHTTPTransport(), + RoundTripper: NewGatewayHTTPTransport(), Logger: func(err error) { logger.LogIf(context.Background(), err) }, diff --git a/cmd/notification.go b/cmd/notification.go index 8ebe0983f..039a178be 100644 --- a/cmd/notification.go +++ b/cmd/notification.go @@ -784,6 +784,9 @@ func (sys *NotificationSys) RemoveNotification(bucketName string) { // RemoveAllRemoteTargets - closes and removes all HTTP/PeerRPC client targets. func (sys *NotificationSys) RemoveAllRemoteTargets() { + sys.Lock() + defer sys.Unlock() + for _, targetMap := range sys.bucketRemoteTargetRulesMap { for targetID := range targetMap { sys.targetList.Remove(targetID) diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 12fe4d2d7..aad7bc8b0 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -657,7 +657,7 @@ var getRemoteInstanceClient = func(r *http.Request, host string) (*miniogo.Core, if err != nil { return nil, err } - core.SetCustomTransport(NewCustomHTTPTransport()) + core.SetCustomTransport(NewGatewayHTTPTransport()) return core, nil } diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index c0370e32f..d4fd392f3 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -444,14 +444,16 @@ func (s *storageRESTServer) WalkHandler(w http.ResponseWriter, r *http.Request) } leafFile := vars[storageRESTLeafFile] - fch, err := s.storage.Walk(volume, dirPath, markerPath, recursive, leafFile, readMetadata, r.Context().Done()) - if err != nil { - s.writeErrorResponse(w, err) - return - } - w.Header().Set(xhttp.ContentType, "text/event-stream") encoder := gob.NewEncoder(w) + done := keepHTTPResponseAlive(w) + defer done() + + fch, err := s.storage.Walk(volume, dirPath, markerPath, recursive, leafFile, readMetadata, r.Context().Done()) + if err != nil { + logger.LogIf(r.Context(), err) + return + } for fi := range fch { encoder.Encode(&fi) } @@ -472,6 +474,7 @@ func (s *storageRESTServer) ListDirHandler(w http.ResponseWriter, r *http.Reques s.writeErrorResponse(w, err) return } + entries, err := s.storage.ListDir(volume, dirPath, count, leafFile) if err != nil { s.writeErrorResponse(w, err) @@ -521,20 +524,21 @@ func (s *storageRESTServer) DeleteFileBulkHandler(w http.ResponseWriter, r *http return } + dErrsResp := &DeleteFileBulkErrsResp{Errs: make([]error, len(filePaths))} + w.Header().Set(xhttp.ContentType, "text/event-stream") encoder := gob.NewEncoder(w) done := keepHTTPResponseAlive(w) errs, err := s.storage.DeleteFileBulk(volume, filePaths) done() - if err != nil { - s.writeErrorResponse(w, err) - return - } - dErrsResp := &DeleteFileBulkErrsResp{Errs: make([]error, len(errs))} - for idx, err := range errs { + for idx := range filePaths { if err != nil { dErrsResp.Errs[idx] = StorageErr(err.Error()) + } else { + if errs[idx] != nil { + dErrsResp.Errs[idx] = StorageErr(errs[idx].Error()) + } } } @@ -567,20 +571,20 @@ func (s *storageRESTServer) DeletePrefixesHandler(w http.ResponseWriter, r *http return } + dErrsResp := &DeletePrefixesErrsResp{Errs: make([]error, len(prefixes))} + w.Header().Set(xhttp.ContentType, "text/event-stream") encoder := gob.NewEncoder(w) done := keepHTTPResponseAlive(w) errs, err := s.storage.DeletePrefixes(volume, prefixes) done() - if err != nil { - s.writeErrorResponse(w, err) - return - } - - dErrsResp := &DeletePrefixesErrsResp{Errs: make([]error, len(errs))} - for idx, err := range errs { + for idx := range prefixes { if err != nil { dErrsResp.Errs[idx] = StorageErr(err.Error()) + } else { + if errs[idx] != nil { + dErrsResp.Errs[idx] = StorageErr(errs[idx].Error()) + } } } encoder.Encode(dErrsResp) diff --git a/cmd/utils.go b/cmd/utils.go index c248df20f..bdf886798 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -151,10 +151,9 @@ const ( // (Acceptable values range from 1 to 10000 inclusive) globalMaxPartID = 10000 - // Default values used while communicating for - // internode communication. - defaultDialTimeout = 15 * time.Second - defaultDialKeepAlive = 20 * time.Second + // Default values used while communicating for internode communication. + defaultDialTimeout = 5 * time.Second + defaultDialKeepAlive = 15 * time.Second ) // isMaxObjectSize - verify if max object size @@ -452,7 +451,8 @@ func newCustomHTTPTransport(tlsConfig *tls.Config, dialTimeout, dialKeepAlive ti Proxy: http.ProxyFromEnvironment, DialContext: newCustomDialContext(dialTimeout, dialKeepAlive), MaxIdleConnsPerHost: 256, - IdleConnTimeout: 60 * time.Second, + IdleConnTimeout: time.Minute, + ResponseHeaderTimeout: 3 * time.Minute, // Set conservative timeouts for MinIO internode. TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 10 * time.Second, TLSClientConfig: tlsConfig, @@ -466,14 +466,17 @@ func newCustomHTTPTransport(tlsConfig *tls.Config, dialTimeout, dialKeepAlive ti } } -// NewCustomHTTPTransport returns a new http configuration +// NewGatewayHTTPTransport returns a new http configuration // used while communicating with the cloud backends. // This sets the value for MaxIdleConnsPerHost from 2 (go default) // to 256. -func NewCustomHTTPTransport() *http.Transport { - return newCustomHTTPTransport(&tls.Config{ +func NewGatewayHTTPTransport() *http.Transport { + tr := newCustomHTTPTransport(&tls.Config{ RootCAs: globalRootCAs, }, defaultDialTimeout, defaultDialKeepAlive)() + // Set aggressive timeouts for gateway + tr.ResponseHeaderTimeout = 30 * time.Second + return tr } // Load the json (typically from disk file). diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 9322357c7..44349b4c6 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -2050,7 +2050,7 @@ func (web *webAPIHandlers) LoginSTS(r *http.Request, args *LoginSTSArgs, reply * } clnt := &http.Client{ - Transport: NewCustomHTTPTransport(), + Transport: NewGatewayHTTPTransport(), } resp, err := clnt.Do(req) @@ -2059,6 +2059,7 @@ func (web *webAPIHandlers) LoginSTS(r *http.Request, args *LoginSTSArgs, reply * return toJSONError(ctx, err) } defer xhttp.DrainBody(resp.Body) + if resp.StatusCode != http.StatusOK { return toJSONError(ctx, errors.New(resp.Status)) } diff --git a/staticcheck.conf b/staticcheck.conf index c01c87fca..fec5f6f33 100644 --- a/staticcheck.conf +++ b/staticcheck.conf @@ -1 +1 @@ -checks = ["all", "-ST1005", "-ST1000", "-SA4000", "-SA9004", "-SA1019", "-SA1008", "-U1000"] +checks = ["all", "-ST1005", "-ST1000", "-SA4000", "-SA9004", "-SA1019", "-SA1008", "-U1000", "-ST1016"]