ec2: Add VPC NetworkInterface-related APIs

ec2: Add VPC NetworkInterface-related APIs

Added the following new API calls:
- CreateNetworkInterface
- DeleteNetworkInterface
- NetworkInterfaces
- AttachNetworkInterface
- DetachNetworkInterface
(and related types/responses)

Modified existing calls/types:
- SecurityGroupInfo now includes VPCId field
- Add CreateSecurityGroupVPC call that does
the same as CreateSecurityGroup, but sets
the VPCId, to create a VPC group

This enables us to handle VPC NICs with
goamz and partially handle private IP
addresses for them. Next, we'll add more
VPC-related stuff to Instances.

Tested live on EC2, extended ec2test package
as needed.

Added a couple of test helpers: createSubnet,
and deleteSubnets, used to make sure we wait
for the events to happen when running against
live EC2 servers, and we don't leave stuff
around after the tests. With these the live
tests pass and clean up properly.

https://code.launchpad.net/~dimitern/goamz/nic-api-calls/+merge/204912

Requires: https://code.launchpad.net/~dimitern/goamz/subnets-api-calls/+merge/204640

(do not edit description out of merge proposal)

[dimitern, 2014-02-05 13:39:04]

Please take a look.

[dimitern, 2014-02-05 14:08:45]

Please take a look.

[rog, 2014-02-05 15:17:35]

LGTM with some minor suggestions below.

Thanks!

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go#newcode635
ec2/ec2.go:635: // CreateSecurityGroup run a CreateSecurityGroup request in EC2,
with
s/run/runs/

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go#newcode643
ec2/ec2.go:643: // CreateSecurityGroupVPC creates a security group in EC2,
associated
s/,//

// If vpcId is empty, this call is equivalent to CreateSecurityGroup.

?

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go
File ec2/networkinterfaces.go (right):

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:80: // You can specify a primary private IP address by
setting
Why have we got both PrivateIPs and PrivateIPAddress?

Couldn't we have PrivateIPs only and avoid the question
of what happens when they're both set?

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:84: // If you don't specify a private IP address, EC2
selects one for you
// If a private IP address is not specified, EC2 selects
// one from the subnet range.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:87: // SecondaryPrivateIPsCount is the number of
secondary private IP
// When SecondaryPrivateIPsCount is non-zero,
// EC2 allocates that number of
// IP addresses from within the subnet range.
// The number of IP addresses you
// can assign to a network interface varies by instance type.

?

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:149: // You must detach the network interface before
you can delete it.
// DeleteNetworkInterface deletes the specified network interface,
// which must have been previously detached.

?

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:192: // AttachNetworkInterfaceResp is the response to a
s/ a/ an/

[dimitern, 2014-02-06 11:49:28]

Please take a look.

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go#newcode635
ec2/ec2.go:635: // CreateSecurityGroup run a CreateSecurityGroup request in EC2,
with
On 2014/02/05 15:17:35, rog wrote:
> s/run/runs/

Done.

https://codereview.appspot.com/54570048/diff/20001/ec2/ec2.go#newcode643
ec2/ec2.go:643: // CreateSecurityGroupVPC creates a security group in EC2,
associated
On 2014/02/05 15:17:35, rog wrote:
> s/,//
> 
> // If vpcId is empty, this call is equivalent to CreateSecurityGroup.
> 
> ?

Done.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go
File ec2/networkinterfaces.go (right):

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:80: // You can specify a primary private IP address by
setting
On 2014/02/05 15:17:35, rog wrote:
> Why have we got both PrivateIPs and PrivateIPAddress?
> 
> Couldn't we have PrivateIPs only and avoid the question
> of what happens when they're both set?

I tried to stay close to the AWS API, which allows both but with some
conditions. Having PrivateIPAddress and setting just that is a shortcut to
setting PrivateIPs: []ec2.PrivateIP{{Address: "..", IsPrimary: true}}. But
perhaps the slightly more verbose syntax will work well to hide the API
complexity.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:84: // If you don't specify a private IP address, EC2
selects one for you
On 2014/02/05 15:17:35, rog wrote:
> // If a private IP address is not specified, EC2 selects
> // one from the subnet range.

Done slightly differently.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:87: // SecondaryPrivateIPsCount is the number of
secondary private IP
On 2014/02/05 15:17:35, rog wrote:
> // When SecondaryPrivateIPsCount is non-zero,
> // EC2 allocates that number of
> // IP addresses from within the subnet range.
> // The number of IP addresses you
> // can assign to a network interface varies by instance type.
> 
> ?

Done.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:149: // You must detach the network interface before
you can delete it.
On 2014/02/05 15:17:35, rog wrote:
> // DeleteNetworkInterface deletes the specified network interface,
> // which must have been previously detached.
> 
> ?

Done.

https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:192: // AttachNetworkInterfaceResp is the response to a
On 2014/02/05 15:17:35, rog wrote:
> s/ a/ an/

Done.

