From 5e6925db36025d32684126295f31da93f51150cf Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Wed, 24 May 2017 17:01:11 -0400 Subject: [PATCH] Clean up the OSB client (#888) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * sending the appropriate query string parameters in deprovision … and not sending a request body Fixes https://github.com/kubernetes-incubator/service-catalog/issues/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 --- .../open_service_broker_client.go | 171 +++++++++--------- .../open_service_broker_client_test.go | 46 +---- .../util/fake_broker_server.go | 12 +- 3 files changed, 98 insertions(+), 131 deletions(-) diff --git a/pkg/brokerapi/openservicebroker/open_service_broker_client.go b/pkg/brokerapi/openservicebroker/open_service_broker_client.go index 9f69575cc85..7db58aa1d6a 100644 --- a/pkg/brokerapi/openservicebroker/open_service_broker_client.go +++ b/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 } diff --git a/pkg/brokerapi/openservicebroker/open_service_broker_client_test.go b/pkg/brokerapi/openservicebroker/open_service_broker_client_test.go index 21cb514d66f..ac94b2f13ca 100644 --- a/pkg/brokerapi/openservicebroker/open_service_broker_client_test.go +++ b/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) { diff --git a/pkg/brokerapi/openservicebroker/util/fake_broker_server.go b/pkg/brokerapi/openservicebroker/util/fake_broker_server.go index 9869856edf9..4e153b056b7 100644 --- a/pkg/brokerapi/openservicebroker/util/fake_broker_server.go +++ b/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" {