Skip to content

Commit

Permalink
fix: Handle invalid content type errors first (#243)
Browse files Browse the repository at this point in the history 
## Which problem is this PR solving?
Updates the error handling logic to check if the error is from an
invalid content type first. This is necessary to ensure we can
successfully create a valid OTLP response. This needs to be first
because it determines how we present any errors back (eg invalid API
ket).

This PR also updates WriteOtlpHttpFailureResponse to check if the error
is an invalid content type, and write a text/pain response if true. This
removes the need for consumers of this function to have to do it
themselves.

## Short description of the changes
- Update each signal's validate headers function to check content type
first
- Update `WriteOtlpHttpResponse` to check if the content type is valid,
if not writes a HTTP status code 415 text/plain error response instead
- Update tests

---------

Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
  • Loading branch information
@MikeGoldsmith @robbkidd
MikeGoldsmith and robbkidd authored Feb 29, 2024
1 parent 52ecefd commit dbe549c
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 54 deletions.
34 changes: 19 additions & 15 deletions otlp/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,40 +132,40 @@ func (ri RequestInfo) hasLegacyKey() bool {

// ValidateTracesHeaders validates required headers/metadata for a trace OTLP request
func (ri *RequestInfo) ValidateTracesHeaders() error {
if !IsContentTypeSupported(ri.ContentType) {
return ErrInvalidContentType
}
if len(ri.ApiKey) == 0 {
return ErrMissingAPIKeyHeader
}
if ri.hasLegacyKey() && len(ri.Dataset) == 0 {
return ErrMissingDatasetHeader
}
if !IsContentTypeSupported(ri.ContentType) {
return ErrInvalidContentType
}
return nil // no error, headers passed all the validations
}

// ValidateMetricsHeaders validates required headers/metadata for a metric OTLP request
func (ri *RequestInfo) ValidateMetricsHeaders() error {
if !IsContentTypeSupported(ri.ContentType) {
return ErrInvalidContentType
}
if len(ri.ApiKey) == 0 {
return ErrMissingAPIKeyHeader
}
if ri.hasLegacyKey() && len(ri.Dataset) == 0 {
return ErrMissingDatasetHeader
}
if !IsContentTypeSupported(ri.ContentType) {
return ErrInvalidContentType
}
return nil // no error, headers passed all the validations
}

// ValidateLogsHeaders validates required headers/metadata for a logs OTLP request
func (ri *RequestInfo) ValidateLogsHeaders() error {
if len(ri.ApiKey) == 0 {
return ErrMissingAPIKeyHeader
}
if !IsContentTypeSupported(ri.ContentType) {
return ErrInvalidContentType
}
if len(ri.ApiKey) == 0 {
return ErrMissingAPIKeyHeader
}
return nil
}

Expand Down Expand Up @@ -224,24 +224,28 @@ func WriteOtlpHttpLogSuccessResponse(w http.ResponseWriter, r *http.Request) err
// WriteOtlpHttpResponse writes a compliant OTLP HTTP response to the given http.ResponseWriter
// based on the provided `contentType`. If an error occurs while marshalling to either json or proto it is returned
// before the http.ResponseWriter is updated. If an error occurs while writing to the http.ResponseWriter it is ignored.
// If an invalid content type is provided, a 415 Unsupported Media Type via text/plain is returned.
func WriteOtlpHttpResponse(w http.ResponseWriter, r *http.Request, statusCode int, m proto.Message) error {
if r == nil {
return fmt.Errorf("nil Request")
}

contentType := r.Header.Get("Content-Type")
var body []byte
var err error
var serializationError error
switch contentType {
case "application/json":
body, err = protojson.Marshal(m)
body, serializationError = protojson.Marshal(m)
case "application/x-protobuf", "application/protobuf":
body, err = proto.Marshal(m)
body, serializationError = proto.Marshal(m)
default:
return ErrInvalidContentType
// If the content type is not supported, return a 415 Unsupported Media Type via text/plain
body = []byte(ErrInvalidContentType.Message)
contentType = "text/plain"
statusCode = ErrInvalidContentType.HTTPStatusCode
}
if err != nil {
return err
if serializationError != nil {
return serializationError
}

// At this point we're committed
Expand Down
78 changes: 39 additions & 39 deletions otlp/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ func TestValidateTracesHeaders(t *testing.T) {
contentType string
err error
}{
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "classic/no dataset", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "", contentType: "", err: ErrMissingDatasetHeader},
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
{name: "classic/no dataset", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "", contentType: "application/protobuf", err: ErrMissingDatasetHeader},
{name: "classic/dataset present", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "dataset", contentType: "application/protobuf", err: nil},
{name: "E&S/no dataset", apikey: "abc123DEF456ghi789jklm", dataset: "", contentType: "application/protobuf", err: nil},
{name: "E&S/dataset present", apikey: "abc123DEF456ghi789jklm", dataset: "dataset", contentType: "application/protobuf", err: nil},
Expand Down Expand Up @@ -157,10 +157,10 @@ func TestValidateMetricsHeaders(t *testing.T) {
contentType string
err error
}{
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
// classic environments need to tell us which dataset to put metrics in
{name: "classic/no dataset", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "", contentType: "", err: ErrMissingDatasetHeader},
{name: "classic/no dataset", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "", contentType: "application/protobuf", err: ErrMissingDatasetHeader},
{name: "classic/dataset present", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "dataset", contentType: "application/protobuf", err: nil},
// dataset header not required for E&S, there's a fallback
{name: "E&S/no dataset", apikey: "abc123DEF456ghi789jklm", dataset: "", contentType: "application/protobuf", err: nil},
Expand Down Expand Up @@ -196,8 +196,8 @@ func TestValidateLogsHeaders(t *testing.T) {
contentType string
err error
}{
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "", err: ErrMissingAPIKeyHeader},
{name: "no key, no dataset", apikey: "", dataset: "", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
{name: "no key, dataset present", apikey: "", dataset: "dataset", contentType: "application/protobuf", err: ErrMissingAPIKeyHeader},
// logs will use dataset header if present, but log ingest will also use service.name in the data
// and we will have a sensible default if neither are present, so a missing dataset header is not an error here
{name: "classic/no dataset", apikey: "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", dataset: "", contentType: "application/protobuf", err: nil},
Expand Down Expand Up @@ -426,9 +426,8 @@ func Test_limitedWriter(t *testing.T) {

func Test_WriteOtlpHttpFailureResponse(t *testing.T) {
tests := []struct {
contentType string
err OTLPError
expectedError error
contentType string
err OTLPError
}{
{
contentType: "application/x-protobuf",
Expand All @@ -454,10 +453,9 @@ func Test_WriteOtlpHttpFailureResponse(t *testing.T) {
{
contentType: "nonsense",
err: OTLPError{
HTTPStatusCode: http.StatusBadRequest,
Message: "test",
HTTPStatusCode: ErrInvalidContentType.HTTPStatusCode,
Message: ErrInvalidContentType.Message,
},
expectedError: ErrInvalidContentType,
},
}
for _, tt := range tests {
Expand All @@ -467,11 +465,9 @@ func Test_WriteOtlpHttpFailureResponse(t *testing.T) {
r.Header.Set("Content-Type", tt.contentType)

err := WriteOtlpHttpFailureResponse(w, r, tt.err)
if tt.expectedError != nil {
assert.Equal(t, tt.expectedError, err)
} else {
assert.NoError(t, err)
assert.NoError(t, err)

if IsContentTypeSupported(tt.contentType) {
assert.Equal(t, tt.contentType, w.Header().Get("Content-Type"))
assert.Equal(t, tt.err.HTTPStatusCode, w.Code)

Expand All @@ -486,6 +482,10 @@ func Test_WriteOtlpHttpFailureResponse(t *testing.T) {
assert.NoError(t, err)
}
assert.Equal(t, tt.err.Message, result.Message)
} else {
assert.Equal(t, "text/plain", w.Header().Get("Content-Type"))
assert.Equal(t, ErrInvalidContentType.HTTPStatusCode, w.Code)
assert.Equal(t, ErrInvalidContentType.Message, w.Body.String())
}
})
}
Expand Down Expand Up @@ -553,8 +553,7 @@ func Test_BytesToTraceID(t *testing.T) {

func Test_WriteOtlpHttpTraceSuccessResponse(t *testing.T) {
tests := []struct {
contentType string
expectedError error
contentType string
}{
{
contentType: "application/x-protobuf",
Expand All @@ -566,8 +565,7 @@ func Test_WriteOtlpHttpTraceSuccessResponse(t *testing.T) {
contentType: "application/json",
},
{
contentType: "nonsense",
expectedError: ErrInvalidContentType,
contentType: "nonsense",
},
}
for _, tt := range tests {
Expand All @@ -577,9 +575,7 @@ func Test_WriteOtlpHttpTraceSuccessResponse(t *testing.T) {
r.Header.Set("Content-Type", tt.contentType)

err := WriteOtlpHttpTraceSuccessResponse(w, r)
if tt.expectedError != nil {
assert.Equal(t, tt.expectedError, err)
} else {
if IsContentTypeSupported(tt.contentType) {
assert.NoError(t, err)

assert.Equal(t, tt.contentType, w.Header().Get("Content-Type"))
Expand All @@ -596,15 +592,18 @@ func Test_WriteOtlpHttpTraceSuccessResponse(t *testing.T) {
assert.NoError(t, err)
}
assert.Nil(t, result.GetPartialSuccess())
} else {
assert.Equal(t, "text/plain", w.Header().Get("Content-Type"))
assert.Equal(t, ErrInvalidContentType.HTTPStatusCode, w.Code)
assert.Equal(t, ErrInvalidContentType.Message, w.Body.String())
}
})
}
}

func Test_WriteOtlpHttpMetricSuccessResponse(t *testing.T) {
tests := []struct {
contentType string
expectedError error
contentType string
}{
{
contentType: "application/x-protobuf",
Expand All @@ -616,8 +615,7 @@ func Test_WriteOtlpHttpMetricSuccessResponse(t *testing.T) {
contentType: "application/json",
},
{
contentType: "nonsense",
expectedError: ErrInvalidContentType,
contentType: "nonsense",
},
}
for _, tt := range tests {
Expand All @@ -627,9 +625,7 @@ func Test_WriteOtlpHttpMetricSuccessResponse(t *testing.T) {
r.Header.Set("Content-Type", tt.contentType)

err := WriteOtlpHttpMetricSuccessResponse(w, r)
if tt.expectedError != nil {
assert.Equal(t, tt.expectedError, err)
} else {
if IsContentTypeSupported(tt.contentType) {
assert.NoError(t, err)

assert.Equal(t, tt.contentType, w.Header().Get("Content-Type"))
Expand All @@ -646,15 +642,18 @@ func Test_WriteOtlpHttpMetricSuccessResponse(t *testing.T) {
assert.NoError(t, err)
}
assert.Nil(t, result.GetPartialSuccess())
} else {
assert.Equal(t, "text/plain", w.Header().Get("Content-Type"))
assert.Equal(t, ErrInvalidContentType.HTTPStatusCode, w.Code)
assert.Equal(t, ErrInvalidContentType.Message, w.Body.String())
}
})
}
}

func Test_WriteOtlpHttpLogSuccessResponse(t *testing.T) {
tests := []struct {
contentType string
expectedError error
contentType string
}{
{
contentType: "application/x-protobuf",
Expand All @@ -666,8 +665,7 @@ func Test_WriteOtlpHttpLogSuccessResponse(t *testing.T) {
contentType: "application/json",
},
{
contentType: "nonsense",
expectedError: ErrInvalidContentType,
contentType: "nonsense",
},
}
for _, tt := range tests {
Expand All @@ -677,9 +675,7 @@ func Test_WriteOtlpHttpLogSuccessResponse(t *testing.T) {
r.Header.Set("Content-Type", tt.contentType)

err := WriteOtlpHttpLogSuccessResponse(w, r)
if tt.expectedError != nil {
assert.Equal(t, tt.expectedError, err)
} else {
if IsContentTypeSupported(tt.contentType) {
assert.NoError(t, err)

assert.Equal(t, tt.contentType, w.Header().Get("Content-Type"))
Expand All @@ -696,6 +692,10 @@ func Test_WriteOtlpHttpLogSuccessResponse(t *testing.T) {
assert.NoError(t, err)
}
assert.Nil(t, result.GetPartialSuccess())
} else {
assert.Equal(t, "text/plain", w.Header().Get("Content-Type"))
assert.Equal(t, ErrInvalidContentType.HTTPStatusCode, w.Code)
assert.Equal(t, ErrInvalidContentType.Message, w.Body.String())
}
})
}
Expand Down

0 comments on commit dbe549c

Please sign in to comment.