[dimitern, 2014-02-07 12:56:27]

Please take a look.

[niemeyer, 2014-02-12 14:22:37]

LGTM

https://codereview.appspot.com/54570048/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/ec2.go#newcode637
ec2/ec2.go:637: // the provided name and description.
Would you mind to update this to be slightly more sane:

// CreateSecurityGroup creates a security group with the provided name
// and description.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go
File ec2/networkinterfaces.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:8: // Written by Gustavo Niemeyer
<gustavo.niemeyer@canonical.com>
Please drop this line.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:52: // NetworkInterface describes a network interface
for AWS VPC.
s/ AWS//

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:78: // Only the SubnetId is required, the rest are
optional.
// SubnetId is the only required field.

This already implies that the rest is optional.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:81: // PrivateIPs slice. Only one of them can be set as
primary.
// Only one provided PrivateIP may be set as primary.

It's implicit that one or more may be provided.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:93: SecondaryPrivateIPsCount int
s/IPsCount/IPCount/?  That seems to read better, and also matches the plural
form used by the underlying EC2 parameter.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:163: // interfaces to the matching ids or filtering
rules.
// NetworkInterfaces returns a list of network interfaces.
//
// If the ids or filter parameters are provided, only matching interfaces
// are returned.

https://codereview.appspot.com/54570048/diff/60001/ec2/subnets_test.go
File ec2/subnets_test.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/subnets_test.go#newcode94
ec2/subnets_test.go:94: // Subnet tests to run against either a local test
server or live on EC2.
s/ to// -- the original sentence was correct ("These tests run against...")

[dimitern, 2014-02-12 14:50:35]

*** Submitted:

ec2: Add VPC NetworkInterface-related APIs

Added the following new API calls:
- CreateNetworkInterface
- DeleteNetworkInterface
- NetworkInterfaces
- AttachNetworkInterface
- DetachNetworkInterface
(and related types/responses)

Modified existing calls/types:
- SecurityGroupInfo now includes VPCId field
- Add CreateSecurityGroupVPC call that does
the same as CreateSecurityGroup, but sets
the VPCId, to create a VPC group

This enables us to handle VPC NICs with
goamz and partially handle private IP
addresses for them. Next, we'll add more
VPC-related stuff to Instances.

Tested live on EC2, extended ec2test package
as needed.

Added a couple of test helpers: createSubnet,
and deleteSubnets, used to make sure we wait
for the events to happen when running against
live EC2 servers, and we don't leave stuff
around after the tests. With these the live
tests pass and clean up properly.

R=rog, niemeyer
CC=
https://codereview.appspot.com/54570048

https://codereview.appspot.com/54570048/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/ec2.go#newcode637
ec2/ec2.go:637: // the provided name and description.
On 2014/02/12 14:22:38, niemeyer wrote:
> Would you mind to update this to be slightly more sane:
> 
> // CreateSecurityGroup creates a security group with the provided name
> // and description.

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go
File ec2/networkinterfaces.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:8: // Written by Gustavo Niemeyer
<gustavo.niemeyer@canonical.com>
On 2014/02/12 14:22:38, niemeyer wrote:
> Please drop this line.

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:52: // NetworkInterface describes a network interface
for AWS VPC.
On 2014/02/12 14:22:38, niemeyer wrote:
> s/ AWS//

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:78: // Only the SubnetId is required, the rest are
optional.
On 2014/02/12 14:22:38, niemeyer wrote:
> // SubnetId is the only required field.
> 
> This already implies that the rest is optional.

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:81: // PrivateIPs slice. Only one of them can be set as
primary.
On 2014/02/12 14:22:38, niemeyer wrote:
> // Only one provided PrivateIP may be set as primary.
> 
> It's implicit that one or more may be provided.

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:93: SecondaryPrivateIPsCount int
On 2014/02/12 14:22:38, niemeyer wrote:
> s/IPsCount/IPCount/?  That seems to read better, and also matches the plural
> form used by the underlying EC2 parameter.

Done. Also, as agreed on IRC, I'll rename NetworkInterfaceOptions to
CreateNetworkInterface.

https://codereview.appspot.com/54570048/diff/60001/ec2/networkinterfaces.go#n...
ec2/networkinterfaces.go:163: // interfaces to the matching ids or filtering
rules.
On 2014/02/12 14:22:38, niemeyer wrote:
> // NetworkInterfaces returns a list of network interfaces.
> //
> // If the ids or filter parameters are provided, only matching interfaces
> // are returned. 

Done.

https://codereview.appspot.com/54570048/diff/60001/ec2/subnets_test.go
File ec2/subnets_test.go (right):

https://codereview.appspot.com/54570048/diff/60001/ec2/subnets_test.go#newcode94
ec2/subnets_test.go:94: // Subnet tests to run against either a local test
server or live on EC2.
On 2014/02/12 14:22:38, niemeyer wrote:
> s/ to// -- the original sentence was correct ("These tests run against...")

Done.