Clean up the OSB client (#888)

* sending the appropriate query string parameters in deprovision

… and not sending a request body

Fixes #883

* fixing fake broker server

it should no longer require a request body for a DELETE request

* checking the response's error before continuing

* handle query parameters properly

Author: krancour

pkg/brokerapi/openservicebroker/open_service_broker_client.go

@@ -35,15 +35,10 @@ import (
 )
 
 const (
-	catalogFormatString                    = "%s/v2/catalog"
-	serviceInstanceFormatString            = "%s/v2/service_instances/%s"
-	serviceInstanceAsyncFormatString       = "%s/v2/service_instances/%s?accepts_incomplete=true"
-	serviceInstanceDeleteFormatString      = "%s/v2/service_instances/%s?service_id=%s&plan_id=%s"
-	serviceInstanceDeleteAsyncFormatString = "%s/v2/service_instances/%s?service_id=%s&plan_id=%s&accepts_incomplete=true"
-	pollingFormatString                    = "%s/v2/service_instances/%s/last_operation?%s"
-	bindingFormatString                    = "%s/v2/service_instances/%s/service_bindings/%s"
-	bindingDeleteFormatString              = "%s/v2/service_instances/%s/service_bindings/%s?service_id=%s&plan_id=%s"
-	queryParamFormatString                 = "%s=%s"
+	catalogFormatString         = "%s/v2/catalog"
+	serviceInstanceFormatString = "%s/v2/service_instances/%s"
+	pollingFormatString         = "%s/v2/service_instances/%s/last_operation"
+	bindingFormatString         = "%s/v2/service_instances/%s/service_bindings/%s"
 
 	httpTimeoutSeconds     = 15
 	pollingIntervalSeconds = 1
@@ -118,7 +113,7 @@ func NewClient(name, url, username, password string) brokerapi.BrokerClient {
 func (c *openServiceBrokerClient) GetCatalog() (*brokerapi.Catalog, error) {
 	catalogURL := fmt.Sprintf(catalogFormatString, c.url)
 
-	req, err := c.newOSBRequest(http.MethodGet, catalogURL, nil)
+	req, err := c.newOSBRequest(http.MethodGet, catalogURL, nil, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -140,16 +135,17 @@ func (c *openServiceBrokerClient) GetCatalog() (*brokerapi.Catalog, error) {
 }
 
 func (c *openServiceBrokerClient) CreateServiceInstance(ID string, req *brokerapi.CreateServiceInstanceRequest) (*brokerapi.CreateServiceInstanceResponse, int, error) {
-	var serviceInstanceURL string
-
-	if req.AcceptsIncomplete {
-		serviceInstanceURL = fmt.Sprintf(serviceInstanceAsyncFormatString, c.url, ID)
-	} else {
-		serviceInstanceURL = fmt.Sprintf(serviceInstanceFormatString, c.url, ID)
-	}
-
-	// TODO: Handle the auth
-	resp, err := sendOSBRequest(c, http.MethodPut, serviceInstanceURL, req)
+	serviceInstanceURL := fmt.Sprintf(serviceInstanceFormatString, c.url, ID)
+
+	resp, err := sendOSBRequest(
+		c,
+		http.MethodPut,
+		serviceInstanceURL,
+		map[string]string{
+			"accepts_incomplete": fmt.Sprintf("%t", req.AcceptsIncomplete),
+		},
+		req,
+	)
 	if err != nil {
 		glog.Errorf("Error sending create service instance request to broker %q at %v: response: %v error: %#v", c.name, serviceInstanceURL, resp, err)
 		return nil, resp.StatusCode, errRequest{message: err.Error()}
@@ -185,16 +181,19 @@ func (c *openServiceBrokerClient) UpdateServiceInstance(ID string, req *brokerap
 }
 
 func (c *openServiceBrokerClient) DeleteServiceInstance(ID string, req *brokerapi.DeleteServiceInstanceRequest) (*brokerapi.DeleteServiceInstanceResponse, int, error) {
-	var serviceInstanceURL string
-
-	if req.AcceptsIncomplete {
-		serviceInstanceURL = fmt.Sprintf(serviceInstanceDeleteAsyncFormatString, c.url, ID, req.ServiceID, req.PlanID)
-	} else {
-		serviceInstanceURL = fmt.Sprintf(serviceInstanceDeleteFormatString, c.url, ID, req.ServiceID, req.PlanID)
-	}
-
-	// TODO: Handle the auth
-	resp, err := sendOSBRequest(c, http.MethodDelete, serviceInstanceURL, req)
+	serviceInstanceURL := fmt.Sprintf(serviceInstanceFormatString, c.url, ID)
+
+	resp, err := sendOSBRequest(
+		c,
+		http.MethodDelete,
+		serviceInstanceURL,
+		map[string]string{
+			"service_id":         req.ServiceID,
+			"plan_id":            req.PlanID,
+			"accepts_incomplete": fmt.Sprintf("%t", req.AcceptsIncomplete),
+		},
+		req,
+	)
 	if err != nil {
 		glog.Errorf("Error sending delete service instance request to broker %q at %v: response: %v error: %#v", c.name, serviceInstanceURL, resp, err)
 		return nil, resp.StatusCode, errRequest{message: err.Error()}
@@ -231,8 +230,12 @@ func (c *openServiceBrokerClient) CreateServiceBinding(instanceID, bindingID str
 
 	serviceBindingURL := fmt.Sprintf(bindingFormatString, c.url, instanceID, bindingID)
 
-	// TODO: Handle the auth
-	createHTTPReq, err := c.newOSBRequest("PUT", serviceBindingURL, bytes.NewReader(jsonBytes))
+	createHTTPReq, err := c.newOSBRequest(
+		http.MethodPut,
+		serviceBindingURL,
+		nil,
+		bytes.NewReader(jsonBytes),
+	)
 	if err != nil {
 		return nil, err
 	}
@@ -264,10 +267,17 @@ func (c *openServiceBrokerClient) CreateServiceBinding(instanceID, bindingID str
 }
 
 func (c *openServiceBrokerClient) DeleteServiceBinding(instanceID, bindingID, serviceID, planID string) error {
-	serviceBindingURL := fmt.Sprintf(bindingDeleteFormatString, c.url, instanceID, bindingID, serviceID, planID)
+	serviceBindingURL := fmt.Sprintf(bindingFormatString, c.url, instanceID, bindingID)
 
-	// TODO: Handle the auth
-	deleteHTTPReq, err := c.newOSBRequest("DELETE", serviceBindingURL, nil)
+	deleteHTTPReq, err := c.newOSBRequest(
+		http.MethodDelete,
+		serviceBindingURL,
+		map[string]string{
+			"service_id": serviceID,
+			"plan_id":    planID,
+		},
+		nil,
+	)
 	if err != nil {
 		glog.Errorf("Failed to create new HTTP request: %v", err)
 		return err
@@ -293,14 +303,24 @@ func (c *openServiceBrokerClient) DeleteServiceBinding(instanceID, bindingID, se
 }
 
 func (c *openServiceBrokerClient) PollServiceInstance(ID string, req *brokerapi.LastOperationRequest) (*brokerapi.LastOperationResponse, int, error) {
-	q, err := createPollParameters(req)
-	if err != nil {
-		glog.Errorf("Failed to create query parameters for poll last operation: %v", err)
-		return nil, 0, err
+	if req.ServiceID == "" {
+		return nil, 0, fmt.Errorf("LastOperationRequest is missing service_id")
 	}
-	url := fmt.Sprintf(pollingFormatString, c.url, ID, q)
-	pollReq := brokerapi.LastOperationRequest{}
-	resp, err := sendOSBRequest(c, http.MethodGet, url, pollReq)
+	if req.PlanID == "" {
+		return nil, 0, fmt.Errorf("LastOperationRequest is missing plan_id")
+	}
+	url := fmt.Sprintf(pollingFormatString, c.url, ID)
+	resp, err := sendOSBRequest(
+		c,
+		http.MethodGet,
+		url,
+		map[string]string{
+			"service_id": req.ServiceID,
+			"plan_id":    req.PlanID,
+			"operation":  req.Operation,
+		},
+		nil,
+	)
 	if err != nil {
 		glog.Errorf("Failed to create new HTTP request: %v", err)
 		return nil, 0, err
@@ -318,58 +338,21 @@ func (c *openServiceBrokerClient) PollServiceInstance(ID string, req *brokerapi.
 	return &lo, resp.StatusCode, nil
 }
 
-// createPollParameters creates the query parameter string from the LastOperationRequest
-// According to the spec, ServiceID and PlanID should be included, so fail requests
-// without them as it indicates programming error on our part.
-func createPollParameters(req *brokerapi.LastOperationRequest) (string, error) {
-	if req.ServiceID == "" {
-		return "", fmt.Errorf("LastOperationRequest is missing service_id")
-	}
-	if req.PlanID == "" {
-		return "", fmt.Errorf("LastOperationRequest is missing plan_id")
-	}
-
-	var buffer bytes.Buffer
-	err := appendQueryParam(&buffer, "service_id", req.ServiceID)
-	if err != nil {
-		return "", err
-	}
-	err = appendQueryParam(&buffer, "plan_id", req.PlanID)
-	if err != nil {
-		return "", err
-	}
-	err = appendQueryParam(&buffer, "operation", req.Operation)
-	if err != nil {
-		return "", err
-	}
-	return buffer.String(), nil
-}
-
-// appendQueryParam appends key=value to buffer if value is non-null.
-// If buffer is non-empty appends &key=value
-func appendQueryParam(buffer *bytes.Buffer, key, value string) error {
-	if value == "" {
-		return nil
-	}
-	if buffer.Len() > 0 {
-		_, err := buffer.WriteString("&")
-		if err != nil {
-			return err
-		}
-	}
-	_, err := buffer.WriteString(fmt.Sprintf(queryParamFormatString, key, value))
-	return err
-}
-
 // SendRequest will serialize 'object' and send it using the given method to
 // the given URL, through the provided client
-func sendOSBRequest(c *openServiceBrokerClient, method string, url string, object interface{}) (*http.Response, error) {
+func sendOSBRequest(
+	c *openServiceBrokerClient,
+	method string,
+	url string,
+	queryParams map[string]string,
+	object interface{},
+) (*http.Response, error) {
 	data, err := json.Marshal(object)
 	if err != nil {
 		return nil, fmt.Errorf("Failed to marshal request: %s", err.Error())
 	}
 
-	req, err := c.newOSBRequest(method, url, bytes.NewReader(data))
+	req, err := c.newOSBRequest(method, url, queryParams, bytes.NewReader(data))
 	if err != nil {
 		return nil, fmt.Errorf("Failed to create request object: %s", err.Error())
 	}
@@ -382,7 +365,12 @@ func sendOSBRequest(c *openServiceBrokerClient, method string, url string, objec
 	return resp, nil
 }
 
-func (c *openServiceBrokerClient) newOSBRequest(method, urlStr string, body io.Reader) (*http.Request, error) {
+func (c *openServiceBrokerClient) newOSBRequest(
+	method string,
+	urlStr string,
+	queryParams map[string]string,
+	body io.Reader,
+) (*http.Request, error) {
 	req, err := http.NewRequest(method, urlStr, body)
 	if err != nil {
 		return nil, err
@@ -392,5 +380,12 @@ func (c *openServiceBrokerClient) newOSBRequest(method, urlStr string, body io.R
 	}
 	req.Header.Add(constants.APIVersionHeader, constants.APIVersion)
 	req.SetBasicAuth(c.username, c.password)
+	if queryParams != nil {
+		q := req.URL.Query()
+		for k, v := range queryParams {
+			q.Set(k, v)
+		}
+		req.URL.RawQuery = q.Encode()
+	}
 	return req, nil
 }

pkg/brokerapi/openservicebroker/open_service_broker_client_test.go

@@ -358,51 +358,19 @@ func TestUnbindGone(t *testing.T) {
 	verifyBindingMethodAndPath(http.MethodDelete, testServiceInstanceID, testServiceBindingID, fbs.Request, t)
 }
 
-func TestCreatePollParametersMissingServiceID(t *testing.T) {
+func TestPollServiceInstanceWithMissingServiceID(t *testing.T) {
+	fbs, fakeBroker := setup()
+	defer fbs.Stop()
+
+	c := NewClient(testBrokerName, fakeBroker.Spec.URL, "", "")
 	r := &brokerapi.LastOperationRequest{PlanID: testPlanID}
-	_, err := createPollParameters(r)
+	_, _, err := c.PollServiceInstance(testServiceInstanceID, r)
 	if err == nil {
-		t.Fatalf("createPollParameters did not fail with missing ServiceID")
+		t.Fatal("PollServiceInstance did not fail with invalid LastOperationRequest")
 	}
 	if !strings.Contains(err.Error(), "missing service_id") {
 		t.Fatalf("Did not find the expected error message 'missing service_id' in error: %s", err)
 	}
-
-}
-
-func TestCreatePollParametersMissingPlanID(t *testing.T) {
-	r := &brokerapi.LastOperationRequest{ServiceID: testServiceID}
-	_, err := createPollParameters(r)
-	if err == nil {
-		t.Fatalf("createPollParameters did not fail with missing PlanID")
-	}
-	if !strings.Contains(err.Error(), "missing plan_id") {
-		t.Fatalf("Did not find the expected error message 'missing plan_id' in error: %s", err)
-	}
-}
-
-func TestCreatePollParametersNoOperation(t *testing.T) {
-	r := &brokerapi.LastOperationRequest{ServiceID: testServiceID, PlanID: testPlanID}
-	q, err := createPollParameters(r)
-	if err != nil {
-		t.Fatalf("createPollParameters failed when expected to succeed: %s", err)
-	}
-	exp := "service_id=" + testServiceID + "&plan_id=" + testPlanID
-	if q != exp {
-		t.Fatalf("expected query parameters %q got %q\n", exp, q)
-	}
-}
-
-func TestCreatePollParametersWithOperation(t *testing.T) {
-	r := &brokerapi.LastOperationRequest{ServiceID: testServiceID, PlanID: testPlanID, Operation: testOperation}
-	q, err := createPollParameters(r)
-	if err != nil {
-		t.Fatalf("createPollParameters failed when expected to succeed: %s", err)
-	}
-	exp := "service_id=" + testServiceID + "&plan_id=" + testPlanID + "&operation=" + testOperation
-	if q != exp {
-		t.Fatalf("expected query parameters %q got %q\n", exp, q)
-	}
 }
 
 func TestPollServiceInstanceWithMissingPlanID(t *testing.T) {

pkg/brokerapi/openservicebroker/util/fake_broker_server.go

@@ -143,11 +143,15 @@ func (f *FakeBrokerServer) provisionHandler(w http.ResponseWriter, r *http.Reque
 func (f *FakeBrokerServer) deprovisionHandler(w http.ResponseWriter, r *http.Request) {
 	glog.Info("fake deprovision called")
 	f.Request = r
-	req := &brokerapi.DeleteServiceInstanceRequest{}
-	if err := util.BodyToObject(r, req); err != nil {
-		w.WriteHeader(http.StatusBadRequest)
-		return
+	req := &brokerapi.DeleteServiceInstanceRequest{
+		ServiceID: r.URL.Query().Get("service_id"),
+		PlanID:    r.URL.Query().Get("plan_id"),
 	}
+	incompleteStr := r.URL.Query().Get("accepts_incomplete")
+	if incompleteStr == "true" {
+		req.AcceptsIncomplete = true
+	}
+
 	f.RequestObject = req
 
 	if r.FormValue(asyncProvisionQueryParamKey) != "true" {