Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Commit

Permalink
Clean up the OSB client (#888)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@krancour
krancour authored and Ville Aikas committed May 24, 2017
1 parent fe6aee9 commit 5e6925d
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 131 deletions.
171 changes: 83 additions & 88 deletions pkg/brokerapi/openservicebroker/open_service_broker_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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()}
Expand Down Expand Up @@ -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()}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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())
}
Expand All @@ -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
Expand All @@ -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
}
46 changes: 7 additions & 39 deletions pkg/brokerapi/openservicebroker/open_service_broker_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 8 additions & 4 deletions pkg/brokerapi/openservicebroker/util/fake_broker_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down

0 comments on commit 5e6925d

Please sign in to comment.