provider/ec2: Use security group ids in EC2 API

provider/ec2: Use security group ids in EC2 API

The additional options available with VPC security groups
comes with the limitation that they must be referred to by
id rather than by name. To support the otherwise mostly
transparent default VPC mode some new accounts use, switch
our provider infrastructure to lookup and use ids.

This adds some additional overhead in ec2 api roundtrips,
which would be avoidable in the ec2-classic case by
detecting if the account is default-vpc enabled using
DescribeAccountAttributes and continuing to just use the
names if not. For now, that is punted on that in favour
of verifying the new codepath works in all cases.

There are a couple of work items pending for this branch.

I have run and confirmed that a subset of the live tests,
including those most related to security group handling,
pass on both classic and default vpc regions. However, a
clean run of the whole live suite on trunk is eluding me.

Two small updates to goamz are required for the local live
suite to pass. These are both approved, but need to land,
and dependencies.csv updated.

https://code.launchpad.net/~gz/juju-core/secgroup_vpc/+merge/188958

(do not edit description out of merge proposal)

[gz, 2013-10-02 23:09:37]

Please take a look.

[rog, 2013-10-03 00:08:52]

LGTM with some thoughts and suggestions below.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (left):

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#oldcode796
provider/ec2/ec2.go:796: SourceGroups: sourceGroups,
This seems suspicious to me. How is the new code accomplishing the same thing as
this? (i.e. allowing inter-instance traffic between the same group only)

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode465
provider/ec2/ec2.go:465: func (e *environ) lookupGroupInfoByName(groupName
string) (ec2.SecurityGroupInfo, error) {
The "lookup" part of this method name seems a bit superfluous to me.

I think "groupInfoByName" and "groupByName" would read quite nicely.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode502
provider/ec2/ec2.go:502: filter.Add("instance.group-id", group.Id)
I think this (and probably other stuff too) will need the dependencies.tsv file
to be updated.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode885
provider/ec2/ec2.go:885: // If it exists, its permissions are set to perms.
Ah, I see now, I think.

This could perhaps do with a comment:
// Any entries in perms without a SourceIPs entry
// will be given permissions for the named group
// only.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode953
provider/ec2/ec2.go:953: // id in source groups, using group ids only.
This comment no longer makes much sense.

How about something like:

// newPermSetForGroup returns a set of all the permissions
// in the given slice of IPPerms. It ignores the name
// and owner id in source groups; any entry with
// no source IPs will be assigned to the given group.

?

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go
File provider/ec2/live_test.go (right):

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:255: inst_ids := []instance.Id{inst0.Id(), inst1.Id()}
s/inst_ids/instIds/

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:256: ids_from_insts := func(insts []instance.Instance)
(ids []instance.Id) {
s/ids_from_insts/idsFromInsts/

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:265: all_insts, err := t.Env.AllInstances()
s/all_insts/allInsts/

[gz, 2013-10-03 14:12:27]

Please take a look.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode465
provider/ec2/ec2.go:465: func (e *environ) lookupGroupInfoByName(groupName
string) (ec2.SecurityGroupInfo, error) {
On 2013/10/03 00:08:53, rog wrote:
> The "lookup" part of this method name seems a bit superfluous to me.
> 
> I think "groupInfoByName" and "groupByName" would read quite nicely.

Done.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode502
provider/ec2/ec2.go:502: filter.Add("instance.group-id", group.Id)
On 2013/10/03 00:08:53, rog wrote:
> I think this (and probably other stuff too) will need the dependencies.tsv
file
> to be updated.

Now it's landed, have updated the dependencies.tsv file.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode885
provider/ec2/ec2.go:885: // If it exists, its permissions are set to perms.
On 2013/10/03 00:08:53, rog wrote:
> Ah, I see now, I think.
> 
> This could perhaps do with a comment:
> // Any entries in perms without a SourceIPs entry
> // will be given permissions for the named group
> // only.

That's reasonable, have added something along those lines.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/ec2.go#newcode953
provider/ec2/ec2.go:953: // id in source groups, using group ids only.
On 2013/10/03 00:08:53, rog wrote:
> This comment no longer makes much sense.
> 
> How about something like:
> 
> // newPermSetForGroup returns a set of all the permissions
> // in the given slice of IPPerms. It ignores the name
> // and owner id in source groups; any entry with
> // no source IPs will be assigned to the given group.
> 
> ?

Have added something along these lines.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go
File provider/ec2/live_test.go (right):

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:255: inst_ids := []instance.Id{inst0.Id(), inst1.Id()}
On 2013/10/03 00:08:53, rog wrote:
> s/inst_ids/instIds/

Done.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:256: ids_from_insts := func(insts []instance.Instance)
(ids []instance.Id) {
On 2013/10/03 00:08:53, rog wrote:
> s/ids_from_insts/idsFromInsts/

Done.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:265: all_insts, err := t.Env.AllInstances()
On 2013/10/03 00:08:53, rog wrote:
> s/all_insts/allInsts/

Done.

https://codereview.appspot.com/14309043/diff/1/provider/ec2/live_test.go#newc...
provider/ec2/live_test.go:435: func hasSecurityGroup(r amzec2.Instance, g
amzec2.SecurityGroup) bool {
Also renamed this from r.