state/*: login to /ENVUUID/api URLs

state/*: login to /ENVUUID/api URLs

This is an evolution of:
  https://codereview.appspot.com/101760046/
  https://code.launchpad.net/~jameinel/juju-core/login-returns-env-tag/+merge/221021

Instead of having Login itself take an Environment Tag, it changes our
API so that we connect to a different URL. Instead of
"wss://address:host/" we now will try to connect to
"wss://address:host/ENVUUID/api".

The change itself involves a few variables:
 1) A new dependency "github.com/bmizerany/pat", which is a reasonably
 simple library that lets you put in patterns to your http.Mux, which
 will then transform "/:path/foo" as matching anything in ":path" and
 adding that as a Query argument instead.

 2) Using that library to expose /:environ/api and .../log, .../charms,
 .../tools. This patch itself doesn't make use of the new URLs, because
 it is not backwards compatible, and I didn't want to quite sort out how
 to do the backwards compatibility yet.

 3) We do use the environ/api one when we know the Environment UUID,
 which Login will return for us (and we will cache).

I did test that this still works against a 1.18 server (it connects to
the /ENVUUID/api address, but as that is a child of / it still works). I
also ran into something a bit surprising, but probably ok. If you
actually force the environ-uuid to be invalid in your .jenv file, the
code will try to connect, and fail with InvalidEnviron. However, our
connection code has lots of resilience built in, so we end up just
falling back to the Config fallback open, which naturally logs in as
just environUUID="", which then finds the right environment UUID and
caches it.

So this code doesn't fix all possible paths, but I think it improves the
state of the world that I'd like to land it and get back to API
versioning.

https://code.launchpad.net/~jameinel/juju-core/login-env-urls/+merge/221632

(do not edit description out of merge proposal)

[jameinel, 2014-06-01 09:43:06]

Please take a look.

[jameinel, 2014-06-01 10:15:53]

Please take a look.

[rog, 2014-06-02 10:08:18]

Looks great in general, with a few queries and suggestions.

https://codereview.appspot.com/102920048/diff/20001/environs/configstore/inte...
File environs/configstore/interface.go (right):

https://codereview.appspot.com/102920048/diff/20001/environs/configstore/inte...
environs/configstore/interface.go:25: EnvironUUID string
Why is this not an environ tag, to match the tag in api.Info ?

https://codereview.appspot.com/102920048/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/api/apiclient.go#ne...
state/api/apiclient.go:151: environUUID = envUUID
why not just use environ tag throughout?

https://codereview.appspot.com/102920048/diff/20001/state/api/export_test.go
File state/api/export_test.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/api/export_test.go#...
state/api/export_test.go:10: SetupWebsocket      = setupWebsocket
s/Setup/SetUp/
?

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:184: handleAll(mux, "/:envuuid/log",
One passing thought - we might be slightly more future proof and "obviously
right" if the path was "/environ/$UUID/..." as we wouldn't need to rely on the
fact that environ uuids can never conflict with any other path.

If you do that, then bmizerany/pat becomes pretty much redundant, because it's
almost as simple to just parse the first element of the URL path as a uuid.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:195: // pat only does "text/plain" responses.
That seems like a reason to avoid bmizerany/pat - if someone sends a method we
don't understand, it would be nice to get a standard error back. handleAll isn't
exhaustive.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:210: handleAll(mux, "/",
http.HandlerFunc(srv.apiHandler))
Can we please deprecate the behaviour that addressing any undefined url gives us
the API? It was always unintentional, and now I believe it's actively harmful.
It should be pretty simple to do - just write a handler that 404's any path
that's not /, and register it to /.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:249: // 2) On firt connect. The environment UUID is
currently
s/firt/first/

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:256: env, err := srv.state.Environment()
Rather than add an extra mongo round trip to every http request, we could store
the UUID in the server.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/common/er...
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/common/er...
state/apiserver/common/errors.go:50: ErrInvalidEnviron = stderrors.New("invalid
environment requested")
ErrUnknownEnviron = "unknown environment" ?

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/httphandl...
File state/apiserver/httphandler.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/httphandl...
state/apiserver/httphandler.go:69: if envUUID != "" {
if envUUID == "" {
    return nil
}

to save indentation?

[hduran, 2014-06-02 13:41:53]

On 2014/06/01 09:43:06, jameinel wrote:
> Please take a look.

LGTM if rog comments are addressed.

[jameinel, 2014-06-05 13:47:19]

I had written this up, but it didn't get published because I switched to git.

https://codereview.appspot.com/102920048/diff/20001/environs/configstore/inte...
File environs/configstore/interface.go (right):

https://codereview.appspot.com/102920048/diff/20001/environs/configstore/inte...
environs/configstore/interface.go:25: EnvironUUID string
On 2014/06/02 10:08:17, rog wrote:
> Why is this not an environ tag, to match the tag in api.Info ?

This is driven primarily from William's strong desire that "tags are in the API,
but don't bleed into the rest of the system".

Specifically, in the .jenv file we store "user" but and not "tag", though
api.Info does have a Tag field and not a User field.

So it generally is:
  a) raw UUID on disk and in state
  b) Tag when passed around the API structures.

I'm considering changing the URL to be /ENVTAG/api instead of /ENVUUID/api. I'm
letting that simmer a bit before I make a final decision.

https://codereview.appspot.com/102920048/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/api/apiclient.go#ne...
state/api/apiclient.go:151: environUUID = envUUID
On 2014/06/02 10:08:17, rog wrote:
> why not just use environ tag throughout?

as mentioned before, this follows the same pattern we use for User vs Tag, etc.

https://codereview.appspot.com/102920048/diff/20001/state/api/export_test.go
File state/api/export_test.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/api/export_test.go#...
state/api/export_test.go:10: SetupWebsocket      = setupWebsocket
On 2014/06/02 10:08:17, rog wrote:
> s/Setup/SetUp/
> ?

Done.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:210: handleAll(mux, "/",
http.HandlerFunc(srv.apiHandler))
On 2014/06/02 10:08:18, rog wrote:
> Can we please deprecate the behaviour that addressing any undefined url gives
us
> the API? It was always unintentional, and now I believe it's actively harmful.
> It should be pretty simple to do - just write a handler that 404's any path
> that's not /, and register it to /.

For compatibility, we still have to serve the API at /. I believe bmizerany
actually does 404 things that aren't at /, but we can write some tests to be
clear about it.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:256: env, err := srv.state.Environment()
On 2014/06/02 10:08:18, rog wrote:
> Rather than add an extra mongo round trip to every http request, we could
store
> the UUID in the server.

Done.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/common/er...
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/common/er...
state/apiserver/common/errors.go:50: ErrInvalidEnviron = stderrors.New("invalid
environment requested")
On 2014/06/02 10:08:18, rog wrote:
> ErrUnknownEnviron = "unknown environment" ?

I was actually pretty dissatisfied with the static error, so I went ahead and
turned it into a typed error of UnknownEnviron, so that we can pass around the
UUID we didn't understand, so we can report it back to the user.

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/httphandl...
File state/apiserver/httphandler.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/httphandl...
state/apiserver/httphandler.go:69: if envUUID != "" {
On 2014/06/02 10:08:18, rog wrote:
> if envUUID == "" {
>     return nil
> }
> 
> to save indentation?

Done.

[rog, 2014-06-05 14:05:04]

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/102920048/diff/20001/state/apiserver/apiserver...
state/apiserver/apiserver.go:210: handleAll(mux, "/",
http.HandlerFunc(srv.apiHandler))
On 2014/06/05 13:47:19, jameinel wrote:
> On 2014/06/02 10:08:18, rog wrote:
> > Can we please deprecate the behaviour that addressing any undefined url
gives
> us
> > the API? It was always unintentional, and now I believe it's actively
harmful.
> > It should be pretty simple to do - just write a handler that 404's any path
> > that's not /, and register it to /.
> 
> For compatibility, we still have to serve the API at /. I believe bmizerany
> actually does 404 things that aren't at /, but we can write some tests to be
> clear about it.

I haven't tried it, but from the examples in the godoc, it
looks like that's not actually the case.

For example (from the godoc):

/hello/:name/
Will match:

/hello/blake/
/hello/keith/foo