Possible CI affecting change

[John Meinel]

...


> To reiterate a little of what I mentioned online:
>
> I think the latter is a good thing to do, and this change seems reasonable
> to do that. For the former, I think that a better approach might be to
> encode the environment UUID into the URL used to attach to the API.
> That can be entirely orthogonal to all the current API handling, and
> easily gets us environment-specific access to other URLs served by
> the API, such as local charm uploading and logs without any other
> change to the protocols.
>
> Does that sound reasonable as an eventual aim? It means that we'd be
> explicitly treating the UUID in the login request as *verification* info,
> not for *selection* of environments, but since there's only one environment
> currently, you won't be able to tell :-)
>
>
So I started looking into this, and it isn't terribly hard to implement
caveat one thing that I'll describe in a bit.
It is nice that we end up with the full set of resources at a given
location (/UUID/log, /UUID/charms, /UUID/api, etc).
It also will probably work much better if we have the medium term
proxy/router logic, as it can inspect the initial GET request, rather than
having to inspect the actual traffic for Login. So all that looks good.

The main problem I'm having now, is figuring out how to get the error back
out. I thought we could do an HTTP 404 with a nicely formatted JSON body.
However, we are using a 3rd party package to do the actual websocket logic.
(go.net/websocket) And the problem there is this line:
        if resp.StatusCode != 101 {
                return ErrBadStatus
        }

So if the GET + Upgrade request it sends gets *anything* other than a 101
Switching Protocols, it just says "ErrBadStatus" and suppresses all further
context. (hmm, sounds like some error suppression we've been discussing
elsewhere.)
At which point, we have know way to find out "was this a 404 for a bad
location", "was this a 401 and I need to pass auth credentials", "was this
a 200 OK and the server just doesn't support websockets", etc.

Since go.net/websocket is doing all of the 'setup an HTTP request and read
the response' in one chunk of code, I don't have a way to intercept
anything. I *could* do the tls.Dial myself, wrap it in my own code around
the bytes-on-the-wire to intercept the HTTP response and actually do
something with it, but that seems ugly and pretty fragile anyway.

After that point, our only other round trip to the server is Login. I
*could* have the server upon connecting immediately put an Error response
onto the RPC stream, but our RPC layer expects everything to be a request +
response. So it would probably read it, and then AFAICT on the logic
Conn.loop will end up in handleResponse, which will say "I don't have a
request for this id", it will read the body into a nil interface, and the
only error we get is from json.Unmarshal.

Even if I fix that up, there is no place in NewClient() && client.Start()
to return an error, so we are again stuck with "the first request has to
fail for us to notice anything is wrong".

I don't really like passing EnvironTag into Login if we are going to be
passing it into the URL. I'd like to validate the environ requested as
early in the stack as possible, but it looks like I just need to pass down
some sort of "you have a pending error" for whatever the next request is.

I *could* rpc.Conn.Serve(srvErrRoot), that just implements an alternative
Admin.Login interface that just returns the env UUID error.

Does this seem close to sane? I'd really rather api.Open could return the
rich error "invalid environment requested" rather than having to wait until
Login to do that. It does happen that all of our callers to api.Open
(outside of the test suite) do have Open call Login for them. So it is
functionally similar, but feels implementation-wise clumsy.

I also don't think I can just patch go.net/websocket because all of its
errors are singletons (ErrBadStatus = &ProtocolError{"bad status"}) so if
anyone was checking their error responses, their code would have to be:
 if err == websocket.ErrBadStatus {
 }
And there isn't a way to turn err into a contextual error and have that
continue to work.

This is certainly a case where annotation rather than suppressing the
underlying error would help, because yes the Websocket failed to start, but
there can be an underlying cause that I can actually do something useful
with. (I can't fix the environ UUID being wrong, but I can at least report
to the user why it failed, reporting to the user that 'juju status' failed
because we couldn't create a websocket isn't very helpful for debugging.)

John
=:->
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20140601/5253aba2/attachment.html>