From 76e28d32150175759056747c09a2979f80229511 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 27 Sep 2018 14:38:53 +0300 Subject: [PATCH 1/4] Support OSB version 2.14 *. Added the Get Service Instance endpoint *. Allow returning the dashboard URL in update operations --- api.go | 56 ++++++++++++++++++++++++++++++++++-- api_test.go | 31 ++++++++++++++++++-- catalog_test.go | 2 +- failure_response_test.go | 4 +-- fakes/fake_service_broker.go | 21 +++++++++++++- fixtures/get_instance.json | 8 ++++++ response.go | 8 ++++++ response_test.go | 2 +- service_broker.go | 14 +++++++++ 9 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 fixtures/get_instance.json diff --git a/api.go b/api.go index e44b8bfb..f9a3425a 100644 --- a/api.go +++ b/api.go @@ -24,7 +24,7 @@ import ( "code.cloudfoundry.org/lager" "github.com/gorilla/mux" - "github.com/pivotal-cf/brokerapi/auth" + "github.com/liorokman/brokerapi/auth" ) const ( @@ -60,6 +60,7 @@ const ( planIdMissingKey = "plan-id-missing" invalidServiceID = "invalid-service-id" invalidPlanID = "invalid-plan-id" + concurrentAccessKey = "get-instance-during-update" ) var ( @@ -84,6 +85,7 @@ func AttachRoutes(router *mux.Router, serviceBroker ServiceBroker, logger lager. handler := serviceBrokerHandler{serviceBroker: serviceBroker, logger: logger} router.HandleFunc("/v2/catalog", handler.catalog).Methods("GET") + router.HandleFunc("/v2/service_instances/{instance_id}", handler.getInstance).Methods("GET") router.HandleFunc("/v2/service_instances/{instance_id}", handler.provision).Methods("PUT") router.HandleFunc("/v2/service_instances/{instance_id}", handler.deprovision).Methods("DELETE") router.HandleFunc("/v2/service_instances/{instance_id}/last_operation", handler.lastOperation).Methods("GET") @@ -290,7 +292,10 @@ func (h serviceBrokerHandler) update(w http.ResponseWriter, req *http.Request) { if updateServiceSpec.IsAsync { statusCode = http.StatusAccepted } - h.respond(w, statusCode, UpdateResponse{OperationData: updateServiceSpec.OperationData}) + h.respond(w, statusCode, UpdateResponse{ + OperationData: updateServiceSpec.OperationData, + DashboardURL: updateServiceSpec.DashboardURL, + }) } func (h serviceBrokerHandler) deprovision(w http.ResponseWriter, req *http.Request) { @@ -353,6 +358,53 @@ func (h serviceBrokerHandler) deprovision(w http.ResponseWriter, req *http.Reque } } +func (h serviceBrokerHandler) getInstance(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + instanceID := vars["instance_id"] + + logger := h.logger.Session(getBindLogKey, lager.Data{ + instanceIDLogKey: instanceID, + }) + + versionCompatibility, err := checkBrokerAPIVersionHdr(req) + if err != nil { + h.respond(w, http.StatusPreconditionFailed, ErrorResponse{ + Description: err.Error(), + }) + logger.Error(apiVersionInvalidKey, err) + return + } + if versionCompatibility.Minor < 14 { + h.respond(w, http.StatusPreconditionFailed, ErrorResponse{ + Description: "get instance endpoint only supported starting with OSB version 2.14", + }) + logger.Error(apiVersionInvalidKey, err) + return + } + + instanceDetails, err := h.serviceBroker.GetInstance(req.Context(), instanceID) + if err != nil { + switch err := err.(type) { + case *FailureResponse: + logger.Error(err.LoggerAction(), err) + h.respond(w, err.ValidatedStatusCode(logger), err.ErrorResponse()) + default: + logger.Error(unknownErrorKey, err) + h.respond(w, http.StatusInternalServerError, ErrorResponse{ + Description: err.Error(), + }) + } + return + } + + h.respond(w, http.StatusOK, GetInstanceResponse{ + ServiceID: instanceDetails.ServiceID, + PlanID: instanceDetails.PlanID, + DashboardURL: instanceDetails.DashboardURL, + Parameters: instanceDetails.Parameters, + }) +} + func (h serviceBrokerHandler) getBinding(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) instanceID := vars["instance_id"] diff --git a/api_test.go b/api_test.go index d38e233d..845f4328 100644 --- a/api_test.go +++ b/api_test.go @@ -29,10 +29,10 @@ import ( "code.cloudfoundry.org/lager" "code.cloudfoundry.org/lager/lagertest" "github.com/drewolson/testflight" + "github.com/liorokman/brokerapi" + "github.com/liorokman/brokerapi/fakes" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/pivotal-cf/brokerapi" - "github.com/pivotal-cf/brokerapi/fakes" ) var _ = Describe("Service Broker API", func() { @@ -182,6 +182,11 @@ var _ = Describe("Service Broker API", func() { makeRequest("GET", "/v2/service_instances/instance-id/last_operation", "{}") Expect(fakeServiceBroker.ReceivedContext).To(BeTrue()) }) + + Specify("a get instance operation endpoint which passes the request context to the broker", func() { + makeRequest("GET", "/v2/service_instances/instance-id", "{}") + Expect(fakeServiceBroker.ReceivedContext).To(BeTrue()) + }) }) Describe("authentication", func() { @@ -297,6 +302,19 @@ var _ = Describe("Service Broker API", func() { }) Describe("instance lifecycle endpoint", func() { + makeGetInstanceRequest := func(instanceID string) *testflight.Response { + response := &testflight.Response{} + testflight.WithServer(brokerAPI, func(r *testflight.Requester) { + path := fmt.Sprintf("/v2/service_instances/%s", instanceID) + request, err := http.NewRequest("GET", path, strings.NewReader("")) + Expect(err).NotTo(HaveOccurred()) + request.Header.Add("X-Broker-API-Version", apiVersion) + request.SetBasicAuth("username", "password") + response = r.Do(request) + }) + return response + } + makeInstanceDeprovisioningRequestFull := func(instanceID, serviceID, planID, queryString string) *testflight.Response { response := &testflight.Response{} testflight.WithServer(brokerAPI, func(r *testflight.Requester) { @@ -348,6 +366,15 @@ var _ = Describe("Service Broker API", func() { Expect(fakeServiceBroker.ProvisionedInstanceIDs).To(ContainElement(instanceID)) }) + It("calls GetInstance on the service broker with the instance id", func() { + makeInstanceProvisioningRequest(instanceID, provisionDetails, "") + Expect(fakeServiceBroker.ProvisionedInstanceIDs).To(ContainElement(instanceID)) + fakeServiceBroker.DashboardURL = "https://example.com/dashboard/some-instance" + resp := makeGetInstanceRequest(instanceID) + Expect(fakeServiceBroker.GetInstanceIDs).To(ContainElement(instanceID)) + Expect(resp.Body).To(MatchJSON(fixture("get_instance.json"))) + }) + Context("when the broker returns some operation data", func() { BeforeEach(func() { fakeServiceBroker = &fakes.FakeServiceBroker{ diff --git a/catalog_test.go b/catalog_test.go index 175d4717..1744a9be 100644 --- a/catalog_test.go +++ b/catalog_test.go @@ -20,7 +20,7 @@ import ( "reflect" "sync" - "github.com/pivotal-cf/brokerapi" + "github.com/liorokman/brokerapi" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) diff --git a/failure_response_test.go b/failure_response_test.go index 059c7d9e..04b93419 100644 --- a/failure_response_test.go +++ b/failure_response_test.go @@ -16,7 +16,7 @@ package brokerapi_test import ( - "github.com/pivotal-cf/brokerapi" + "github.com/liorokman/brokerapi" "errors" @@ -65,7 +65,7 @@ var _ = Describe("FailureResponse", func() { Expect(newError.Error()).To(Equal("my error message and some more details")) Expect(newError.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden)) Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction())) - + errorResponse, typeCast := newError.ErrorResponse().(brokerapi.ErrorResponse) Expect(typeCast).To(BeTrue()) Expect(errorResponse.Error).To(Equal("some-key")) diff --git a/fakes/fake_service_broker.go b/fakes/fake_service_broker.go index a6649936..a0bf227e 100644 --- a/fakes/fake_service_broker.go +++ b/fakes/fake_service_broker.go @@ -4,7 +4,7 @@ import ( "context" "errors" - "github.com/pivotal-cf/brokerapi" + "github.com/liorokman/brokerapi" ) type FakeServiceBroker struct { @@ -15,6 +15,7 @@ type FakeServiceBroker struct { ProvisionedInstanceIDs []string DeprovisionedInstanceIDs []string UpdatedInstanceIDs []string + GetInstanceIDs []string BoundInstanceIDs []string BoundBindingIDs []string @@ -232,6 +233,24 @@ func (fakeBroker *FakeServiceBroker) Update(context context.Context, instanceID return brokerapi.UpdateServiceSpec{IsAsync: fakeBroker.ShouldReturnAsync, OperationData: fakeBroker.OperationDataToReturn}, nil } +func (fakeBroker *FakeServiceBroker) GetInstance(context context.Context, instanceID string) (brokerapi.GetInstanceDetailsSpec, error) { + fakeBroker.BrokerCalled = true + + if val, ok := context.Value("test_context").(bool); ok { + fakeBroker.ReceivedContext = val + } + + fakeBroker.GetInstanceIDs = append(fakeBroker.GetInstanceIDs, instanceID) + return brokerapi.GetInstanceDetailsSpec{ + ServiceID: fakeBroker.ServiceID, + PlanID: fakeBroker.PlanID, + DashboardURL: fakeBroker.DashboardURL, + Parameters: map[string]interface{}{ + "param1": "value1", + }, + }, nil +} + func (fakeBroker *FakeServiceBroker) Deprovision(context context.Context, instanceID string, details brokerapi.DeprovisionDetails, asyncAllowed bool) (brokerapi.DeprovisionServiceSpec, error) { fakeBroker.BrokerCalled = true diff --git a/fixtures/get_instance.json b/fixtures/get_instance.json new file mode 100644 index 00000000..06e1828a --- /dev/null +++ b/fixtures/get_instance.json @@ -0,0 +1,8 @@ +{ + "service_id" : "0A789746-596F-4CEA-BFAC-A0795DA056E3", + "plan_id" : "plan-id", + "dashboard_url" : "https://example.com/dashboard/some-instance", + "parameters": { + "param1" : "value1" + } +} diff --git a/response.go b/response.go index 1481298e..297265ca 100644 --- a/response.go +++ b/response.go @@ -31,8 +31,16 @@ type ProvisioningResponse struct { OperationData string `json:"operation,omitempty"` } +type GetInstanceResponse struct { + ServiceID string `json:"service_id"` + PlanID string `json:"plan_id"` + DashboardURL string `json:"dashboard_url,omitempty"` + Parameters interface{} `json:"parameters,omitempty"` +} + type UpdateResponse struct { OperationData string `json:"operation,omitempty"` + DashboardURL string `json:"dashboard_url,omitempty"` } type DeprovisionResponse struct { diff --git a/response_test.go b/response_test.go index c7b3efef..d1c7e4a6 100644 --- a/response_test.go +++ b/response_test.go @@ -18,7 +18,7 @@ package brokerapi_test import ( "encoding/json" - "github.com/pivotal-cf/brokerapi" + "github.com/liorokman/brokerapi" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) diff --git a/service_broker.go b/service_broker.go index 6e4cc018..ca52d6a4 100644 --- a/service_broker.go +++ b/service_broker.go @@ -27,6 +27,7 @@ type ServiceBroker interface { Provision(ctx context.Context, instanceID string, details ProvisionDetails, asyncAllowed bool) (ProvisionedServiceSpec, error) Deprovision(ctx context.Context, instanceID string, details DeprovisionDetails, asyncAllowed bool) (DeprovisionServiceSpec, error) + GetInstance(ctx context.Context, instanceID string) (GetInstanceDetailsSpec, error) Bind(ctx context.Context, instanceID, bindingID string, details BindDetails, asyncAllowed bool) (Binding, error) Unbind(ctx context.Context, instanceID, bindingID string, details UnbindDetails, asyncAllowed bool) (UnbindSpec, error) @@ -81,6 +82,13 @@ type ProvisionedServiceSpec struct { OperationData string } +type GetInstanceDetailsSpec struct { + ServiceID string `json:"service_id"` + PlanID string `json:"plan_id"` + DashboardURL string `json:"dashboard_url"` + Parameters interface{} `json:"parameters"` +} + type UnbindSpec struct { IsAsync bool OperationData string @@ -110,6 +118,7 @@ type UnbindDetails struct { type UpdateServiceSpec struct { IsAsync bool OperationData string + DashboardURL string } type DeprovisionServiceSpec struct { @@ -199,6 +208,7 @@ const ( planChangeUnsupportedMsg = "The requested plan migration cannot be performed" rawInvalidParamsMsg = "The format of the parameters is not valid JSON" appGuidMissingMsg = "app_guid is a required field but was not provided" + concurrentInstanceAccessMsg = "instance is being updated and cannot be retrieved" ) var ( @@ -244,4 +254,8 @@ var ( ErrPlanQuotaExceeded = errors.New(servicePlanQuotaExceededMsg) ErrServiceQuotaExceeded = errors.New(serviceQuotaExceededMsg) + + ErrConcurrentInstanceAccess = NewFailureResponseBuilder( + errors.New(concurrentInstanceAccessMsg), http.StatusUnprocessableEntity, concurrentAccessKey, + ).WithErrorKey("ConcurrencyError") ) From e50825f7393e95bc4edccff18f7a93d06d41202e Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 27 Sep 2018 14:41:48 +0300 Subject: [PATCH 2/4] Prepare for merge with upstream --- api.go | 2 +- api_test.go | 4 ++-- catalog_test.go | 2 +- failure_response_test.go | 2 +- fakes/fake_service_broker.go | 2 +- response_test.go | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api.go b/api.go index f9a3425a..d775300d 100644 --- a/api.go +++ b/api.go @@ -24,7 +24,7 @@ import ( "code.cloudfoundry.org/lager" "github.com/gorilla/mux" - "github.com/liorokman/brokerapi/auth" + "github.com/pivotal-cf/brokerapi/auth" ) const ( diff --git a/api_test.go b/api_test.go index 845f4328..27027718 100644 --- a/api_test.go +++ b/api_test.go @@ -29,10 +29,10 @@ import ( "code.cloudfoundry.org/lager" "code.cloudfoundry.org/lager/lagertest" "github.com/drewolson/testflight" - "github.com/liorokman/brokerapi" - "github.com/liorokman/brokerapi/fakes" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/pivotal-cf/brokerapi" + "github.com/pivotal-cf/brokerapi/fakes" ) var _ = Describe("Service Broker API", func() { diff --git a/catalog_test.go b/catalog_test.go index 1744a9be..22a6838d 100644 --- a/catalog_test.go +++ b/catalog_test.go @@ -20,9 +20,9 @@ import ( "reflect" "sync" - "github.com/liorokman/brokerapi" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/pivotal-cf/brokerapi" ) var _ = Describe("Catalog", func() { diff --git a/failure_response_test.go b/failure_response_test.go index 04b93419..8a203335 100644 --- a/failure_response_test.go +++ b/failure_response_test.go @@ -16,7 +16,7 @@ package brokerapi_test import ( - "github.com/liorokman/brokerapi" + "github.com/pivotal-cf/brokerapi" "errors" diff --git a/fakes/fake_service_broker.go b/fakes/fake_service_broker.go index a0bf227e..a5b5d162 100644 --- a/fakes/fake_service_broker.go +++ b/fakes/fake_service_broker.go @@ -4,7 +4,7 @@ import ( "context" "errors" - "github.com/liorokman/brokerapi" + "github.com/pivotal-cf/brokerapi" ) type FakeServiceBroker struct { diff --git a/response_test.go b/response_test.go index d1c7e4a6..e4d6d1e1 100644 --- a/response_test.go +++ b/response_test.go @@ -18,9 +18,9 @@ package brokerapi_test import ( "encoding/json" - "github.com/liorokman/brokerapi" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/pivotal-cf/brokerapi" ) var _ = Describe("Catalog Response", func() { From 8211136646cfb12b4abac7e2ee718cdeca8a6877 Mon Sep 17 00:00:00 2001 From: Derik Evangelista Date: Fri, 26 Oct 2018 14:51:56 +0100 Subject: [PATCH 3/4] Remove duplicated DashboardURL from struct definition [#160819509] Co-authored-by: Winna Bridgewater --- response.go | 1 - service_broker.go | 1 - 2 files changed, 2 deletions(-) diff --git a/response.go b/response.go index 2123b986..d49a257e 100644 --- a/response.go +++ b/response.go @@ -41,7 +41,6 @@ type GetInstanceResponse struct { type UpdateResponse struct { DashboardURL string `json:"dashboard_url,omitempty"` OperationData string `json:"operation,omitempty"` - DashboardURL string `json:"dashboard_url,omitempty"` } type DeprovisionResponse struct { diff --git a/service_broker.go b/service_broker.go index 98d19334..b781740e 100644 --- a/service_broker.go +++ b/service_broker.go @@ -119,7 +119,6 @@ type UpdateServiceSpec struct { IsAsync bool DashboardURL string OperationData string - DashboardURL string } type DeprovisionServiceSpec struct { From d378e482b8bdb45158775f90496b094736151984 Mon Sep 17 00:00:00 2001 From: Winna Bridgewater Date: Fri, 26 Oct 2018 15:40:20 +0100 Subject: [PATCH 4/4] Add missing tests for non-happy scenarios on GetInstance * Fix log key for GetInstance [#160819509] Co-authored-by: Derik Evangelista --- api.go | 8 ++++-- api_test.go | 54 ++++++++++++++++++++++++++++++++++++ fakes/fake_service_broker.go | 3 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/api.go b/api.go index bc04acac..170b7e01 100644 --- a/api.go +++ b/api.go @@ -32,6 +32,7 @@ const ( deprovisionLogKey = "deprovision" bindLogKey = "bind" getBindLogKey = "getBinding" + getInstanceLogKey = "getInstance" unbindLogKey = "unbind" updateLogKey = "update" lastOperationLogKey = "lastOperation" @@ -294,7 +295,7 @@ func (h serviceBrokerHandler) update(w http.ResponseWriter, req *http.Request) { } h.respond(w, statusCode, UpdateResponse{ OperationData: updateServiceSpec.OperationData, - DashboardURL: updateServiceSpec.DashboardURL, + DashboardURL: updateServiceSpec.DashboardURL, }) } @@ -362,7 +363,7 @@ func (h serviceBrokerHandler) getInstance(w http.ResponseWriter, req *http.Reque vars := mux.Vars(req) instanceID := vars["instance_id"] - logger := h.logger.Session(getBindLogKey, lager.Data{ + logger := h.logger.Session(getInstanceLogKey, lager.Data{ instanceIDLogKey: instanceID, }) @@ -375,8 +376,9 @@ func (h serviceBrokerHandler) getInstance(w http.ResponseWriter, req *http.Reque return } if versionCompatibility.Minor < 14 { + err = errors.New("get instance endpoint only supported starting with OSB version 2.14") h.respond(w, http.StatusPreconditionFailed, ErrorResponse{ - Description: "get instance endpoint only supported starting with OSB version 2.14", + Description: err.Error(), }) logger.Error(apiVersionInvalidKey, err) return diff --git a/api_test.go b/api_test.go index 3e4dd13b..85c17731 100644 --- a/api_test.go +++ b/api_test.go @@ -1245,6 +1245,60 @@ var _ = Describe("Service Broker API", func() { }) }) }) + + Describe("getting instance", func() { + It("returns the appropriate status code when it fails with a known error", func() { + fakeServiceBroker.GetInstanceError = brokerapi.NewFailureResponse(errors.New("some error"), http.StatusUnprocessableEntity, "fire") + + response := makeGetInstanceRequest("instance-id") + + Expect(response.StatusCode).To(Equal(http.StatusUnprocessableEntity)) + Expect(lastLogLine().Message).To(ContainSubstring("broker-api.getInstance.fire")) + Expect(lastLogLine().Data["error"]).To(ContainSubstring("some error")) + }) + + It("returns 500 when it fails with an unknown error", func() { + fakeServiceBroker.GetInstanceError = errors.New("failed to get instance") + + response := makeGetInstanceRequest("instance-id") + + Expect(response.StatusCode).To(Equal(500)) + Expect(lastLogLine().Message).To(ContainSubstring("broker-api.getInstance.unknown-error")) + Expect(lastLogLine().Data["error"]).To(ContainSubstring("failed to get instance")) + }) + + Context("the request is malformed", func() { + It("missing header X-Broker-API-Version", func() { + apiVersion = "" + response := makeGetInstanceRequest("instance-id") + Expect(response.StatusCode).To(Equal(412)) + Expect(lastLogLine().Message).To(ContainSubstring(".getInstance.broker-api-version-invalid")) + Expect(lastLogLine().Data["error"]).To(ContainSubstring("X-Broker-API-Version Header not set")) + }) + + It("has wrong version of API", func() { + apiVersion = "1.1" + response := makeGetInstanceRequest("instance-id") + Expect(response.StatusCode).To(Equal(412)) + Expect(lastLogLine().Message).To(ContainSubstring(".getInstance.broker-api-version-invalid")) + Expect(lastLogLine().Data["error"]).To(ContainSubstring("X-Broker-API-Version Header must be 2.x")) + }) + + It("is using api version < 2.14", func() { + apiVersion = "2.13" + response := makeGetInstanceRequest("instance-id") + Expect(response.StatusCode).To(Equal(412)) + + Expect(lastLogLine().Message).To(ContainSubstring(".getInstance.broker-api-version-invalid")) + Expect(lastLogLine().Data["error"]).To(ContainSubstring("get instance endpoint only supported starting with OSB version 2.14")) + }) + + It("missing instance-id", func() { + response := makeGetInstanceRequest("") + Expect(response.StatusCode).To(Equal(404)) + }) + }) + }) }) Describe("binding lifecycle endpoint", func() { diff --git a/fakes/fake_service_broker.go b/fakes/fake_service_broker.go index 35088378..6859c08f 100644 --- a/fakes/fake_service_broker.go +++ b/fakes/fake_service_broker.go @@ -34,6 +34,7 @@ type FakeServiceBroker struct { DeprovisionError error LastOperationError error UpdateError error + GetInstanceError error BrokerCalled bool LastOperationState brokerapi.LastOperationState @@ -248,7 +249,7 @@ func (fakeBroker *FakeServiceBroker) GetInstance(context context.Context, instan Parameters: map[string]interface{}{ "param1": "value1", }, - }, nil + }, fakeBroker.GetInstanceError } func (fakeBroker *FakeServiceBroker) Deprovision(context context.Context, instanceID string, details brokerapi.DeprovisionDetails, asyncAllowed bool) (brokerapi.DeprovisionServiceSpec, error) {