various: change API.Login() to handle EnvironTag

various: change API.Login() to handle EnvironTag

This changes a bit of Login handling to allow us to pass in an
EnvironTag which will get validated against the running environment. We
also return EnvironTag as part of the Login response, since otherwise
the existing client doesn't have a good way of finding out that
information.

It then goes around and updates a bunch of places so that we can store
the EnvironTag in our .jenv and read it back out (and pass it in).

I went with a new error message (invalid environment requested), rather
than just ErrPerm, partially because I have already validated the Login
credentials. (Since one user is intended to have access to multiple
environments, it made sense to layer it that way). It does still return
the CodeUnauthorized.

It is still valid to pass in an EnvironTag of "". Eventually we may
change how Login works under that circumstance (for Multi Environment
support), but for now this gets us closer to where we want to be.

If we get to the point that Bootstrap actually stashes our api
information it could also stash the environment tag. However, in the
short term if it just connects to the API like everything else, it will
get the environment UUID cached, which will let us be a bit more
protected from accidentally connecting to the wrong environment.

This might have implications for CI, as I believe they try to re-use
things like CACerts so that they can use the same .jenv files across
mulitple bootstraps. If so, this will start causing the client to get
rejected because the actual environment has changed. Though I think that
is the behavior that we decided we wanted.

https://code.launchpad.net/~jameinel/juju-core/login-returns-env-tag/+merge/221021

(do not edit description out of merge proposal)

[jameinel, 2014-05-27 06:48:38]

Please take a look.

[fwereade, 2014-05-27 07:27:58]

This looks good. In the absence of completed API versioning I'd actually be
happy to land this as is -- but I'd appreciate your thoughts on consolidating
the configstore and agentconfig bits...

https://codereview.appspot.com/101760046/diff/1/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/101760046/diff/1/environs/configstore/disk.go#...
environs/configstore/disk.go:144: EnvironTag: info.EnvInfo.EnvironTag,
I think we want to *store* the uuid, not the tag -- the tags really are just for
the API, and I would prefer that we not let them leak into users' worldviews.
Tags coming back from the API are fine and good, but no further.

https://codereview.appspot.com/101760046/diff/1/environs/configstore/interfac...
File environs/configstore/interface_test.go (right):

https://codereview.appspot.com/101760046/diff/1/environs/configstore/interfac...
environs/configstore/interface_test.go:51: EnvironTag: "environ-dead-beef",
this choice of "uuid" makes me unreasonably happy :)

https://codereview.appspot.com/101760046/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/101760046/diff/1/juju/api.go#newcode358
juju/api.go:358: EnvironTag: apiInfo.EnvironTag,
I won't keep saying it, but this is the place to just extract the uuid and store
it.

https://codereview.appspot.com/101760046/diff/1/juju/api.go#newcode387
juju/api.go:387: _ = newEnvironTag
?

https://codereview.appspot.com/101760046/diff/1/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/101760046/diff/1/state/apiserver/admin.go#newc...
state/apiserver/admin.go:142: // trigger an error for the Environment() call?
Heh. You could, inside state, slip in and delete the document; but Idon't think
there's a way to do it here. (Stop the mongo? ;p)

https://codereview.appspot.com/101760046/diff/1/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/1/state/apiserver/login_test.go...
state/apiserver/login_test.go:530: }
I guess we have enough tests that it accepts a missing environ tag already?

I'm wondering whether we should be fixing this stuff for the agents as well.
It's less pressing, but still.

(FWIW, I feel that all this code to connect and store changes etc etc really
should be exactly the same on agent and client -- the only interesting
difference is the extra stuff that agents need to do to change their passwords,
and the client will need the same functionality once we expose
pass-word-changing there.)

[jameinel, 2014-05-27 09:53:30]

Please take a look.

