Use multiple base urls for simplestreams

Use multiple base urls for simplestreams

The simplestreams image metadata lookup now takes multiple base urls, and looks at each
in turn to try and find an index file containing the requested image. The order of looking
is:
- public bucket (if openstack)
- product-streams keystone catalog entry (if openstack)
- default cloud-images.ubuntu.com url

The ec2 and openstack providers have been configured to generate the above urls. The next 
branch will use these urls and the simplestreams support to find the image id for the 
relevant image based on series, arch etc.

https://code.launchpad.net/~wallyworld/juju-core/simplestreams-multiple-indices/+merge/162919

Requires: https://code.launchpad.net/~wallyworld/juju-core/oxygen-data-parse/+merge/162308

(do not edit description out of merge proposal)

[wallyworld, 2013-05-08 02:36:34]

Please take a look.

[wallyworld, 2013-05-10 16:29:41]

Please take a look.

[wallyworld, 2013-05-16 00:31:05]

Please take a look.

[jameinel, 2013-05-16 06:47:13]

LGTM

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
generally I really prefer the:

environs.IsNotFound(err)

method. This still works, but I'm wondering if you prefer it?

[fwereade, 2013-05-16 08:59:11]

a few questions...

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go#newcode361
environs/ec2/ec2.go:361: // Add the simplestreams base URL off the public
bucket.
Why not the private bucket?

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:389: // The public bucket URL ends with
"/public-tools".
Sorry, I'm not clear why this is. Is it because of that swapping out of tools
for public-tools? If so, I'm pretty sure that's not correct -- tools
(public-tools) is for *tools*.

And... shouldn't that be a signed URL anyway?

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
On 2013/05/16 06:47:13, jameinel wrote:
> generally I really prefer the:
> 
> environs.IsNotFound(err)
> 
> method. This still works, but I'm wondering if you prefer it?

I don't think there is an environs.IsNotFound.

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
environs/openstack/local_test.go:423: c.Check(strings.HasSuffix(urls[1],
"/imagemetadata"), Equals, true)
Both unsigned..?

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider....
environs/openstack/provider.go:626: publicBucketURL, err :=
e.publicStorageUnlocked.URL("")
Again, why not private?

[jameinel, 2013-05-16 09:06:19]

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go#newcode361
environs/ec2/ec2.go:361: // Add the simplestreams base URL off the public
bucket.
On 2013/05/16 08:59:11, fwereade wrote:
> Why not the private bucket?

When I chatted about this, it is because the immediate point where the code
wants to read the index files doesn't have the credentials to read the private
bucket.

However, when we discussed it, anything that wanted to start an actual instance
needs to have the credentials for the provisioner (compute) so you would expect
that it would have access to the data store (swift/s3).

So it seems like we *should* be able to use the private bucket, I'm not sure how
well that is exposed.

Certainly at this level, you just have a bunch of URLs, and you can't just read
from a private URL (you have to go through the provider which has the auth
credentials).

So maybe just for simple unification (each place you probe is just a URL)?

[wallyworld, 2013-05-17 01:36:03]

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/ec2.go#newcode361
environs/ec2/ec2.go:361: // Add the simplestreams base URL off the public
bucket.
On 2013/05/16 08:59:11, fwereade wrote:
> Why not the private bucket?

The simplestreams stuff is all premised on being able to access an index file
relative to a base URL and then navigate to a product file relative to the same
base path. Given that the required authenticated access via a provider storage
interface doesn't really lend itself to the current URL based approach, I'd like
to leave it for now and revisit later if it comes up as an issue.

Update: we discussed this on a hangout and the current approach is acceptable.

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:389: // The public bucket URL ends with
"/public-tools".
On 2013/05/16 08:59:11, fwereade wrote:
> Sorry, I'm not clear why this is. Is it because of that swapping out of tools
> for public-tools? If so, I'm pretty sure that's not correct -- tools
> (public-tools) is for *tools*.
> 
> And... shouldn't that be a signed URL anyway?

The public-tools is an artifact of other unrelated work to prevent tests
accidentally overwriting a real public bucket. There's a bug to clean up how our
tests work.

The use of the public bucket for metadata is actually removed in a downstream
branch since it's not needed for EC2 because the metadata is all available via
simplestreams.

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
On 2013/05/16 06:47:13, jameinel wrote:
> generally I really prefer the:
> 
> environs.IsNotFound(err)
> 
> method. This still works, but I'm wondering if you prefer it?

