Support public bucket url in keystone catalog

Support public bucket url in keystone catalog

To avoid the need for each and every user of an openstack deployment to have
to know the public bucket url and configure their own local system with it, this
branch supports reading the public bucket url from the keystone catalog if it
is present. Some refactoring was required to how the environ's PublicStorage 
instance was created, since now authenticated access to the server is required
in order to read the keystone catalog.

https://code.launchpad.net/~wallyworld/juju-core/use-juju-tools-endpoint/+merge/164838

Requires: https://code.launchpad.net/~wallyworld/juju-core/remove-obsolete-default-image-id/+merge/164669

(do not edit description out of merge proposal)

[wallyworld, 2013-05-21 05:38:57]

Please take a look.

[dimitern, 2013-05-21 08:21:48]

LGTM modulo the below.

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

https://codereview.appspot.com/9606046/diff/1/environs/openstack/local_test.g...
environs/openstack/local_test.go:460: config["public-bucket-url"] =
"http://127.0.0.1/public-bucket"
please set "control-bucket" here explicitly as well - it makes it easier to
follow, rather than having to go and check what makeTestConfig sets.

https://codereview.appspot.com/9606046/diff/1/environs/openstack/local_test.g...
environs/openstack/local_test.go:472: var err error
ditto

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

https://codereview.appspot.com/9606046/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:439: // No public bucket URL is specified, so we
will instead create the public bucket
wait.. isn't that a bit sneaky? public bucket are supposed to be shared, so
creating it here seems wrong - please explain why this is needed.

[wallyworld, 2013-05-22 04:43:27]

Please take a look.

[wallyworld, 2013-05-22 07:25:06]

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

https://codereview.appspot.com/9606046/diff/1/environs/openstack/local_test.g...
environs/openstack/local_test.go:460: config["public-bucket-url"] =
"http://127.0.0.1/public-bucket"
On 2013/05/21 08:21:48, dimitern wrote:
> please set "control-bucket" here explicitly as well - it makes it easier to
> follow, rather than having to go and check what makeTestConfig sets.

Hmmm. I disagree with the need for this. makeTestConfig() has been the way it is
since before this branch and it's whole reason for existence is so we don't need
to have duplicate code. I didn't write makeTestConfig(), just made use of it
here.

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

https://codereview.appspot.com/9606046/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:439: // No public bucket URL is specified, so we
will instead create the public bucket
On 2013/05/21 08:21:48, dimitern wrote:
> wait.. isn't that a bit sneaky? public bucket are supposed to be shared, so
> creating it here seems wrong - please explain why this is needed.

A shared public bucket is optional. If one is not defined, the behaviour is to
create a so-called juju-dist "public" bucket on the user's private account. This
is how the code has always been. It only appears as a change in this mp due to
come slight refactoring of the surrounding logic.

[jameinel, 2013-05-22 11:09:29]

I think there is a logic error in
 if publicBucketURL == "" || err != nil {

otherwise LGTM.

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

https://codereview.appspot.com/9606046/diff/5001/environs/openstack/provider....
environs/openstack/provider.go:429: if ecfg.publicBucket() != "" {
I realize this adds some redundancy, but you could reduce some indentation with:

if ecfg.publicBucket() == "" {
  e.publicStoragUnlocked = environs.EmptyStorage
  return e.publicStorageUnlocked
}

The size of the block inside this if seems large enough to belong in its own
helper function.

https://codereview.appspot.com/9606046/diff/5001/environs/openstack/provider....
environs/openstack/provider.go:440: if publicBucketURL == "" || err != nil {
So if we have an error, then we always use publicBucketURL even if it is empty?
That doesn't seem right.
Do you mean && err == nil ?

A helper could take publicBucketName, publicBucketURL, and e.client, which might
be easier to directly test. But it could also be over refactoring.

[wallyworld, 2013-05-22 23:37:49]

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

https://codereview.appspot.com/9606046/diff/5001/environs/openstack/provider....
environs/openstack/provider.go:440: if publicBucketURL == "" || err != nil {
On 2013/05/22 11:09:29, jameinel wrote:
> So if we have an error, then we always use publicBucketURL even if it is
empty?
> That doesn't seem right.
> Do you mean && err == nil ?
> 
> A helper could take publicBucketName, publicBucketURL, and e.client, which
might
> be easier to directly test. But it could also be over refactoring.

I've refactored the code, it's nicer now"

[wallyworld, 2013-05-23 00:05:39]

*** Submitted:

Support public bucket url in keystone catalog

To avoid the need for each and every user of an openstack deployment to have
to know the public bucket url and configure their own local system with it, this
branch supports reading the public bucket url from the keystone catalog if it
is present. Some refactoring was required to how the environ's PublicStorage 
instance was created, since now authenticated access to the server is required
in order to read the keystone catalog.

R=dimitern, jameinel
CC=
https://codereview.appspot.com/9606046