From 62cc5a04a71b3ed50f8cfe2038837fa1f13ef961 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Mon, 4 Mar 2024 15:53:00 +0300 Subject: [PATCH] [#328] Log error on failed response writing Signed-off-by: Denis Kirillov --- api/handler/cors.go | 10 +++++-- api/handler/head.go | 5 +++- api/handler/multipart_upload.go | 5 +++- api/handler/put.go | 21 ++++++++++--- api/handler/util.go | 7 +++-- api/middleware/auth.go | 8 +++-- api/middleware/policy.go | 4 ++- api/middleware/response.go | 53 +++++++++++++++++++++------------ api/router.go | 14 +++++++-- internal/logs/logs.go | 1 + 10 files changed, 94 insertions(+), 34 deletions(-) diff --git a/api/handler/cors.go b/api/handler/cors.go index 0294e222..69684fa6 100644 --- a/api/handler/cors.go +++ b/api/handler/cors.go @@ -66,7 +66,10 @@ func (h *handler) PutBucketCorsHandler(w http.ResponseWriter, r *http.Request) { return } - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } func (h *handler) DeleteBucketCorsHandler(w http.ResponseWriter, r *http.Request) { @@ -200,7 +203,10 @@ func (h *handler) Preflight(w http.ResponseWriter, r *http.Request) { if o != wildcard { w.Header().Set(api.AccessControlAllowCredentials, "true") } - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } return } } diff --git a/api/handler/head.go b/api/handler/head.go index 8a52c3e5..8a3bf665 100644 --- a/api/handler/head.go +++ b/api/handler/head.go @@ -140,7 +140,10 @@ func (h *handler) HeadBucketHandler(w http.ResponseWriter, r *http.Request) { w.Header().Set(api.ContainerZone, bktInfo.Zone) } - middleware.WriteResponse(w, http.StatusOK, nil, middleware.MimeNone) + if err = middleware.WriteResponse(w, http.StatusOK, nil, middleware.MimeNone); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } func (h *handler) setLockingHeaders(bktInfo *data.BucketInfo, lockInfo data.LockInfo, header http.Header) error { diff --git a/api/handler/multipart_upload.go b/api/handler/multipart_upload.go index 73141731..1a062e57 100644 --- a/api/handler/multipart_upload.go +++ b/api/handler/multipart_upload.go @@ -266,7 +266,10 @@ func (h *handler) UploadPartHandler(w http.ResponseWriter, r *http.Request) { } w.Header().Set(api.ETag, data.Quote(hash)) - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } func (h *handler) UploadPartCopy(w http.ResponseWriter, r *http.Request) { diff --git a/api/handler/put.go b/api/handler/put.go index 9f8f866b..525da9de 100644 --- a/api/handler/put.go +++ b/api/handler/put.go @@ -337,7 +337,10 @@ func (h *handler) PutObjectHandler(w http.ResponseWriter, r *http.Request) { w.Header().Set(api.ETag, data.Quote(objInfo.ETag(h.cfg.MD5Enabled()))) - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } func (h *handler) getBodyReader(r *http.Request) (io.ReadCloser, error) { @@ -602,7 +605,11 @@ func (h *handler) PostObject(w http.ResponseWriter, r *http.Request) { ETag: data.Quote(objInfo.ETag(h.cfg.MD5Enabled())), } w.WriteHeader(status) - if _, err = w.Write(middleware.EncodeResponse(resp)); err != nil { + respData, err := middleware.EncodeResponse(resp) + if err != nil { + h.logAndSendError(w, "encode response", reqInfo, err) + } + if _, err = w.Write(respData); err != nil { h.logAndSendError(w, "something went wrong", reqInfo, err) } return @@ -873,7 +880,10 @@ func (h *handler) createBucketHandlerPolicy(w http.ResponseWriter, r *http.Reque return } - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } func (h *handler) createBucketHandlerACL(w http.ResponseWriter, r *http.Request) { @@ -942,7 +952,10 @@ func (h *handler) createBucketHandlerACL(w http.ResponseWriter, r *http.Request) return } - middleware.WriteSuccessResponseHeadersOnly(w) + if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { + h.logAndSendError(w, "write response", reqInfo, err) + return + } } const s3ActionPrefix = "s3:" diff --git a/api/handler/util.go b/api/handler/util.go index af595c74..591837bc 100644 --- a/api/handler/util.go +++ b/api/handler/util.go @@ -31,9 +31,12 @@ func (h *handler) reqLogger(ctx context.Context) *zap.Logger { func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) { err = handleDeleteMarker(w, err) - code := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err)) + if code, wrErr := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err)); wrErr != nil { + additional = append(additional, zap.NamedError("write_response_error", wrErr)) + } else { + additional = append(additional, zap.Int("status", code)) + } fields := []zap.Field{ - zap.Int("status", code), zap.String("request_id", reqInfo.RequestID), zap.String("method", reqInfo.API), zap.String("bucket", reqInfo.BucketName), diff --git a/api/middleware/auth.go b/api/middleware/auth.go index 08e3d750..950e3c7a 100644 --- a/api/middleware/auth.go +++ b/api/middleware/auth.go @@ -59,7 +59,9 @@ func Auth(center Center, log *zap.Logger) Func { if _, ok := err.(apiErrors.Error); !ok { err = apiErrors.GetAPIError(apiErrors.ErrAccessDenied) } - WriteErrorResponse(w, GetReqInfo(r.Context()), err) + if _, wrErr := WriteErrorResponse(w, GetReqInfo(r.Context()), err); wrErr != nil { + reqLogOrDefault(ctx, log).Error(logs.FailedToWriteResponse, zap.Error(wrErr)) + } return } } else { @@ -97,7 +99,9 @@ func FrostfsIDValidation(frostfsID FrostFSIDValidator, log *zap.Logger) Func { if err = validateBearerToken(frostfsID, bd.Gate.BearerToken); err != nil { reqLogOrDefault(ctx, log).Error(logs.FrostfsIDValidationFailed, zap.Error(err)) - WriteErrorResponse(w, GetReqInfo(r.Context()), err) + if _, wrErr := WriteErrorResponse(w, GetReqInfo(r.Context()), err); wrErr != nil { + reqLogOrDefault(ctx, log).Error(logs.FailedToWriteResponse, zap.Error(wrErr)) + } return } diff --git a/api/middleware/policy.go b/api/middleware/policy.go index bc479b56..02cf50a8 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -47,7 +47,9 @@ func PolicyCheck(cfg PolicyConfig) Func { ctx := r.Context() if err := policyCheck(r, cfg); err != nil { reqLogOrDefault(ctx, cfg.Log).Error(logs.PolicyValidationFailed, zap.Error(err)) - WriteErrorResponse(w, GetReqInfo(ctx), err) + if _, wrErr := WriteErrorResponse(w, GetReqInfo(ctx), err); wrErr != nil { + reqLogOrDefault(ctx, cfg.Log).Error(logs.FailedToWriteResponse, zap.Error(wrErr)) + } return } diff --git a/api/middleware/response.go b/api/middleware/response.go index f458de0a..54f5b2ff 100644 --- a/api/middleware/response.go +++ b/api/middleware/response.go @@ -118,7 +118,8 @@ var s3ErrorResponseMap = map[string]string{ } // WriteErrorResponse writes error headers. -func WriteErrorResponse(w http.ResponseWriter, reqInfo *ReqInfo, err error) int { +// returns http error code and error in case of failure of response writing. +func WriteErrorResponse(w http.ResponseWriter, reqInfo *ReqInfo, err error) (int, error) { code := http.StatusInternalServerError if e, ok := err.(errors.Error); ok { @@ -134,9 +135,14 @@ func WriteErrorResponse(w http.ResponseWriter, reqInfo *ReqInfo, err error) int // Generates error response. errorResponse := getAPIErrorResponse(reqInfo, err) - encodedErrorResponse := EncodeResponse(errorResponse) - WriteResponse(w, code, encodedErrorResponse, MimeXML) - return code + encodedErrorResponse, err := EncodeResponse(errorResponse) + if err != nil { + return 0, fmt.Errorf("encode response: %w", err) + } + if err = WriteResponse(w, code, encodedErrorResponse, MimeXML); err != nil { + return 0, fmt.Errorf("write response: %w", err) + } + return code, nil } // Write http common headers. @@ -157,7 +163,7 @@ func removeSensitiveHeaders(h http.Header) { } // WriteResponse writes given statusCode and response into w (with mType header if set). -func WriteResponse(w http.ResponseWriter, statusCode int, response []byte, mType mimeType) { +func WriteResponse(w http.ResponseWriter, statusCode int, response []byte, mType mimeType) error { setCommonHeaders(w) if mType != MimeNone { w.Header().Set(hdrContentType, string(mType)) @@ -165,37 +171,46 @@ func WriteResponse(w http.ResponseWriter, statusCode int, response []byte, mType w.Header().Set(hdrContentLength, strconv.Itoa(len(response))) w.WriteHeader(statusCode) if response == nil { - return + return nil } - WriteResponseBody(w, response) + return WriteResponseBody(w, response) } // WriteResponseBody writes response into w. -func WriteResponseBody(w http.ResponseWriter, response []byte) { - _, _ = w.Write(response) +func WriteResponseBody(w http.ResponseWriter, response []byte) error { + if _, err := w.Write(response); err != nil { + return err + } + if flusher, ok := w.(http.Flusher); ok { flusher.Flush() } + + return nil } // EncodeResponse encodes the response headers into XML format. -func EncodeResponse(response interface{}) []byte { +func EncodeResponse(response interface{}) ([]byte, error) { var bytesBuffer bytes.Buffer bytesBuffer.WriteString(xml.Header) - _ = xml. - NewEncoder(&bytesBuffer). - Encode(response) - return bytesBuffer.Bytes() + if err := xml.NewEncoder(&bytesBuffer).Encode(response); err != nil { + return nil, err + } + + return bytesBuffer.Bytes(), nil } // EncodeResponseNoHeader encodes response without setting xml.Header. // Should be used with periodicXMLWriter which sends xml.Header to the client // with whitespaces to keep connection alive. -func EncodeResponseNoHeader(response interface{}) []byte { +func EncodeResponseNoHeader(response interface{}) ([]byte, error) { var bytesBuffer bytes.Buffer - _ = xml.NewEncoder(&bytesBuffer).Encode(response) - return bytesBuffer.Bytes() + if err := xml.NewEncoder(&bytesBuffer).Encode(response); err != nil { + return nil, err + } + + return bytesBuffer.Bytes(), nil } // EncodeToResponse encodes the response into ResponseWriter. @@ -227,8 +242,8 @@ func EncodeToResponseNoHeader(w http.ResponseWriter, response interface{}) error // WriteSuccessResponseHeadersOnly writes HTTP (200) OK response with no data // to the client. -func WriteSuccessResponseHeadersOnly(w http.ResponseWriter) { - WriteResponse(w, http.StatusOK, nil, MimeNone) +func WriteSuccessResponseHeadersOnly(w http.ResponseWriter) error { + return WriteResponse(w, http.StatusOK, nil, MimeNone) } // Error -- Returns S3 error string. diff --git a/api/router.go b/api/router.go index 7e6e3490..c873b711 100644 --- a/api/router.go +++ b/api/router.go @@ -178,14 +178,24 @@ func errorResponseHandler(w http.ResponseWriter, r *http.Request) { reqInfo := s3middleware.GetReqInfo(ctx) desc := fmt.Sprintf("Unknown API request at %s", r.URL.Path) - s3middleware.WriteErrorResponse(w, reqInfo, errors.Error{ + _, wrErr := s3middleware.WriteErrorResponse(w, reqInfo, errors.Error{ Code: "UnknownAPIRequest", Description: desc, HTTPStatusCode: http.StatusBadRequest, }) if log := s3middleware.GetReqLog(ctx); log != nil { - log.Error(logs.RequestUnmatched, zap.String("method", reqInfo.API), zap.String("http method", r.Method), zap.String("url", r.RequestURI)) + fields := []zap.Field{ + zap.String("method", reqInfo.API), + zap.String("http method", r.Method), + zap.String("url", r.RequestURI), + } + + if wrErr != nil { + fields = append(fields, zap.NamedError("write_response_error", wrErr)) + } + + log.Error(logs.RequestUnmatched, fields...) } } diff --git a/internal/logs/logs.go b/internal/logs/logs.go index de2d3447..fd3207ee 100644 --- a/internal/logs/logs.go +++ b/internal/logs/logs.go @@ -149,4 +149,5 @@ const ( FailedToGenerateRequestID = "failed to generate request id" InvalidBucketObjectLockEnabledHeader = "invalid X-Amz-Bucket-Object-Lock-Enabled header" InvalidTreeKV = "invalid tree service meta KV" + FailedToWriteResponse = "failed to write response" )