https://codereview.appspot.com/101760046/diff/1/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/101760046/diff/1/environs/configstore/disk.go#...
environs/configstore/disk.go:144: EnvironTag: info.EnvInfo.EnvironTag,
On 2014/05/27 07:27:57, fwereade wrote:
> I think we want to *store* the uuid, not the tag -- the tags really are just
for
> the API, and I would prefer that we not let them leak into users' worldviews.
> Tags coming back from the API are fine and good, but no further.

Fine with me. I wasn't sure if bare UUIDs were better to store than tags. I
realized that in the API things are tags, and in State they are IDs, but wasn't
sure about user level stuff.

Done.

I ended up sticking with the api.State object has an EnvironTag() method, and
api.Info thinks of tags, but configstore.EnvironInfo uses EnvironUUID.

https://codereview.appspot.com/101760046/diff/1/environs/configstore/interfac...
File environs/configstore/interface_test.go (right):

https://codereview.appspot.com/101760046/diff/1/environs/configstore/interfac...
environs/configstore/interface_test.go:51: EnvironTag: "environ-dead-beef",
On 2014/05/27 07:27:57, fwereade wrote:
> this choice of "uuid" makes me unreasonably happy :)

:)

https://codereview.appspot.com/101760046/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/101760046/diff/1/juju/api.go#newcode358
juju/api.go:358: EnvironTag: apiInfo.EnvironTag,
On 2014/05/27 07:27:57, fwereade wrote:
> I won't keep saying it, but this is the place to just extract the uuid and
store
> it.

Done.

https://codereview.appspot.com/101760046/diff/1/juju/api.go#newcode358
juju/api.go:358: EnvironTag: apiInfo.EnvironTag,
On 2014/05/27 07:27:57, fwereade wrote:
> I won't keep saying it, but this is the place to just extract the uuid and
store
> it.

Done.

https://codereview.appspot.com/101760046/diff/1/juju/api.go#newcode387
juju/api.go:387: _ = newEnvironTag
On 2014/05/27 07:27:57, fwereade wrote:
> ?

temporary workaround for golang not allowing unused vars while incrementally
building code. removed.

https://codereview.appspot.com/101760046/diff/1/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/101760046/diff/1/state/apiserver/admin.go#newc...
state/apiserver/admin.go:142: // trigger an error for the Environment() call?
On 2014/05/27 07:27:58, fwereade wrote:
> Heh. You could, inside state, slip in and delete the document; but Idon't
think
> there's a way to do it here. (Stop the mongo? ;p)

Yeah, it is just one of those things where you "have" to propagate errors, but I
really don't like touching the stuff without actually checking what it does.
Should this actually be returning some other error for the API? Is it sane to
just blat out whatever internal failure occured?

We don't really have concrete decisions around this sort of thing (that I can
tell).

https://codereview.appspot.com/101760046/diff/1/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/1/state/apiserver/login_test.go...
state/apiserver/login_test.go:530: }
On 2014/05/27 07:27:58, fwereade wrote:
> I guess we have enough tests that it accepts a missing environ tag already?

We do, but I'm much happier to do this sort of thing explicitly rather than
assume that there is a side effect of some other test somewhere. This is
actually tested by the LoginReturnsEnvironTag code path, but I'll make it more
explicit.

It is one of those cases where yes, params.Creds{...} will pass in EnvironTag:
"" if I don't specify it, but it actually makes the test clearer that we are
explicitly testing the case where it is not set.

> 
> I'm wondering whether we should be fixing this stuff for the agents as well.
> It's less pressing, but still.
> 
> (FWIW, I feel that all this code to connect and store changes etc etc really
> should be exactly the same on agent and client -- the only interesting
> difference is the extra stuff that agents need to do to change their
passwords,
> and the client will need the same functionality once we expose
> pass-word-changing there.)

I might as well look into it now since I'm here.
The both go via api.Open which has all the logic to accept an environment tag
and return one in the State object that is opened.
It would be interesting if agent/config.go could be unified with
environs/configstore. Would we be trying to change agents to use a .jenv
formatted file as well?

I think that is out of scope for what I'm doing, though.

