diff --git a/pkg/distributor/errors.go b/pkg/distributor/errors.go index f61a23ff3ec..115585b6979 100644 --- a/pkg/distributor/errors.go +++ b/pkg/distributor/errors.go @@ -248,6 +248,8 @@ func toGRPCError(pushErr error, serviceOverloadErrorEnabled bool) error { errCode = codes.FailedPrecondition case mimirpb.CIRCUIT_BREAKER_OPEN: errCode = codes.Unavailable + case mimirpb.METHOD_NOT_ALLOWED: + errCode = codes.Unimplemented } } stat := status.New(errCode, pushErr.Error()) @@ -296,7 +298,11 @@ func wrapPartitionPushError(err error, partitionID int32) error { func isIngesterClientError(err error) bool { var ingesterPushErr ingesterPushError if errors.As(err, &ingesterPushErr) { - return ingesterPushErr.errorCause() == mimirpb.BAD_DATA + switch ingesterPushErr.errorCause() { + case mimirpb.BAD_DATA, mimirpb.METHOD_NOT_ALLOWED: + return true + } + return false } // This code is needed for backwards compatibility, since ingesters may still return errors with HTTP status diff --git a/pkg/distributor/errors_test.go b/pkg/distributor/errors_test.go index ddc0b94c471..0d95b96f030 100644 --- a/pkg/distributor/errors_test.go +++ b/pkg/distributor/errors_test.go @@ -364,6 +364,18 @@ func TestToGRPCError(t *testing.T) { expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, circuitbreaker.ErrOpen), expectedErrorDetails: &mimirpb.ErrorDetails{Cause: mimirpb.CIRCUIT_BREAKER_OPEN}, }, + "an ingesterPushError with METHOD_NOT_ALLOWED cause gets translated into a Unimplemented error with METHOD_NOT_ALLOWED cause": { + err: newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID), + expectedGRPCCode: codes.Unimplemented, + expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg), + expectedErrorDetails: &mimirpb.ErrorDetails{Cause: mimirpb.METHOD_NOT_ALLOWED}, + }, + "a DoNotLogError of an ingesterPushError with METHOD_NOT_ALLOWED cause gets translated into a Unimplemented error with METHOD_NOT_ALLOWED cause": { + err: middleware.DoNotLogError{Err: newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID)}, + expectedGRPCCode: codes.Unimplemented, + expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg), + expectedErrorDetails: &mimirpb.ErrorDetails{Cause: mimirpb.METHOD_NOT_ALLOWED}, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -486,7 +498,11 @@ func TestIsIngesterClientError(t *testing.T) { err: ingesterPushError{cause: mimirpb.BAD_DATA}, expectedOutcome: true, }, - "an ingesterPushError with error cause different from BAD_DATA is not a client error": { + "an ingesterPushError with error cause METHOD_NOT_ALLOWED is a client error": { + err: ingesterPushError{cause: mimirpb.METHOD_NOT_ALLOWED}, + expectedOutcome: true, + }, + "an ingesterPushError with other error cause is not a client error": { err: ingesterPushError{cause: mimirpb.SERVICE_UNAVAILABLE}, expectedOutcome: false, }, diff --git a/pkg/distributor/push.go b/pkg/distributor/push.go index 440ad1cb536..4df320efd4f 100644 --- a/pkg/distributor/push.go +++ b/pkg/distributor/push.go @@ -228,6 +228,8 @@ func toHTTPStatus(ctx context.Context, pushErr error, limits *validation.Overrid return http.StatusServiceUnavailable case mimirpb.CIRCUIT_BREAKER_OPEN: return http.StatusServiceUnavailable + case mimirpb.METHOD_NOT_ALLOWED: + return http.StatusMethodNotAllowed } } diff --git a/pkg/distributor/push_test.go b/pkg/distributor/push_test.go index 43840faeaa8..7e443e55f9f 100644 --- a/pkg/distributor/push_test.go +++ b/pkg/distributor/push_test.go @@ -1091,6 +1091,16 @@ func TestHandler_ToHTTPStatus(t *testing.T) { expectedHTTPStatus: http.StatusBadRequest, expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg), }, + "an ingesterPushError with METHOD_NOT_ALLOWED cause gets translated into an HTTP 405": { + err: newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID), + expectedHTTPStatus: http.StatusMethodNotAllowed, + expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg), + }, + "a DoNotLogError of an ingesterPushError with METHOD_NOT_ALLOWED cause gets translated into an HTTP 405": { + err: middleware.DoNotLogError{Err: newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID)}, + expectedHTTPStatus: http.StatusMethodNotAllowed, + expectedErrorMsg: fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg), + }, "an ingesterPushError with TSDB_UNAVAILABLE cause gets translated into an HTTP 503": { err: newIngesterPushError(createStatusWithDetails(t, codes.Internal, originalMsg, mimirpb.TSDB_UNAVAILABLE), ingesterID), expectedHTTPStatus: http.StatusServiceUnavailable, diff --git a/pkg/ruler/compat_test.go b/pkg/ruler/compat_test.go index 5de40bc2386..c51bd33a41c 100644 --- a/pkg/ruler/compat_test.go +++ b/pkg/ruler/compat_test.go @@ -238,6 +238,11 @@ func TestPusherErrors(t *testing.T) { expectedWrites: 1, expectedFailures: 0, }, + "a METHOD_NOT_ALLOWED push error is reported as failure": { + returnedError: mustStatusWithDetails(codes.Unimplemented, mimirpb.METHOD_NOT_ALLOWED).Err(), + expectedWrites: 1, + expectedFailures: 1, + }, "a TSDB_UNAVAILABLE push error is reported as failure": { returnedError: mustStatusWithDetails(codes.FailedPrecondition, mimirpb.TSDB_UNAVAILABLE).Err(), expectedWrites: 1,