diff --git a/cmd/carbonapi/http/find_handlers.go b/cmd/carbonapi/http/find_handlers.go index 298cae2c4..b84d318e0 100644 --- a/cmd/carbonapi/http/find_handlers.go +++ b/cmd/carbonapi/http/find_handlers.go @@ -23,6 +23,7 @@ import ( "github.com/go-graphite/carbonapi/date" "github.com/go-graphite/carbonapi/intervalset" utilctx "github.com/go-graphite/carbonapi/util/ctx" + "github.com/go-graphite/carbonapi/zipper/helper" ) // Find handler and it's helper functions @@ -283,7 +284,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { if returnCode < 300 { multiGlobs = &pbv3.MultiGlobResponse{Metrics: []pbv3.GlobResponse{}} } else { - setError(w, &accessLogDetails, http.StatusText(returnCode), returnCode, uid.String()) + setError(w, &accessLogDetails, helper.MerryRootError(err), returnCode, uid.String()) // We don't want to log this as an error if it's something normal // Normal is everything that is >= 500. So if config.Config.NotFoundStatusCode is 500 - this will be // logged as error diff --git a/cmd/carbonapi/http/helper.go b/cmd/carbonapi/http/helper.go index d35f5a56b..7757e94cd 100644 --- a/cmd/carbonapi/http/helper.go +++ b/cmd/carbonapi/http/helper.go @@ -2,6 +2,7 @@ package http import ( "fmt" + "html" "net/http" "strings" "time" @@ -229,6 +230,27 @@ func splitRemoteAddr(addr string) (string, string) { return tmp[0], tmp[1] } +func stripKey(key string, n int) string { + if len(key) > n+3 { + key = key[:n/2] + "..." + key[n/2+1:] + } + return key +} + +// stripError for strip network errors (ip and other private info) +func stripError(err string) string { + if strings.Contains(err, "connection refused") { + return "connection refused" + } else if strings.Contains(err, " lookup ") { + return "lookup error" + } else if strings.Contains(err, "broken pipe") { + return "broken pipe" + } else if strings.Contains(err, " connection reset ") { + return "connection reset" + } + return html.EscapeString(err) +} + func buildParseErrorString(target, e string, err error) string { msg := fmt.Sprintf("%s\n\n%-20s: %s\n", http.StatusText(http.StatusBadRequest), "Target", target) if err != nil { @@ -285,9 +307,56 @@ func timestampTruncate(ts int64, duration time.Duration, durations []config.Dura func setError(w http.ResponseWriter, accessLogDetails *carbonapipb.AccessLogDetails, msg string, status int, carbonapiUUID string) { w.Header().Set(ctxHeaderUUID, carbonapiUUID) - http.Error(w, http.StatusText(status)+": "+msg, status) + if msg == "" { + msg = http.StatusText(status) + } accessLogDetails.Reason = msg accessLogDetails.HTTPCode = int32(status) + msg = html.EscapeString(stripError(msg)) + http.Error(w, msg, status) +} + +func joinErrors(errMap map[string]string, sep string, status int) (msg, reason string) { + if len(errMap) == 0 { + msg = http.StatusText(status) + } else { + var buf, rBuf strings.Builder + buf.Grow(512) + rBuf.Grow(512) + + // map is unsorted, can produce flapping ordered output, not critical + for k, err := range errMap { + if buf.Len() > 0 { + buf.WriteString(sep) + rBuf.WriteString(sep) + } + buf.WriteString(html.EscapeString(stripKey(k, 128))) + rBuf.WriteString(k) + buf.WriteString(": ") + rBuf.WriteString(": ") + buf.WriteString(html.EscapeString(stripError(err))) + rBuf.WriteString(err) + } + + msg = buf.String() + reason = rBuf.String() + } + return +} + +func setErrors(w http.ResponseWriter, accessLogDetails *carbonapipb.AccessLogDetails, errMamp map[string]string, status int, carbonapiUUID string) { + w.Header().Set(ctxHeaderUUID, carbonapiUUID) + var msg string + if status != http.StatusOK { + if len(errMamp) == 0 { + msg = http.StatusText(status) + accessLogDetails.Reason = msg + } else { + msg, accessLogDetails.Reason = joinErrors(errMamp, "\n", status) + } + } + accessLogDetails.HTTPCode = int32(status) + http.Error(w, msg, status) } func queryLengthLimitExceeded(query []string, maxLength uint64) bool { diff --git a/cmd/carbonapi/http/render_handler.go b/cmd/carbonapi/http/render_handler.go index f2938527c..af3b6f65e 100644 --- a/cmd/carbonapi/http/render_handler.go +++ b/cmd/carbonapi/http/render_handler.go @@ -353,16 +353,16 @@ func renderHandler(w http.ResponseWriter, r *http.Request) { // Obtain error code from the errors // In case we have only "Not Found" errors, result should be 404 // Otherwise it should be 500 - var errMsgs []string + var errMsgs map[string]string returnCode, errMsgs = helper.MergeHttpErrorMap(errors) - logger.Debug("error response or no response", zap.Strings("error", errMsgs)) + logger.Debug("error response or no response", zap.Any("error", errMsgs)) // Allow override status code for 404-not-found replies. if returnCode == http.StatusNotFound { returnCode = config.Config.NotFoundStatusCode } if returnCode == http.StatusBadRequest || returnCode == http.StatusNotFound || returnCode == http.StatusForbidden || returnCode >= 500 { - setError(w, accessLogDetails, strings.Join(errMsgs, ","), returnCode, uid.String()) + setErrors(w, accessLogDetails, errMsgs, returnCode, uid.String()) logAsError = true return } diff --git a/cmd/carbonapi/http/tags_handler.go b/cmd/carbonapi/http/tags_handler.go index 2663f37d3..4f3216e2c 100644 --- a/cmd/carbonapi/http/tags_handler.go +++ b/cmd/carbonapi/http/tags_handler.go @@ -21,9 +21,10 @@ import ( func tagHandler(w http.ResponseWriter, r *http.Request) { t0 := time.Now() uuid := uuid.NewV4() + carbonapiUUID := uuid.String() // TODO: Migrate to context.WithTimeout - ctx := utilctx.SetUUID(r.Context(), uuid.String()) + ctx := utilctx.SetUUID(r.Context(), carbonapiUUID) requestHeaders := utilctx.GetLogHeaders(ctx) username, _, _ := r.BasicAuth() @@ -39,7 +40,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { var accessLogDetails = &carbonapipb.AccessLogDetails{ Handler: "tags", Username: username, - CarbonapiUUID: uuid.String(), + CarbonapiUUID: carbonapiUUID, URL: r.URL.Path, PeerIP: srcIP, PeerPort: srcPort, @@ -81,7 +82,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { rawQuery := q.Encode() if queryLengthLimitExceeded(r.Form["query"], config.Config.MaxQueryLength) { - setError(w, accessLogDetails, "query length limit exceeded", http.StatusBadRequest, uuid.String()) + setError(w, accessLogDetails, "query length limit exceeded", http.StatusBadRequest, carbonapiUUID) logAsError = true return } @@ -123,7 +124,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { } w.Header().Set("Content-Type", contentTypeJSON) - w.Header().Set(ctxHeaderUUID, uuid.String()) + w.Header().Set(ctxHeaderUUID, carbonapiUUID) _, _ = w.Write(b) accessLogDetails.Runtime = time.Since(t0).Seconds() accessLogDetails.HTTPCode = http.StatusOK diff --git a/cmd/mockbackend/e2etesting.go b/cmd/mockbackend/e2etesting.go index b8ab00ef3..381c674c5 100644 --- a/cmd/mockbackend/e2etesting.go +++ b/cmd/mockbackend/e2etesting.go @@ -13,6 +13,7 @@ import ( "net/url" "os" "reflect" + "sort" "strconv" "strings" "sync" @@ -45,6 +46,8 @@ type Query struct { type ExpectedResponse struct { HttpCode int `yaml:"httpCode"` ContentType string `yaml:"contentType"` + ErrBody string `yaml:"errBody"` + ErrSort bool `yaml:"errSort"` ExpectedResults []ExpectedResult `yaml:"expectedResults"` } @@ -245,8 +248,22 @@ func doTest(logger *zap.Logger, t *Query, verbose bool) []error { ) } - // We don't need to actually check body of response if we expect any sort of error (4xx/5xx) + // We don't need to actually check body of response if we expect any sort of error (4xx/5xx), but for check error handling do this if t.ExpectedResponse.HttpCode >= 300 { + if t.ExpectedResponse.ErrBody != "" { + errStr := string(b) + if t.ExpectedResponse.ErrSort && strings.Index(errStr, "\n") != len(errStr)-1 { + // resort error string + errs := sort.StringSlice(strings.Split(errStr, "\n")) + if errs[len(errs)-1] == "" { + errs = errs[:len(errs)-1] + } + errStr = strings.Join(errs, "\n") + "\n" + } + if t.ExpectedResponse.ErrBody != errStr { + failures = append(failures, merry2.Errorf("mismatch error body, got '%s', expected '%s'", string(b), t.ExpectedResponse.ErrBody)) + } + } return failures } diff --git a/cmd/mockbackend/testcases/find_error/find_error.yaml b/cmd/mockbackend/testcases/find_error/find_error.yaml index 53b5003f7..ad1fdf526 100644 --- a/cmd/mockbackend/testcases/find_error/find_error.yaml +++ b/cmd/mockbackend/testcases/find_error/find_error.yaml @@ -61,6 +61,25 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "Service Unavailable\n" + + # 503 + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=c&query=d&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "timeout while fetching Response\n" + + # 503 + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=d&query=e&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "Service Unavailable\n" # 503, partial success - endpoint: "http://127.0.0.1:8081" @@ -69,6 +88,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "Service Unavailable\n" listeners: - address: ":9070" @@ -91,3 +111,7 @@ listeners: "d": pathExpression: "d" code: 503 + + "e": + pathExpression: "e" + code: 503 diff --git a/cmd/mockbackend/testcases/render_error_all/render_error_all.yaml b/cmd/mockbackend/testcases/render_error_all/render_error_all.yaml index d312d89d0..b8ee69c27 100644 --- a/cmd/mockbackend/testcases/render_error_all/render_error_all.yaml +++ b/cmd/mockbackend/testcases/render_error_all/render_error_all.yaml @@ -45,6 +45,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "c: timeout while fetching Response\n" # 503 - endpoint: "http://127.0.0.1:8081" @@ -53,6 +54,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "d: Service Unavailable\n" # partial success - endpoint: "http://127.0.0.1:8081" @@ -61,6 +63,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "d: Service Unavailable\n" # partial success, must fail, target d failed - endpoint: "http://127.0.0.1:8081" @@ -69,6 +72,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "divideSeries(a,d): Service Unavailable\n" listeners: - address: ":9070" diff --git a/cmd/mockbackend/testcases/render_error_all_rr/render_error_all_rr.yaml b/cmd/mockbackend/testcases/render_error_all_rr/render_error_all_rr.yaml index a401b99ae..2c0e64748 100644 --- a/cmd/mockbackend/testcases/render_error_all_rr/render_error_all_rr.yaml +++ b/cmd/mockbackend/testcases/render_error_all_rr/render_error_all_rr.yaml @@ -81,6 +81,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "d: Service Unavailable\n" # partial success - endpoint: "http://127.0.0.1:8081" @@ -89,6 +90,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "d: Service Unavailable\n" # partial success # TODO: must fail, target d failed @@ -98,6 +100,7 @@ test: expectedResponse: httpCode: 503 contentType: "text/plain; charset=utf-8" + errBody: "divideSeries(a,d): Service Unavailable\n" listeners: - address: ":9070" diff --git a/cmd/mockbackend/testcases/render_error_refused/carbonapi.yaml b/cmd/mockbackend/testcases/render_error_refused/carbonapi.yaml new file mode 100644 index 000000000..e9109f72d --- /dev/null +++ b/cmd/mockbackend/testcases/render_error_refused/carbonapi.yaml @@ -0,0 +1,57 @@ +listen: "localhost:8081" +expvar: + enabled: true + pprofEnabled: false + listen: "" +concurency: 1000 +notFoundStatusCode: 200 +cache: + type: "mem" + size_mb: 0 + defaultTimeoutSec: 60 +cpus: 0 +tz: "" +maxBatchSize: 0 +graphite: + host: "" + interval: "60s" + prefix: "carbon.api" + pattern: "{prefix}.{fqdn}" +idleConnections: 10 +pidFile: "" +upstreams: + buckets: 10 + timeouts: + find: "2s" + render: "5s" + connect: "200ms" + concurrencyLimitPerServer: 0 + keepAliveInterval: "30s" + maxIdleConnsPerHost: 100 + backendsv2: + backends: + - + groupName: "mock-001" + protocol: "auto" + lbMethod: "all" + maxTries: 1 + maxBatchSize: 0 + keepAliveInterval: "10s" + concurrencyLimit: 0 + forceAttemptHTTP2: true + maxIdleConnsPerHost: 1000 + timeouts: + find: "3s" + render: "5s" + connect: "200ms" + servers: + - "http://127.0.0.1:9071" +graphite09compat: false +expireDelaySec: 10 +logger: + - logger: "" + file: "stderr" + level: "debug" + encoding: "console" + encodingTime: "iso8601" + encodingDuration: "seconds" diff --git a/cmd/mockbackend/testcases/render_error_refused/render_error_refused.yaml b/cmd/mockbackend/testcases/render_error_refused/render_error_refused.yaml new file mode 100644 index 000000000..0ca748654 --- /dev/null +++ b/cmd/mockbackend/testcases/render_error_refused/render_error_refused.yaml @@ -0,0 +1,72 @@ +version: "v1" +test: + apps: + - name: "carbonapi" + binary: "./carbonapi" + args: + - "-config" + - "./cmd/mockbackend/testcases/connection_refused/carbonapi.yaml" + - "-exact-config" + queries: + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/render/?target=a&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "a: connection refused\n" + errSort: true + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/render/?target=a&target=b&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "a: connection refused\nb: connection refused\n" + errSort: true + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find/?query=a&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "connection refused\n" + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find/?query=a&query=b&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + errBody: "connection refused\n" + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/tags/autoComplete/values?expr=tag2%3Dv1&tag=tag4" + expectedResponse: + httpCode: 200 + contentType: "application/json" + expectedResults: + - tagsAutocompelete: [] + # TODO: query must fail + # httpCode: 503 + # contentType: "text/plain; charset=utf-8" + # errBody: "connection refused\n" + # errSort: true + +listeners: + - address: ":9070" + expressions: + "a": + pathExpression: "a" + data: + - metricName: "a" + values: [0,1,2,2,3] + + # timeout + "c": + pathExpression: "c" + code: 404 + replyDelayMS: 7000 + + "d": + pathExpression: "d" + code: 503 diff --git a/zipper/helper/errors.go b/zipper/helper/errors.go new file mode 100644 index 000000000..1a59f612e --- /dev/null +++ b/zipper/helper/errors.go @@ -0,0 +1,168 @@ +package helper + +import ( + "context" + "net" + "net/http" + "net/url" + "strings" + + "github.com/ansel1/merry" + "github.com/go-graphite/carbonapi/pkg/parser" + "github.com/go-graphite/carbonapi/zipper/types" +) + +func requestError(err error, server string) merry.Error { + // with code InternalServerError by default, overwritten by custom error + if merry.Is(err, context.DeadlineExceeded) { + return types.ErrTimeoutExceeded.WithValue("server", server).WithCause(err) + } + if urlErr, ok := err.(*url.Error); ok { + if netErr, ok := urlErr.Err.(*net.OpError); ok { + return types.ErrBackendError.WithValue("server", server).WithCause(netErr) + } + } + if netErr, ok := err.(*net.OpError); ok { + return types.ErrBackendError.WithValue("server", server).WithCause(netErr) + } + return types.ErrResponceError.WithValue("server", server) +} + +func HttpErrorCode(err merry.Error) (code int) { + if err == nil { + code = http.StatusOK + } else { + c := merry.RootCause(err) + if c == nil { + c = err + } + + code = merry.HTTPCode(err) + if code == http.StatusNotFound { + return + } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { + // check for invalid args, see applyByNode rewrite function + code = http.StatusBadRequest + } + + if code == http.StatusGatewayTimeout || code == http.StatusBadGateway || merry.Is(c, types.ErrFailedToFetch) { + // simplify code, one error type for communications errors, all we can retry + code = http.StatusServiceUnavailable + } + } + + return +} + +// for stable return code on multiply errors +func recalcCode(code, newCode int) int { + if newCode == http.StatusGatewayTimeout || newCode == http.StatusBadGateway { + // simplify code, one error type for communications errors, all we can retry + newCode = http.StatusServiceUnavailable + } + if code == 0 || code == http.StatusNotFound { + return newCode + } + + if newCode >= 400 && newCode < 500 && code >= 400 && code < 500 { + if newCode == http.StatusBadRequest { + return newCode + } else if newCode == http.StatusForbidden && code != http.StatusBadRequest { + return newCode + } + } + if newCode < code { + code = newCode + } + return code +} + +// MerryRootError strip merry error chain +func MerryRootError(err error) string { + c := merry.RootCause(err) + if c == nil { + c = err + } + return merryError(c) +} + +func merryError(err error) string { + if msg := merry.Message(err); len(msg) > 0 { + return strings.TrimRight(msg, "\n") + } else { + return err.Error() + } +} + +func MergeHttpErrors(errors []merry.Error) (int, []string) { + returnCode := http.StatusNotFound + errMsgs := make([]string, 0) + for _, err := range errors { + c := merry.RootCause(err) + if c == nil { + c = err + } + + code := merry.HTTPCode(err) + if code == http.StatusNotFound { + continue + } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { + // check for invalid args, see applyByNode rewrite function + code = http.StatusBadRequest + } + + errMsgs = append(errMsgs, merryError(c)) + + returnCode = recalcCode(returnCode, code) + } + + return returnCode, errMsgs +} + +func MergeHttpErrorMap(errorsMap map[string]merry.Error) (returnCode int, errMap map[string]string) { + returnCode = http.StatusNotFound + errMap = make(map[string]string) + for key, err := range errorsMap { + c := merry.RootCause(err) + if c == nil { + c = err + } + + code := merry.HTTPCode(err) + if code == http.StatusNotFound { + continue + } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { + // check for invalid args, see applyByNode rewrite function + code = http.StatusBadRequest + } + + msg := merryError(c) + errMap[key] = msg + returnCode = recalcCode(returnCode, code) + } + + return +} + +func HttpErrorByCode(err merry.Error) merry.Error { + var returnErr merry.Error + if err == nil { + returnErr = types.ErrNoMetricsFetched + } else { + code := merry.HTTPCode(err) + msg := stripHtmlTags(merry.Message(err), 0) + if code == http.StatusForbidden { + returnErr = types.ErrForbidden + if len(msg) > 0 { + // pass message to caller + returnErr = returnErr.WithMessage(msg) + } + } else if code == http.StatusServiceUnavailable || code == http.StatusBadGateway || code == http.StatusGatewayTimeout { + returnErr = types.ErrFailedToFetch.WithHTTPCode(code).WithMessage(msg) + } else { + returnErr = types.ErrFailed.WithHTTPCode(code).WithMessage(msg) + } + } + + return returnErr +} diff --git a/zipper/helper/errors_test.go b/zipper/helper/errors_test.go new file mode 100644 index 000000000..87bcd983e --- /dev/null +++ b/zipper/helper/errors_test.go @@ -0,0 +1,279 @@ +package helper + +import ( + "fmt" + "net" + "net/http" + "reflect" + "testing" + + "github.com/ansel1/merry" + "github.com/go-graphite/carbonapi/zipper/types" +) + +func TestMergeHttpErrors(t *testing.T) { + tests := []struct { + name string + errors []merry.Error + wantCode int + want []string + }{ + { + name: "NotFound", + errors: []merry.Error{}, + wantCode: http.StatusNotFound, + want: []string{}, + }, + { + name: "NetErr", + errors: []merry.Error{ + types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")}).WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: []string{"connect: refused"}, + }, + { + name: "NetErr (incapsulated)", + errors: []merry.Error{ + types.ErrMaxTriesExceeded.WithCause(types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")})).WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: []string{"connect: refused"}, + }, + { + name: "ServiceUnavailable", + errors: []merry.Error{ + merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: []string{"unavaliable"}, + }, + { + name: "GatewayTimeout and ServiceUnavailable", + errors: []merry.Error{ + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: []string{"timeout", "unavaliable"}, + }, + { + name: "ServiceUnavailable and GatewayTimeout", + errors: []merry.Error{ + merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusServiceUnavailable, + want: []string{"unavaliable", "timeout"}, + }, + { + name: "Forbidden and GatewayTimeout", + errors: []merry.Error{ + merry.New("limit").WithHTTPCode(http.StatusForbidden), + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusForbidden, + want: []string{"limit", "timeout"}, + }, + { + name: "GatewayTimeout and Forbidden", + errors: []merry.Error{ + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + merry.New("limit").WithHTTPCode(http.StatusForbidden), + }, + wantCode: http.StatusForbidden, + want: []string{"timeout", "limit"}, + }, + { + name: "InternalServerError and Forbidden", + errors: []merry.Error{ + merry.New("error").WithHTTPCode(http.StatusInternalServerError), + merry.New("limit").WithHTTPCode(http.StatusForbidden), + }, + wantCode: http.StatusForbidden, + want: []string{"error", "limit"}, + }, + { + name: "InternalServerError and GatewayTimeout", + errors: []merry.Error{ + merry.New("error").WithHTTPCode(http.StatusInternalServerError), + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusInternalServerError, + want: []string{"error", "timeout"}, + }, + { + name: "GatewayTimeout and InternalServerError", + errors: []merry.Error{ + merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + merry.New("error").WithHTTPCode(http.StatusInternalServerError), + }, + wantCode: http.StatusInternalServerError, + want: []string{"timeout", "error"}, + }, + { + name: "BadRequest and Forbidden", + errors: []merry.Error{ + merry.New("error").WithHTTPCode(http.StatusBadRequest), + merry.New("limit").WithHTTPCode(http.StatusForbidden), + }, + wantCode: http.StatusBadRequest, + want: []string{"error", "limit"}, + }, + { + name: "Forbidden and BadRequest", + errors: []merry.Error{ + merry.New("limit").WithHTTPCode(http.StatusForbidden), + merry.New("error").WithHTTPCode(http.StatusBadRequest), + }, + wantCode: http.StatusBadRequest, + want: []string{"limit", "error"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotCode, got := MergeHttpErrors(tt.errors) + if gotCode != tt.wantCode { + t.Errorf("MergeHttpErrors() gotCode = %v, want %v", gotCode, tt.wantCode) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("MergeHttpErrors() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestMergeHttpErrorMap(t *testing.T) { + tests := []struct { + name string + errors map[string]merry.Error + wantCode int + want map[string]string + }{ + { + name: "NotFound", + errors: map[string]merry.Error{}, + wantCode: http.StatusNotFound, + want: map[string]string{}, + }, + { + name: "NetErr", + errors: map[string]merry.Error{ + "a": types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")}).WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: map[string]string{"a": "connect: refused"}, + }, + { + name: "NetErr (incapsulated)", + errors: map[string]merry.Error{ + "b": types.ErrMaxTriesExceeded.WithCause(types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")})).WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: map[string]string{"b": "connect: refused"}, + }, + { + name: "ServiceUnavailable", + errors: map[string]merry.Error{ + "d": merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: map[string]string{"d": "unavaliable"}, + }, + { + name: "GatewayTimeout and ServiceUnavailable", + errors: map[string]merry.Error{ + "a": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + "de": merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + }, + wantCode: http.StatusServiceUnavailable, + want: map[string]string{"a": "timeout", "de": "unavaliable"}, + }, + { + name: "ServiceUnavailable and GatewayTimeout", + errors: map[string]merry.Error{ + "de": merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), + "a": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusServiceUnavailable, + want: map[string]string{"a": "timeout", "de": "unavaliable"}, + }, + { + name: "Forbidden and GatewayTimeout", + errors: map[string]merry.Error{ + "de": merry.New("limit").WithHTTPCode(http.StatusForbidden), + "c": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusForbidden, + want: map[string]string{"c": "timeout", "de": "limit"}, + }, + { + name: "GatewayTimeout and Forbidden", + errors: map[string]merry.Error{ + "a": merry.New("limit").WithHTTPCode(http.StatusForbidden), + "c": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusForbidden, + want: map[string]string{"a": "limit", "c": "timeout"}, + }, + { + name: "InternalServerError and Forbidden", + errors: map[string]merry.Error{ + "a": merry.New("error").WithHTTPCode(http.StatusInternalServerError), + "cd": merry.New("limit").WithHTTPCode(http.StatusForbidden), + }, + wantCode: http.StatusForbidden, + want: map[string]string{"a": "error", "cd": "limit"}, + }, + { + name: "InternalServerError and GatewayTimeout", + errors: map[string]merry.Error{ + "a": merry.New("error").WithHTTPCode(http.StatusInternalServerError), + "b": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + }, + wantCode: http.StatusInternalServerError, + want: map[string]string{"a": "error", "b": "timeout"}, + }, + { + name: "GatewayTimeout and InternalServerError", + errors: map[string]merry.Error{ + "a": merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), + "cd": merry.New("error").WithHTTPCode(http.StatusInternalServerError), + }, + wantCode: http.StatusInternalServerError, + want: map[string]string{"a": "timeout", "cd": "error"}, + }, + { + name: "BadRequest and Forbidden", + errors: map[string]merry.Error{ + "de": merry.New("error").WithHTTPCode(http.StatusBadRequest), + "a": merry.New("limit").WithHTTPCode(http.StatusForbidden), + }, + wantCode: http.StatusBadRequest, + want: map[string]string{"a": "limit", "de": "error"}, + }, + { + name: "Forbidden and BadRequest", + errors: map[string]merry.Error{ + "a": merry.New("limit").WithHTTPCode(http.StatusForbidden), + "b{c,de,klmn}.cde.d{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}.b{c,de,klmn}.cde.d{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}": merry.New("error").WithHTTPCode(http.StatusBadRequest), + }, + wantCode: http.StatusBadRequest, + want: map[string]string{ + "a": "limit", + "b{c,de,klmn}.cde.d{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}.b{c,de,klmn}.cde.d{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}.e{c,de,klmn}.k{c,de,klmn}": "error", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotCode, got := MergeHttpErrorMap(tt.errors) + if gotCode != tt.wantCode { + t.Errorf("MergeHttpErrors() gotCode = %v, want %v", gotCode, tt.wantCode) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("MergeHttpErrors() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/zipper/helper/requests.go b/zipper/helper/requests.go index b31e99a13..2b8d0ed2d 100644 --- a/zipper/helper/requests.go +++ b/zipper/helper/requests.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "io" - "net" "net/http" "net/url" "strings" @@ -15,7 +14,6 @@ import ( "go.uber.org/zap" "github.com/go-graphite/carbonapi/limiter" - "github.com/go-graphite/carbonapi/pkg/parser" util "github.com/go-graphite/carbonapi/util/ctx" "github.com/go-graphite/carbonapi/zipper/types" ) @@ -82,124 +80,6 @@ func stripHtmlTags(s string, maxLen int) string { return s } -func requestError(err error, server string) merry.Error { - // with code InternalServerError by default, overwritten by custom error - if merry.Is(err, context.DeadlineExceeded) { - return types.ErrTimeoutExceeded.WithValue("server", server).WithCause(err) - } - if urlErr, ok := err.(*url.Error); ok { - if netErr, ok := urlErr.Err.(*net.OpError); ok { - return types.ErrBackendError.WithValue("server", server).WithCause(netErr) - } - } - if netErr, ok := err.(*net.OpError); ok { - return types.ErrBackendError.WithValue("server", server).WithCause(netErr) - } - return types.ErrResponceError.WithValue("server", server) -} - -func HttpErrorCode(err merry.Error) (code int) { - if err == nil { - code = http.StatusOK - } else { - c := merry.RootCause(err) - if c == nil { - c = err - } - - code = merry.HTTPCode(err) - if code == http.StatusNotFound { - return - } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { - // check for invalid args, see applyByNode rewrite function - code = http.StatusBadRequest - } - - if code == http.StatusGatewayTimeout || code == http.StatusBadGateway || merry.Is(c, types.ErrFailedToFetch) { - // simplify code, one error type for communications errors, all we can retry - code = http.StatusServiceUnavailable - } - } - - return -} - -func MergeHttpErrors(errors []merry.Error) (int, []string) { - returnCode := http.StatusNotFound - errMsgs := make([]string, 0) - for _, err := range errors { - c := merry.RootCause(err) - if c == nil { - c = err - } - - code := merry.HTTPCode(err) - if code == http.StatusNotFound { - continue - } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { - // check for invalid args, see applyByNode rewrite function - code = http.StatusBadRequest - } - - if msg := merry.Message(c); len(msg) > 0 { - errMsgs = append(errMsgs, strings.TrimRight(msg, "\n")) - } else { - errMsgs = append(errMsgs, c.Error()) - } - - if code == http.StatusGatewayTimeout || code == http.StatusBadGateway { - // simplify code, one error type for communications errors, all we can retry - code = http.StatusServiceUnavailable - } - - if code == http.StatusBadRequest { - // The 400 is returned on wrong requests, e.g. non-existent functions - returnCode = code - } else if returnCode == http.StatusNotFound || code == http.StatusForbidden { - // First error or access denied (may be limits or other) - returnCode = code - } else if code != http.StatusServiceUnavailable { - returnCode = code - } - } - - return returnCode, errMsgs -} - -func MergeHttpErrorMap(errorsMap map[string]merry.Error) (int, []string) { - errors := make([]merry.Error, len(errorsMap)) - i := 0 - for _, err := range errorsMap { - errors[i] = err - i++ - } - - return MergeHttpErrors(errors) -} - -func HttpErrorByCode(err merry.Error) merry.Error { - var returnErr merry.Error - if err == nil { - returnErr = types.ErrNoMetricsFetched - } else { - code := merry.HTTPCode(err) - msg := stripHtmlTags(merry.Message(err), 0) - if code == http.StatusForbidden { - returnErr = types.ErrForbidden - if len(msg) > 0 { - // pass message to caller - returnErr = returnErr.WithMessage(msg) - } - } else if code == http.StatusServiceUnavailable || code == http.StatusBadGateway || code == http.StatusGatewayTimeout { - returnErr = types.ErrFailedToFetch.WithHTTPCode(code).WithMessage(msg) - } else { - returnErr = types.ErrFailed.WithHTTPCode(code).WithMessage(msg) - } - } - - return returnErr -} - type ServerResponse struct { Server string Response []byte @@ -332,7 +212,7 @@ func (c *HttpQuery) doRequest(ctx context.Context, logger *zap.Logger, server, u return &ServerResponse{Server: server, Response: body}, nil } -func (c *HttpQuery) DoQuery(ctx context.Context, logger *zap.Logger, uri string, r types.Request) (*ServerResponse, merry.Error) { +func (c *HttpQuery) DoQuery(ctx context.Context, logger *zap.Logger, uri string, r types.Request) (resp *ServerResponse, err merry.Error) { maxTries := c.maxTries if len(c.servers) > maxTries { maxTries = len(c.servers) @@ -345,11 +225,14 @@ func (c *HttpQuery) DoQuery(ctx context.Context, logger *zap.Logger, uri string, res, err := c.doRequest(ctx, logger, server, uri, r) if err != nil { logger.Debug("have errors", - zap.Error(err), + zap.String("error", err.Error()), + zap.String("server", server), ) e = e.WithCause(err).WithHTTPCode(merry.HTTPCode(err)) code = merry.HTTPCode(err) + // TODO (msaf1980): may be metric for server failures ? + // TODO (msaf1980): may be retry policy for avoid retry bad queries ? continue } @@ -359,7 +242,7 @@ func (c *HttpQuery) DoQuery(ctx context.Context, logger *zap.Logger, uri string, return nil, types.ErrMaxTriesExceeded.WithCause(e).WithHTTPCode(code) } -func (c *HttpQuery) DoQueryToAll(ctx context.Context, logger *zap.Logger, uri string, r types.Request) ([]*ServerResponse, merry.Error) { +func (c *HttpQuery) DoQueryToAll(ctx context.Context, logger *zap.Logger, uri string, r types.Request) (resp []*ServerResponse, err merry.Error) { maxTries := c.maxTries if len(c.servers) > maxTries { maxTries = len(c.servers) diff --git a/zipper/helper/requests_test.go b/zipper/helper/requests_test.go index 0b60d7187..afd2ecb36 100644 --- a/zipper/helper/requests_test.go +++ b/zipper/helper/requests_test.go @@ -1,151 +1,12 @@ package helper import ( - "fmt" - "net" - "net/http" - "reflect" + "strconv" "testing" - "github.com/ansel1/merry" - "github.com/go-graphite/carbonapi/zipper/types" "github.com/stretchr/testify/assert" ) -func TestMergeHttpErrors(t *testing.T) { - type args struct { - } - tests := []struct { - name string - errors []merry.Error - wantCode int - want []string - }{ - { - name: "NotFound", - errors: []merry.Error{}, - wantCode: http.StatusNotFound, - want: []string{}, - }, - { - name: "NetErr", - errors: []merry.Error{ - types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")}).WithHTTPCode(http.StatusServiceUnavailable), - }, - wantCode: http.StatusServiceUnavailable, - want: []string{"connect: refused"}, - }, - { - name: "NetErr (incapsulated)", - errors: []merry.Error{ - types.ErrMaxTriesExceeded.WithCause(types.ErrBackendError.WithValue("server", "test").WithCause(&net.OpError{Op: "connect", Err: fmt.Errorf("refused")})).WithHTTPCode(http.StatusServiceUnavailable), - }, - wantCode: http.StatusServiceUnavailable, - want: []string{"connect: refused"}, - }, - { - name: "ServiceUnavailable", - errors: []merry.Error{ - merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), - }, - wantCode: http.StatusServiceUnavailable, - want: []string{"unavaliable"}, - }, - { - name: "GatewayTimeout and ServiceUnavailable", - errors: []merry.Error{ - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), - }, - wantCode: http.StatusServiceUnavailable, - want: []string{"timeout", "unavaliable"}, - }, - { - name: "ServiceUnavailable and GatewayTimeout", - errors: []merry.Error{ - merry.New("unavaliable").WithHTTPCode(http.StatusServiceUnavailable), - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - }, - wantCode: http.StatusServiceUnavailable, - want: []string{"unavaliable", "timeout"}, - }, - { - name: "Forbidden and GatewayTimeout", - errors: []merry.Error{ - merry.New("limit").WithHTTPCode(http.StatusForbidden), - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - }, - wantCode: http.StatusForbidden, - want: []string{"limit", "timeout"}, - }, - { - name: "GatewayTimeout and Forbidden", - errors: []merry.Error{ - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - merry.New("limit").WithHTTPCode(http.StatusForbidden), - }, - wantCode: http.StatusForbidden, - want: []string{"timeout", "limit"}, - }, - { - name: "InternalServerError and Forbidden", - errors: []merry.Error{ - merry.New("error").WithHTTPCode(http.StatusInternalServerError), - merry.New("limit").WithHTTPCode(http.StatusForbidden), - }, - wantCode: http.StatusForbidden, - want: []string{"error", "limit"}, - }, - { - name: "InternalServerError and GatewayTimeout", - errors: []merry.Error{ - merry.New("error").WithHTTPCode(http.StatusInternalServerError), - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - }, - wantCode: http.StatusInternalServerError, - want: []string{"error", "timeout"}, - }, - { - name: "GatewayTimeout and InternalServerError", - errors: []merry.Error{ - merry.New("timeout").WithHTTPCode(http.StatusGatewayTimeout), - merry.New("error").WithHTTPCode(http.StatusInternalServerError), - }, - wantCode: http.StatusInternalServerError, - want: []string{"timeout", "error"}, - }, - { - name: "BadRequest and Forbidden", - errors: []merry.Error{ - merry.New("error").WithHTTPCode(http.StatusBadRequest), - merry.New("limit").WithHTTPCode(http.StatusForbidden), - }, - wantCode: http.StatusForbidden, // Last win - want: []string{"error", "limit"}, - }, - { - name: "Forbidden and BadRequest", - errors: []merry.Error{ - merry.New("limit").WithHTTPCode(http.StatusForbidden), - merry.New("error").WithHTTPCode(http.StatusBadRequest), - }, - wantCode: http.StatusBadRequest, // Last win - want: []string{"limit", "error"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotCode, got := MergeHttpErrors(tt.errors) - if gotCode != tt.wantCode { - t.Errorf("MergeHttpErrors() gotCode = %v, want %v", gotCode, tt.wantCode) - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("MergeHttpErrors() got = %v, want %v", got, tt.want) - } - }) - } -} - func Test_stripHtmlTags(t *testing.T) { tests := []struct { name string @@ -184,3 +45,27 @@ func Test_stripHtmlTags(t *testing.T) { }) } } + +func Test_recalcCode(t *testing.T) { + tests := []struct { + code int + newCode int + want int + }{ + {code: 500, newCode: 403, want: 403}, + {code: 403, newCode: 500, want: 403}, + {code: 403, newCode: 400, want: 400}, + {code: 400, newCode: 403, want: 400}, + {code: 500, newCode: 503, want: 500}, + {code: 503, newCode: 500, want: 500}, + {code: 503, newCode: 502, want: 503}, + {code: 0, newCode: 502, want: 503}, + } + for i, tt := range tests { + t.Run(strconv.Itoa(i), func(t *testing.T) { + if got := recalcCode(tt.code, tt.newCode); got != tt.want { + t.Errorf("recalcCode(%d, %d) = %d, want %d", tt.code, tt.newCode, got, tt.want) + } + }) + } +}