Merge lp://staging/~wallyworld/juju-core/simplestreams-multiple-indices into lp://staging/~juju/juju-core/trunk

Reviewers: mp+162919_code.launchpad.net,

Message:
Please take a look.

Description:
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
- 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)

Please review this at https://codereview.appspot.com/9128047/

Affected files:
   A [revision details]
   M environs/ec2/ec2.go
   M environs/ec2/export_test.go
   M environs/ec2/local_test.go
   M environs/imagemetadata/simplestreams.go
   M environs/imagemetadata/simplestreams_test.go
   M environs/openstack/export_test.go
   M environs/openstack/local_test.go
   M environs/openstack/provider.go

Please take a look.

https://codereview.appspot.com/9128047/

Please take a look.

https://codereview.appspot.com/9128047/

LGTM

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

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go#newcode212
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?

https://codereview.appspot.com/9128047/

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#newcode389
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/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go#newcode212
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_test.go
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go#newcode423
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.go#newcode626
environs/openstack/provider.go:626: publicBucketURL, err :=
e.publicStorageUnlocked.URL("")
Again, why not private?

https://codereview.appspot.com/9128047/

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)?

https://codereview.appspot.com/9128047/

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#newcode389
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/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go#newcode212
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_test.go
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go#newcode423
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.go#newcode626
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 bas...

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

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go#newcode423
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?

https://codereview.appspot.com/9128047/

On 2013/05/17 11:42:14, jameinel wrote:

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

https://codereview.appspot.com/9128047/diff/7001/environs/openstack/local_test.go#newcode423
> 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.

https://codereview.appspot.com/9128047/

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

https://codereview.appspot.com/9128047/diff/7001/environs/imagemetadata/simplestreams.go#newcode212
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.

https://codereview.appspot.com/9128047/

*** 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

https://codereview.appspot.com/9128047/