diff --git a/neutron/cmd/upgrade_checks/checks.py b/neutron/cmd/upgrade_checks/checks.py index 167db37fbbf..dfec070e14d 100644 --- a/neutron/cmd/upgrade_checks/checks.py +++ b/neutron/cmd/upgrade_checks/checks.py @@ -40,8 +40,10 @@ from neutron.db.models import l3 as l3_models from neutron.db.models import l3ha as l3ha_models from neutron.db.models.plugins.ml2 import vlanallocation from neutron.db.models import segment +from neutron.db.models import tag as tag_model from neutron.db import models_v2 from neutron.db.qos import models as qos_models +from neutron.extensions import tagging from neutron.objects import ports as port_obj from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import impl_idl_ovn from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client @@ -180,6 +182,18 @@ def get_duplicated_ha_networks_per_project(): return query.all() +def is_tags_limit_reached_for_any_resource(): + """Return True if any resource has more tags then defined limit.""" + ctx = context.get_admin_context() + with db_api.CONTEXT_READER.using(ctx): + query = ctx.session.query( + tag_model.Tag, + func.count(tag_model.Tag.tag)) + query = query.group_by(tag_model.Tag.standard_attr_id) + query = query.having(func.count() > tagging.MAX_TAGS_COUNT) + return bool(query.one_or_none()) + + def get_ovn_client(): global _OVN_CLIENT if _OVN_CLIENT is None: @@ -227,6 +241,8 @@ class CoreChecks(base.BaseChecks): self.ml2_ovs_igmp_flood_check), (_('Floating IP Port forwarding and OVN L3 plugin configuration'), self.ovn_port_forwarding_configuration_check), + (_('Existing tags exceeds limit per resource'), + self.tags_over_limit_check) ] @staticmethod @@ -620,3 +636,25 @@ class CoreChecks(base.BaseChecks): 'can not be used with ML2/OVN backend, distributed ' 'floating IPs and provider network type(s) used as ' 'tenant networks.')) + + @staticmethod + def tags_over_limit_check(checker): + + if not cfg.CONF.database.connection: + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _("Database connection string is not set. Check for " + "extra_dhcp_opts can't be done.")) + if is_tags_limit_reached_for_any_resource(): + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _('Some resources have already more than %(limit)d tags ' + 'created. There is a limit of %(limit)d tags per ' + 'resource and because of that limit creation of new ' + 'tags for the resources that exceed the limit will ' + 'not be possible until some of the tags are removed.' % + {'limit': tagging.MAX_TAGS_COUNT})) + return upgradecheck.Result( + upgradecheck.Code.SUCCESS, + _('Number of tags for each resource is below the limit of %d. ' % + tagging.MAX_TAGS_COUNT)) diff --git a/neutron/extensions/tagging.py b/neutron/extensions/tagging.py index 4a8583f3f00..543f2dc7152 100644 --- a/neutron/extensions/tagging.py +++ b/neutron/extensions/tagging.py @@ -50,6 +50,7 @@ TAGS_ANY = TAGS + '-any' NOT_TAGS = 'not-' + TAGS NOT_TAGS_ANY = NOT_TAGS + '-any' MAX_TAG_LEN = 255 +MAX_TAGS_COUNT = 50 TAG_PLUGIN_TYPE = 'TAG' TAG_SUPPORTED_RESOURCES = standard_attr.get_tag_resource_parent_map() @@ -105,6 +106,14 @@ def validate_tag(tag): raise exceptions.InvalidInput(error_message=msg) +def validate_tags_limit(resource, tags): + tags = set(tags) + if len(tags) > MAX_TAGS_COUNT: + msg = (_("The number of tags exceed the per-resource limit of %d") + % MAX_TAGS_COUNT) + raise exceptions.BadRequest(resource=resource, msg=msg) + + def validate_tags(body): if not isinstance(body, dict) or 'tags' not in body: raise exceptions.InvalidInput(error_message=_("Invalid tags body")) @@ -210,6 +219,7 @@ class TaggingController: rinfo = self._get_resource_info(ctx, kwargs, tags=body[TAGS]) policy.enforce(ctx, 'create_{}:{}'.format(rinfo.obj_type, TAGS), rinfo.obj) + validate_tags_limit(rinfo.obj_type, body['tags']) notify_tag_action(ctx, 'create.start', rinfo.obj_type, rinfo.obj['id'], body['tags']) result = self.plugin.create_tags(ctx, rinfo.obj_type, @@ -226,6 +236,10 @@ class TaggingController: rinfo = self._get_resource_info(ctx, kwargs, tags=[id]) policy.enforce(ctx, 'update_{}:{}'.format(rinfo.obj_type, TAGS), rinfo.obj) + current_tags = self.plugin.get_tags( + ctx, rinfo.obj_type, rinfo.obj['id'])['tags'] + new_tags = current_tags + [id] + validate_tags_limit(rinfo.obj_type, new_tags) notify_tag_action(ctx, 'create.start', rinfo.obj_type, rinfo.obj['id'], [id]) result = self.plugin.update_tag(ctx, rinfo.obj_type, @@ -244,6 +258,7 @@ class TaggingController: ctx, self._get_policy_action("update", rinfo.obj_type), rinfo.obj) + validate_tags_limit(rinfo.obj_type, body['tags']) notify_tag_action(ctx, 'update.start', rinfo.obj_type, rinfo.obj['id'], body['tags']) result = self.plugin.update_tags(ctx, rinfo.obj_type, diff --git a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py index dc3c77434b2..c286828e5a5 100644 --- a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py +++ b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py @@ -330,3 +330,21 @@ class TestChecks(base.BaseTestCase): mock.ANY) self.assertEqual(Code.WARNING, result.code) validate_mock.assert_called_once_with() + + def test_tags_over_limit_check_success(self): + with mock.patch.object( + checks, 'is_tags_limit_reached_for_any_resource', + return_value=False + ) as is_tags_limit_reached_for_any_resource: + result = checks.CoreChecks.tags_over_limit_check(mock.ANY) + self.assertEqual(Code.SUCCESS, result.code) + is_tags_limit_reached_for_any_resource.assert_called_once_with() + + def test_tags_over_limit_check_failure(self): + with mock.patch.object( + checks, 'is_tags_limit_reached_for_any_resource', + return_value=True + ) as is_tags_limit_reached_for_any_resource: + result = checks.CoreChecks.tags_over_limit_check(mock.ANY) + self.assertEqual(Code.WARNING, result.code) + is_tags_limit_reached_for_any_resource.assert_called_once_with() diff --git a/neutron/tests/unit/extensions/test_tagging.py b/neutron/tests/unit/extensions/test_tagging.py index 1355f521326..8366fb72c35 100644 --- a/neutron/tests/unit/extensions/test_tagging.py +++ b/neutron/tests/unit/extensions/test_tagging.py @@ -20,6 +20,7 @@ import netaddr from neutron_lib.api import attributes from neutron_lib import constants as n_const from neutron_lib import context +from neutron_lib import exceptions from neutron_lib.utils import net as net_utils from oslo_utils import uuidutils @@ -281,3 +282,196 @@ class TaggingControllerDbTestCase(testlib_api.WebTestCase): res = self.tc._get_resource_info(self.ctx, {'wrong_resource_id': missing_id}) self.assertEqual(tagging.EMPTY_RESOURCE_INFO, res) + + def test_create_tags_for_resource_below_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + tags = ['tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT - 1)] + body = {'tags': tags} + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.tc.create(req, body) + + get_res.assert_called_once_with(self.ctx, mock.ANY, tags=tags) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.create_tags.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY, body) + notify.assert_has_calls([ + mock.call(self.ctx, 'create.start', mock.ANY, mock.ANY, tags), + mock.call(self.ctx, 'create.end', mock.ANY, mock.ANY, tags)]) + + def test_create_tags_for_resource_over_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + tags = ['tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT + 1)] + body = {'tags': tags} + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.assertRaises( + exceptions.BadRequest, + self.tc.create, req, body) + + get_res.assert_called_once_with(self.ctx, mock.ANY, tags=tags) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.create_tags.assert_not_called() + notify.assert_not_called() + + def test_update_tag_for_resource_below_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + kwargs = {'network_id': uuidutils.generate_uuid()} + existing_tags = [ + 'tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT - 2)] + new_tag = 'new-tag' + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + self.tc.plugin.get_tags.return_value = {'tags': existing_tags} + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.tc.update(req, new_tag, **kwargs) + + get_res.assert_called_once_with(self.ctx, kwargs, tags=[new_tag]) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.get_tags.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY) + self.tc.plugin.update_tag.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY, new_tag) + notify.assert_has_calls([ + mock.call( + self.ctx, 'create.start', mock.ANY, mock.ANY, [new_tag]), + mock.call( + self.ctx, 'create.end', mock.ANY, mock.ANY, [new_tag])]) + + def test_update_exising_tag_for_resource_above_max_tags_limit(self): + """Test to ensure that TaggingController.update() method is idempotent. + """ + + req = mock.Mock(context=self.ctx) + kwargs = {'network_id': uuidutils.generate_uuid()} + existing_tags = [ + 'tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT)] + new_tag = existing_tags[0] + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + self.tc.plugin.get_tags.return_value = {'tags': existing_tags} + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.tc.update(req, new_tag, **kwargs) + + get_res.assert_called_once_with(self.ctx, kwargs, tags=[new_tag]) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.get_tags.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY) + self.tc.plugin.update_tag.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY, new_tag) + notify.assert_has_calls([ + mock.call( + self.ctx, 'create.start', mock.ANY, mock.ANY, [new_tag]), + mock.call( + self.ctx, 'create.end', mock.ANY, mock.ANY, [new_tag])]) + + def test_update_tag_for_resource_over_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + kwargs = {'network_id': uuidutils.generate_uuid()} + existing_tags = ['tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT)] + new_tag = 'new-tag' + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + self.tc.plugin.get_tags.return_value = {'tags': existing_tags} + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.assertRaises( + exceptions.BadRequest, + self.tc.update, req, new_tag, **kwargs) + + get_res.assert_called_once_with(self.ctx, kwargs, tags=[new_tag]) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.get_tags.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY) + self.tc.plugin.update_tag.assert_not_called() + notify.assert_not_called() + + def test_update_all_tags_for_resource_below_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + tags = ['tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT - 1)] + body = {'tags': tags} + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.tc.update_all(req, body) + + get_res.assert_called_once_with(self.ctx, mock.ANY, tags=tags) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.update_tags.assert_called_once_with( + self.ctx, mock.ANY, mock.ANY, body) + notify.assert_has_calls([ + mock.call(self.ctx, 'update.start', mock.ANY, mock.ANY, tags), + mock.call(self.ctx, 'update.end', mock.ANY, mock.ANY, tags)]) + + def test_update_all_tags_for_resource_over_max_tags_limit(self): + req = mock.Mock(context=self.ctx) + tags = ['tag-%i' % i for i in range(tagging.MAX_TAGS_COUNT + 1)] + body = {'tags': tags} + obj_mock = { + 'id': uuidutils.generate_uuid() + } + with mock.patch.object(self.tc, '_get_resource_info') as get_res, \ + mock.patch('neutron.policy.enforce') as policy_enforce, \ + mock.patch.object(tagging, 'notify_tag_action') as notify: + self.tc.plugin = mock.Mock() + get_res.return_value = mock.MagicMock(obj_type='networks', + obj=obj_mock) + + self.assertRaises( + exceptions.BadRequest, + self.tc.update_all, req, body) + + get_res.assert_called_once_with(self.ctx, mock.ANY, tags=tags) + policy_enforce.assert_called_once_with( + self.ctx, mock.ANY, obj_mock) + self.tc.plugin.update_tags.assert_not_called() + notify.assert_not_called() diff --git a/releasenotes/notes/Add-limit-of-the-tags-for-resources-23f825398d301d20.yaml b/releasenotes/notes/Add-limit-of-the-tags-for-resources-23f825398d301d20.yaml new file mode 100644 index 00000000000..2b8b57fe025 --- /dev/null +++ b/releasenotes/notes/Add-limit-of-the-tags-for-resources-23f825398d301d20.yaml @@ -0,0 +1,13 @@ +--- +upgrade: + - | + A limit on the number of tags per resource has been added. In case when + there are any resources with more than ``50`` tags created, it will not be + possible to create or modify existing tags for such resource until some of + them are deleted. +fixes: + - | + Fix bug `2091410 `_ which + could cause potential Denial of Service by adding a lot of tags to a single + resource, like, for example ``network``. Now there is a limit of ``50`` tags + for every resource.