Repository: honeycombio/refinery
Pull request #1019 (2024-02-29 11:21:31)

maint: Update Husky to handle invalid content type errors
## Which problem is this PR solving?
Updates Husky to v0.25.1 which includes fixes handling invalid content types for OTLP requests.

See the following PR for changes:
- https://github.com/honeycombio/husky/pull/243

**NOTE**: This PR is based on an unreleased version of husky. We'll wait for husky to be released before merging this fix.

## Short description of the changes
- Updating Husky to 0.25.1
- Updating the error handler to use husky to write a text/plain response for invalid content type errors
- Move validation of API keys from config after request headers check



Patch:
diff --git a/go.mod b/go.mod
index 018f2bf..e05d5c4 100644
--- a/go.mod
+++ b/go.mod
@@ -16,7 +16,7 @@ require (
 	github.com/grafana/pyroscope-go/godeltaprof v0.1.7
 	github.com/hashicorp/golang-lru/v2 v2.0.7
 	github.com/honeycombio/dynsampler-go v0.6.0
-	github.com/honeycombio/husky v0.24.0
+	github.com/honeycombio/husky v0.25.1
 	github.com/honeycombio/libhoney-go v1.20.0
 	github.com/jessevdk/go-flags v1.5.0
 	github.com/json-iterator/go v1.1.12
diff --git a/go.sum b/go.sum
index acd05bb..affeb48 100644
--- a/go.sum
+++ b/go.sum
@@ -66,8 +66,8 @@ github.com/honeycombio/cuckoofilter v0.0.0-20230630225016-cf48793fb7c1 h1:MXRxSU
 github.com/honeycombio/cuckoofilter v0.0.0-20230630225016-cf48793fb7c1/go.mod h1:VvArVnXCHeB+tpxMOLWvaoItNHWYEgOx8Lgs1JoS+cI=
 github.com/honeycombio/dynsampler-go v0.6.0 h1:fs4mrfeFGU5V+ClwpblFzbWqn4Apb+lKlE7Ja5zL22I=
 github.com/honeycombio/dynsampler-go v0.6.0/go.mod h1:pJqWFeoMN3syX74PEvlusieyGBbtIBjmTVjLc3thmK4=
-github.com/honeycombio/husky v0.24.0 h1:/kaua19ixUFVBLirnHMvffDdK0QSEy1pkG1ZTOWm/nk=
-github.com/honeycombio/husky v0.24.0/go.mod h1:7qtQ3fCfYpfhN57rE+yS1St1QpnWKUTGLtpQVuWa9bQ=
+github.com/honeycombio/husky v0.25.1 h1:5fjysSfjcXXYtBpsgIjkLd4oSf+xyvabgrvfOhoef6k=
+github.com/honeycombio/husky v0.25.1/go.mod h1:t0eBcATeP38Acksmr0Ayh2gaNUS4FKPOVuhoYNKY9ps=
 github.com/honeycombio/libhoney-go v1.20.0 h1:PL54R0P9vxIyb28H3twbLb+DCqQlJdMQM55VZg1abKA=
 github.com/honeycombio/libhoney-go v1.20.0/go.mod h1:RIaurCpfg5NDWSEV8t3QLcda9dUAiVNyWeHRAaSpN90=
 github.com/honeycombio/opentelemetry-proto-go/otlp v0.19.0-compat h1:fMpIzVAl5C260HisnRWV//vfckZIC4qvn656M3VLLOY=
diff --git a/route/errors.go b/route/errors.go
index ba3ed5f..f619d31 100644
--- a/route/errors.go
+++ b/route/errors.go
@@ -77,22 +77,12 @@ func (r *Router) handlerReturnWithError(w http.ResponseWriter, he handlerError,
 
 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
+	if err := husky.WriteOtlpHttpFailureResponse(w, req, otlpErr); 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(otlpErr.HTTPStatusCode)
-		_, _ = io.WriteString(w, otlpErr.Message)
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = io.WriteString(w, resp)
 	}
-
 }
diff --git a/route/otlp_trace.go b/route/otlp_trace.go
index 6f74300..e2ad7cf 100644
--- a/route/otlp_trace.go
+++ b/route/otlp_trace.go
@@ -15,11 +15,6 @@ import (
 func (r *Router) postOTLP(w http.ResponseWriter, req *http.Request) {
 	ri := huskyotlp.GetRequestInfoFromHttpHeaders(req.Header)
 
-	if !r.Config.IsAPIKeyValid(ri.ApiKey) {
-		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.handleOTLPFailureResponse(w, req, huskyotlp.ErrInvalidContentType)
@@ -29,6 +24,11 @@ func (r *Router) postOTLP(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	if !r.Config.IsAPIKeyValid(ri.ApiKey) {
+		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
+	}
+
 	result, err := huskyotlp.TranslateTraceRequestFromReader(req.Body, ri)
 	if err != nil {
 		r.handleOTLPFailureResponse(w, req, huskyotlp.OTLPError{Message: err.Error(), HTTPStatusCode: http.StatusInternalServerError})


Test patch:
diff --git a/route/otlp_trace_test.go b/route/otlp_trace_test.go
index 10c9f8e..e34d2b3 100644
--- a/route/otlp_trace_test.go
+++ b/route/otlp_trace_test.go
@@ -5,6 +5,7 @@ import (
 	"compress/gzip"
 	"context"
 	"encoding/hex"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -18,6 +19,7 @@ import (
 	"github.com/honeycombio/refinery/transmit"
 	"github.com/klauspost/compress/zstd"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	collectortrace "go.opentelemetry.io/proto/otlp/collector/trace/v1"
 	common "go.opentelemetry.io/proto/otlp/common/v1"
 	resource "go.opentelemetry.io/proto/otlp/resource/v1"
@@ -193,6 +195,58 @@ func TestOTLPHandler(t *testing.T) {
 		mockTransmission.Flush()
 	})
 
+	t.Run("invalid headers", func(t *testing.T) {
+		req := &collectortrace.ExportTraceServiceRequest{}
+		body, err := proto.Marshal(req)
+		assert.NoError(t, err)
+		anEmptyRequestBody := bytes.NewReader(body) // Empty because we're testing headers, not the body.
+
+		testCases := []struct {
+			name                        string
+			requestContentType          string
+			expectedResponseStatus      int
+			expectedResponseContentType string
+			expectedResponseBody        string
+		}{
+			{
+				name:                        "no key/bad content-type",
+				requestContentType:          "application/nope",
+				expectedResponseStatus:      http.StatusUnsupportedMediaType, // Prioritize erroring on bad content type over other header issues.
+				expectedResponseContentType: "text/plain",
+				expectedResponseBody:        huskyotlp.ErrInvalidContentType.Message,
+			},
+			{
+				name:                        "no key/json",
+				requestContentType:          "application/json",
+				expectedResponseStatus:      http.StatusUnauthorized,
+				expectedResponseContentType: "application/json",
+				expectedResponseBody:        fmt.Sprintf("{\"message\":\"%s\"}", huskyotlp.ErrMissingAPIKeyHeader.Message),
+			},
+			{
+				name:                        "no key/protobuf",
+				requestContentType:          "application/protobuf",
+				expectedResponseStatus:      http.StatusUnauthorized,
+				expectedResponseContentType: "application/protobuf",
+				expectedResponseBody:        fmt.Sprintf("\x12!%s", huskyotlp.ErrMissingAPIKeyHeader.Message),
+			},
+		}
+
+		for _, tC := range testCases {
+			t.Run(tC.name, func(t *testing.T) {
+				request, err := http.NewRequest("POST", "/v1/traces", anEmptyRequestBody)
+				require.NoError(t, err)
+				request.Header = http.Header{}
+				request.Header.Set("content-type", tC.requestContentType)
+				response := httptest.NewRecorder()
+				router.postOTLP(response, request)
+
+				assert.Equal(t, tC.expectedResponseStatus, response.Code)
+				assert.Equal(t, tC.expectedResponseContentType, response.Header().Get("content-type"))
+				assert.Equal(t, tC.expectedResponseBody, response.Body.String())
+			})
+		}
+	})
+
 	t.Run("can receive OTLP over HTTP/protobuf", func(t *testing.T) {
 		req := &collectortrace.ExportTraceServiceRequest{
 			ResourceSpans: []*trace.ResourceSpans{{