Bug 1536659 - bind PUT returns http code 202 when operation runs async #669

mhrivnak · 2018-01-19T21:16:14Z

Previously the http response incorrectly had code 201.

Once async logic was removed from buildBindRequest, it looked an awful lot like
a simple constructor. That's why I moved it next to the type definition and
renamed it NewBindRequest to match golang convention.

fixes #667

coveralls · 2018-01-19T23:19:11Z

Changes Unknown when pulling 27f7b30 on mhrivnak:async-bind-202 into ** on openshift:master**.

jmrodri

I'd model it more like Provision

jmrodri · 2018-01-20T00:42:36Z

pkg/broker/broker.go

@@ -79,7 +79,7 @@ type Broker interface {
 	Provision(uuid.UUID, *ProvisionRequest, bool) (*ProvisionResponse, error)
 	Update(uuid.UUID, *UpdateRequest, bool) (*UpdateResponse, error)
 	Deprovision(apb.ServiceInstance, string, bool, bool) (*DeprovisionResponse, error)
-	Bind(apb.ServiceInstance, uuid.UUID, *BindRequest, bool) (*BindResponse, error)
+	Bind(apb.ServiceInstance, uuid.UUID, *BindRequest, bool) (*BindResponse, bool, error)


Why are we returning a bool?

jmrodri · 2018-01-20T00:42:46Z

pkg/broker/broker.go

@@ -817,12 +817,14 @@ func (a AnsibleBroker) GetBind(instance apb.ServiceInstance, bindingUUID uuid.UU
 	}

 	log.Debug("broker.GetBind: we got the bind credentials")
-	return a.buildBindResponse(provExtCreds, bindExtCreds, false, "")
+	return NewBindResponse(provExtCreds, bindExtCreds)


I like this better.

jmrodri · 2018-01-20T00:46:48Z

pkg/handler/handler.go

@@ -560,7 +560,7 @@ func (h handler) bind(w http.ResponseWriter, r *http.Request, params map[string]
 	}

 	// process binding request
-	resp, err := h.broker.Bind(serviceInstance, bindingUUID, req, async)
+	resp, ranAsync, err := h.broker.Bind(serviceInstance, bindingUUID, req, async)


I don't think we need to return this boolean. We know if we are async based on the async. Provision returns similar values without returning a ranAsync. I'd prefer to model it similar to Provision.

if err != nil { switch err { case broker.ErrorDuplicate: writeResponse(w, http.StatusConflict, broker.ProvisionResponse{}) case broker.ErrorProvisionInProgress: writeResponse(w, http.StatusAccepted, resp) case broker.ErrorAlreadyProvisioned: writeResponse(w, http.StatusOK, resp) case broker.ErrorNotFound: writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()}) default: writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()}) } } else if async { writeDefaultResponse(w, http.StatusAccepted, resp, err) } else { writeDefaultResponse(w, http.StatusCreated, resp, err) }

When async == true for the argument being passed into the broker's Bind function, there are at least two cases where there will not be asynchronous execution.

if the binding already exists

if the broker is configured for launch_apb_on_bind = false

In theory there could be other reasons for doing it synchronously even if async == true.

Put another way, when async == true, all that really means is that the client specified accepts_incomplete=true. That is not a guarantee that asynchronous execution will happen. The handler must allow the broker to decide whether async execution is appropriate, and must then compose an http response based on what the broker decided. And the only way for the handler to know what the broker decided is if the broker tells it, hence the bool.

A different but perfectly valid (I think?) way to handle this would be to add an attribute to the response object and communicate through that. But since the response object gets directly serialized into an HTTP response body, that might be overloading its definition.

We could add a private field "async" to the response, and then a method that returns whether the attribute was async or not, and I assume that would allow the field to be omitted from serialization to JSON? Something like:

type BindResponse struct { Credentials map[string]interface{} `json:"credentials,omitempty"` SyslogDrainURL string `json:"syslog_drain_url,omitempty"` RouteServiceURL string `json:"route_service_url,omitempty"` VolumeMounts []interface{} `json:"volume_mounts,omitempty"` Operation string `json:"operation,omitempty"` ranAsync bool } // returns true if the operation executed asynchronously, otherwise false func (b BindResponse) RanAsync() (bool) return b.ranAsync

Even if that's doable, it's probably simpler, and definitely just as effective, to return a separate value.