Sadly only goose has nice error helpers like that

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
environs/openstack/local_test.go:423: c.Check(strings.HasSuffix(urls[1],
"/imagemetadata"), Equals, true)
On 2013/05/16 08:59:11, fwereade wrote:
> Both unsigned..?

Yes. But in a downstream branch, the use of the public bucket URL for EC2 is
removed.

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/provider....
environs/openstack/provider.go:626: publicBucketURL, err :=
e.publicStorageUnlocked.URL("")
On 2013/05/16 08:59:11, fwereade wrote:
> Again, why not private?

The URL based access used by simplestreams doesn't really lend itself to the
client based access to the private bucket.

[jameinel, 2013-05-17 11:42:14]

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
environs/openstack/local_test.go:423: c.Check(strings.HasSuffix(urls[1],
"/imagemetadata"), Equals, true)
> Yes. But in a downstream branch, the use of the public bucket URL for EC2 is
> removed.

Doesn't that negate the ability to inject your own images?

[wallyworld, 2013-05-17 11:53:26]

On 2013/05/17 11:42:14, jameinel wrote:
>
https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
> File environs/openstack/local_test.go (right):
> 
>
https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_tes...
> environs/openstack/local_test.go:423: c.Check(strings.HasSuffix(urls[1],
> "/imagemetadata"), Equals, true)
> > Yes. But in a downstream branch, the use of the public bucket URL for EC2 is
> > removed.
> 
> Doesn't that negate the ability to inject your own images?

The soon to be implemented default-image-id constraint (and legacy env
parameter) will allow a particular published image to be selected, so long as
the series and arch are compatible. If we want to allow an arbitary,
non-published (ie not in simplestreams) image to be used, then we will need to
rework how the simplestreams support is implemented, as it requires
unauthenticated access to files via a URL. Perhaps we could used signed URLs for
EC2.

[TheMue, 2013-05-21 16:07:22]

Only few comments.

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
On 2013/05/17 01:36:03, wallyworld wrote:
> On 2013/05/16 06:47:13, jameinel wrote:
> > generally I really prefer the:
> > 
> > environs.IsNotFound(err)
> > 
> > method. This still works, but I'm wondering if you prefer it?
> 
> Sadly only goose has nice error helpers like that

IsXyzError() looks better, yes. So it should be added to each own error type.
But in case it returns true sometimes the asserted type is needed (if it
contains more information than just a string). And then still a construct
similar to the one above is added (not in this case, as the asserted error isn't
used directly).

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:250: if resp.StatusCode != http.StatusOK
{
Ah, using the status constants is a good catch.

[wallyworld, 2013-05-21 22:01:52]

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simpl...
environs/imagemetadata/simplestreams.go:212: if _, ok :=
err.(*environs.NotFoundError); ok {
On 2013/05/21 16:07:22, TheMue wrote:
> On 2013/05/17 01:36:03, wallyworld wrote:
> > On 2013/05/16 06:47:13, jameinel wrote:
> > > generally I really prefer the:
> > > 
> > > environs.IsNotFound(err)
> > > 
> > > method. This still works, but I'm wondering if you prefer it?
> > 
> > Sadly only goose has nice error helpers like that
> 
> IsXyzError() looks better, yes. So it should be added to each own error type.
> But in case it returns true sometimes the asserted type is needed (if it
> contains more information than just a string). And then still a construct
> similar to the one above is added (not in this case, as the asserted error
isn't
> used directly).

I've added an environs.IsNotFoundError() method and replaced all occurrences of 
if _, ok := err.(*environs.NotFoundError); ok {
}
with
if environs.IsNotFoundError(err) {
}

(there were about 7 of them)
The asserted NotFoundError is not used anywhere so the simple bool method is all
that is needed.

[wallyworld, 2013-05-22 23:46:43]

*** Submitted:

Use multiple base urls for simplestreams

The simplestreams image metadata lookup now takes multiple base urls, and looks
at each
in turn to try and find an index file containing the requested image. The order
of looking
is:
- public bucket (if openstack)
- product-streams keystone catalog entry (if openstack)
- default cloud-images.ubuntu.com url

The ec2 and openstack providers have been configured to generate the above urls.
The next 
branch will use these urls and the simplestreams support to find the image id
for the 
relevant image based on series, arch etc.

R=jameinel, fwereade, TheMue
CC=
https://codereview.appspot.com/9128047