From: Oleg Bondarev Date: Tue, 2 Jun 2015 13:14:40 +0000 (+0300) Subject: Fix callback registry notification for security group rule X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=76b4803530c0d5f47659aa519585178cc33cba46;p=openstack-build%2Fneutron-build.git Fix callback registry notification for security group rule Some housekeeping was done in - SecurityGroupDbMixin: - create_rule_bulk() calls to create_rule(); - registry notification is in create_rule(); - separate validation for a single rule and for a group of rules - SecurityGroupServerRpcMixin: - overriden methods call to corresponding super class methods; Hopefully code is now self-documented enough Closes-Bug: #1461024 Change-Id: Ia75d7e206716bbe74aae89e4cebd0c2c40af68a8 --- diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 3caca9bbc..3e24dc13e 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -347,40 +347,36 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): security_group_rules) def create_security_group_rule_bulk_native(self, context, - security_group_rule): - r = security_group_rule['security_group_rules'] - + security_group_rules): + rules = security_group_rules['security_group_rules'] scoped_session(context.session) security_group_id = self._validate_security_group_rules( - context, security_group_rule) + context, security_group_rules) with context.session.begin(subtransactions=True): if not self.get_security_group(context, security_group_id): raise ext_sg.SecurityGroupNotFound(id=security_group_id) - self._check_for_duplicate_rules(context, r) + self._check_for_duplicate_rules(context, rules) ret = [] - for rule_dict in r: - rule = rule_dict['security_group_rule'] - tenant_id = self._get_tenant_id_for_create(context, rule) - db = SecurityGroupRule( - id=(rule.get('id') or uuidutils.generate_uuid()), - tenant_id=tenant_id, - security_group_id=rule['security_group_id'], - direction=rule['direction'], - remote_group_id=rule.get('remote_group_id'), - ethertype=rule['ethertype'], - protocol=rule['protocol'], - port_range_min=rule['port_range_min'], - port_range_max=rule['port_range_max'], - remote_ip_prefix=rule.get('remote_ip_prefix')) - context.session.add(db) - ret.append(self._make_security_group_rule_dict(db)) - return ret + for rule_dict in rules: + res_rule_dict = self._create_security_group_rule( + context, rule_dict, validate=False) + ret.append(res_rule_dict) + return ret def create_security_group_rule(self, context, security_group_rule): + return self._create_security_group_rule(context, security_group_rule) + + def _create_security_group_rule(self, context, security_group_rule, + validate=True): + if validate: + self._validate_security_group_rule(context, security_group_rule) + self._check_for_duplicate_rules_in_db(context, security_group_rule) + + rule_dict = security_group_rule['security_group_rule'] kwargs = { 'context': context, - 'security_group_rule': security_group_rule, + 'security_group_rule': rule_dict } # NOTE(armax): a callback exception here will prevent the request # from being processed. This is a hook point for backend's validation; @@ -392,15 +388,26 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): except exceptions.CallbackFailure as e: raise ext_sg.SecurityGroupConflict(reason=e) - bulk_rule = {'security_group_rules': [security_group_rule]} - sg_rule_dict = self.create_security_group_rule_bulk_native( - context, bulk_rule)[0] - - kwargs['security_group_rule'] = sg_rule_dict + tenant_id = self._get_tenant_id_for_create(context, rule_dict) + with context.session.begin(subtransactions=True): + db = SecurityGroupRule( + id=(rule_dict.get('id') or uuidutils.generate_uuid()), + tenant_id=tenant_id, + security_group_id=rule_dict['security_group_id'], + direction=rule_dict['direction'], + remote_group_id=rule_dict.get('remote_group_id'), + ethertype=rule_dict['ethertype'], + protocol=rule_dict['protocol'], + port_range_min=rule_dict['port_range_min'], + port_range_max=rule_dict['port_range_max'], + remote_ip_prefix=rule_dict.get('remote_ip_prefix')) + context.session.add(db) + res_rule_dict = self._make_security_group_rule_dict(db) + kwargs['security_group_rule'] = res_rule_dict registry.notify( resources.SECURITY_GROUP_RULE, events.AFTER_CREATE, self, **kwargs) - return sg_rule_dict + return res_rule_dict def _get_ip_proto_number(self, protocol): if protocol is None: @@ -436,45 +443,50 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): raise ext_sg.SecurityGroupMissingIcmpType( value=rule['port_range_max']) - def _validate_security_group_rules(self, context, security_group_rules): - """Check that rules being installed. - - Check that all rules belong to the same security - group, remote_group_id/security_group_id belong to the same tenant, - and rules are valid. + def _validate_single_tenant_and_group(self, security_group_rules): + """Check that all rules belong to the same security group and tenant """ - new_rules = set() - tenant_ids = set() + sg_groups = set() + tenants = set() for rule_dict in security_group_rules['security_group_rules']: - rule = rule_dict.get('security_group_rule') - new_rules.add(rule['security_group_id']) - - self._validate_port_range(rule) - self._validate_ip_prefix(rule) - - if rule['remote_ip_prefix'] and rule['remote_group_id']: - raise ext_sg.SecurityGroupRemoteGroupAndRemoteIpPrefix() - - if rule['tenant_id'] not in tenant_ids: - tenant_ids.add(rule['tenant_id']) - remote_group_id = rule.get('remote_group_id') - # Check that remote_group_id exists for tenant - if remote_group_id: - self.get_security_group(context, remote_group_id, - tenant_id=rule['tenant_id']) - if len(new_rules) > 1: - raise ext_sg.SecurityGroupNotSingleGroupRules() - security_group_id = new_rules.pop() - - # Confirm single tenant and that the tenant has permission + rule = rule_dict['security_group_rule'] + sg_groups.add(rule['security_group_id']) + if len(sg_groups) > 1: + raise ext_sg.SecurityGroupNotSingleGroupRules() + + tenants.add(rule['tenant_id']) + if len(tenants) > 1: + raise ext_sg.SecurityGroupRulesNotSingleTenant() + return sg_groups.pop() + + def _validate_security_group_rule(self, context, security_group_rule): + rule = security_group_rule['security_group_rule'] + self._validate_port_range(rule) + self._validate_ip_prefix(rule) + + if rule['remote_ip_prefix'] and rule['remote_group_id']: + raise ext_sg.SecurityGroupRemoteGroupAndRemoteIpPrefix() + + remote_group_id = rule['remote_group_id'] + # Check that remote_group_id exists for tenant + if remote_group_id: + self.get_security_group(context, remote_group_id, + tenant_id=rule['tenant_id']) + + security_group_id = rule['security_group_id'] + + # Confirm that the tenant has permission # to add rules to this security group. - if len(tenant_ids) > 1: - raise ext_sg.SecurityGroupRulesNotSingleTenant() - for tenant_id in tenant_ids: - self.get_security_group(context, security_group_id, - tenant_id=tenant_id) + self.get_security_group(context, security_group_id, + tenant_id=rule['tenant_id']) return security_group_id + def _validate_security_group_rules(self, context, security_group_rules): + sg_id = self._validate_single_tenant_and_group(security_group_rules) + for rule in security_group_rules['security_group_rules']: + self._validate_security_group_rule(context, rule) + return sg_id + def _make_security_group_rule_dict(self, security_group_rule, fields=None): res = {'id': security_group_rule['id'], 'tenant_id': security_group_rule['tenant_id'], @@ -513,23 +525,27 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): raise ext_sg.DuplicateSecurityGroupRuleInPost(rule=i) found_self = True - # Check in database if rule exists - filters = self._make_security_group_rule_filter_dict(i) - db_rules = self.get_security_group_rules(context, filters) - # Note(arosen): the call to get_security_group_rules wildcards - # values in the filter that have a value of [None]. For - # example, filters = {'remote_group_id': [None]} will return - # all security group rules regardless of their value of - # remote_group_id. Therefore it is not possible to do this - # query unless the behavior of _get_collection() - # is changed which cannot be because other methods are already - # relying on this behavior. Therefore, we do the filtering - # below to check for these corner cases. - for db_rule in db_rules: - # need to remove id from db_rule for matching - id = db_rule.pop('id') - if (i['security_group_rule'] == db_rule): - raise ext_sg.SecurityGroupRuleExists(id=id) + self._check_for_duplicate_rules_in_db(context, i) + + def _check_for_duplicate_rules_in_db(self, context, security_group_rule): + # Check in database if rule exists + filters = self._make_security_group_rule_filter_dict( + security_group_rule) + db_rules = self.get_security_group_rules(context, filters) + # Note(arosen): the call to get_security_group_rules wildcards + # values in the filter that have a value of [None]. For + # example, filters = {'remote_group_id': [None]} will return + # all security group rules regardless of their value of + # remote_group_id. Therefore it is not possible to do this + # query unless the behavior of _get_collection() + # is changed which cannot be because other methods are already + # relying on this behavior. Therefore, we do the filtering + # below to check for these corner cases. + for db_rule in db_rules: + # need to remove id from db_rule for matching + id = db_rule.pop('id') + if (security_group_rule['security_group_rule'] == db_rule): + raise ext_sg.SecurityGroupRuleExists(id=id) def _validate_ip_prefix(self, rule): """Check that a valid cidr was specified as remote_ip_prefix diff --git a/neutron/db/securitygroups_rpc_base.py b/neutron/db/securitygroups_rpc_base.py index 56c14fe5c..3e90c124b 100644 --- a/neutron/db/securitygroups_rpc_base.py +++ b/neutron/db/securitygroups_rpc_base.py @@ -69,18 +69,17 @@ class SecurityGroupServerRpcMixin(sg_db.SecurityGroupDbMixin): for device in devices] def create_security_group_rule(self, context, security_group_rule): - bulk_rule = {'security_group_rules': [security_group_rule]} - rule = self.create_security_group_rule_bulk_native(context, - bulk_rule)[0] + rule = super(SecurityGroupServerRpcMixin, + self).create_security_group_rule(context, + security_group_rule) sgids = [rule['security_group_id']] self.notifier.security_groups_rule_updated(context, sgids) return rule - def create_security_group_rule_bulk(self, context, - security_group_rule): + def create_security_group_rule_bulk(self, context, security_group_rules): rules = super(SecurityGroupServerRpcMixin, self).create_security_group_rule_bulk_native( - context, security_group_rule) + context, security_group_rules) sgids = set([r['security_group_id'] for r in rules]) self.notifier.security_groups_rule_updated(context, list(sgids)) return rules diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 0626f9ca6..db98f4622 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -62,11 +62,15 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): self.mixin.update_security_group(self.ctx, 'foo_id', secgroup) def test_create_security_group_rule_conflict(self): - with mock.patch.object(registry, "notify") as mock_notify: + with mock.patch.object(self.mixin, '_validate_security_group_rule'),\ + mock.patch.object(self.mixin, + '_check_for_duplicate_rules_in_db'),\ + mock.patch.object(registry, "notify") as mock_notify: mock_notify.side_effect = exceptions.CallbackFailure(Exception()) with testtools.ExpectedException( securitygroup.SecurityGroupConflict): - self.mixin.create_security_group_rule(self.ctx, mock.ANY) + self.mixin.create_security_group_rule( + self.ctx, mock.MagicMock()) def test_delete_security_group_rule_in_use(self): with mock.patch.object(registry, "notify") as mock_notify: