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