From 795aa6b9fa792ec31c211cb690cae905c067dcf5 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 28 Apr 2020 09:07:07 +0000 Subject: [PATCH] Limit ml2_vlan_allocations.vlan_id value in DB backend Limit "ml2_vlan_allocations.vlan_id" values stored in the DB by adding a check constraint in the DB engine. This check will verify that vlan_id is in the interval [1, 4094]. Change-Id: Ie6453cb7a2ef0c43baf540c49a03079f4c8d3818 Closes-Bug: #1870400 --- neutron/cmd/upgrade_checks/checks.py | 43 ++++++++++++++- .../alembic_migrations/versions/EXPAND_HEAD | 2 +- ...5060830_limit_vlan_allocation_id_values.py | 55 +++++++++++++++++++ .../db/models/plugins/ml2/vlanallocation.py | 12 +++- .../unit/cmd/upgrade_checks/test_checks.py | 11 ++++ ...an_allocations_in_db-93083c6c4923403a.yaml | 8 +++ 6 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/victoria/expand/dfe425060830_limit_vlan_allocation_id_values.py create mode 100644 releasenotes/notes/limit_vlan_allocations_in_db-93083c6c4923403a.yaml diff --git a/neutron/cmd/upgrade_checks/checks.py b/neutron/cmd/upgrade_checks/checks.py index e8c50b85bcd..5d455afb79f 100644 --- a/neutron/cmd/upgrade_checks/checks.py +++ b/neutron/cmd/upgrade_checks/checks.py @@ -18,10 +18,12 @@ from neutron_lib.db import model_query from oslo_config import cfg from oslo_serialization import jsonutils from oslo_upgradecheck import upgradecheck +from sqlalchemy import or_ from neutron._i18n import _ from neutron.cmd.upgrade_checks import base from neutron.db.models import agent as agent_model +from neutron.db.models.plugins.ml2 import vlanallocation from neutron.db import models_v2 @@ -71,6 +73,15 @@ def get_ovn_db_revisions(): "SELECT version_num from %s;" % OVN_ALEMBIC_TABLE_NAME)] # nosec +def count_vlan_allocations_invalid_segmentation_id(): + ctx = context.get_admin_context() + query = ctx.session.query(vlanallocation.VlanAllocation) + query = query.filter(or_( + vlanallocation.VlanAllocation.vlan_id < constants.MIN_VLAN_TAG, + vlanallocation.VlanAllocation.vlan_id > constants.MAX_VLAN_TAG)) + return query.count() + + class CoreChecks(base.BaseChecks): def get_checks(self): @@ -83,7 +94,9 @@ class CoreChecks(base.BaseChecks): (_("Networking-ovn database revision"), self.ovn_db_revision_check), (_("NIC Switch agent check kernel"), - self.nic_switch_agent_min_kernel_check) + self.nic_switch_agent_min_kernel_check), + (_("VLAN allocations valid segmentation ID check"), + self.vlan_allocations_segid_check), ] @staticmethod @@ -240,3 +253,31 @@ class CoreChecks(base.BaseChecks): return upgradecheck.Result( upgradecheck.Code.SUCCESS, _("No NIC Switch agents detected.")) + + @staticmethod + def vlan_allocations_segid_check(checker): + """Checks that "ml2_vlan_allocations.vlan_id" has a valid value + + Database register column "ml2_vlan_allocations.vlan_id" must be between + 1 and 4094. + """ + if not cfg.CONF.database.connection: + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _("Database connection string is not set. Check for VLAN " + "allocations with invalid segmentation IDs can't be done.")) + + count = count_vlan_allocations_invalid_segmentation_id() + if count: + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _("There are %(count)s registers in 'ml2_vlan_allocations' " + "table with an invalid segmentation ID. 'vlan_id' must be " + "between %(min_vlan)s and %(max_vlan)s") % + {'count': count, 'min_vlan': constants.MIN_VLAN_TAG, + 'max_vlan': constants.MAX_VLAN_TAG}) + + return upgradecheck.Result( + upgradecheck.Code.SUCCESS, + _("All 'ml2_vlan_allocations' registers have a valid " + "segmentation ID.")) diff --git a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD index b94754e9e6e..6c4a8611f30 100644 --- a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD +++ b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD @@ -1 +1 @@ -d8bdf05313f4 +dfe425060830 diff --git a/neutron/db/migration/alembic_migrations/versions/victoria/expand/dfe425060830_limit_vlan_allocation_id_values.py b/neutron/db/migration/alembic_migrations/versions/victoria/expand/dfe425060830_limit_vlan_allocation_id_values.py new file mode 100644 index 00000000000..aa13f480e30 --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/victoria/expand/dfe425060830_limit_vlan_allocation_id_values.py @@ -0,0 +1,55 @@ +# Copyright 2020 OpenStack Foundation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +from alembic import op +from neutron_lib import constants + + +"""limit vlan allocation id values + +Revision ID: dfe425060830 +Revises: d8bdf05313f4 +Create Date: 2020-04-27 16:58:33.110194 + +""" + +# revision identifiers, used by Alembic. +revision = 'dfe425060830' +down_revision = 'd8bdf05313f4' + + +# NOTE(ralonsoh): "CHECK CONSTRAINT" is a feature heterogeneously implemented +# in different database engines and versions. For example: +# - MySQL: since version 8.0.16 (April 2019) +# https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-16.html +# - MariaDB: since version 10.2.1 (July 2016) +# https://mariadb.com/kb/en/mariadb-1021-release-notes/ +# - PostgreSQL: since version 9.4 (December 2014) +# https://www.postgresql.org/docs/9.4/ddl-constraints.html +# +# If the DB engine does not support yet this feature, it will be ignored. The +# VLAN tag constraint is enforced in the Neutron API. This extra enforcement +# in the DB engine wants to mimic the limitation imposed in the API side and +# avoid any spurious modification. + +def upgrade(): + # https://docs.sqlalchemy.org/en/13/core/constraints.html#check-constraint + constraint = ('vlan_id>=%(min_vlan)s AND vlan_id<=%(max_vlan)s' % + {'min_vlan': constants.MIN_VLAN_TAG, + 'max_vlan': constants.MAX_VLAN_TAG}) + op.create_check_constraint( + constraint_name='check_ml2_vlan_allocations0vlan_id', + table_name='ml2_vlan_allocations', + condition=constraint) diff --git a/neutron/db/models/plugins/ml2/vlanallocation.py b/neutron/db/models/plugins/ml2/vlanallocation.py index e36258ce2f2..3d7a34852d9 100644 --- a/neutron/db/models/plugins/ml2/vlanallocation.py +++ b/neutron/db/models/plugins/ml2/vlanallocation.py @@ -10,10 +10,17 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron_lib import constants from neutron_lib.db import model_base import sqlalchemy as sa +# https://docs.sqlalchemy.org/en/13/core/constraints.html#check-constraint +VLAN_CONSTRAINT = ('vlan_id>=%(min_vlan)s AND vlan_id<=%(max_vlan)s' % + {'min_vlan': constants.MIN_VLAN_TAG, + 'max_vlan': constants.MAX_VLAN_TAG}) + + class VlanAllocation(model_base.BASEV2): """Represent allocation state of a vlan_id on a physical network. @@ -32,7 +39,10 @@ class VlanAllocation(model_base.BASEV2): __table_args__ = ( sa.Index('ix_ml2_vlan_allocations_physical_network_allocated', 'physical_network', 'allocated'), - model_base.BASEV2.__table_args__,) + sa.CheckConstraint(sqltext=VLAN_CONSTRAINT, + name='check_ml2_vlan_allocations0vlan_id'), + model_base.BASEV2.__table_args__, + ) physical_network = sa.Column(sa.String(64), nullable=False, primary_key=True) diff --git a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py index a9749b038cc..9744c133b22 100644 --- a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py +++ b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py @@ -179,3 +179,14 @@ class TestChecks(base.BaseTestCase): self.assertEqual(Code.WARNING, result.code) self.assertIn('Host A', result.details) self.assertIn('Host B', result.details) + + def test_vlan_allocations_segid_check(self): + cases = ([0, Code.SUCCESS], [1, Code.WARNING]) + with mock.patch.object( + checks, 'count_vlan_allocations_invalid_segmentation_id') \ + as mock_count: + for count, returned_code in cases: + mock_count.return_value = count + result = checks.CoreChecks.vlan_allocations_segid_check( + mock.ANY) + self.assertEqual(returned_code, result.code) diff --git a/releasenotes/notes/limit_vlan_allocations_in_db-93083c6c4923403a.yaml b/releasenotes/notes/limit_vlan_allocations_in_db-93083c6c4923403a.yaml new file mode 100644 index 00000000000..8552167155a --- /dev/null +++ b/releasenotes/notes/limit_vlan_allocations_in_db-93083c6c4923403a.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + Limit the ML2 VLAN allocations to [1, 4094] values in the database engine. + This constraint, enforced in the database engine, could not be supported + yet. In this case, it will be ignored. For more information, see the note + in + ``neutron.db.migration.alembic_migrations.versions.victoria.expand.dfe425060830_limit_vlan_allocation_id_values.py``.