environs/ec2: specify public-bucket-region

environs/ec2: specify public-bucket-region

Fixes LP #1083017

Our public bucket, juju-dist, lives in the us-east-1 region and can only be accessed via that endpoint. If you attempt to do so via another endpoint you get an unhelpful 301 error with an xml blob telling you how to request the bucket. The solution is to add a configuration option, with sensible default, to ec2/config.go to specify the region of the public bucket independantly of the private bucket.

https://code.launchpad.net/~dave-cheney/juju-core/052-environs-ec2-always-request-public-tools-from-us-east-1-II/+merge/136299

Requires: https://code.launchpad.net/~dave-cheney/juju-core/056-environs-ec2-tests-pass-on-quantal/+merge/136297

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-11-27 00:07:39]

Please take a look.

[dave_cheney.net, 2012-11-27 03:27:20]

Please take a look.

[jameinel, 2012-11-27 04:22:51]

LGTM

I'm still wondering if it would be better to put the tools in each region, and
then default the public-bucket-region to the same value as the other region. As
an immediate "let's allow us to work on non us-east-1" this seems ok.

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config.go#newco...
environs/ec2/config.go:84: }
Should this be "invalid" vs "unknown"? Just a thought. invalid sounds like it is
improperly formatted (has a quote, non-ascii, etc) vs unknown meaning it isn't
recognized. I don't think it is a big deal, though. Pick what you like.

[dave_cheney.net, 2012-11-27 04:28:55]

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config.go#newco...
environs/ec2/config.go:84: }
On 2012/11/27 04:22:51, john.meinel wrote:
> Should this be "invalid" vs "unknown"? Just a thought. invalid sounds like it
is
> improperly formatted (has a quote, non-ascii, etc) vs unknown meaning it isn't
> recognized. I don't think it is a big deal, though. Pick what you like.

Good point. It is consistent with the previous region check. Invalid in the
sense means it's not listed in the set of known regions in goamz/aws/aws.go. We
dont' let people make up their own regions because that would be chaos

[niemeyer, 2012-11-27 11:40:46]

Looks very nice, but it's lacking tests.

Something somewhere somehow must verify that the public bucket is actually
accessed in the given region.

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config_test.go
File environs/ec2/config_test.go (right):

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:41: pbucketregion string
s/r/R/ as the rest

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

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:33: "public-bucket-region": "test",
What's the effect of this change? There's nothing in the test verifying this.
This sounds similar to the previous CL where there were changes here too.

[dave_cheney.net, 2012-11-27 11:50:02]

Please take a look.

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config_test.go
File environs/ec2/config_test.go (right):

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:41: pbucketregion string
On 2012/11/27 11:40:47, niemeyer wrote:
> s/r/R/ as the rest

Done.

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

https://codereview.appspot.com/6854098/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:33: "public-bucket-region": "test",
On 2012/11/27 11:40:47, niemeyer wrote:
> What's the effect of this change? There's nothing in the test verifying this.
> This sounds similar to the previous CL where there were changes here too.

This is required. If it is not here then public-bucket-region will take the
schema default and try to access the real juju-dist bucket and the test will
fail.

[niemeyer, 2012-11-27 13:05:00]

Sure, but we still need to test the *intention* of the change. Just saying "if I
take this off it blows up" is not the same as "when I put it in the given region
is used for the public bucket".

We should talk about this live somewhere. That same issue is coming up in
different CLs.

[dave_cheney.net, 2012-11-30 01:22:12]

Please take a look.

[dave_cheney.net, 2012-11-30 01:22:38]

On 2012/11/27 13:05:00, niemeyer wrote:
> Sure, but we still need to test the *intention* of the change. Just saying "if
I
> take this off it blows up" is not the same as "when I put it in the given
region
> is used for the public bucket".
> 
> We should talk about this live somewhere. That same issue is coming up in
> different CLs.

Thank you for your patience. Please take a look.

[niemeyer, 2012-11-30 17:13:47]

LGTM, thanks!

[dave_cheney.net, 2012-11-30 19:10:36]

*** Submitted:

environs/ec2: specify public-bucket-region

Fixes LP #1083017

Our public bucket, juju-dist, lives in the us-east-1 region and can only be
accessed via that endpoint. If you attempt to do so via another endpoint you get
an unhelpful 301 error with an xml blob telling you how to request the bucket.
The solution is to add a configuration option, with sensible default, to
ec2/config.go to specify the region of the public bucket independantly of the
private bucket.

R=jameinel, niemeyer
CC=
https://codereview.appspot.com/6854098