azure/WithErrorUnlessStatusCode: fixing a panic when the response is …

…a literal value `null` (#605) This commit fixes a panic within `WithErrorUnlessStatusCode` when the response returns a literal value of `null`: ``` :status: 202 cache-control: no-cache pragma: no-cache content-length: 4 content-type: application/json; charset=utf-8 expires: -1 location: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-la-210108080034968174/providers/Microsoft.OperationalInsights/clusters/acctest-LA-210108080034968174/operationresults/00000000-0000-0000-0000-000000000000?api-version=2020-08-01 azure-asyncoperation: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.OperationalInsights/locations/westeurope/operationStatuses/00000000-0000-0000-0000-000000000000?api-version=2015-11-01-preview x-ams-apiversion: WebAPI1.0 cachecontrol: no-cache x-ms-request-id: 00000000-0000-0000-0000-000000000000 strict-transport-security: max-age=31536000; includeSubDomains x-content-type-options: nosniff server: Microsoft-IIS/10.0 server: Microsoft-IIS/10.0 x-powered-by: ASP.NET x-powered-by: ASP.NET x-ms-ratelimit-remaining-subscription-writes: 1199 x-ms-correlation-request-id: 00000000-0000-0000-0000-000000000000 x-ms-routing-request-id: UKWEST:20210108T103620Z:00000000-0000-0000-0000-000000000000 date: Fri, 08 Jan 2021 10:36:19 GMT null ``` Where this panics when calling the nil `e.ServiceError` when accessing `e.ServiceError.Message`: ``` panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x7d1542] goroutine 1516 [running]: github.com/Azure/go-autorest/autorest/azure.WithErrorUnlessStatusCode.func1.1(0xc001a45170, 0x0, 0x0) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/azure/azure.go:308 +0x4c2 github.com/Azure/go-autorest/autorest.ResponderFunc.Respond(0xc00130b020, 0xc001a45170, 0x203000, 0xc0013ccd48) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:41 +0x30 github.com/Azure/go-autorest/autorest.ByUnmarshallingJSON.func1.1(0xc001a45170, 0xc0013cce30, 0x7c53cd) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:180 +0x63 github.com/Azure/go-autorest/autorest.ResponderFunc.Respond(0xc00130b1d0, 0xc001a45170, 0x5ad9080, 0xc00130b1d0) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:41 +0x30 github.com/Azure/go-autorest/autorest.ByClosing.func1.1(0xc001a45170, 0x5ad9080, 0xc001556380) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:129 +0x3d github.com/Azure/go-autorest/autorest.ResponderFunc.Respond(0xc001556380, 0xc001a45170, 0x3, 0x5ad9080) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:41 +0x30 github.com/Azure/go-autorest/autorest.Respond(0xc001a45170, 0xc0013ccf40, 0x3, 0x3, 0xc00012eb00, 0xc0013ccf50) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/go-autorest/autorest@v0.11.10/responder.go:78 +0x6b github.com/Azure/azure-sdk-for-go/services/operationalinsights/mgmt/2020-08-01/operationalinsights.ClustersClient.UpdateResponder(0x5ad42c0, 0xc000e14840, 0x5ad90a0, 0xc0028f39b0, 0xc0028f3a10, 0x0, 0xdf8475800, 0xd18c2e2800, 0x3, 0x6fc23ac00, ...) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/azure-sdk-for-go@v49.2.0+incompatible/services/operationalinsights/mgmt/2020-08-01/operationalinsights/clusters.go:632 +0x105 github.com/Azure/azure-sdk-for-go/services/operationalinsights/mgmt/2020-08-01/operationalinsights.ClustersClient.Update(0x5ad42c0, 0xc000e14840, 0x5ad90a0, 0xc0028f39b0, 0xc0028f3a10, 0x0, 0xdf8475800, 0xd18c2e2800, 0x3, 0x6fc23ac00, ...) /var/lib/teamcity/go/1.15.5/pkg/mod/github.com/!azure/azure-sdk-for-go@v49.2.0+incompatible/services/operationalinsights/mgmt/2020-08-01/operationalinsights/clusters.go:591 +0x8e5 ``` As such this commit checks to confirm if `e.ServiceError` is nil - then assigns an "unknown error" response with the response body if possible. Without this change, the tests fail: ``` $ go test -v ./autorest/azure -run=TestWithErrorUnlessStatusCode_ -count=1 === RUN TestWithErrorUnlessStatusCode_NotAnAzureError --- PASS: TestWithErrorUnlessStatusCode_NotAnAzureError (0.00s) === RUN TestWithErrorUnlessStatusCode_FoundAzureErrorWithoutDetails --- PASS: TestWithErrorUnlessStatusCode_FoundAzureErrorWithoutDetails (0.00s) === RUN TestWithErrorUnlessStatusCode_FoundAzureFullError --- PASS: TestWithErrorUnlessStatusCode_FoundAzureFullError (0.00s) === RUN TestWithErrorUnlessStatusCode_LiteralNullValueInResponse azure_test.go:348: azure: service error is not unmarshaled properly. expected="Code=\"Unknown\" Message=\"Unknown service error\" Details=[{\"HttpResponse.Body\":\"null\"}]" got=<nil> --- FAIL: TestWithErrorUnlessStatusCode_LiteralNullValueInResponse (0.00s) === RUN TestWithErrorUnlessStatusCode_NoAzureError --- PASS: TestWithErrorUnlessStatusCode_NoAzureError (0.00s) === RUN TestWithErrorUnlessStatusCode_UnwrappedError --- PASS: TestWithErrorUnlessStatusCode_UnwrappedError (0.00s) FAIL FAIL github.com/Azure/go-autorest/autorest/azure 0.012s FAIL ``` With this change, the tests pass: ``` $ go test -v ./autorest/azure -run=TestWithErrorUnlessStatusCode_ -count=1 === RUN TestWithErrorUnlessStatusCode_NotAnAzureError --- PASS: TestWithErrorUnlessStatusCode_NotAnAzureError (0.00s) === RUN TestWithErrorUnlessStatusCode_FoundAzureErrorWithoutDetails --- PASS: TestWithErrorUnlessStatusCode_FoundAzureErrorWithoutDetails (0.00s) === RUN TestWithErrorUnlessStatusCode_FoundAzureFullError --- PASS: TestWithErrorUnlessStatusCode_FoundAzureFullError (0.00s) === RUN TestWithErrorUnlessStatusCode_LiteralNullValueInResponse --- PASS: TestWithErrorUnlessStatusCode_LiteralNullValueInResponse (0.00s) === RUN TestWithErrorUnlessStatusCode_NoAzureError --- PASS: TestWithErrorUnlessStatusCode_NoAzureError (0.00s) === RUN TestWithErrorUnlessStatusCode_UnwrappedError --- PASS: TestWithErrorUnlessStatusCode_UnwrappedError (0.00s) PASS ok github.com/Azure/go-autorest/autorest/azure 0.011s ``` Co-authored-by: Joel Hendrix <jhendrix@microsoft.com>
Azure · Jan 11, 2021 · 9fc88b1 · 9fc88b1
1 parent 3fb5326
commit 9fc88b1
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 1 deletion.
diff --git a/autorest/azure/azure.go b/autorest/azure/azure.go
@@ -37,6 +37,9 @@ const (
 	// should be included in the response.
 	HeaderReturnClientID = "x-ms-return-client-request-id"
 
+	// HeaderContentType is the type of the content in the HTTP response.
+	HeaderContentType = "Content-Type"
+
 	// HeaderRequestID is the Azure extension header of the service generated request ID returned
 	// in the response.
 	HeaderRequestID = "x-ms-request-id"
@@ -309,8 +312,22 @@ func WithErrorUnlessStatusCode(codes ...int) autorest.RespondDecorator {
 					if err := decoder.Decode(&e.ServiceError); err != nil {
 						return err
 					}
+
+					// for example, should the API return the literal value `null` as the response
+					if e.ServiceError == nil {
+						e.ServiceError = &ServiceError{
+							Code:    "Unknown",
+							Message: "Unknown service error",
+							Details: []map[string]interface{}{
+								{
+									"HttpResponse.Body": b.String(),
+								},
+							},
+						}
+					}
 				}
-				if e.ServiceError.Message == "" {
+
+				if e.ServiceError != nil && e.ServiceError.Message == "" {
 					// if we're here it means the returned error wasn't OData v4 compliant.
 					// try to unmarshal the body in hopes of getting something.
 					rawBody := map[string]interface{}{}

diff --git a/autorest/azure/azure_test.go b/autorest/azure/azure_test.go
@@ -315,6 +315,40 @@ func TestWithErrorUnlessStatusCode_FoundAzureFullError(t *testing.T) {
 
 }
 
+func TestWithErrorUnlessStatusCode_LiteralNullValueInResponse(t *testing.T) {
+	// As found in the Log Analytics Cluster API
+	// API Bug: https://github.com/Azure/azure-rest-api-specs/issues/12331
+	j := `null`
+	r := mocks.NewResponseWithContent(j)
+	mocks.SetResponseHeader(r, HeaderContentType, "application/json; charset=utf-8")
+	r.Request = mocks.NewRequest()
+	r.StatusCode = http.StatusInternalServerError
+	r.Status = http.StatusText(r.StatusCode)
+
+	err := autorest.Respond(r,
+		WithErrorUnlessStatusCode(http.StatusOK),
+		autorest.ByClosing())
+	if err == nil {
+		t.Fatalf("azure: returned nil error for proper error response")
+	}
+	azErr, ok := err.(*RequestError)
+	if !ok {
+		t.Fatalf("azure: returned error is not azure.RequestError: %T", err)
+	}
+
+	expected := &ServiceError{
+		Code:    "Unknown",
+		Message: "Unknown service error",
+		Details: []map[string]interface{}{
+			{"HttpResponse.Body": "null"},
+		},
+	}
+
+	if !reflect.DeepEqual(expected, azErr.ServiceError) {
+		t.Fatalf("azure: service error is not unmarshaled properly. expected=%q\ngot=%q", expected, azErr.ServiceError)
+	}
+}
+
 func TestWithErrorUnlessStatusCode_NoAzureError(t *testing.T) {
 	j := `{
 		"Status":"NotFound"