Upload tools API

Upload tools API

An API is provided to allow clients to upload
tools. This is used by the upgrade command and
removes the need for it to have to create an
environment and upload directly to storage. It 
also allows us to switch to non cloud storage.

A lot of the changes are due to code refactoring.
The implementation uses an authenticated HTTP
endpoint which shares code with the charm upload
API. So common code was extracted and shared.

The sync tools code was refactored to split up
the building and uploading of tools. This allows
the building part to be stubbed out for testing.
A followup branch can rework the bootstrap code
to take advantage of this.

https://code.launchpad.net/~wallyworld/juju-core/upload-tools-api/+merge/210764

(do not edit description out of merge proposal)

[wallyworld, 2014-03-13 09:59:10]

Please take a look.

[wallyworld, 2014-03-13 10:42:18]

Please take a look.

[jameinel, 2014-03-13 13:34:14]

I really like having the tools get uploaded via a POST rather than directly to
storage. I had a few concerns, but I'd like to see this progress.

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:268: uploaded, err =
context.uploadTools1dot17(builtTools, series...)
Shouldn't this be 1dot16 ? UploadTools didn't exist in 1.16 either, but maybe we
never got to this area of the code then?

I would normally say we wouldn't need explicit 1.17 but we've been asked to
preserve the ability to upgrade away from 1.17.3, so this is nice to have.

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:341:
envtesting.MustUploadFakeToolsVersions(s.Conn.Environ.Storage(), versions...)
Couldn't this actually use the UploadTools API we just created?

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:207: func cloneToolsForSeries(toolsInfo *BuiltTools,
series ...string) error {
Since we have simplestreams to figure out this information, is it possible that
we don't actually need to create multiple files, but can just point them all to
the same .tgz ?
(maybe we parse the filename somewhere still.)

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:329: if err := cloneToolsForSeries(builtTools,
fakeSeries...); err != nil {
Given that this is now a public function, shouldn't there be an associated test
being added in environs/sync/sync_test.go ?

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

https://codereview.appspot.com/75330043/diff/20001/state/api/client.go#newcod...
state/api/client.go:587: // API server is 1.17 or older, so tools upload
// API server is older than 1.17.5, so tools upload ...

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:156: charmHandler.errorSender = charmHandler
This seems really strange to make errorSender point to itself.
If they implement the interface, why do we need the extra pointer?

Perhaps that code could be in a:
commonErrorSender object instead?

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/httphandle...
state/apiserver/httphandler.go:25: errorSender
It might be bad go form to embed an interface in a struct. (You collect structs
into structs, and combine interfaces into interfaces, but mixing them is
probably not great.)

I'm not positive about it, but certainly having to do:

foo.specialInterface = foo

doesn't look right to me.

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/httphandle...
state/apiserver/httphandler.go:59: return err
I realize this isn't your code, but it seems like we could do the "Is this a
User" check before we do the "is the password correct" check:

eg:
+ _, _, err = names.ParseTag(tagPass[0], names.UserTagKind)
+ if err != nil {
+ return common.ErrBadCreds
+ }
+ entity, err := checkCreds(h.state, params.Creds{
+ AuthTag: tagPass[0],
+ Password: tagPass[1],
+ })
+ if err != nil {
+ return err
+ }

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/tools.go#n...
state/apiserver/tools.go:102: tmpDir, err := ioutil.TempDir("", "uploadtools-")
bikeshed: "juju-upload-tools-" ?

[fwereade, 2014-03-13 22:22:25]

This looks really nice. If you can address my/jam's comments to the satisfaction
of a reviewer in a compatible timezone I'd be happy to see it land.

(I forget precisely what magic Sync does wrt combining simplestreams data from
different sources, but so long as that magic still exists I'm happy)

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:223: type upgradeContext struct {
I may be flattering myself, but I *think* I wanted to call this a context
originally. /smug

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:341:
envtesting.MustUploadFakeToolsVersions(s.Conn.Environ.Storage(), versions...)
On 2014/03/13 13:34:15, jameinel wrote:
> Couldn't this actually use the UploadTools API we just created?

I can see that there's not much incentive to bring that in as a middleman,
though.

Can we at least use the non-Must funcs that take a *C instead of panicking,
though?

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:207: func cloneToolsForSeries(toolsInfo *BuiltTools,
series ...string) error {
On 2014/03/13 13:34:15, jameinel wrote:
> Since we have simplestreams to figure out this information, is it possible
that
> we don't actually need to create multiple files, but can just point them all
to
> the same .tgz ?
> (maybe we parse the filename somewhere still.)

+1000 -- the repeated uploads of the same bits are far from the worst aspect of
upload-tools, but they still suck.

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:255: // BuiltTools contains metadata about a tools tarball
resulting from
s/about/for/?

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:329: if err := cloneToolsForSeries(builtTools,
fakeSeries...); err != nil {
On 2014/03/13 13:34:15, jameinel wrote:
> Given that this is now a public function, shouldn't there be an associated
test
> being added in environs/sync/sync_test.go ?

+1

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

https://codereview.appspot.com/75330043/diff/20001/state/api/client.go#newcod...
state/api/client.go:555: fakeSeries ...string) (tools *tools.Tools, err error) {
I find this really hard to read... can you put up with something more like:

func (receiver) Method(
    arg, arg, arg, arg,
) (
    result, result,
) {
    ...

...with additional linebreaks as seems suitable?

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:156: charmHandler.errorSender = charmHandler
On 2014/03/13 13:34:15, jameinel wrote:
> This seems really strange to make errorSender point to itself.
> If they implement the interface, why do we need the extra pointer?
> 
> Perhaps that code could be in a:
> commonErrorSender object instead?

Yeah, I'm having some difficulty here too. Maybe just needs a comment somewhere
explaining why it's necessary?

[wallyworld, 2014-03-13 22:48:39]

Please take a look.

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:268: uploaded, err =
context.uploadTools1dot17(builtTools, series...)
On 2014/03/13 13:34:15, jameinel wrote:
> Shouldn't this be 1dot16 ? UploadTools didn't exist in 1.16 either, but maybe
we
> never got to this area of the code then?
> 
> I would normally say we wouldn't need explicit 1.17 but we've been asked to
> preserve the ability to upgrade away from 1.17.3, so this is nice to have.

Yes, the 1.16 code path is different. This 1.17 bit only exists so we can
upgrade from 1.17. Once 1.18 ships, we can delete a bunch of code in here. It's
all a bit messy because there's a few APIs involved and different ones were in
1.17 vs 1.16 vs 1.18

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/75330043/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:341:
envtesting.MustUploadFakeToolsVersions(s.Conn.Environ.Storage(), versions...)
On 2014/03/13 22:22:26, fwereade wrote:
> On 2014/03/13 13:34:15, jameinel wrote:
> > Couldn't this actually use the UploadTools API we just created?
> 
> I can see that there's not much incentive to bring that in as a middleman,
> though.
> 
> Can we at least use the non-Must funcs that take a *C instead of panicking,
> though?

I used AssertUploadFakeToolsVersions but had to make that method upload metadata
to be compatible.

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:207: func cloneToolsForSeries(toolsInfo *BuiltTools,
series ...string) error {
On 2014/03/13 13:34:15, jameinel wrote:
> Since we have simplestreams to figure out this information, is it possible
that
> we don't actually need to create multiple files, but can just point them all
to
> the same .tgz ?
> (maybe we parse the filename somewhere still.)

Yes, that's the plan. I tried doing that but ran into issues with test failures
and wanted to try and get this landed. It's not a regression because that's how
we've always done it but it will be fixed in a followup.

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:207: func cloneToolsForSeries(toolsInfo *BuiltTools,
series ...string) error {
On 2014/03/13 22:22:26, fwereade wrote:
> On 2014/03/13 13:34:15, jameinel wrote:
> > Since we have simplestreams to figure out this information, is it possible
> that
> > we don't actually need to create multiple files, but can just point them all
> to
> > the same .tgz ?
> > (maybe we parse the filename somewhere still.)
> 
> +1000 -- the repeated uploads of the same bits are far from the worst aspect
of
> upload-tools, but they still suck.

Agreed. But it will have to be in a follow up.

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:255: // BuiltTools contains metadata about a tools tarball
resulting from
On 2014/03/13 22:22:26, fwereade wrote:
> s/about/for/?

Done.

https://codereview.appspot.com/75330043/diff/20001/environs/sync/sync.go#newc...
environs/sync/sync.go:329: if err := cloneToolsForSeries(builtTools,
fakeSeries...); err != nil {
On 2014/03/13 13:34:15, jameinel wrote:
> Given that this is now a public function, shouldn't there be an associated
test
> being added in environs/sync/sync_test.go ?

Done.

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

https://codereview.appspot.com/75330043/diff/20001/state/api/client.go#newcod...
state/api/client.go:555: fakeSeries ...string) (tools *tools.Tools, err error) {
On 2014/03/13 22:22:26, fwereade wrote:
> I find this really hard to read... can you put up with something more like:
> 
> func (receiver) Method(
>     arg, arg, arg, arg,
> ) (
>     result, result,
> ) {
>     ...
> 
> ...with additional linebreaks as seems suitable?

Done.

https://codereview.appspot.com/75330043/diff/20001/state/api/client.go#newcod...
state/api/client.go:587: // API server is 1.17 or older, so tools upload
On 2014/03/13 13:34:15, jameinel wrote:
> // API server is older than 1.17.5, so tools upload ...

Done.

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:156: charmHandler.errorSender = charmHandler
On 2014/03/13 13:34:15, jameinel wrote:
> This seems really strange to make errorSender point to itself.
> If they implement the interface, why do we need the extra pointer?
> 
> Perhaps that code could be in a:
> commonErrorSender object instead?

The reason is that the common base struct, httpHandler, uses sendError. Go only
has static linking and doesn't support abstract methods so the only way to give
httpHandler access to the specific sendError for the struct in which it is
contained is to use an embedded interface. The charm and tools handlers
implement sendError differently so it can't be done as a common struct. I can't
recall where, but we've use a similar pattern elsewhere. It seems to work ok.

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:156: charmHandler.errorSender = charmHandler
On 2014/03/13 22:22:26, fwereade wrote:
> On 2014/03/13 13:34:15, jameinel wrote:
> > This seems really strange to make errorSender point to itself.
> > If they implement the interface, why do we need the extra pointer?
> > 
> > Perhaps that code could be in a:
> > commonErrorSender object instead?
> 
> Yeah, I'm having some difficulty here too. Maybe just needs a comment
somewhere
> explaining why it's necessary?

I added a comment in the httpHandler next to the embedded interface, plus I'll
add one here.

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/httphandle...
state/apiserver/httphandler.go:25: errorSender
On 2014/03/13 13:34:15, jameinel wrote:
> It might be bad go form to embed an interface in a struct. (You collect
structs
> into structs, and combine interfaces into interfaces, but mixing them is
> probably not great.)
> 
> I'm not positive about it, but certainly having to do:
> 
> foo.specialInterface = foo
> 
> doesn't look right to me.

Embedded interfaces are fine AFAIK. We've used them elsewhere in Juju but I
can't recall where, just remember seeing them. eg we embedded error in various
places. It's a convenient way to plug in behaviour into a struct.

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/httphandle...
state/apiserver/httphandler.go:59: return err
On 2014/03/13 13:34:15, jameinel wrote:
> I realize this isn't your code, but it seems like we could do the "Is this a
> User" check before we do the "is the password correct" check:
> 
> eg:
> + _, _, err = names.ParseTag(tagPass[0], names.UserTagKind)
> + if err != nil {
> + return common.ErrBadCreds
> + }
> + entity, err := checkCreds(h.state, params.Creds{
> + AuthTag: tagPass[0],
> + Password: tagPass[1],
> + })
> + if err != nil {
> + return err
> + }

Done.

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/tools.go#n...
state/apiserver/tools.go:102: tmpDir, err := ioutil.TempDir("", "uploadtools-")
On 2014/03/13 13:34:15, jameinel wrote:
> bikeshed: "juju-upload-tools-" ?

Done.

[fwereade, 2014-03-13 22:55:53]

LGTM, thanks.

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

https://codereview.appspot.com/75330043/diff/20001/state/apiserver/httphandle...
state/apiserver/httphandler.go:25: errorSender
On 2014/03/13 22:48:40, wallyworld wrote:
> On 2014/03/13 13:34:15, jameinel wrote:
> > It might be bad go form to embed an interface in a struct. (You collect
> structs
> > into structs, and combine interfaces into interfaces, but mixing them is
> > probably not great.)
> > 
> > I'm not positive about it, but certainly having to do:
> > 
> > foo.specialInterface = foo
> > 
> > doesn't look right to me.
> 
> Embedded interfaces are fine AFAIK. We've used them elsewhere in Juju but I
> can't recall where, just remember seeing them. eg we embedded error in various
> places. It's a convenient way to plug in behaviour into a struct.

Yeah, I think it's fine fwiw.

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

https://codereview.appspot.com/75330043/diff/40001/state/apiserver/apiserver....
state/apiserver/apiserver.go:157: charmHandler.httpHandler.errorSender =
charmHandler
that `.httpHandler` makes everything clear enough that the comment is almost
redundant -- I didn't quite follow your live explanation but it's clear now --
but I'm reluctant to actually drop it. It's good as it is.