@mhrivnak very good explanation to why you did it differently than provision. Okay that makes sense now. Sorry I missed that.

jmrodri · 2018-01-20T00:47:27Z

pkg/handler/handler_test.go

@@ -93,9 +93,9 @@ func (m MockBroker) Deprovision(apb.ServiceInstance, string, bool, bool) (*broke
 	m.called("deprovision", true)
 	return nil, m.Err
 }
-func (m MockBroker) Bind(apb.ServiceInstance, uuid.UUID, *broker.BindRequest, bool) (*broker.BindResponse, error) {
+func (m MockBroker) Bind(apb.ServiceInstance, uuid.UUID, *broker.BindRequest, bool) (*broker.BindResponse, bool, error) {


-1 to returning bool.

jmrodri · 2018-01-22T14:32:19Z

@mhrivnak lint check failed. Please run make check for all the checks or make lint to verify the linter only.

mhrivnak · 2018-01-22T15:10:19Z

@jmrodri thanks for pointing that out. I fixed one of the two lint errors, but the other appears to be failing on master and is unrelated to this PR:

pkg/broker/util.go:136:2: can probably use "var formDefinition []interface{}" instead

eriknelson · 2018-01-23T15:03:35Z

pkg/broker/broker.go

-
-	log.Debugf("provision bind creds: %v", pCreds.Credentials)
-	return &BindResponse{Credentials: pCreds.Credentials}, nil
+	resp, err := NewBindResponse(provExtCreds, bindExtCreds)


+1, no dependency on the state of the broker, no reason to have it as a method.

jmrodri · 2018-01-23T15:26:44Z

@mhrivnak needs rebase

Previously the http response incorrectly had code 201. Once async logic was removed from buildBindRequest, it looked an awful lot like a simple constructor. That's why I moved it next to the type definition and renamed it NewBindRequest to match golang convention. fixes openshift#667

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 19, 2018

jmrodri self-requested a review January 19, 2018 21:28

jmrodri added bug spec-compliance labels Jan 19, 2018

jmrodri suggested changes Jan 20, 2018

View reviewed changes

jmrodri approved these changes Jan 20, 2018

View reviewed changes

rthallisey added the 3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 label Jan 22, 2018

mhrivnak force-pushed the async-bind-202 branch from 27f7b30 to 880408f Compare January 22, 2018 15:10

eriknelson self-requested a review January 22, 2018 21:31

eriknelson approved these changes Jan 23, 2018

View reviewed changes

jmrodri added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2018

mhrivnak force-pushed the async-bind-202 branch from 880408f to abeb66c Compare January 23, 2018 16:16

jmrodri merged commit 564f80f into openshift:master Jan 23, 2018

mhrivnak deleted the async-bind-202 branch January 23, 2018 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1536659 - bind PUT returns http code 202 when operation runs async #669

Bug 1536659 - bind PUT returns http code 202 when operation runs async #669

mhrivnak commented Jan 19, 2018

coveralls commented Jan 19, 2018

jmrodri left a comment

jmrodri Jan 20, 2018

jmrodri Jan 20, 2018

jmrodri Jan 20, 2018

mhrivnak Jan 20, 2018

jmrodri Jan 20, 2018

jmrodri Jan 20, 2018

jmrodri commented Jan 22, 2018

mhrivnak commented Jan 22, 2018

eriknelson Jan 23, 2018

jmrodri commented Jan 23, 2018

Bug 1536659 - bind PUT returns http code 202 when operation runs async #669

Bug 1536659 - bind PUT returns http code 202 when operation runs async #669

Conversation

mhrivnak commented Jan 19, 2018

coveralls commented Jan 19, 2018

jmrodri left a comment

Choose a reason for hiding this comment

jmrodri Jan 20, 2018

Choose a reason for hiding this comment

jmrodri Jan 20, 2018

Choose a reason for hiding this comment

jmrodri Jan 20, 2018

Choose a reason for hiding this comment

mhrivnak Jan 20, 2018

Choose a reason for hiding this comment

jmrodri Jan 20, 2018

Choose a reason for hiding this comment

jmrodri Jan 20, 2018

Choose a reason for hiding this comment

jmrodri commented Jan 22, 2018

mhrivnak commented Jan 22, 2018

eriknelson Jan 23, 2018

Choose a reason for hiding this comment

jmrodri commented Jan 23, 2018