Merge lp:~gz/goamz/secgroups_on_instance into lp:goamz

[Martin Packman]

ec2: Support groups at instance and reservation

Per the 2011-12-15 wsdl description of the EC2 api, the
GroupSetType containing security group details appears
on both ReservationInfoType and RunningInstancesSetType
inside it.

This branch updates the code to parse the details at both
levels in a RunInstances or DescribeInstances response,
and updates the mock server to return the same details
at both levels.

Change is required as it seems the reservation group set
is empty, while the instance group set is not in some
circumstances.

https://codereview.appspot.com/14302043/

[Martin Packman (gz)]

Reviewers: mp+188906_code.launchpad.net,

Message:
Please take a look.

Description:
ec2: Support groups at instance and reservation

Per the 2011-12-15 wsdl description of the EC2 api, the
GroupSetType containing security group details appears
on both ReservationInfoType and RunningInstancesSetType
inside it.

This branch updates the code to parse the details at both
levels in a RunInstances or DescribeInstances response,
and updates the mock server to return the same details
at both levels.

Change is required as it seems the reservation group set
is empty, while the instance group set is not in some
circumstances.

https://code.launchpad.net/~gz/goamz/secgroups_on_instance/+merge/188906

(do not edit description out of merge proposal)

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

Affected files (+35, -22 lines):
   A [revision details]
   M ec2/ec2.go
   M ec2/ec2_test.go
   M ec2/ec2test/server.go
   M ec2/responses_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: ec2/ec2.go
=== modified file 'ec2/ec2.go'
--- ec2/ec2.go 2013-08-15 15:39:51 +0000
+++ ec2/ec2.go 2013-10-02 19:10:36 +0000
@@ -226,22 +226,23 @@
  //
  // See http://goo.gl/OCH8a for more details.
  type Instance struct {
- InstanceId string `xml:"instanceId"`
- InstanceType string `xml:"instanceType"`
- ImageId string `xml:"imageId"`
- PrivateDNSName string `xml:"privateDnsName"`
- DNSName string `xml:"dnsName"`
- IPAddress string `xml:"ipAddress"`
- PrivateIPAddress string `xml:"privateIpAddress"`
- KeyName string `xml:"keyName"`
- AMILaunchIndex int `xml:"amiLaunchIndex"`
- Hypervisor string `xml:"hypervisor"`
- VirtType string `xml:"virtualizationType"`
- Monitoring string `xml:"monitoring>state"`
- AvailZone string `xml:"placement>availabilityZone"`
- PlacementGroupName string `xml:"placement>groupName"`
- State InstanceState `xml:"instanceState"`
- Tags []Tag `xml:"tagSet>item"`
+ InstanceId string `xml:"instanceId"`
+ InstanceType string `xml:"instanceType"`
+ ImageId string `xml:"imageId"`
+ PrivateDNSName string `xml:"privateDnsName"`
+ DNSName string `xml:"dnsName"`
+ IPAddress string `xml:"ipAddress"`
+ PrivateIPAddress string `xml:"privateIpAddress"`
+ KeyName string `xml:"keyName"`
+ AMILaunchIndex int `xml:"amiLaunchIndex"`
+ Hypervisor string `xml:"hypervisor"`
+ VirtType string `xml:"virtualizationType"`
+ Monitoring string `xml:"monitoring>state"`
+ AvailZone string `xml:"placement>availabilityZone"`
...

[Roger Peppe (rogpeppe)]

On 2013/10/02 19:18:08, gz wrote:
> Please take a look.

LGTM but perhaps a test in ec2i_test.go might be appropriate too?

https://codereview.appspot.com/14302043/

[Martin Packman (gz)]

On 2013/10/02 19:59:34, rog wrote:

> LGTM but perhaps a test in ec2i_test.go might be appropriate too?

There doesn't seem to be a suitable existing test there,
TestRunAndTerminate doesn't give the instance a security group, and
TestSecurityGroups doesn't run an instance. I'm somewhat loath to add a
new test in that style, especially as we have some coverage via the
juju-core live tests.

https://codereview.appspot.com/14302043/

[Martin Packman (gz)]

*** Submitted:

ec2: Support groups at instance and reservation

Per the 2011-12-15 wsdl description of the EC2 api, the
GroupSetType containing security group details appears
on both ReservationInfoType and RunningInstancesSetType
inside it.

This branch updates the code to parse the details at both
levels in a RunInstances or DescribeInstances response,
and updates the mock server to return the same details
at both levels.

Change is required as it seems the reservation group set
is empty, while the instance group set is not in some
circumstances.

R=rog
CC=
https://codereview.appspot.com/14302043

https://codereview.appspot.com/14302043/

=== modified file 'ec2/ec2.go'
--- ec2/ec2.go	2013-08-15 15:39:51 +0000
+++ ec2/ec2.go	2013-10-02 19:18:28 +0000
@@ -226,22 +226,23 @@
 //
 // See http://goo.gl/OCH8a for more details.
 type Instance struct {
-	InstanceId         string        `xml:"instanceId"`
-	InstanceType       string        `xml:"instanceType"`
-	ImageId            string        `xml:"imageId"`
-	PrivateDNSName     string        `xml:"privateDnsName"`
-	DNSName            string        `xml:"dnsName"`
-	IPAddress          string        `xml:"ipAddress"`
-	PrivateIPAddress   string        `xml:"privateIpAddress"`
-	KeyName            string        `xml:"keyName"`
-	AMILaunchIndex     int           `xml:"amiLaunchIndex"`
-	Hypervisor         string        `xml:"hypervisor"`
-	VirtType           string        `xml:"virtualizationType"`
-	Monitoring         string        `xml:"monitoring>state"`
-	AvailZone          string        `xml:"placement>availabilityZone"`
-	PlacementGroupName string        `xml:"placement>groupName"`
-	State              InstanceState `xml:"instanceState"`
-	Tags               []Tag         `xml:"tagSet>item"`
+	InstanceId         string          `xml:"instanceId"`
+	InstanceType       string          `xml:"instanceType"`
+	ImageId            string          `xml:"imageId"`
+	PrivateDNSName     string          `xml:"privateDnsName"`
+	DNSName            string          `xml:"dnsName"`
+	IPAddress          string          `xml:"ipAddress"`
+	PrivateIPAddress   string          `xml:"privateIpAddress"`
+	KeyName            string          `xml:"keyName"`
+	AMILaunchIndex     int             `xml:"amiLaunchIndex"`
+	Hypervisor         string          `xml:"hypervisor"`
+	VirtType           string          `xml:"virtualizationType"`
+	Monitoring         string          `xml:"monitoring>state"`
+	AvailZone          string          `xml:"placement>availabilityZone"`
+	PlacementGroupName string          `xml:"placement>groupName"`
+	State              InstanceState   `xml:"instanceState"`
+	Tags               []Tag           `xml:"tagSet>item"`
+	SecurityGroups     []SecurityGroup `xml:"groupSet>item"`
 }
 
 // RunInstances starts new instances in EC2.

=== modified file 'ec2/ec2_test.go'
--- ec2/ec2_test.go	2013-08-15 15:39:51 +0000
+++ ec2/ec2_test.go	2013-10-02 19:18:28 +0000
@@ -207,11 +207,12 @@
 	c.Assert(resp.RequestId, Equals, "98e3c9a4-848c-4d6d-8e8a-b1bdEXAMPLE")
 	c.Assert(resp.Reservations, HasLen, 2)
 
+	expectedGroups := []ec2.SecurityGroup{{Name: "default", Id: "sg-67ad940e"}}
 	r0 := resp.Reservations[0]
 	c.Assert(r0.ReservationId, Equals, "r-b27e30d9")
 	c.Assert(r0.OwnerId, Equals, "999988887777")
 	c.Assert(r0.RequesterId, Equals, "854251627541")
-	c.Assert(r0.SecurityGroups, DeepEquals, []ec2.SecurityGroup{{Name: "default", Id: "sg-67ad940e"}})
+	c.Assert(r0.SecurityGroups, DeepEquals, expectedGroups)
 	c.Assert(r0.Instances, HasLen, 1)
 
 	r0i := r0.Instances[0]
@@ -221,6 +222,7 @@
 	c.Assert(r0i.PrivateIPAddress, Equals, "10.198.85.190")
 	c.Assert(r0i.IPAddress, Equals, "174.129.165.232")
 	c.Assert(r0i.AvailZone, Equals, "us-east-1b")
+	c.Assert(r0i.SecurityGroups, DeepEquals, expectedGroups)
 }
 
 func (s *S) TestDescribeInstancesExample2(c *C) {

=== modified file 'ec2/ec2test/server.go'
--- ec2/ec2test/server.go	2013-08-15 16:29:50 +0000
+++ ec2/ec2test/server.go	2013-10-02 19:18:28 +0000
@@ -664,22 +664,24 @@
 	resp.RequestId = reqId
 	for _, r := range srv.reservations {
 		var instances []ec2.Instance
+		var groups []ec2.SecurityGroup
+		for _, g := range r.groups {
+			groups = append(groups, g.ec2SecurityGroup())
+		}
 		for _, inst := range r.instances {
 			if len(insts) > 0 && !insts[inst] {
 				continue
 			}
 			ok, err := f.ok(inst)
 			if ok {
-				instances = append(instances, inst.ec2instance())
+				instance := inst.ec2instance()
+				instance.SecurityGroups = groups
+				instances = append(instances, instance)
 			} else if err != nil {
 				fatalf(400, "InvalidParameterValue", "describe instances: %v", err)
 			}
 		}
 		if len(instances) > 0 {
-			var groups []ec2.SecurityGroup
-			for _, g := range r.groups {
-				groups = append(groups, g.ec2SecurityGroup())
-			}
 			resp.Reservations = append(resp.Reservations, ec2.Reservation{
 				ReservationId:  r.id,
 				OwnerId:        ownerId,

=== modified file 'ec2/responses_test.go'
--- ec2/responses_test.go	2012-10-17 22:47:42 +0000
+++ ec2/responses_test.go	2013-10-02 19:18:28 +0000
@@ -177,6 +177,12 @@
           <clientToken/>
           <tagSet/>
           <hypervisor>xen</hypervisor>
+          <groupSet>
+            <item>
+              <groupId>sg-67ad940e</groupId>
+              <groupName>default</groupName>
+            </item>
+          </groupSet>
        </item>
       </instancesSet>
       <requesterId>854251627541</requesterId>