Osb responses to include the broker name if an error occurs (#323)

* Broker name is now included in bad http responses for better troubleshooting

* Add test which verifies broker name is in the response of osb requests

* When broker error description is empty add default description

* Remove unused type

* Change response content

* Fix not including broker response when json is valid

* Handle broker response is not a JSON object

* Fix test name

api/osb/osb_controller.go

@@ -27,6 +27,9 @@ import (
 	"net/url"
 	"regexp"
 
+	"github.com/tidwall/gjson"
+	"github.com/tidwall/sjson"
+
 	"github.com/sirupsen/logrus"
 
 	"github.com/Peripli/service-manager/pkg/log"
@@ -123,15 +126,38 @@ func (c *Controller) proxy(r *web.Request, logger *logrus.Entry, broker *types.S
 
 	proxy.ServeHTTP(recorder, modifiedRequest)
 
-	respBody, err := ioutil.ReadAll(recorder.Body)
+	brokerResponseBody, err := ioutil.ReadAll(recorder.Body)
 	if err != nil {
 		return nil, err
 	}
 
+	responseBody := brokerResponseBody
+
+	if !gjson.ValidBytes(brokerResponseBody) {
+		recorder.Header().Set("Content-Type", "application/json")
+		responseBody, err = sjson.SetBytes(nil, "description", fmt.Sprintf("Service broker %s responded with invalid JSON: %s", broker.Name, brokerResponseBody))
+		if err != nil {
+			return nil, err
+		}
+	} else if recorder.Code > 399 || recorder.Code < 100 {
+		recorder.Header().Set("Content-Type", "application/json")
+		description := gjson.GetBytes(brokerResponseBody, "description").String()
+		if description == "" {
+			description = string(brokerResponseBody)
+		}
+		if !gjson.ParseBytes(brokerResponseBody).IsObject() {
+			brokerResponseBody = nil
+		}
+		responseBody, err = sjson.SetBytes(brokerResponseBody, "description", fmt.Sprintf("Service broker %s failed with: %s", broker.Name, description))
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	resp := &web.Response{
 		StatusCode: recorder.Code,
 		Header:     recorder.Header(),
-		Body:       respBody,
+		Body:       responseBody,
 	}
 	return resp, nil
 }

test/osb_test/osb_test.go

@@ -17,6 +17,7 @@ package osb_test
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -32,6 +33,7 @@ import (
 	"testing"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 )
 
@@ -65,7 +67,7 @@ func TestOSB(t *testing.T) {
 
 func assertFailingBrokerError(req *httpexpect.Response) {
 	req.Status(http.StatusNotAcceptable).JSON().Object().
-		Value("description").String().Contains("Failing service broker error")
+		Value("description").String().Match("Service broker .* failed with: Failing service broker error")
 }
 
 func assertMissingBrokerError(req *httpexpect.Response) {
@@ -155,6 +157,7 @@ var _ = Describe("Service Manager OSB API", func() {
 
 		failingBrokerServer  *common.BrokerServer
 		failingBrokerID      string
+		failingBrokerName    string
 		smUrlToFailingBroker string
 
 		queryParameterVerificationServer   *common.BrokerServer
@@ -223,7 +226,9 @@ var _ = Describe("Service Manager OSB API", func() {
 		simpleBrokerCatalogID, _, brokerServerWithSimpleCatalog := ctx.RegisterBrokerWithCatalog(simpleCatalog)
 		smUrlToSimpleBrokerCatalogBroker = brokerServerWithSimpleCatalog.URL() + "/v1/osb/" + simpleBrokerCatalogID
 
-		failingBrokerID, _, failingBrokerServer = ctx.RegisterBroker()
+		var failingBrokerObject common.Object
+		failingBrokerID, failingBrokerObject, failingBrokerServer = ctx.RegisterBroker()
+		failingBrokerName = failingBrokerObject["name"].(string)
 		smUrlToFailingBroker = failingBrokerServer.URL() + "/v1/osb/" + failingBrokerID
 
 		UUID, err := uuid.NewV4()
@@ -728,6 +733,23 @@ var _ = Describe("Service Manager OSB API", func() {
 		})
 	})
 
+	DescribeTable("Malfunctioning broker",
+		func(statusCode int, responseBody, expectedDescriptionPattern string) {
+			failingBrokerServer.ServiceInstanceHandler = func(rw http.ResponseWriter, _ *http.Request) {
+				rw.Header().Set("Content-Type", "application/json")
+				rw.WriteHeader(statusCode)
+				rw.Write([]byte(responseBody))
+			}
+			expectedDescription := fmt.Sprintf(expectedDescriptionPattern, failingBrokerName)
+			ctx.SMWithBasic.PUT(smUrlToFailingBroker+"/v2/service_instances/12345").WithHeader("X-Broker-API-Version", "oidc_authn.13").
+				WithJSON(getDummyService()).Expect().Status(statusCode).JSON().Object().
+				Value("description").String().Equal(expectedDescription)
+		},
+		Entry("when broker response is not a valid json, should return an OSB compliant error", http.StatusCreated, "[not a json]", "Service broker %s responded with invalid JSON: [not a json]"),
+		Entry("when broker returns a valid json which is not an object, should return the broker's response", http.StatusBadRequest, "3", "Service broker %s failed with: 3"),
+		Entry("when broker returns error without description, should assing broker's response body as description", http.StatusBadRequest, `{"error": "ErrorType"}`, `Service broker %s failed with: {"error": "ErrorType"}`),
+		Entry("when broker response is JSON array, should return it in description", http.StatusBadRequest, `[1,2,3]`, `Service broker %s failed with: [1,2,3]`),
+	)
 })
 
 type prefixedBrokerHandler struct {