return a clear error when http/protobuf server returns json (equinix-…

…labs#263)

* go mod tidy

* check for servers out of spec

In equinix-labs#262 it was reported that a vendor's OTLP server responded to an
http/protobuf trace upload with a JSON response. I checked with a vendor
contact and they confirmed this is not supposed to happen and means
their server is out of spec.

I might be convinced to just accept the JSON as long as it's a valid
ExportTraceServiceResponse but for now, add an error that informs users
clearly that it's the server's fault.

* make code a bit more panic-safe and report missing content-type header

* add tests, make code gooderer

Tests were failing on the new check, which is good, because they were
under-specified. They now set a Content-Type header to pass what should
pass. Added 2 tests for the new server compliance checks too.

* remove debug log

go.sum

@@ -91,25 +91,17 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
-github.com/stretchr/testify v1.8.3 h1:RP3t2pwF7cMEbC1dqtB6poj3niw/9gnV4Cjg5oW5gtY=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778/go.mod h1:2MuV+tbUrU1zIOPMxZ5EncGwgmMJsa+9ucAQZXxsObs=
 github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no=
 github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
-go.opentelemetry.io/otel v1.16.0 h1:Z7GVAX/UkAXPKsy94IU+i6thsQS4nb7LviLpnaNeW8s=
-go.opentelemetry.io/otel v1.16.0/go.mod h1:vl0h9NUa1D5s1nv3A5vZOYWn8av4K8Ml6JDeHrT/bx4=
 go.opentelemetry.io/otel v1.18.0 h1:TgVozPGZ01nHyDZxK5WGPFB9QexeTMXEH7+tIClWfzs=
 go.opentelemetry.io/otel v1.18.0/go.mod h1:9lWqYO0Db579XzVuCKFNPDl4s73Voa+zEck3wHaAYQI=
-go.opentelemetry.io/otel/metric v1.16.0 h1:RbrpwVG1Hfv85LgnZ7+txXioPDoh6EdbZHo26Q3hqOo=
-go.opentelemetry.io/otel/metric v1.16.0/go.mod h1:QE47cpOmkwipPiefDwo2wDzwJrlfxxNYodqc4xnGCo4=
 go.opentelemetry.io/otel/metric v1.18.0 h1:JwVzw94UYmbx3ej++CwLUQZxEODDj/pOuTCvzhtRrSQ=
 go.opentelemetry.io/otel/metric v1.18.0/go.mod h1:nNSpsVDjWGfb7chbRLUNW+PBNdcSTHD4Uu5pfFMOI0k=
-go.opentelemetry.io/otel/sdk v1.16.0 h1:Z1Ok1YsijYL0CSJpHt4cS3wDDh7p572grzNrBMiMWgE=
-go.opentelemetry.io/otel/sdk v1.16.0/go.mod h1:tMsIuKXuuIWPBAOrH+eHtvhTL+SntFtXF9QD68aP6p4=
 go.opentelemetry.io/otel/sdk v1.18.0 h1:e3bAB0wB3MljH38sHzpV/qWrOTCFrdZF2ct9F8rBkcY=
 go.opentelemetry.io/otel/sdk v1.18.0/go.mod h1:1RCygWV7plY2KmdskZEDDBs4tJeHG92MdHZIluiYs/M=
-go.opentelemetry.io/otel/trace v1.16.0 h1:8JRpaObFoW0pxuVPapkgH8UhHQj+bJW8jJsCZEu5MQs=
-go.opentelemetry.io/otel/trace v1.16.0/go.mod h1:Yt9vYq1SdNz3xdjZZK7wcXv1qv2pwLkqr2QVwea0ef0=
 go.opentelemetry.io/otel/trace v1.18.0 h1:NY+czwbHbmndxojTEKiSMHkG2ClNH2PwmcHrdo0JY10=
 go.opentelemetry.io/otel/trace v1.18.0/go.mod h1:T2+SGJGuYZY3bjj5rgh/hN7KIrlpWC5nS8Mjvzckz+0=
 go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I=
@@ -138,8 +130,6 @@ golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
-golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o=
 golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=

otlpclient/otlp_client_http.go

@@ -88,6 +88,14 @@ func (hc *HttpClient) UploadTraces(ctx context.Context, rsps []*tracepb.Resource
 // processHTTPStatus takes the http.Response and body, returning the same bool, error
 // as retryFunc. Mostly it's broken out so it can be unit tested.
 func processHTTPStatus(ctx context.Context, resp *http.Response, body []byte) (context.Context, bool, time.Duration, error) {
+	// #262 a vendor OTLP server is out of spec and returns JSON instead of protobuf
+	ctype := resp.Header.Get("Content-Type")
+	if ctype == "" {
+		return ctx, false, 0, fmt.Errorf("server is out of specification: Content-Type header is missing or mangled")
+	} else if ctype != "application/x-protobuf" {
+		return ctx, false, 0, fmt.Errorf("server is out of specification: expected content type application/x-protobuf but got %q", ctype)
+	}
+
 	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
 		// success & partial success
 		// spec says server MUST send 200 OK, we'll be generous and accept any 200

otlpclient/otlp_client_http_test.go

@@ -14,6 +14,9 @@ import (
 )
 
 func TestProcessHTTPStatus(t *testing.T) {
+	headers := http.Header{
+		"Content-Type": []string{"application/x-protobuf"},
+	}
 
 	for _, tc := range []struct {
 		resp      *http.Response
@@ -25,6 +28,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 200,
+				Header:     headers,
 			},
 			body:      etsrSuccessBody(),
 			keepgoing: false,
@@ -34,6 +38,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 200,
+				Header:     headers,
 			},
 			body:      etsrPartialSuccessBody(),
 			keepgoing: false,
@@ -43,6 +48,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 500,
+				Header:     headers,
 			},
 			body:      errorBody(500, "xyz"),
 			keepgoing: false,
@@ -52,6 +58,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 429,
+				Header:     headers,
 			},
 			body:      errorBody(429, "xyz"),
 			keepgoing: true,
@@ -60,6 +67,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 502,
+				Header:     headers,
 			},
 			body:      errorBody(502, "xyz"),
 			keepgoing: true,
