Bug 1537367 - missing last_operation for bindings #677
Conversation
openshift-ci-robot
added
the
size/M
|
@mhrivnak Have a looksie |
jmrodri
force-pushed
the
lastoperation
branch
from
a60ecdd
to
4673fb5
Compare
|
related: #475 |
rthallisey
added
the
3.9 | release-1.1
| bindingUUID := uuid.Parse(params["binding_uuid"]) | ||
| if bindingUUID == nil { | ||
| writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: "invalid binding_uuid"}) | ||
| return |
There was a problem hiding this comment.
It would be an improvement to also check for existence of the binding in etcd. Since we have no mechanism to clean up JobState records, they could live on even if a BindInstance has been removed. In that case, I think this function would find the JobState and return a response as if the BindInstance still existed or was in-progress.
It may be an edge case that a client would call this endpoint after a binding was deleted, but it's probably worth supporting for the sake of correctness.
pkg/handler/handler.go
Outdated
| req := broker.LastOperationRequest{} | ||
|
|
||
| // operation is rqeuired | ||
| // operation is required |
There was a problem hiding this comment.
Speaking of this comment, the code below actually makes it optional. It just logs a warning but continues its lookup using just the service ID. I think it would be better to start enforcing what this comment says, and make it required, if not for the service last operation, at least for this case of a binding last operation.
If we let it continue without the operation, I'm not sure how it would behave. Maybe it would just fail to lookup the state? Or would it grab some other JobState for the same service ID, but not necessarily the right one? Either way, it's better to not let it get that far.
There was a problem hiding this comment.
It returns an error
< HTTP/1.1 400 Bad Request
< Content-Type: application/json
< Date: Tue, 23 Jan 2018 16:33:31 GMT
< Content-Length: 124
< Set-Cookie: d05351dd24520dce41f310fcfc112523=2808fe5497bbaeea73011198cb5cc014; path=/; HttpOnly; Secure
<
{
"description": "operation not supplied for a last_operation with instance_uuid [3aae62b1-f990-43de-89b6-25ad2c69e4e1]"
}
4673fb5
to
f05e704
Compare
|
@mhrivnak addressed both of your comments. |
| if op := r.FormValue("operation"); op != "" { | ||
| req.Operation = op | ||
| } else { | ||
| log.Warning(fmt.Sprintf("operation not supplied, relying solely on the instance_uuid [%s]", instanceUUID)) | ||
| errmsg := fmt.Sprintf("operation not supplied for a last_operation with instance_uuid [%s]", instanceUUID) |
There was a problem hiding this comment.
Would it make sense to shuffle this around to something like this, to factor out the else?
if op := r.FormValue("operation"); op == "" {
// do error stuff
return
}
req.Operation = op
...
| @@ -692,13 +694,38 @@ func (h handler) lastoperation(w http.ResponseWriter, r *http.Request, params ma | |||
| return | |||
| } | |||
|
|
|||
| // we have a binding job | |||
| if strings.Index(r.URL.Path, "/service_bindings/") > 0 { | |||
There was a problem hiding this comment.
strings.Contains might be more helpful when reading this? it is basically just a wrapper around what you are doing though so meh.
There was a problem hiding this comment.
Using strings.Contains would make the code read a little better.
| @@ -692,13 +694,38 @@ func (h handler) lastoperation(w http.ResponseWriter, r *http.Request, params ma | |||
| return | |||
| } | |||
|
|
|||
| // we have a binding job | |||
| if strings.Index(r.URL.Path, "/service_bindings/") > 0 { | |||
There was a problem hiding this comment.
Using strings.Contains would make the code read a little better.
Missing last_operation for the async binds. The service_instance method would work because our jobs are the same. But OSB platforms will want to call the last_operation binding.