From 1c9aac8f5d084b1cb84ae88f476ec957328f76f5 Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Wed, 27 Oct 2021 19:35:56 +0200
Subject: [PATCH] Prevent table and column alter and drop

Due to Cinder's rolling upgrade support we have to be very careful with
table drops and column alterations and drops.

In our Unit Tests legacy migrations walk, where we go through the
migrations to confirm they are properly applied, we raise an error on
those 3 operations to prevent them from being casually added in new
migrations.

Those checks are missing for the Alembic migrations, so this patch adds
them there as well while consolidating the check code for both types of
migrations into a single decorator called `prevent_drop_alter`.

Change-Id: I577f65968feb9c10488a19c6f99e8c0b720a433d
---
 cinder/tests/unit/db/test_migrations.py     | 69 +++++++++++++++++++++
 doc/source/contributor/rolling.upgrades.rst |  8 +--
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/cinder/tests/unit/db/test_migrations.py b/cinder/tests/unit/db/test_migrations.py
index 2d3abff2316..567d1ba91bf 100644
--- a/cinder/tests/unit/db/test_migrations.py
+++ b/cinder/tests/unit/db/test_migrations.py
@@ -16,6 +16,9 @@ the test case runs a series of test cases to ensure that migrations work
 properly and that no data loss occurs if possible.
 """
 
+import functools
+from unittest import mock
+
 from alembic import command as alembic_api
 from alembic import script as alembic_script
 import fixtures
@@ -33,6 +36,52 @@ from cinder.db.sqlalchemy import models
 from cinder.tests import fixtures as cinder_fixtures
 
 
+def prevent_drop_alter(func):
+    """Decorator to prevent dropping or altering tables and columns.
+
+    With rolling upgrades we shouldn't blindly allow dropping or altering
+    tables and columns, since that can easily break them.
+
+    Dropping and altering should be done in a backward-compatible manner.  A
+    more detailed explanation is provided in Cinder's developer documentation.
+
+    To properly work, the first parameter of the decorated method must be a
+    class or instance with the DROP_ALTER_EXCEPTIONS and FORBIDDEN_METHODS
+    attribute, and the second parameter must be the version (legacy migrations)
+    or revision (alembic migrations).
+
+    Reviewers should be very careful when adding exceptions to
+    DROP_ALTER_EXCEPTIONS and make sure that in the previous release there was
+    nothing using that column, not even an ORM model (unless the whole ORM
+    model was not being used)
+    """
+
+    @functools.wraps(func)
+    def wrapper(self, revision, *args, **kwargs):
+        exceptions = getattr(self, 'DROP_ALTER_EXCEPTIONS', [])
+        do_ban = revision not in exceptions
+
+        patchers = []
+
+        if do_ban:
+            forbidden = getattr(self, 'FORBIDDEN_METHODS', [])
+            for path in forbidden:
+                txt = (f'Migration {revision}: Operation {path}() is not '
+                       'allowed in a DB migration')
+                patcher = mock.patch(path, autospec=True,
+                                     side_effect=Exception(txt))
+                patcher.start()
+                patchers.append(patcher)
+
+        try:
+            return func(self, revision, *args, **kwargs)
+        finally:
+            for patcher in patchers:
+                patcher.stop()
+
+    return wrapper
+
+
 class CinderModelsMigrationsSync(test_migrations.ModelsMigrationsSync):
     """Test sqlalchemy-migrate migrations."""
 
@@ -122,6 +171,25 @@ class MigrationsWalk(
     TIMEOUT_SCALING_FACTOR = 4
     BOOL_TYPE = sqlalchemy.types.BOOLEAN
 
+    # NOTE: List of migrations where we allow dropping/altring things.
+    # Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE WITHOUT CARE, and make
+    # sure that in the previous release there was nothing using that column,
+    # not even an ORM model (unless the whole ORM model was not being used)
+    # See prevent_drop_alter method docstring.
+    DROP_ALTER_EXCEPTIONS = [
+        # Drops and alters from initial migration have already been accepted
+        '921e1a36b076',
+        # Making shared_targets explicitly nullable (DB already allowed it)
+        'c92a3e68beed',
+        # Migration 89aa6f9639f9 doesn't fail because it's for a SQLAlquemy
+        # internal table, and we only check Cinder's tables.
+    ]
+    FORBIDDEN_METHODS = ('alembic.operations.Operations.alter_column',
+                         'alembic.operations.Operations.drop_column',
+                         'alembic.operations.Operations.drop_table',
+                         'alembic.operations.BatchOperations.alter_column',
+                         'alembic.operations.BatchOperations.drop_column')
+
     def setUp(self):
         super().setUp()
         self.engine = enginefacade.writer.get_engine()
@@ -129,6 +197,7 @@ class MigrationsWalk(
         self.config = migration._find_alembic_conf()
         self.init_version = '921e1a36b076'
 
+    @prevent_drop_alter
     def _migrate_up(self, revision, connection):
         check_method = getattr(self, f'_check_{revision}', None)
         if revision != self.init_version:  # no tests for the initial revision
diff --git a/doc/source/contributor/rolling.upgrades.rst b/doc/source/contributor/rolling.upgrades.rst
index 5c994358829..3425e6cedcb 100644
--- a/doc/source/contributor/rolling.upgrades.rst
+++ b/doc/source/contributor/rolling.upgrades.rst
@@ -92,10 +92,10 @@ Dropping a column not referenced in SQLAlchemy code
 ...................................................
 
 When we want to remove a column that wasn't present in any SQLAlchemy model or
-it was in the model, but model was not referenced in any SQLAlchemy API
-function (this basically means that N-1 wasn't depending on the presence of
-that column in the DB), then the situation is simple. We should be able to
-safely drop the column in N release.
+it was in the model, but model was not referenced anywhere in our code (this
+basically means that N-1 wasn't depending on the presence of that column in the
+DB), then the situation is simple. We should be able to safely drop the column
+in N release.
 
 Removal of unnecessary column
 .............................