I do think that agent.conf should probably come pre-seeded with the environment
UUID for all cases that we actually care about. I'm a little concerned about how
the upgrade process would do migration, though, since it isn't information that
we otherwise just have on hand.

I can do the same "upgrade on API Login" that we do for clients, or we could try
to find a way to do it via the actual upgrade logic. (Upgrade itself has an API
connection, right? so it should be able to run an Upgrade step that pulls it out
of there. I'll at least dig into it some more.)

[jameinel, 2014-05-28 05:51:56]

Please take a look.

[axw, 2014-05-29 03:59:25]

LGTM

https://codereview.appspot.com/101760046/diff/40001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/api.go#newcode271
juju/api.go:271: environUUID := ""
s/environUUID/environTag/

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go#newc...
juju/apiconn_test.go:283: // The local cache doesn't have an EnvironTag, which
the API does
I think this is testing something that is currently impossible. I suppose it
doesn't hurt to be cautious of bugs introduced in the server though.

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go
File state/api/state_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go#n...
state/api/state_test.go:105: c.Check(apistate.EnvironTag(), gc.Equals,
env.Tag())
An explicit test for Login with envtag=="" would be good here.

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go#...
state/apiserver/admin.go:138: if c.EnvironTag != "" {
Perhaps add a comment here, saying why we allow an unspecified EnvironTag, lest
someone thinks it's a bug and breaks it?

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
state/apiserver/login_test.go:519: 
func (s *loginSuite) TestLoginAcceptsEmptyEnvironTag(c *gc.C) {
...
}

?

[jameinel, 2014-06-01 09:53:31]

Thanks for your review. I've superseded this request with this one:
https://codereview.appspot.com/102920048/

However, hopefully I've managed to address/respond to all of the comments you've
made here.

https://codereview.appspot.com/101760046/diff/40001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/api.go#newcode271
juju/api.go:271: environUUID := ""
On 2014/05/29 03:59:25, axw wrote:
> s/environUUID/environTag/

Done.

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go#newc...
juju/apiconn_test.go:283: // The local cache doesn't have an EnvironTag, which
the API does
On 2014/05/29 03:59:25, axw wrote:
> I think this is testing something that is currently impossible. I suppose it
> doesn't hurt to be cautious of bugs introduced in the server though.

It actually happens in the test suite. Specifically our test suite has a lot of
places where we have no machine-0 (and thus no API host ports).

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go
File state/api/state_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go#n...
state/api/state_test.go:105: c.Check(apistate.EnvironTag(), gc.Equals,
env.Tag())
On 2014/05/29 03:59:25, axw wrote:
> An explicit test for Login with envtag=="" would be good here.

I believe I have that test in the follow up.

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go#...
state/apiserver/admin.go:138: if c.EnvironTag != "" {
On 2014/05/29 03:59:25, axw wrote:
> Perhaps add a comment here, saying why we allow an unspecified EnvironTag,
lest
> someone thinks it's a bug and breaks it?

This code is now moved, because the EnvironUUID is in the URL instead of as a
parameter to Login.

However, I added what is hopefully a good comment to "validateEnvironUUID".

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
state/apiserver/login_test.go:519: 
On 2014/05/29 03:59:25, axw wrote:
> func (s *loginSuite) TestLoginAcceptsEmptyEnvironTag(c *gc.C) {
> ...
> }
> 
> ?

If you look, "TestLoginReportsEnvironTag" explicitly passes EnvironTag:"" which
is the test you're looking for. Would you prefer a different name?

[axw, 2014-06-01 12:14:57]

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_tes...
state/apiserver/login_test.go:519: 
On 2014/06/01 09:53:31, jameinel wrote:
> On 2014/05/29 03:59:25, axw wrote:
> > func (s *loginSuite) TestLoginAcceptsEmptyEnvironTag(c *gc.C) {
> > ...
> > }
> > 
> > ?
> 
> If you look, "TestLoginReportsEnvironTag" explicitly passes EnvironTag:""
which
> is the test you're looking for. Would you prefer a different name?
> 

Sorry, don't know how I missed that. That's fine.