From 6f5aef97d94dd267d944d4e973757a2e2b9bf0b3 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 28 Feb 2024 12:40:26 -0600 Subject: [PATCH] fix: Fix OTLP http responses (#1010) ## Which problem is this PR solving? Refinery was not returning the correct OTLP payload and not always using the correct `Content-Type` in the response. - closes https://github.com/honeycombio/refinery/issues/1008 ## Short description of the changes This PR attempts to fix the OTLP response while leaving other protocols unaffected. - Creates a new error handler method for the OTLP endpoint - Ensures a `ExportTraceServiceResponse` object is returned for success and a grpc `Status` object is used for failure. - Changes the "bad API key" response code from 400 to 401. --------- Co-authored-by: Robb Kidd --- app/app_test.go | 2 +- route/errors.go | 23 +++++++++++++++++++++++ route/otlp_trace.go | 14 ++++++++------ route/otlp_trace_test.go | 4 ++-- route/route.go | 1 - 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/app/app_test.go b/app/app_test.go index 1ad2065088..c6cd558dfc 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -317,7 +317,7 @@ func TestAppIntegrationWithUnauthorizedKey(t *testing.T) { resp, err := http.DefaultTransport.RoundTrip(req) assert.NoError(t, err) - assert.Equal(t, 400, resp.StatusCode) + assert.Equal(t, 401, resp.StatusCode) data, err := io.ReadAll(resp.Body) resp.Body.Close() assert.NoError(t, err) diff --git a/route/errors.go b/route/errors.go index c901f8be32..ba3ed5f964 100644 --- a/route/errors.go +++ b/route/errors.go @@ -2,6 +2,7 @@ package route import ( "fmt" + "io" "net/http" "runtime/debug" @@ -73,3 +74,25 @@ func (r *Router) handlerReturnWithError(w http.ResponseWriter, he handlerError, w.Write(jsonErrMsg) } + +func (r *Router) handleOTLPFailureResponse(w http.ResponseWriter, req *http.Request, otlpErr husky.OTLPError) { + r.Logger.Error().Logf(otlpErr.Error()) + + if otlpErr != husky.ErrInvalidContentType { + err := husky.WriteOtlpHttpFailureResponse(w, req, otlpErr) + if err != nil { + // If we made it here we had a problem writing an OTLP HTTP response + resp := fmt.Sprintf("failed to write otlp http response, %v", err.Error()) + r.Logger.Error().Logf(resp) + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, resp) + } + } else { + // Since we have an invalid content type for an OTLP HTTP response we choose to use to text/plain + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(otlpErr.HTTPStatusCode) + _, _ = io.WriteString(w, otlpErr.Message) + } + +} diff --git a/route/otlp_trace.go b/route/otlp_trace.go index dc07fdcb1c..6f743006f8 100644 --- a/route/otlp_trace.go +++ b/route/otlp_trace.go @@ -16,29 +16,31 @@ func (r *Router) postOTLP(w http.ResponseWriter, req *http.Request) { ri := huskyotlp.GetRequestInfoFromHttpHeaders(req.Header) if !r.Config.IsAPIKeyValid(ri.ApiKey) { - err := fmt.Errorf("api key %s not found in list of authorized keys", ri.ApiKey) - r.handlerReturnWithError(w, ErrAuthNeeded, err) + r.handleOTLPFailureResponse(w, req, huskyotlp.OTLPError{Message: fmt.Sprintf("api key %s not found in list of authorized keys", ri.ApiKey), HTTPStatusCode: http.StatusUnauthorized}) return } if err := ri.ValidateTracesHeaders(); err != nil { if errors.Is(err, huskyotlp.ErrInvalidContentType) { - r.handlerReturnWithError(w, ErrInvalidContentType, err) + r.handleOTLPFailureResponse(w, req, huskyotlp.ErrInvalidContentType) } else { - r.handlerReturnWithError(w, ErrAuthNeeded, err) + r.handleOTLPFailureResponse(w, req, huskyotlp.OTLPError{Message: err.Error(), HTTPStatusCode: http.StatusUnauthorized}) } return } result, err := huskyotlp.TranslateTraceRequestFromReader(req.Body, ri) if err != nil { - r.handlerReturnWithError(w, ErrUpstreamFailed, err) + r.handleOTLPFailureResponse(w, req, huskyotlp.OTLPError{Message: err.Error(), HTTPStatusCode: http.StatusInternalServerError}) return } if err := processTraceRequest(req.Context(), r, result.Batches, ri.ApiKey); err != nil { - r.handlerReturnWithError(w, ErrUpstreamFailed, err) + r.handleOTLPFailureResponse(w, req, huskyotlp.OTLPError{Message: err.Error(), HTTPStatusCode: http.StatusInternalServerError}) + return } + + _ = huskyotlp.WriteOtlpHttpTraceSuccessResponse(w, req) } type TraceServer struct { diff --git a/route/otlp_trace_test.go b/route/otlp_trace_test.go index efdc672798..10c9f8e6ee 100644 --- a/route/otlp_trace_test.go +++ b/route/otlp_trace_test.go @@ -317,7 +317,7 @@ func TestOTLPHandler(t *testing.T) { w := httptest.NewRecorder() router.postOTLP(w, request) assert.Equal(t, w.Code, http.StatusOK) - assert.Equal(t, "", w.Body.String()) + assert.Equal(t, "{}", w.Body.String()) assert.Equal(t, 2, len(mockTransmission.Events)) mockTransmission.Flush() @@ -409,7 +409,7 @@ func TestOTLPHandler(t *testing.T) { w := httptest.NewRecorder() router.postOTLP(w, request) - assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Equal(t, http.StatusUnauthorized, w.Code) assert.Contains(t, w.Body.String(), "not found in list of authorized keys") assert.Equal(t, 0, len(mockTransmission.Events)) diff --git a/route/route.go b/route/route.go index b608445039..58ca7c80e1 100644 --- a/route/route.go +++ b/route/route.go @@ -904,7 +904,6 @@ func (r *Router) Watch(req *grpc_health_v1.HealthCheckRequest, server grpc_healt func (r *Router) AddOTLPMuxxer(muxxer *mux.Router) { // require an auth header for OTLP requests otlpMuxxer := muxxer.PathPrefix("/v1/").Methods("POST").Subrouter() - otlpMuxxer.Use(r.apiKeyChecker) // handle OTLP trace requests otlpMuxxer.HandleFunc("/traces", r.postOTLP).Name("otlp")