From ba21072a43183388e53f47bcdac074cb6246ed83 Mon Sep 17 00:00:00 2001 From: MotoKen Date: Tue, 31 Jan 2012 15:35:02 +0800 Subject: Correct checking existence of security group rule Fixes bug #900031 Change-Id: I4194610ce53d1c74bd99b6878339da6e0b6a3a73 --- nova/api/ec2/cloud.py | 19 ++++++------- .../openstack/compute/contrib/security_groups.py | 19 ++++++------- .../compute/contrib/test_security_groups.py | 32 ++++++++++++++++++++-- 3 files changed, 45 insertions(+), 25 deletions(-) (limited to 'nova') diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index 4c5e07f9f..df238de7e 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') -- cgit