@@ -68,6 +76,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 503,
+				Header:     headers,
 			},
 			body:      errorBody(503, "xyz"),
 			keepgoing: true,
@@ -76,6 +85,7 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 504,
+				Header:     headers,
 			},
 			body:      errorBody(504, "xyz"),
 			keepgoing: true,
@@ -85,18 +95,39 @@ func TestProcessHTTPStatus(t *testing.T) {
 		{
 			resp: &http.Response{
 				StatusCode: 301,
+				Header:     headers,
 			},
 			body:      errorBody(301, "xyz"),
 			keepgoing: false,
 			err:       fmt.Errorf("server returned unsupported code 301"),
 		},
 		// shouldn't happen in the real world...
 		{
-			resp:      &http.Response{},
+			resp:      &http.Response{Header: headers},
 			body:      []byte(""),
 			keepgoing: false,
 			err:       fmt.Errorf("BUG: fell through error checking with status code 0"),
 		},
+		// return a decent error for out-of-spec servers that return JSON after a protobuf payload
+		{
+			resp: &http.Response{
+				StatusCode: 200,
+				Header:     http.Header{"Content-Type": []string{"application/json"}},
+			},
+			body:      []byte(`{"some": "json"}`),
+			keepgoing: false,
+			err:       fmt.Errorf(`server is out of specification: expected content type application/x-protobuf but got "application/json"`),
+		},
+		// spec requires headers so report that as a server problem too
+		{
+			resp: &http.Response{
+				StatusCode: 200,
+				// no headers!
+			},
+			body:      []byte(""),
+			keepgoing: false,
+			err:       fmt.Errorf("server is out of specification: Content-Type header is missing or mangled"),
+		},
 	} {
 		ctx := context.Background()
 		_, kg, _, err := processHTTPStatus(ctx, tc.resp, tc.body)