Merge "Correct checking existence of security group rule"

3 files changed, 45 insertions, 25 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index 5b7841ad5..8fa348129 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -594,17 +594,14 @@ class CloudController(object):
            defined in the given security group.
         """
         for rule in security_group.rules:
-            if 'group_id' in values:
-                if rule['group_id'] == values['group_id']:
-                    return rule['id']
-            else:
-                is_duplicate = True
-                for key in ('cidr', 'from_port', 'to_port', 'protocol'):
-                    if rule[key] != values[key]:
-                        is_duplicate = False
-                        break
-                if is_duplicate:
-                    return rule['id']
+            is_duplicate = True
+            keys = ('group_id', 'cidr', 'from_port', 'to_port', 'protocol')
+            for key in keys:
+                if rule.get(key) != values.get(key):
+                    is_duplicate = False
+                    break
+            if is_duplicate:
+                return rule['id']
         return False
 
     def revoke_security_group_ingress(self, context, group_name=None,
diff --git a/nova/api/openstack/compute/contrib/security_groups.py b/nova/api/openstack/compute/contrib/security_groups.py
index 45d72bf61..1faa0537a 100644
--- a/nova/api/openstack/compute/contrib/security_groups.py
+++ b/nova/api/openstack/compute/contrib/security_groups.py
@@ -379,17 +379,14 @@ class SecurityGroupRulesController(SecurityGroupController):
            defined in the given security group.
         """
         for rule in security_group.rules:
-            if 'group_id' in values:
-                if rule['group_id'] == values['group_id']:
-                    return True
-            else:
-                is_duplicate = True
-                for key in ('cidr', 'from_port', 'to_port', 'protocol'):
-                    if rule[key] != values[key]:
-                        is_duplicate = False
-                        break
-                if is_duplicate:
-                    return True
+            is_duplicate = True
+            keys = ('group_id', 'cidr', 'from_port', 'to_port', 'protocol')
+            for key in keys:
+                if rule.get(key) != values.get(key):
+                    is_duplicate = False
+                    break
+            if is_duplicate:
+                return True
         return False
 
     def _rule_args_to_dict(self, context, to_port=None, from_port=None,
diff --git a/nova/tests/api/openstack/compute/contrib/test_security_groups.py b/nova/tests/api/openstack/compute/contrib/test_security_groups.py
index 2acc513a9..b0b2064c5 100644
--- a/nova/tests/api/openstack/compute/contrib/test_security_groups.py
+++ b/nova/tests/api/openstack/compute/contrib/test_security_groups.py
@@ -521,7 +521,7 @@ class TestSecurityGroupRules(test.TestCase):
                           "10.2.3.124/24")
 
     def test_create_by_group_id(self):
-        rule = security_group_rule_template(group_id='1')
+        rule = security_group_rule_template(group_id=1)
 
         req = fakes.HTTPRequest.blank('/v2/fake/os-security-group-rules')
         res_dict = self.controller.create(req, {'security_group_rule': rule})
@@ -530,6 +530,23 @@ class TestSecurityGroupRules(test.TestCase):
         self.assertNotEquals(security_group_rule['id'], 0)
         self.assertEquals(security_group_rule['parent_group_id'], 2)
 
+    def test_create_by_same_group_id(self):
+        rule1 = security_group_rule_template(group_id=1, from_port=80,
+                                             to_port=80)
+        self.parent_security_group['rules'] = [security_group_rule_db(rule1)]
+
+        rule2 = security_group_rule_template(group_id=1, from_port=81,
+                                             to_port=81)
+
+        req = fakes.HTTPRequest.blank('/v2/fake/os-security-group-rules')
+        res_dict = self.controller.create(req, {'security_group_rule': rule2})
+
+        security_group_rule = res_dict['security_group_rule']
+        self.assertNotEquals(security_group_rule['id'], 0)
+        self.assertEquals(security_group_rule['parent_group_id'], 2)
+        self.assertEquals(security_group_rule['from_port'], 81)
+        self.assertEquals(security_group_rule['to_port'], 81)
+
     def test_create_by_invalid_cidr_json(self):
         rules = {
                   "security_group_rule": {
@@ -571,7 +588,7 @@ class TestSecurityGroupRules(test.TestCase):
         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create,
                           req, {'security_group_rule': rule})
 
-    def test_create_add_existing_rules(self):
+    def test_create_add_existing_rules_by_cidr(self):
         rule = security_group_rule_template(cidr='10.0.0.0/24')
 
         self.parent_security_group['rules'] = [security_group_rule_db(rule)]
@@ -580,6 +597,15 @@ class TestSecurityGroupRules(test.TestCase):
         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create,
                           req, {'security_group_rule': rule})
 
+    def test_create_add_existing_rules_by_group_id(self):
+        rule = security_group_rule_template(group_id=1)
+
+        self.parent_security_group['rules'] = [security_group_rule_db(rule)]
+
+        req = fakes.HTTPRequest.blank('/v2/fake/os-security-group-rules')
+        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create,
+                          req, {'security_group_rule': rule})
+
     def test_create_with_no_body(self):
         req = fakes.HTTPRequest.blank('/v2/fake/os-security-group-rules')
         self.assertRaises(webob.exc.HTTPUnprocessableEntity,
@@ -711,7 +737,7 @@ class TestSecurityGroupRules(test.TestCase):
         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create,
                           req, {'security_group_rule': rule})
 
-    def test_create_rule_with_same_group_parent_id(self):
+    def test_create_with_same_group_parent_id_and_group_id(self):
         rule = security_group_rule_template(group_id=2)
 
         req = fakes.HTTPRequest.blank('/v2/fake/os-security-group-rules')