Fix a bug that would make spawning new instances fail if no port/protocol is given (for rules granting access for other security groups).

5 files changed, 78 insertions, 6 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index fb1afa43a..23ac30494 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -594,18 +594,31 @@ class CloudController(object):
         g['ipPermissions'] = []
         for rule in group.rules:
             r = {}
-            r['ipProtocol'] = rule.protocol
-            r['fromPort'] = rule.from_port
-            r['toPort'] = rule.to_port
             r['groups'] = []
             r['ipRanges'] = []
             if rule.group_id:
                 source_group = db.security_group_get(context, rule.group_id)
                 r['groups'] += [{'groupName': source_group.name,
                                  'userId': source_group.project_id}]
+                if rule.protocol:
+                    r['ipProtocol'] = rule.protocol
+                    r['fromPort'] = rule.from_port
+                    r['toPort'] = rule.to_port
+                    g['ipPermissions'] += [dict(r)]
+                else:
+                    for protocol, min_port, max_port in (('icmp', -1, -1),
+                                                         ('tcp', 1, 65535),
+                                                         ('udp', 1, 65536)):
+                        r['ipProtocol'] = protocol
+                        r['fromPort'] = min_port
+                        r['toPort'] = max_port
+                        g['ipPermissions'] += [dict(r)]
             else:
+                r['ipProtocol'] = rule.protocol
+                r['fromPort'] = rule.from_port
+                r['toPort'] = rule.to_port
                 r['ipRanges'] += [{'cidrIp': rule.cidr}]
-            g['ipPermissions'] += [r]
+                g['ipPermissions'] += [r]
         return g
 
     def _rule_args_to_dict(self, context, kwargs):
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 8ea154490..5889f5fc3 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -2827,12 +2827,14 @@ def security_group_rule_get_by_security_group(context, security_group_id,
         result = session.query(models.SecurityGroupIngressRule).\
                          filter_by(deleted=can_read_deleted(context)).\
                          filter_by(parent_group_id=security_group_id).\
+                         options(joinedload_all('grantee_group')).\
                          all()
     else:
         # TODO(vish): Join to group and check for project_id
         result = session.query(models.SecurityGroupIngressRule).\
                          filter_by(deleted=False).\
                          filter_by(parent_group_id=security_group_id).\
+                         options(joinedload_all('grantee_group')).\
                          all()
     return result
 
diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py
index 7bdae0552..cc85cbd95 100644
--- a/nova/tests/api/ec2/test_cloud.py
+++ b/nova/tests/api/ec2/test_cloud.py
@@ -305,6 +305,61 @@ class CloudTestCase(test.TestCase):
                   'ip_protocol': u'tcp'}]}
         self.assertTrue(authz(self.context, group_name=sec['name'], **kwargs))
 
+    def test_describe_security_group_ingress_groups(self):
+        kwargs = {'project_id': self.context.project_id, 'name': 'test'}
+        sec1 = db.security_group_create(self.context, kwargs)
+        sec2 = db.security_group_create(self.context,
+                                       {'project_id': 'someuser',
+                                        'name': 'somegroup1'})
+        sec3 = db.security_group_create(self.context,
+                                       {'project_id': 'someuser',
+                                        'name': 'othergroup2'})
+        authz = self.cloud.authorize_security_group_ingress
+        kwargs = {'ip_permissions': [
+                  {'groups': {'1': {'user_id': u'someuser',
+                                    'group_name': u'somegroup1'}}},
+                  {'ip_protocol': 'tcp',
+                   'from_port': 80,
+                   'to_port': 80,
+                   'groups': {'1': {'user_id': u'someuser',
+                                    'group_name': u'othergroup2'}}}]}
+        self.assertTrue(authz(self.context, group_name=sec1['name'], **kwargs))
+        describe = self.cloud.describe_security_groups
+        groups = describe(self.context, group_name=['test'])
+        self.assertEquals(len(groups['securityGroupInfo']), 1)
+        actual_rules = groups['securityGroupInfo'][0]['ipPermissions']
+        self.assertEquals(len(actual_rules), 4)
+        expected_rules = [{'fromPort': -1,
+                           'groups': [{'groupName': 'somegroup1',
+                                       'userId': 'someuser'}],
+                           'ipProtocol': 'icmp',
+                           'ipRanges': [],
+                           'toPort': -1},
+                          {'fromPort': 1,
+                           'groups': [{'groupName': u'somegroup1',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': 'tcp',
+                           'ipRanges': [],
+                           'toPort': 65535},
+                          {'fromPort': 1,
+                           'groups': [{'groupName': u'somegroup1',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': 'udp',
+                           'ipRanges': [],
+                           'toPort': 65536},
+                          {'fromPort': 80,
+                           'groups': [{'groupName': u'othergroup2',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': u'tcp',
+                           'ipRanges': [],
+                           'toPort': 80}]
+        for rule in expected_rules:
+            self.assertTrue(rule in actual_rules)
+
+        db.security_group_destroy(self.context, sec3['id'])
+        db.security_group_destroy(self.context, sec2['id'])
+        db.security_group_destroy(self.context, sec1['id'])
+
     def test_revoke_security_group_ingress(self):
         kwargs = {'project_id': self.context.project_id, 'name': 'test'}
         sec = db.security_group_create(self.context, kwargs)
diff --git a/nova/tests/test_api.py b/nova/tests/test_api.py
index 526d1c490..e9f1145dd 100644
--- a/nova/tests/test_api.py
+++ b/nova/tests/test_api.py
@@ -515,7 +515,7 @@ class ApiEc2TestCase(test.TestCase):
         # be good enough for that.
         for group in rv:
             if group.name == security_group_name:
-                self.assertEquals(len(group.rules), 1)
+                self.assertEquals(len(group.rules), 3)
                 self.assertEquals(len(group.rules[0].grants), 1)
                 self.assertEquals(str(group.rules[0].grants[0]), '%s-%s' %
                                   (other_security_group_name, 'fake'))
diff --git a/nova/virt/libvirt/firewall.py b/nova/virt/libvirt/firewall.py
index 0db10c7ce..c6253511e 100644
--- a/nova/virt/libvirt/firewall.py
+++ b/nova/virt/libvirt/firewall.py
@@ -663,7 +663,9 @@ class IptablesFirewallDriver(FirewallDriver):
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'
 
-                args = ['-j ACCEPT', '-p', protocol]
+                args = ['-j ACCEPT']
+                if protocol:
+                    args += ['-p', protocol]
 
                 if protocol in ['udp', 'tcp']:
                     if rule.from_port == rule.to_port: