Merge lp:~gz/juju-core/secgroup_vpc into lp:~go-bot/juju-core/trunk

[Martin Packman]

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.

https://codereview.appspot.com/14309043/

R=rogpeppe

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://codereview.appspot.com/14309043/

[Martin Packman (gz)]

Reviewers: mp+188958_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/14309043/

Affected files (+124, -53 lines):
   A [revision details]
   M provider/ec2/ec2.go
   M provider/ec2/live_test.go

[Roger Peppe (rogpeppe)]

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#newcode255
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#newcode256
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#newcode265
provider/ec2/live_test.go:265: all_insts, err := t.Env.AllInstances()
s/all_insts/allInsts/

https://codereview.appspot.com/14309043/

[Martin Packman (gz)]

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#newcode255
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#newcode256
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#newcode265
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#newcode435
provider/ec2/live_test.go:435: func hasSecurityGroup(r amzec2.Instance,
g amzec2.SecurityGroup) bool {
Also renamed this from r.

https://codereview.appspot.com/14309043/

=== modified file 'dependencies.tsv'
--- dependencies.tsv	2013-09-15 07:36:52 +0000
+++ dependencies.tsv	2013-10-03 14:20:06 +0000
@@ -2,7 +2,7 @@
 code.google.com/p/go.net	hg	3591c18acabc99439c783463ef00e6dc277eee39	77
 labix.org/v2/mgo	bzr	[e-mail]-20130830194015-zk51jnp704tqmp7n	240
 launchpad.net/gnuflag	bzr	[e-mail]-20121003093437-zcyyw0lpvj2nifpk	12
-launchpad.net/goamz	bzr	[e-mail]-20130820120225-azobjlnr281zn72q	40
+launchpad.net/goamz	bzr	[e-mail]-20131003121033-vsyrs6xtmr1le102	42
 launchpad.net/gocheck	bzr	[e-mail]-20130302024745-6ikofwq2c03h7giu	85
 launchpad.net/golxc	bzr	[e-mail]-20130617094614-jelomk87defuetol	5
 launchpad.net/gomaasapi	bzr	tarmac-20130730082905-neqonnq9tk03ll8f	43

=== modified file 'provider/ec2/ec2.go'
--- provider/ec2/ec2.go	2013-10-02 22:22:01 +0000
+++ provider/ec2/ec2.go	2013-10-03 14:20:06 +0000
@@ -459,6 +459,49 @@
 	return e.terminateInstances(ids)
 }
 
+// groupInfoByName returns information on the security group
+// with the given name including rules and other details.
+func (e *environ) groupInfoByName(groupName string) (ec2.SecurityGroupInfo, error) {
+	// Non-default VPC does not support name-based group lookups, can
+	// use a filter by group name instead when support is needed.
+	limitToGroups := []ec2.SecurityGroup{{Name: groupName}}
+	resp, err := e.ec2().SecurityGroups(limitToGroups, nil)
+	if err != nil {
+		return ec2.SecurityGroupInfo{}, err
+	}
+	if len(resp.Groups) != 1 {
+		return ec2.SecurityGroupInfo{}, fmt.Errorf("expected one security group named %q, got %v", groupName, resp.Groups)
+	}
+	return resp.Groups[0], nil
+}
+
+// groupByName returns the security group with the given name.
+func (e *environ) groupByName(groupName string) (ec2.SecurityGroup, error) {
+	groupInfo, err := e.groupInfoByName(groupName)
+	return groupInfo.SecurityGroup, err
+}
+
+// addGroupFilter sets a limit an instance filter so only those machines
+// with the juju environment wide security group associated will be listed.
+//
+// An EC2 API call is required to resolve the group name to an id, as VPC
+// enabled accounts do not support name based filtering.
+// TODO: Detect classic accounts and just filter by name for those.
+//
+// Callers must handle InvalidGroup.NotFound errors to mean the same as no
+// matching instances.
+func (e *environ) addGroupFilter(filter *ec2.Filter) error {
+	groupName := e.jujuGroupName()
+	group, err := e.groupByName(groupName)
+	if err != nil {
+		return err
+	}
+	// EC2 should support filtering with and without the 'instance.'
+	// prefix, but only the form with seems to work with default VPC.
+	filter.Add("instance.group-id", group.Id)
+	return nil
+}
+
 // gatherInstances tries to get information on each instance
 // id whose corresponding insts slot is nil.
 // It returns environs.ErrPartialInstances if the insts
@@ -475,7 +518,13 @@
 	}
 	filter := ec2.NewFilter()
 	filter.Add("instance-state-name", "pending", "running")
-	filter.Add("group-name", e.jujuGroupName())
+	err := e.addGroupFilter(filter)
+	if err != nil {
+		if ec2ErrCode(err) == "InvalidGroup.NotFound" {
+			return environs.ErrPartialInstances
+		}
+		return err
+	}
 	filter.Add("instance-id", need...)
 	resp, err := e.ec2().Instances(nil, filter)
 	if err != nil {
@@ -538,7 +587,13 @@
 func (e *environ) AllInstances() ([]instance.Instance, error) {
 	filter := ec2.NewFilter()
 	filter.Add("instance-state-name", "pending", "running")
-	filter.Add("group-name", e.jujuGroupName())
+	err := e.addGroupFilter(filter)
+	if err != nil {
+		if ec2ErrCode(err) == "InvalidGroup.NotFound" {
+			return nil, nil
+		}
+		return nil, err
+	}
 	resp, err := e.ec2().Instances(nil, filter)
 	if err != nil {
 		return nil, err
@@ -576,9 +631,12 @@
 		return nil
 	}
 	// Give permissions for anyone to access the given ports.
+	g, err := e.groupByName(name)
+	if err != nil {
+		return err
+	}
 	ipPerms := portsToIPPerms(ports)
-	g := ec2.SecurityGroup{Name: name}
-	_, err := e.ec2().AuthorizeSecurityGroup(g, ipPerms)
+	_, err = e.ec2().AuthorizeSecurityGroup(g, ipPerms)
 	if err != nil && ec2ErrCode(err) == "InvalidPermission.Duplicate" {
 		if len(ports) == 1 {
 			return nil
@@ -608,8 +666,11 @@
 	// Revoke permissions for anyone to access the given ports.
 	// Note that ec2 allows the revocation of permissions that aren't
 	// granted, so this is naturally idempotent.
-	g := ec2.SecurityGroup{Name: name}
-	_, err := e.ec2().RevokeSecurityGroup(g, portsToIPPerms(ports))
+	g, err := e.groupByName(name)
+	if err != nil {
+		return err
+	}
+	_, err = e.ec2().RevokeSecurityGroup(g, portsToIPPerms(ports))
 	if err != nil {
 		return fmt.Errorf("cannot close ports: %v", err)
 	}
@@ -617,15 +678,11 @@
 }
 
 func (e *environ) portsInGroup(name string) (ports []instance.Port, err error) {
-	g := ec2.SecurityGroup{Name: name}
-	resp, err := e.ec2().SecurityGroups([]ec2.SecurityGroup{g}, nil)
+	group, err := e.groupInfoByName(name)
 	if err != nil {
 		return nil, err
 	}
-	if len(resp.Groups) != 1 {
-		return nil, fmt.Errorf("expected one security group, got %d", len(resp.Groups))
-	}
-	for _, p := range resp.Groups[0].IPPerms {
+	for _, p := range group.IPPerms {
 		if len(p.SourceIPs) != 1 {
 			logger.Warningf("unexpected IP permission found: %v", p)
 			continue
@@ -767,7 +824,6 @@
 // addition, a specific machine security group is created for each
 // machine, so that its firewall rules can be configured per machine.
 func (e *environ) setUpGroups(machineId string, statePort, apiPort int) ([]ec2.SecurityGroup, error) {
-	sourceGroups := []ec2.UserSecurityGroup{{Name: e.jujuGroupName()}}
 	jujuGroup, err := e.ensureGroup(e.jujuGroupName(),
 		[]ec2.IPPerm{
 			{
@@ -789,22 +845,19 @@
 				SourceIPs: []string{"0.0.0.0/0"},
 			},
 			{
-				Protocol:     "tcp",
-				FromPort:     0,
-				ToPort:       65535,
-				SourceGroups: sourceGroups,
-			},
-			{
-				Protocol:     "udp",
-				FromPort:     0,
-				ToPort:       65535,
-				SourceGroups: sourceGroups,
-			},
-			{
-				Protocol:     "icmp",
-				FromPort:     -1,
-				ToPort:       -1,
-				SourceGroups: sourceGroups,
+				Protocol: "tcp",
+				FromPort: 0,
+				ToPort:   65535,
+			},
+			{
+				Protocol: "udp",
+				FromPort: 0,
+				ToPort:   65535,
+			},
+			{
+				Protocol: "icmp",
+				FromPort: -1,
+				ToPort:   -1,
 			},
 		})
 	if err != nil {
@@ -829,6 +882,8 @@
 // ensureGroup returns the security group with name and perms.
 // If a group with name does not exist, one will be created.
 // If it exists, its permissions are set to perms.
+// Any entries in perms without SourceIPs will be granted for
+// the named group only.
 func (e *environ) ensureGroup(name string, perms []ec2.IPPerm) (g ec2.SecurityGroup, err error) {
 	ec2inst := e.ec2()
 	resp, err := ec2inst.CreateSecurityGroup(name, "juju group")
@@ -849,10 +904,10 @@
 		// description here, but if it does it's probably due
 		// to something deliberately playing games with juju,
 		// so we ignore it.
-		have = newPermSet(info.IPPerms)
 		g = info.SecurityGroup
+		have = newPermSetForGroup(info.IPPerms, g)
 	}
-	want := newPermSet(perms)
+	want := newPermSetForGroup(perms, g)
 	revoke := make(permSet)
 	for p := range have {
 		if !want[p] {
@@ -885,19 +940,20 @@
 // to access the given range of ports. Only one of groupName or ipAddr
 // should be non-empty.
 type permKey struct {
-	protocol  string
-	fromPort  int
-	toPort    int
-	groupName string
-	ipAddr    string
+	protocol string
+	fromPort int
+	toPort   int
+	groupId  string
+	ipAddr   string
 }
 
 type permSet map[permKey]bool
 
-// newPermSet returns a set of all the permissions in the
+// newPermSetForGroup returns a set of all the permissions in the
 // given slice of IPPerms. It ignores the name and owner
-// id in source groups, using group ids only.
-func newPermSet(ps []ec2.IPPerm) permSet {
+// id in source groups, and any entry with no source ips will
+// be granted for the given group only.
+func newPermSetForGroup(ps []ec2.IPPerm, group ec2.SecurityGroup) permSet {
 	m := make(permSet)
 	for _, p := range ps {
 		k := permKey{
@@ -905,13 +961,13 @@
 			fromPort: p.FromPort,
 			toPort:   p.ToPort,
 		}
-		for _, g := range p.SourceGroups {
-			k.groupName = g.Name
-			m[k] = true
-		}
-		k.groupName = ""
-		for _, ip := range p.SourceIPs {
-			k.ipAddr = ip
+		if len(p.SourceIPs) > 0 {
+			for _, ip := range p.SourceIPs {
+				k.ipAddr = ip
+				m[k] = true
+			}
+		} else {
+			k.groupId = group.Id
 			m[k] = true
 		}
 	}
@@ -932,7 +988,7 @@
 		if p.ipAddr != "" {
 			ipp.SourceIPs = []string{p.ipAddr}
 		} else {
-			ipp.SourceGroups = []ec2.UserSecurityGroup{{Name: p.groupName}}
+			ipp.SourceGroups = []ec2.UserSecurityGroup{{Id: p.groupId}}
 		}
 		ps = append(ps, ipp)
 	}

=== modified file 'provider/ec2/live_test.go'
--- provider/ec2/live_test.go	2013-09-30 19:40:06 +0000
+++ provider/ec2/live_test.go	2013-10-03 14:20:06 +0000
@@ -147,6 +147,7 @@
 }
 
 func (t *LiveTests) TestInstanceGroups(c *gc.C) {
+	t.PrepareOnce(c)
 	ec2conn := ec2.EnvironEC2(t.Env)
 
 	groups := amzec2.SecurityGroupNames(
@@ -235,20 +236,35 @@
 	for _, r := range resp.Reservations {
 		c.Assert(r.Instances, gc.HasLen, 1)
 		// each instance must be part of the general juju group.
-		msg := gc.Commentf("reservation %#v", r)
-		c.Assert(hasSecurityGroup(r, groups[0]), gc.Equals, true, msg)
 		inst := r.Instances[0]
+		msg := gc.Commentf("instance %#v", inst)
+		c.Assert(hasSecurityGroup(inst, groups[0]), gc.Equals, true, msg)
 		switch instance.Id(inst.InstanceId) {
 		case inst0.Id():
-			c.Assert(hasSecurityGroup(r, groups[1]), gc.Equals, true, msg)
-			c.Assert(hasSecurityGroup(r, groups[2]), gc.Equals, false, msg)
+			c.Assert(hasSecurityGroup(inst, groups[1]), gc.Equals, true, msg)
+			c.Assert(hasSecurityGroup(inst, groups[2]), gc.Equals, false, msg)
 		case inst1.Id():
-			c.Assert(hasSecurityGroup(r, groups[2]), gc.Equals, true, msg)
-			c.Assert(hasSecurityGroup(r, groups[1]), gc.Equals, false, msg)
+			c.Assert(hasSecurityGroup(inst, groups[2]), gc.Equals, true, msg)
+			c.Assert(hasSecurityGroup(inst, groups[1]), gc.Equals, false, msg)
 		default:
 			c.Errorf("unknown instance found: %v", inst)
 		}
 	}
+
+	// Check that listing those instances finds them using the groups
+	instIds := []instance.Id{inst0.Id(), inst1.Id()}
+	idsFromInsts := func(insts []instance.Instance) (ids []instance.Id) {
+		for _, inst := range insts {
+			ids = append(ids, inst.Id())
+		}
+		return ids
+	}
+	insts, err := t.Env.Instances(instIds)
+	c.Assert(err, gc.IsNil)
+	c.Assert(instIds, jc.SameContents, idsFromInsts(insts))
+	allInsts, err := t.Env.AllInstances()
+	c.Assert(err, gc.IsNil)
+	c.Assert(instIds, jc.SameContents, idsFromInsts(allInsts))
 }
 
 func (t *LiveTests) TestDestroy(c *gc.C) {
@@ -416,9 +432,9 @@
 	return gi.SecurityGroup
 }
 
-func hasSecurityGroup(r amzec2.Reservation, g amzec2.SecurityGroup) bool {
-	for _, rg := range r.SecurityGroups {
-		if rg.Id == g.Id {
+func hasSecurityGroup(inst amzec2.Instance, group amzec2.SecurityGroup) bool {
+	for _, instGroup := range inst.SecurityGroups {
+		if instGroup.Id == group.Id {
 			return true
 		}
 	}