db: Handle parameters in DB strings

As part of 'nova.db.migration', it is necessary to override the
'sqlalchemy.url' config option used by alembic. We were doing this but
we weren't handling the fact that we could receive encoded URL strings.
This causes issues for the ConfigParser instead used under the hood, as
noted in the alembic docs [1]:

  value – the value. Note that this value is passed to ConfigParser.set,
  which supports variable interpolation using pyformat (e.g.
  %(some_value)s). A raw percent sign not part of an interpolation
  symbol must therefore be escaped, e.g. %%. The given value may refer
  to another value already in the file using the interpolation format.

Resolve the issue by escaping % with %%. The engine.url is always
encoded therefore this can be done unconditionally.

[1] https://alembic.sqlalchemy.org/en/latest/api/config.html#alembic.config.Config.set_main_option

Closes-Bug: #1940555
Change-Id: I74de55107a80af13df348f2bce49415b08028746
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Co-Authored-By: Sean Mooney <work@seanmooney.info>
This commit is contained in:
Stephen Finucane 2021-08-23 17:42:08 +01:00 committed by Sean Mooney
parent cf07df57e7
commit 6c5dd864c2
2 changed files with 57 additions and 6 deletions

View File

@ -128,10 +128,15 @@ def db_sync(version=None, database='main', context=None):
repository = _find_migrate_repo(database)
config = _find_alembic_conf(database)
# discard the URL encoded in alembic.ini in favour of the URL configured
# for the engine by the database fixtures, casting from
# 'sqlalchemy.engine.url.URL' to str in the process
config.set_main_option('sqlalchemy.url', str(engine.url))
# discard the URL stored in alembic.ini in favour of the URL configured
# for the engine, casting from 'sqlalchemy.engine.url.URL' to str in the
# process
# NOTE(sean-k-mooney): the engine has already url encoded the connection
# string using a mix of url encode styles for different parts of the url.
# since we are updating the alembic config parser instance we need to
# escape '%' to '%%' to account for ConfigParser's string interpolation.
url = str(engine.url).replace('%', '%%')
config.set_main_option('sqlalchemy.url', url)
# if we're in a deployment where sqlalchemy-migrate is already present,
# then apply all the updates for that and fake apply the initial alembic

View File

@ -12,11 +12,17 @@
# License for the specific language governing permissions and limitations
# under the License.
import urllib
import fixtures
import mock
from alembic import command as alembic_api
from alembic.runtime import migration as alembic_migration
from migrate import exceptions as migrate_exceptions
from migrate.versioning import api as migrate_api
import mock
from oslo_db.sqlalchemy import enginefacade
from nova.db.api import api as api_db_api
from nova.db.main import api as main_db_api
@ -25,6 +31,36 @@ from nova import exception
from nova import test
class TestDBURL(test.NoDBTestCase):
def test_db_sync_with_special_symbols_in_connection_string(self):
qargs = 'read_default_group=data with/a+percent_%-and%20symbols!'
url = f"sqlite:///:memory:?{qargs}"
self.flags(connection=url, group='database')
# since the engine.url is immutable it will never get updated
# once its created so reusing the engine instance would break
# this test.
engine = enginefacade.writer.get_engine()
self.useFixture(
fixtures.MonkeyPatch(
'nova.db.migration._get_engine',
mock.Mock(return_value=engine)))
alembic_config = migration._find_alembic_conf()
with mock.patch.object(
migration, '_find_alembic_conf', return_value=alembic_config):
migration.db_sync()
actual = alembic_config.get_main_option('sqlalchemy.url')
expected = (
"sqlite:///:memory:?read_default_group=data+with%2Fa"
"+percent_%25-and+symbols%21"
)
self.assertEqual(expected, actual)
self.assertEqual(
urllib.parse.unquote_plus(url),
urllib.parse.unquote_plus(actual)
)
class TestDBSync(test.NoDBTestCase):
def test_db_sync_invalid_databse(self):
@ -50,6 +86,13 @@ class TestDBSync(test.NoDBTestCase):
mock_find_conf, mock_is_migrate, mock_is_alembic, mock_init,
mock_upgrade,
):
# return an encoded URL to mimic sqlalchemy
mock_get_engine.return_value.url = (
'mysql+pymysql://nova:pass@192.168.24.3/nova?'
'read_default_file=%2Fetc%2Fmy.cnf.d%2Fnova.cnf'
'&read_default_group=nova'
)
mock_is_migrate.return_value = has_migrate
mock_is_alembic.return_value = has_alembic
@ -59,7 +102,10 @@ class TestDBSync(test.NoDBTestCase):
mock_find_repo.assert_called_once_with('main')
mock_find_conf.assert_called_once_with('main')
mock_find_conf.return_value.set_main_option.assert_called_once_with(
'sqlalchemy.url', str(mock_get_engine.return_value.url),
'sqlalchemy.url',
'mysql+pymysql://nova:pass@192.168.24.3/nova?' # ...
'read_default_file=%%2Fetc%%2Fmy.cnf.d%%2Fnova.cnf' # ...
'&read_default_group=nova'
)
mock_is_migrate.assert_called_once_with(
mock_get_engine.return_value, mock_find_repo.return_value)