Amazon Elastic Load Balancing support

Amazon Elastic Load Balancing support

This change adds support for basic operations for Amazon's Elastic Load Balancing service

https://code.launchpad.net/~flaviamissi/goamz/goamz/+merge/141371

(do not edit description out of merge proposal)

[flaviamissi, 2012-12-27 16:40:14]

Please take a look.

[flaviamissi, 2013-01-07 12:24:41]

Please take a look.

[flaviamissi, 2013-01-07 18:37:06]

Please take a look.

[niemeyer, 2013-02-28 21:39:19]

Great work, Flavia. A few initial comments:

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go
File elb/elb.go (right):

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode24
elb/elb.go:24: // The creation of a Load Balancer may differ inside EC2 and VPC.
It's not clear what this means.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode27
elb/elb.go:27: type CreateLoadBalancer struct {
I think we can short names in this package. This is the elb package, meaning
Elastic Load Balancer. Everything here is about load balancers.

I suggest s/CreateLoadBalancer/Create/ here. Will provide further suggestions on
this line below.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode36
elb/elb.go:36: // Listener to configure in Load Balancer.
// A Listener represents a process that listens for client connection
// requests, and maps a port and protocol in the load balancer,
// to a port and protocol on the backend instances.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode47
elb/elb.go:47: // Response to a CreateLoadBalance request.
// CreateResp represents a response to a Create request.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode50
elb/elb.go:50: type CreateLoadBalancerResp struct {
s/CreateLoadBalancerResp/CreateResp/

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode58
elb/elb.go:58: // Creates a Load Balancer in Amazon.
// Create creates a new load balancer.

It creates in Amazon or whoever else the endpoint represents. We have other docs
like this, but we should fix them over time.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode61
elb/elb.go:61: func (elb *ELB) CreateLoadBalancer(options *CreateLoadBalancer)
(resp *CreateLoadBalancerResp, err error) {
s/CreateLoadBalancer/Create/g

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode70
elb/elb.go:70: // Deletes a Load Balancer.
// Delete deletes the load balancer with name.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode84
elb/elb.go:84: 
// RegisterInstancesResp represents a response to a RegisterInstances request.
//
// See http://goo.gl/x9hru for more details.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode86
elb/elb.go:86: InstanceIds []string
`xml:"RegisterInstancesWithLoadBalancerResult>Instances>member>InstanceId"`
OMG.. beautiful XML element name.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode89
elb/elb.go:89: // Register N instances with a given Load Balancer.
// RegisterInstances registers into the named load balancer all
// the EC2 instances with the provided instance ids.

Note how as a convention in Go code, the first word in a method documentation is
always the method name itself.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode92
elb/elb.go:92: func (elb *ELB) RegisterInstancesWithLoadBalancer(instanceIds
[]string, lbName string) (resp *RegisterInstancesResp, err error) {
s/RegisterInstancesWithLoadBalancer/RegisterInstances/

Also, can we please have the lbName name first in the parameter list?  The thing
we're operating on generally comes first, as a common rule of thumb.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode93
elb/elb.go:93: // TODO: change params order and use ..., e.g (lbName string,
instanceIds ...string)
I suggest not doing that. This kind of call convention is useful mainly when you
will *type* the multiple arguments, such as in:

    log.Print("Hey", "there")

That'll pretty much never be the case with this package. People will have lists
of ids and so on, in the code itself, and will pretty much always be operating
them with slices. So it's actually more convenient to have them as slices in the
parameter list too.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode109
elb/elb.go:109: // Deregister N instances from a given Load Balancer.
// DeregisterInstances drops from the named load balancer all
// the EC2 instances with the provided instance ids.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode112
elb/elb.go:112: func (elb *ELB) DeregisterInstancesFromLoadBalancer(instanceIds
[]string, lbName string) (resp *SimpleResp, err error) {
s/DeregisterInstancesFromLoadBalancer/DeregisterInstances/

and inverting the parameters as mentioned above.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode113
elb/elb.go:113: // TODO: change params order and use ..., e.g (lbName string,
instanceIds ...string)
Same as above.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode133
elb/elb.go:133: type LoadBalancerDescription struct {
It's a bit fuzzy to me if this is a LoadBalancerInfo, or it's in fact the
LoadBalancer itself. Maybe we should keep LoadBalancer reserved for an object we
can use to manipulate things? Not sure.. I'll be thinking about this while this
review goes through.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode156
elb/elb.go:156: func (elb *ELB) DescribeLoadBalancers(names ...string)
(*DescribeLoadBalancerResp, error) {
Also wondering about this one in the same context above.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode185
elb/elb.go:185: type Instance struct {
Hmm.. doesn't feel like this is a proper type to have here. This is
ec2.Instance.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode189
elb/elb.go:189: type ListenerDescription struct {
Also a bit of a strange type. How is PolicyName part of a listener description
and not part of a listener? Why do we need both a listener description and a
listener?

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode226
elb/elb.go:226: type InstanceState struct {
Same comment as for the Instance type.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode236
elb/elb.go:236: func (elb *ELB) DescribeInstanceHealth(lbName string,
instanceIds ...string) (*DescribeInstanceHealthResp, error) {
Can you please apply the same comments regarding the documentation as the others
above?

Also, as described above, I think this should take instanceIds []string instead.

I'll stop the review here for now, as this is a pretty large branch. I'll come
back to it once these initial points are addressed/discussed, if that's alright.

[flaviamissi, 2013-03-08 19:04:18]

Please take a look.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go
File elb/elb.go (right):

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode24
elb/elb.go:24: // The creation of a Load Balancer may differ inside EC2 and VPC.
On 2013/02/28 21:39:19, niemeyer wrote:
> It's not clear what this means.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode27
elb/elb.go:27: type CreateLoadBalancer struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> I think we can short names in this package. This is the elb package, meaning
> Elastic Load Balancer. Everything here is about load balancers.
> 
> I suggest s/CreateLoadBalancer/Create/ here. Will provide further suggestions
on
> this line below.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode36
elb/elb.go:36: // Listener to configure in Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // A Listener represents a process that listens for client connection
> // requests, and maps a port and protocol in the load balancer,
> // to a port and protocol on the backend instances.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode47
elb/elb.go:47: // Response to a CreateLoadBalance request.
On 2013/02/28 21:39:19, niemeyer wrote:
> // CreateResp represents a response to a Create request.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode50
elb/elb.go:50: type CreateLoadBalancerResp struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBalancerResp/CreateResp/

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode58
elb/elb.go:58: // Creates a Load Balancer in Amazon.
On 2013/02/28 21:39:19, niemeyer wrote:
> // Create creates a new load balancer.
> 
> It creates in Amazon or whoever else the endpoint represents. We have other
docs
> like this, but we should fix them over time.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode61
elb/elb.go:61: func (elb *ELB) CreateLoadBalancer(options *CreateLoadBalancer)
(resp *CreateLoadBalancerResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBalancer/Create/g

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode70
elb/elb.go:70: // Deletes a Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // Delete deletes the load balancer with name.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode84
elb/elb.go:84: 
On 2013/02/28 21:39:19, niemeyer wrote:
> // RegisterInstancesResp represents a response to a RegisterInstances request.
> //
> // See http://goo.gl/x9hru for more details.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode86
elb/elb.go:86: InstanceIds []string
`xml:"RegisterInstancesWithLoadBalancerResult>Instances>member>InstanceId"`
On 2013/02/28 21:39:19, niemeyer wrote:
> OMG.. beautiful XML element name.

yeah.. :|

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode89
elb/elb.go:89: // Register N instances with a given Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // RegisterInstances registers into the named load balancer all
> // the EC2 instances with the provided instance ids.
> 
> Note how as a convention in Go code, the first word in a method documentation
is
> always the method name itself.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode92
elb/elb.go:92: func (elb *ELB) RegisterInstancesWithLoadBalancer(instanceIds
[]string, lbName string) (resp *RegisterInstancesResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/RegisterInstancesWithLoadBalancer/RegisterInstances/
> 
> Also, can we please have the lbName name first in the parameter list?  The
thing
> we're operating on generally comes first, as a common rule of thumb.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode109
elb/elb.go:109: // Deregister N instances from a given Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // DeregisterInstances drops from the named load balancer all
> // the EC2 instances with the provided instance ids. 

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode112
elb/elb.go:112: func (elb *ELB) DeregisterInstancesFromLoadBalancer(instanceIds
[]string, lbName string) (resp *SimpleResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/DeregisterInstancesFromLoadBalancer/DeregisterInstances/
> 
> and inverting the parameters as mentioned above.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode113
elb/elb.go:113: // TODO: change params order and use ..., e.g (lbName string,
instanceIds ...string)
On 2013/02/28 21:39:19, niemeyer wrote:
> Same as above.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode133
elb/elb.go:133: type LoadBalancerDescription struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> It's a bit fuzzy to me if this is a LoadBalancerInfo, or it's in fact the
> LoadBalancer itself. Maybe we should keep LoadBalancer reserved for an object
we
> can use to manipulate things? Not sure.. I'll be thinking about this while
this
> review goes through.
I share that doubt with you, I used LoadBalancerDescription to follow amazon's
api convention, although for me it is ok to use LoadBalancer.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode185
elb/elb.go:185: type Instance struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Hmm.. doesn't feel like this is a proper type to have here. This is
> ec2.Instance.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode189
elb/elb.go:189: type ListenerDescription struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Also a bit of a strange type. How is PolicyName part of a listener description
> and not part of a listener? Why do we need both a listener description and a
> listener?
Yeah, it's amazon that makes those distinctions, when you call
DescribeLoadBalancers action, instead of a list of Listeners you'll get a list
of ListenerDescriptions...

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode226
elb/elb.go:226: type InstanceState struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Same comment as for the Instance type.

The ec2.InstanceState type has different fields, with different xml element
names, so we actually need this type here... :/

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode236
elb/elb.go:236: func (elb *ELB) DescribeInstanceHealth(lbName string,
instanceIds ...string) (*DescribeInstanceHealthResp, error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> Can you please apply the same comments regarding the documentation as the
others
> above?
> 
> Also, as described above, I think this should take instanceIds []string
instead.
> 
> I'll stop the review here for now, as this is a pretty large branch. I'll come
> back to it once these initial points are addressed/discussed, if that's
alright.

Yeap!