Use query parameters for service instance last operation

Co-authored-by: Danail Branekov <[email protected]>

controllers/controllers/services/instances/managed/controller.go

@@ -135,9 +135,9 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 		return r.provisionServiceInstance(ctx, osbapiClient, serviceInstance, servicePlan, serviceOffering)
 	}
 
-	lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetLastOperationPayload{
+	lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetLastOperationRequest{
 		ID: serviceInstance.Name,
-		GetLastOperationRequest: osbapi.GetLastOperationRequest{
+		GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
 			ServiceId: serviceOffering.Spec.BrokerCatalog.ID,
 			PlanID:    servicePlan.Spec.BrokerCatalog.ID,
 			Operation: serviceInstance.Status.ProvisionOperation,

controllers/controllers/services/instances/managed/controller_test.go

@@ -171,9 +171,9 @@ var _ = Describe("CFServiceInstance", func() {
 
 			g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 0))
 			_, lastOp := brokerClient.GetServiceInstanceLastOperationArgsForCall(brokerClient.GetServiceInstanceLastOperationCallCount() - 1)
-			g.Expect(lastOp).To(Equal(osbapi.GetLastOperationPayload{
+			g.Expect(lastOp).To(Equal(osbapi.GetLastOperationRequest{
 				ID: instance.Name,
-				GetLastOperationRequest: osbapi.GetLastOperationRequest{
+				GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
 					ServiceId: "service-offering-id",
 					PlanID:    "service-plan-id",
 					Operation: "operation-1",
@@ -351,9 +351,9 @@ var _ = Describe("CFServiceInstance", func() {
 			Eventually(func(g Gomega) {
 				g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 1))
 				_, actualLastOpPayload := brokerClient.GetServiceInstanceLastOperationArgsForCall(1)
-				g.Expect(actualLastOpPayload).To(Equal(osbapi.GetLastOperationPayload{
+				g.Expect(actualLastOpPayload).To(Equal(osbapi.GetLastOperationRequest{
 					ID: instance.Name,
-					GetLastOperationRequest: osbapi.GetLastOperationRequest{
+					GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
 						ServiceId: "service-offering-id",
 						PlanID:    "service-plan-id",
 						Operation: "operation-1",
@@ -516,9 +516,9 @@ var _ = Describe("CFServiceInstance", func() {
 			Eventually(func(g Gomega) {
 				g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(Equal(1))
 				_, actualLastOpPayload := brokerClient.GetServiceInstanceLastOperationArgsForCall(0)
-				g.Expect(actualLastOpPayload).To(Equal(osbapi.GetLastOperationPayload{
+				g.Expect(actualLastOpPayload).To(Equal(osbapi.GetLastOperationRequest{
 					ID: instance.Name,
-					GetLastOperationRequest: osbapi.GetLastOperationRequest{
+					GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
 						ServiceId: "service-offering-id",
 						PlanID:    "service-plan-id",
 						Operation: "operation-1",

controllers/controllers/services/osbapi/client.go

@@ -40,7 +40,7 @@ func NewClient(broker Broker, httpClient *http.Client) *Client {
 func (c *Client) GetCatalog(ctx context.Context) (Catalog, error) {
 	statusCode, resp, err := c.newBrokerRequester().
 		forBroker(c.broker).
-		sendRequest(ctx, "/v2/catalog", http.MethodGet, nil)
+		sendRequest(ctx, "/v2/catalog", http.MethodGet, nil, nil)
 	if err != nil {
 		return Catalog{}, fmt.Errorf("get catalog request failed: %w", err)
 	}
@@ -66,6 +66,7 @@ func (c *Client) Provision(ctx context.Context, payload InstanceProvisionPayload
 			ctx,
 			"/v2/service_instances/"+payload.InstanceID,
 			http.MethodPut,
+			nil,
 			payload.InstanceProvisionRequest,
 		)
 	if err != nil {
@@ -97,6 +98,7 @@ func (c *Client) Deprovision(ctx context.Context, payload InstanceDeprovisionPay
 			ctx,
 			"/v2/service_instances/"+payload.ID,
 			http.MethodDelete,
+			nil,
 			payload.InstanceDeprovisionRequest,
 		)
 	if err != nil {
@@ -116,14 +118,19 @@ func (c *Client) Deprovision(ctx context.Context, payload InstanceDeprovisionPay
 	return response, nil
 }
 
-func (c *Client) GetServiceInstanceLastOperation(ctx context.Context, payload GetLastOperationPayload) (LastOperationResponse, error) {
+func (c *Client) GetServiceInstanceLastOperation(ctx context.Context, payload GetLastOperationRequest) (LastOperationResponse, error) {
 	statusCode, respBytes, err := c.newBrokerRequester().
 		forBroker(c.broker).
 		sendRequest(
 			ctx,
 			"/v2/service_instances/"+payload.ID+"/last_operation",
 			http.MethodGet,
-			payload.GetLastOperationRequest,
+			map[string]string{
+				"service_id": payload.ServiceId,
+				"plan_id":    payload.PlanID,
+				"operation":  payload.Operation,
+			},
+			nil,
 		)
 	if err != nil {
 		return LastOperationResponse{}, fmt.Errorf("getting last operation request failed: %w", err)
@@ -153,6 +160,7 @@ func (c *Client) Bind(ctx context.Context, payload BindPayload) (BindResponse, e
 			ctx,
 			"/v2/service_instances/"+payload.InstanceID+"/service_bindings/"+payload.BindingID,
 			http.MethodPut,
+			nil,
 			payload.BindRequest,
 		)
 	if err != nil {
@@ -209,7 +217,7 @@ func (r *brokerRequester) async() *brokerRequester {
 	return r
 }
 
-func (r *brokerRequester) sendRequest(ctx context.Context, requestPath string, method string, payload any) (int, []byte, error) {
+func (r *brokerRequester) sendRequest(ctx context.Context, requestPath string, method string, queryParams map[string]string, payload any) (int, []byte, error) {
 	requestUrl, err := url.JoinPath(r.broker.URL, requestPath)
 	if err != nil {
 		return 0, nil, fmt.Errorf("failed to build broker requestUrl for path %q: %w", requestPath, err)
@@ -230,11 +238,18 @@ func (r *brokerRequester) sendRequest(ctx context.Context, requestPath string, m
 		return 0, nil, fmt.Errorf("failed to create new HTTP request: %w", err)
 	}
 	req.Header.Add("X-Broker-API-Version", osbapiVersion)
+
+	queryValues := req.URL.Query()
+	for queryParam, queryParamValue := range queryParams {
+		if queryParamValue == "" {
+			continue
+		}
+		queryValues.Add(queryParam, queryParamValue)
+	}
 	if r.acceptsIncomplete {
-		queryValues := req.URL.Query()
 		queryValues.Add("accepts_incomplete", "true")
-		req.URL.RawQuery = queryValues.Encode()
 	}
+	req.URL.RawQuery = queryValues.Encode()
 
 	authHeader, err := r.buildAuthorizationHeaderValue()
 	if err != nil {

controllers/controllers/services/osbapi/client_test.go

@@ -314,8 +314,9 @@ var _ = Describe("OSBAPI Client", func() {
 
 		Describe("GetLastOperation", func() {
 			var (
-				lastOpResp osbapi.LastOperationResponse
-				lastOpErr  error
+				lastOpResp           osbapi.LastOperationResponse
+				lastOpErr            error
+				lastOperationRequest osbapi.GetLastOperationRequest
 			)
 
 			BeforeEach(func() {
@@ -327,17 +328,19 @@ var _ = Describe("OSBAPI Client", func() {
 					},
 					http.StatusOK,
 				)
-			})
 
-			JustBeforeEach(func() {
-				lastOpResp, lastOpErr = brokerClient.GetServiceInstanceLastOperation(ctx, osbapi.GetLastOperationPayload{
+				lastOperationRequest = osbapi.GetLastOperationRequest{
 					ID: "my-service-instance",
-					GetLastOperationRequest: osbapi.GetLastOperationRequest{
+					GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
 						ServiceId: "service-guid",
 						PlanID:    "plan-guid",
 						Operation: "op-guid",
 					},
-				})
+				}
+			})
+
+			JustBeforeEach(func() {
+				lastOpResp, lastOpErr = brokerClient.GetServiceInstanceLastOperation(ctx, lastOperationRequest)
 			})
 
 			It("gets the last operation", func() {
@@ -356,17 +359,31 @@ var _ = Describe("OSBAPI Client", func() {
 
 				Expect(requests[0].Method).To(Equal(http.MethodGet))
 				Expect(requests[0].URL.Path).To(Equal("/v2/service_instances/my-service-instance/last_operation"))
+				Expect(requests[0].URL.Query()).To(BeEquivalentTo(map[string][]string{
+					"service_id": {"service-guid"},
+					"plan_id":    {"plan-guid"},
+					"operation":  {"op-guid"},
+				}))
 
 				requestBytes, err := io.ReadAll(requests[0].Body)
 				Expect(err).NotTo(HaveOccurred())
-				requestBody := map[string]any{}
-				Expect(json.Unmarshal(requestBytes, &requestBody)).To(Succeed())
+				Expect(requestBytes).To(BeEmpty())
+			})
 
-				Expect(requestBody).To(MatchAllKeys(Keys{
-					"service_id": Equal("service-guid"),
-					"plan_id":    Equal("plan-guid"),
-					"operation":  Equal("op-guid"),
-				}))
+			When("request parameters are not specified", func() {
+				BeforeEach(func() {
+					lastOperationRequest = osbapi.GetLastOperationRequest{
+						ID: "my-service-instance",
+					}
+				})
+
+				It("does not specify http request query parameters", func() {
+					Expect(lastOpErr).NotTo(HaveOccurred())
+					requests := brokerServer.ServedRequests()
+
+					Expect(requests).To(HaveLen(1))
+					Expect(requests[0].URL.Query()).To(BeEmpty())
+				})
 			})
 
 			When("getting the last operation request fails", func() {

controllers/controllers/services/osbapi/clientfactory.go

@@ -18,7 +18,7 @@ import (
 type BrokerClient interface {
 	Provision(context.Context, InstanceProvisionPayload) (ServiceInstanceOperationResponse, error)
 	Deprovision(context.Context, InstanceDeprovisionPayload) (ServiceInstanceOperationResponse, error)
-	GetServiceInstanceLastOperation(context.Context, GetLastOperationPayload) (LastOperationResponse, error)
+	GetServiceInstanceLastOperation(context.Context, GetLastOperationRequest) (LastOperationResponse, error)
 	GetCatalog(context.Context) (Catalog, error)
 	Bind(context.Context, BindPayload) (BindResponse, error)
 }

controllers/controllers/services/osbapi/types.go

@@ -41,12 +41,12 @@ type InstanceProvisionRequest struct {
 	Parameters map[string]any `json:"parameters"`
 }
 
-type GetLastOperationPayload struct {
+type GetLastOperationRequest struct {
 	ID string
-	GetLastOperationRequest
+	GetLastOperationRequestParameters
 }
 
-type GetLastOperationRequest struct {
+type GetLastOperationRequestParameters struct {
 	ServiceId string `json:"service_id"`
 	PlanID    string `json:"plan_id"`
 	Operation string `json:"operation"`