distributor, ruler: expect METHOD_NOT_ALLOWED from pusher (#7645)

* distributor, ruler: expect METHOD_NOT_ALLOWED from pusher Signed-off-by: Vladimir Varankin <[email protected]> * ingester: map METHOD_NOT_ALLOWED to status error Signed-off-by: Vladimir Varankin <[email protected]> --------- Signed-off-by: Vladimir Varankin <[email protected]>
grafana · Mar 18, 2024 · c0038b1 · c0038b1
1 parent 83dc978
commit c0038b1
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 1 deletion.
diff --git 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())

diff --git 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 not a client error": {
+			err:             ingesterPushError{cause: mimirpb.METHOD_NOT_ALLOWED},
+			expectedOutcome: false,
+		},
+		"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
@@ -228,6 +228,9 @@ 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 a 501 (and not 405) to explicitly signal a misconfiguration and to possibly track that amongst other 5xx errors.
+			return http.StatusNotImplemented
 		}
 	}
 

diff --git 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 501": {
+			err:                newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID),
+			expectedHTTPStatus: http.StatusNotImplemented,
+			expectedErrorMsg:   fmt.Sprintf("%s %s: %s", failedPushingToIngesterMessage, ingesterID, originalMsg),
+		},
+		"a DoNotLogError of an ingesterPushError with METHOD_NOT_ALLOWED cause gets translated into an HTTP 501": {
+			err:                middleware.DoNotLogError{Err: newIngesterPushError(createStatusWithDetails(t, codes.Unimplemented, originalMsg, mimirpb.METHOD_NOT_ALLOWED), ingesterID)},
+			expectedHTTPStatus: http.StatusNotImplemented,
+			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/ingester/errors.go b/pkg/ingester/errors.go
@@ -584,6 +584,8 @@ func mapPushErrorToErrorWithStatus(err error) error {
 			wrappedErr = middleware.DoNotLogError{Err: err}
 		case mimirpb.TSDB_UNAVAILABLE:
 			errCode = codes.Internal
+		case mimirpb.METHOD_NOT_ALLOWED:
+			errCode = codes.Unimplemented
 		}
 	}
 	return newErrorWithStatus(wrappedErr, errCode)
@@ -603,6 +605,8 @@ func mapPushErrorToErrorWithHTTPOrGRPCStatus(err error) error {
 			return newErrorWithStatus(middleware.DoNotLogError{Err: err}, codes.Unavailable)
 		case mimirpb.TSDB_UNAVAILABLE:
 			return newErrorWithHTTPStatus(err, http.StatusServiceUnavailable)
+		case mimirpb.METHOD_NOT_ALLOWED:
+			return newErrorWithStatus(err, codes.Unimplemented)
 		}
 	}
 	return err
@@ -620,6 +624,8 @@ func mapReadErrorToErrorWithStatus(err error) error {
 			errCode = codes.ResourceExhausted
 		case mimirpb.SERVICE_UNAVAILABLE:
 			errCode = codes.Unavailable
+		case mimirpb.METHOD_NOT_ALLOWED:
+			return newErrorWithStatus(err, codes.Unimplemented)
 		}
 	}
 	return newErrorWithStatus(err, errCode)
@@ -637,6 +643,8 @@ func mapReadErrorToErrorWithHTTPOrGRPCStatus(err error) error {
 			return newErrorWithHTTPStatus(err, http.StatusServiceUnavailable)
 		case mimirpb.SERVICE_UNAVAILABLE:
 			return newErrorWithStatus(err, codes.Unavailable)
+		case mimirpb.METHOD_NOT_ALLOWED:
+			return newErrorWithStatus(err, codes.Unimplemented)
 		}
 	}
 	return err

diff --git a/pkg/ingester/errors_test.go b/pkg/ingester/errors_test.go
@@ -413,6 +413,18 @@ func TestMapPushErrorToErrorWithStatus(t *testing.T) {
 			expectedMessage: fmt.Sprintf("wrapped: %s", newUnavailableError(services.Stopping).Error()),
 			expectedDetails: &mimirpb.ErrorDetails{Cause: mimirpb.SERVICE_UNAVAILABLE},
 		},
+		"an ingesterPushGrpcDisabledError gets translated into an errorWithStatus Unimplemented error with details": {
+			err:             ingesterPushGrpcDisabledError{},
+			expectedCode:    codes.Unimplemented,
+			expectedMessage: ingesterPushGrpcDisabledMsg,
+			expectedDetails: &mimirpb.ErrorDetails{Cause: mimirpb.METHOD_NOT_ALLOWED},
+		},
+		"a wrapped ingesterPushGrpcDisabledError gets translated into an errorWithStatus Unimplemented error": {
+			err:             fmt.Errorf("wrapped: %w", ingesterPushGrpcDisabledError{}),
+			expectedCode:    codes.Unimplemented,
+			expectedMessage: fmt.Sprintf("wrapped: %s", ingesterPushGrpcDisabledMsg),
+			expectedDetails: &mimirpb.ErrorDetails{Cause: mimirpb.METHOD_NOT_ALLOWED},
+		},
 		"an instanceLimitReachedError gets translated into a non-loggable errorWithStatus Unavailable error with details": {
 			err:              newInstanceLimitReachedError("instance limit reached"),
 			expectedCode:     codes.Unavailable,
@@ -688,6 +700,20 @@ func TestMapPushErrorToErrorWithHTTPOrGRPCStatus(t *testing.T) {
 				http.StatusBadRequest,
 			),
 		},
+		"an ingesterPushGrpcDisabledError gets translated into an errorWithStatus Unimplemented error": {
+			err: ingesterPushGrpcDisabledError{},
+			expectedTranslation: newErrorWithStatus(
+				ingesterPushGrpcDisabledError{},
+				codes.Unimplemented,
+			),
+		},
+		"a wrapped ingesterPushGrpcDisabledError gets translated into an errorWithStatus Unimplemented error": {
+			err: fmt.Errorf("wrapped: %w", ingesterPushGrpcDisabledError{}),
+			expectedTranslation: newErrorWithStatus(
+				fmt.Errorf("wrapped: %w", ingesterPushGrpcDisabledError{}),
+				codes.Unimplemented,
+			),
+		},
 	}
 
 	for name, tc := range testCases {

diff --git 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,