diff --git a/changes/issue-15197-dont-send-500-debug-error b/changes/issue-15197-dont-send-500-debug-error new file mode 100644 index 0000000000..16fc6cd8f0 --- /dev/null +++ b/changes/issue-15197-dont-send-500-debug-error @@ -0,0 +1 @@ +- dont always send a 500 from the /debug/errors endpoint diff --git a/server/fleet/service.go b/server/fleet/service.go index 73c0c3ad85..dea06339ee 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -84,8 +84,8 @@ type Service interface { // to fleetd (formerly orbit). GetOrbitConfig(ctx context.Context) (OrbitConfig, error) - // ReceiveFleetdError handles an erorr report from a `fleetd` component - ReceiveFleetdError(ctx context.Context, errData FleetdError) error + // LogFleetdError logs an error report from a `fleetd` component + LogFleetdError(ctx context.Context, errData FleetdError) error // SetOrUpdateDeviceAuthToken creates or updates a device auth token for the given host. SetOrUpdateDeviceAuthToken(ctx context.Context, authToken string) error diff --git a/server/service/device_client.go b/server/service/device_client.go index 64e3c36c58..c3016d539f 100644 --- a/server/service/device_client.go +++ b/server/service/device_client.go @@ -225,15 +225,7 @@ func (dc *DeviceClient) ReportError(token string, fleetdErr fleet.FleetdError) e return retry.Do( func() error { err := dc.request(verb, path, token, "", req, nil) - scerr, ok := err.(*statusCodeErr) - - // as backwards as this seems, this endpoint returns a - // `500` status code when we post an error (it might - // return `4xx` errors if the request is malformed or - // unauthenticated) - // - // see https://github.com/fleetdm/fleet/issues/13238#issuecomment-1671769460 for more details - if !ok || scerr.code != http.StatusInternalServerError { + if err != nil { return err } diff --git a/server/service/devices.go b/server/service/devices.go index 3f01318af3..00bf3e7021 100644 --- a/server/service/devices.go +++ b/server/service/devices.go @@ -434,29 +434,28 @@ type fleetdErrorResponse struct{} func (r fleetdErrorResponse) error() error { return nil } -// for now, this endpoint must always return a 500 status code, this -// way errors are picked up and reported by any monitoring tool that -// looks for 5xx errors. -// -// since the handler is returning an error, this is redundant, but I'm adding -// it as a safeguard. -// -// See: https://github.com/fleetdm/fleet/issues/13238#issuecomment-1671769460 -func (r fleetdErrorResponse) Status() int { return http.StatusInternalServerError } - func fleetdError(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) { req := request.(*fleetdErrorRequest) - err := svc.ReceiveFleetdError(ctx, req.FleetdError) - // return the error as the second parameter to get better logs in the server. - return fleetdErrorResponse{}, err + err := svc.LogFleetdError(ctx, req.FleetdError) + if err != nil { + return nil, err + } + return fleetdErrorResponse{}, nil } -func (svc *Service) ReceiveFleetdError(ctx context.Context, fleetdError fleet.FleetdError) error { +func (svc *Service) LogFleetdError(ctx context.Context, fleetdError fleet.FleetdError) error { if !svc.authz.IsAuthenticatedWith(ctx, authz.AuthnDeviceToken) { return ctxerr.Wrap(ctx, fleet.NewPermissionError("forbidden: only device-authenticated hosts can access this endpoint")) } - return ctxerr.WrapWithData(ctx, fleetdError, "receive fleetd error", fleetdError.ToMap()) + svc.logger.Log( + "msg", + "fleetd error", + "error", + ctxerr.WrapWithData(ctx, fleetdError, "receive fleetd error", fleetdError.ToMap()), + ) + + return nil } //////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/integration_desktop_test.go b/server/service/integration_desktop_test.go index 6a540eda51..cd82faa191 100644 --- a/server/service/integration_desktop_test.go +++ b/server/service/integration_desktop_test.go @@ -304,7 +304,7 @@ func (s *integrationTestSuite) TestErrorReporting() { res = s.DoRawNoAuth("POST", "/api/latest/fleet/device/"+token+"/debug/errors", jsonData, http.StatusBadRequest) res.Body.Close() - res = s.DoRawNoAuth("POST", "/api/latest/fleet/device/"+token+"/debug/errors", []byte("{}"), http.StatusInternalServerError) + res = s.DoRawNoAuth("POST", "/api/latest/fleet/device/"+token+"/debug/errors", []byte("{}"), http.StatusOK) res.Body.Close() testTime, err := time.Parse(time.RFC3339, "1969-06-19T21:44:05Z") @@ -318,6 +318,6 @@ func (s *integrationTestSuite) TestErrorReporting() { } errBytes, err := json.Marshal(ferr) require.NoError(t, err) - res = s.DoRawNoAuth("POST", "/api/latest/fleet/device/"+token+"/debug/errors", errBytes, http.StatusInternalServerError) + res = s.DoRawNoAuth("POST", "/api/latest/fleet/device/"+token+"/debug/errors", errBytes, http.StatusOK) res.Body.Close() }