From 79d7a4e8da6f1118b5c235928876cf78085f4332 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 7 May 2018 14:53:18 +0200 Subject: [PATCH] Fix google backup driver Current backup driver is broken due to the upper constraints of google-api-python-client which is set to 1.6.6, but after 1.6.0 parameter http and credentials are mutually exclusive, the driver is also using the no longer preferred Google authentication library. This patch fixes this by only using the http_proxy environmental variable to set the proxying and adding support for google-auth and using it whenever possible. Closes-Bug: #1736569 Closes-Bug: #1769660 Change-Id: If1e34ea112b0cff328a58ec2fc9c3f5a5005f1c6 --- cinder/backup/drivers/{google.py => gcs.py} | 60 ++++++--- cinder/backup/manager.py | 123 +++++++++--------- cinder/opts.py | 4 +- .../unit/backup/drivers/test_backup_google.py | 71 ++++++++-- cinder/tests/unit/backup/test_backup.py | 1 + ...-auth-for-gcs-backup-1642cd0e741fbdf9.yaml | 13 ++ 6 files changed, 183 insertions(+), 89 deletions(-) rename cinder/backup/drivers/{google.py => gcs.py} (89%) create mode 100644 releasenotes/notes/google-auth-for-gcs-backup-1642cd0e741fbdf9.yaml diff --git a/cinder/backup/drivers/google.py b/cinder/backup/drivers/gcs.py similarity index 89% rename from cinder/backup/drivers/google.py rename to cinder/backup/drivers/gcs.py index c8b190aae1b..65aff80bb93 100644 --- a/cinder/backup/drivers/google.py +++ b/cinder/backup/drivers/gcs.py @@ -27,13 +27,26 @@ Server-centric flow is used for authentication. """ import base64 +from distutils import version import hashlib -import httplib2 +import os +try: + from google.auth import exceptions as gexceptions + from google.oauth2 import service_account + import google_auth_httplib2 +except ImportError: + service_account = google_auth_httplib2 = gexceptions = None + +try: + from oauth2client import client +except ImportError: + client = None + +import googleapiclient from googleapiclient import discovery from googleapiclient import errors from googleapiclient import http -from oauth2client import client from oslo_config import cfg from oslo_log import log as logging from oslo_utils import timeutils @@ -99,6 +112,7 @@ gcsbackup_service_opts = [ CONF = cfg.CONF CONF.register_opts(gcsbackup_service_opts) +OAUTH_EXCEPTIONS = None def gcs_logger(func): @@ -107,7 +121,7 @@ def gcs_logger(func): return func(self, *args, **kwargs) except errors.Error as err: raise exception.GCSApiFailure(reason=err) - except client.Error as err: + except OAUTH_EXCEPTIONS as err: raise exception.GCSOAuth2Failure(reason=err) except Exception as err: raise exception.GCSConnectionFailure(reason=err) @@ -120,8 +134,8 @@ class GoogleBackupDriver(chunkeddriver.ChunkedBackupDriver): """Provides backup, restore and delete of backup objects within GCS.""" def __init__(self, context, db=None): + global OAUTH_EXCEPTIONS backup_bucket = CONF.backup_gcs_bucket - backup_credential = CONF.backup_gcs_credential_file self.gcs_project_id = CONF.backup_gcs_project_id chunk_size_bytes = CONF.backup_gcs_object_size sha_block_size_bytes = CONF.backup_gcs_block_size @@ -131,27 +145,41 @@ class GoogleBackupDriver(chunkeddriver.ChunkedBackupDriver): backup_bucket, enable_progress_timer, db) - credentials = client.GoogleCredentials.from_stream(backup_credential) self.reader_chunk_size = CONF.backup_gcs_reader_chunk_size self.writer_chunk_size = CONF.backup_gcs_writer_chunk_size self.bucket_location = CONF.backup_gcs_bucket_location self.storage_class = CONF.backup_gcs_storage_class self.num_retries = CONF.backup_gcs_num_retries - http_user_agent = http.set_user_agent( - httplib2.Http(proxy_info=self.get_gcs_proxy_info()), - CONF.backup_gcs_user_agent) + + # Set or overwrite environmental proxy variables for httplib2 since + # it's the only mechanism supported when using googleapiclient with + # google-auth + if CONF.backup_gcs_proxy_url: + os.environ['http_proxy'] = CONF.backup_gcs_proxy_url + + backup_credential = CONF.backup_gcs_credential_file + # If we have google client that support google-auth library + # (v1.6.0 or higher) and all required libraries are installed use + # google-auth for the credentials + if (version.LooseVersion(googleapiclient.__version__) >= + version.LooseVersion('1.6.0') and service_account): + creds = service_account.Credentials.from_service_account_file( + backup_credential) + OAUTH_EXCEPTIONS = (gexceptions.RefreshError, + gexceptions.DefaultCredentialsError) + + # Can't use google-auth, use deprecated oauth2client + else: + creds = client.GoogleCredentials.from_stream(backup_credential) + OAUTH_EXCEPTIONS = client.Error + self.conn = discovery.build('storage', 'v1', - http=http_user_agent, - credentials=credentials) + # Avoid log error on oauth2client >= 4.0.0 + cache_discovery=False, + credentials=creds) self.resumable = self.writer_chunk_size != -1 - def get_gcs_proxy_info(self): - if CONF.backup_gcs_proxy_url: - return httplib2.proxy_info_from_url(CONF.backup_gcs_proxy_url) - else: - return httplib2.proxy_info_from_environment() - def check_for_setup_error(self): required_options = ('backup_gcs_bucket', 'backup_gcs_credential_file', 'backup_gcs_project_id') diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 29f80adce50..64b34e1e7ab 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -90,6 +90,11 @@ CONF.register_opts(backup_manager_opts) CONF.import_opt('use_multipath_for_image_xfer', 'cinder.volume.driver') CONF.import_opt('num_volume_device_scan_tries', 'cinder.volume.driver') QUOTAS = quota.QUOTAS +MAPPING = { + # Module name "google" conflicts with google library namespace inside the + # driver when it imports google.auth + 'cinder.backup.drivers.google': 'cinder.backup.drivers.gcs', +} # TODO(geguileo): Once Eventlet issue #432 gets fixed we can just tpool.execute @@ -113,12 +118,13 @@ class BackupManager(manager.ThreadPoolManager): self.is_initialized = False self._set_tpool_size(CONF.backup_native_threads_pool_size) self._process_number = kwargs.get('process_number', 1) - - @property - def driver_name(self): - """This function maps old backup services to backup drivers.""" - - return CONF.backup_driver + self.driver_name = CONF.backup_driver + if self.driver_name in MAPPING: + new_name = MAPPING[self.driver_name] + LOG.warning('Backup driver path %s is deprecated, update your ' + 'configuration to the new path %s', + self.driver_name, new_name) + self.driver_name = new_name def get_backup_driver(self, context): driver = None @@ -503,6 +509,28 @@ class BackupManager(manager.ThreadPoolManager): context, backup) return updates + def _is_our_backup(self, backup): + # Accept strings and Service OVO + if not isinstance(backup, six.string_types): + backup = backup.service + + if not backup: + return True + + # TODO(tommylikehu): We upgraded the 'driver_name' from module + # to class name, so we use 'in' here to match two namings, + # this can be replaced with equal sign during next + # release (Rocky). + if self.driver_name.startswith(backup): + return True + + # We support renaming of drivers, so check old names as well + for key, value in MAPPING.items(): + if key.startswith(backup) and self.driver_name.startswith(value): + return True + + return False + def restore_backup(self, context, backup, volume_id): """Restore volume backups from configured backup service.""" LOG.info('Restore backup started, backup: %(backup_id)s ' @@ -548,19 +576,13 @@ class BackupManager(manager.ThreadPoolManager): 'backup_id': backup['id'], 'backup_size': backup['size']}) - backup_service = backup['service'] - configured_service = self.driver_name - # TODO(tommylikehu): We upgraded the 'driver_name' from module - # to class name, so we use 'in' here to match two namings, - # this can be replaced with equal sign during next - # release (Rocky). - if backup_service not in configured_service: + if not self._is_our_backup(backup): err = _('Restore backup aborted, the backup service currently' ' configured [%(configured_service)s] is not the' ' backup service that was used to create this' ' backup [%(backup_service)s].') % { - 'configured_service': configured_service, - 'backup_service': backup_service, + 'configured_service': self.driver_name, + 'backup_service': backup.service, } backup.status = fields.BackupStatus.AVAILABLE backup.save() @@ -691,23 +713,17 @@ class BackupManager(manager.ThreadPoolManager): self._update_backup_error(backup, err, status) raise exception.InvalidBackup(reason=err) - backup_service = backup['service'] - if backup_service is not None: - configured_service = self.driver_name - # TODO(tommylikehu): We upgraded the 'driver_name' from module - # to class name, so we use 'in' here to match two namings, - # this can be replaced with equal sign during next - # release (Rocky). - if backup_service not in configured_service: - err = _('Delete backup aborted, the backup service currently' - ' configured [%(configured_service)s] is not the' - ' backup service that was used to create this' - ' backup [%(backup_service)s].')\ - % {'configured_service': configured_service, - 'backup_service': backup_service} - self._update_backup_error(backup, err) - raise exception.InvalidBackup(reason=err) + if not self._is_our_backup(backup): + err = _('Delete backup aborted, the backup service currently' + ' configured [%(configured_service)s] is not the' + ' backup service that was used to create this' + ' backup [%(backup_service)s].')\ + % {'configured_service': self.driver_name, + 'backup_service': backup.service} + self._update_backup_error(backup, err) + raise exception.InvalidBackup(reason=err) + if backup.service: try: backup_service = self.get_backup_driver(context) backup_service.delete_backup(backup) @@ -787,19 +803,13 @@ class BackupManager(manager.ThreadPoolManager): raise exception.InvalidBackup(reason=err) backup_record = {'backup_service': backup.service} - backup_service = backup.service - configured_service = self.driver_name - # TODO(tommylikehu): We upgraded the 'driver_name' from module - # to class name, so we use 'in' here to match two namings, - # this can be replaced with equal sign during next - # release (Rocky). - if backup_service not in configured_service: + if not self._is_our_backup(backup): err = (_('Export record aborted, the backup service currently ' 'configured [%(configured_service)s] is not the ' 'backup service that was used to create this ' 'backup [%(backup_service)s].') % - {'configured_service': configured_service, - 'backup_service': backup_service}) + {'configured_service': self.driver_name, + 'backup_service': backup.service}) raise exception.InvalidBackup(reason=err) # Call driver to create backup description string @@ -834,7 +844,7 @@ class BackupManager(manager.ThreadPoolManager): LOG.info('Import record started, backup_url: %s.', backup_url) # Can we import this backup? - if backup_service != self.driver_name: + if not self._is_our_backup(backup_service): # No, are there additional potential backup hosts in the list? if len(backup_hosts) > 0: # try the next host on the list, maybe he can import @@ -946,27 +956,22 @@ class BackupManager(manager.ThreadPoolManager): {'backup_id': backup.id, 'status': status}) - backup_service_name = backup.service - LOG.info('Backup service: %s.', backup_service_name) - if backup_service_name is not None: - configured_service = self.driver_name - # TODO(tommylikehu): We upgraded the 'driver_name' from module - # to class name, so we use 'in' here to match two namings, - # this can be replaced with equal sign during next - # release (Rocky). - if backup_service_name not in configured_service: - err = _('Reset backup status aborted, the backup service' - ' currently configured [%(configured_service)s] ' - 'is not the backup service that was used to create' - ' this backup [%(backup_service)s].') % \ - {'configured_service': configured_service, - 'backup_service': backup_service_name} - raise exception.InvalidBackup(reason=err) + LOG.info('Backup service: %s.', backup.service) + if not self._is_our_backup(backup): + err = _('Reset backup status aborted, the backup service' + ' currently configured [%(configured_service)s] ' + 'is not the backup service that was used to create' + ' this backup [%(backup_service)s].') % \ + {'configured_service': self.driver_name, + 'backup_service': backup.service} + raise exception.InvalidBackup(reason=err) + + if backup.service is not None: # Verify backup try: # check whether the backup is ok or not - if (status == fields.BackupStatus.AVAILABLE - and backup['status'] != fields.BackupStatus.RESTORING): + if (status == fields.BackupStatus.AVAILABLE and + backup['status'] != fields.BackupStatus.RESTORING): # check whether we could verify the backup is ok or not backup_service = self.get_backup_driver(context) if isinstance(backup_service, diff --git a/cinder/opts.py b/cinder/opts.py index 9ec766c9e44..59eae69d648 100644 --- a/cinder/opts.py +++ b/cinder/opts.py @@ -35,8 +35,8 @@ from cinder.backup import api as cinder_backup_api from cinder.backup import chunkeddriver as cinder_backup_chunkeddriver from cinder.backup import driver as cinder_backup_driver from cinder.backup.drivers import ceph as cinder_backup_drivers_ceph +from cinder.backup.drivers import gcs as cinder_backup_drivers_gcs from cinder.backup.drivers import glusterfs as cinder_backup_drivers_glusterfs -from cinder.backup.drivers import google as cinder_backup_drivers_google from cinder.backup.drivers import nfs as cinder_backup_drivers_nfs from cinder.backup.drivers import posix as cinder_backup_drivers_posix from cinder.backup.drivers import swift as cinder_backup_drivers_swift @@ -217,8 +217,8 @@ def list_opts(): cinder_backup_chunkeddriver.chunkedbackup_service_opts, cinder_backup_driver.service_opts, cinder_backup_drivers_ceph.service_opts, + cinder_backup_drivers_gcs.gcsbackup_service_opts, cinder_backup_drivers_glusterfs.glusterfsbackup_service_opts, - cinder_backup_drivers_google.gcsbackup_service_opts, cinder_backup_drivers_nfs.nfsbackup_service_opts, cinder_backup_drivers_posix.posixbackup_service_opts, cinder_backup_drivers_swift.swiftbackup_service_opts, diff --git a/cinder/tests/unit/backup/drivers/test_backup_google.py b/cinder/tests/unit/backup/drivers/test_backup_google.py index f266112ab49..500a079ec57 100644 --- a/cinder/tests/unit/backup/drivers/test_backup_google.py +++ b/cinder/tests/unit/backup/drivers/test_backup_google.py @@ -32,7 +32,7 @@ from eventlet import tpool import mock from oslo_utils import units -from cinder.backup.drivers import google as google_dr +from cinder.backup.drivers import gcs as google_dr from cinder import context from cinder import db from cinder import exception @@ -219,21 +219,17 @@ class GoogleBackupDriverTestCase(test.TestCase): @gcs_client @mock.patch('httplib2.proxy_info_from_url') def test_backup_proxy_configured(self, mock_proxy_info): - google_dr.CONF.set_override("backup_gcs_proxy_url", - "http://myproxy.example.com") + # Configuration overwrites enviromental variable + proxy_cfg = "http://myproxy.example.com" + os.environ['http_proxy'] = proxy_cfg + '_fake' + google_dr.CONF.set_override("backup_gcs_proxy_url", proxy_cfg) google_dr.GoogleBackupDriver(self.ctxt) - mock_proxy_info.assert_called_with("http://myproxy.example.com") + self.assertEqual(proxy_cfg, os.environ.get('http_proxy')) @gcs_client - @mock.patch('httplib2.proxy_info_from_environment') - def test_backup_proxy_environment(self, mock_proxy_env): - google_dr.GoogleBackupDriver(self.ctxt) - mock_proxy_env.assert_called_once_with() - - @gcs_client - @mock.patch('cinder.backup.drivers.google.GoogleBackupDriver.' + @mock.patch('cinder.backup.drivers.gcs.GoogleBackupDriver.' '_send_progress_end') - @mock.patch('cinder.backup.drivers.google.GoogleBackupDriver.' + @mock.patch('cinder.backup.drivers.gcs.GoogleBackupDriver.' '_send_progress_notification') def test_backup_default_container_notify(self, _send_progress, _send_progress_end): @@ -612,3 +608,54 @@ class GoogleBackupDriverTestCase(test.TestCase): self.assertEqual('none', result[0]) self.assertEqual(already_compressed_data, result[1]) + + @mock.patch('googleapiclient.__version__', '1.5.5') + @mock.patch.object(google_dr.client.GoogleCredentials, 'from_stream') + @mock.patch.object(google_dr.discovery, 'build') + @mock.patch.object(google_dr, 'service_account') + def test_non_google_auth_version(self, account, build, from_stream): + # Prior to v1.6.0 Google api client doesn't support google-auth library + google_dr.CONF.set_override('backup_gcs_credential_file', + 'credentials_file') + + google_dr.GoogleBackupDriver(self.ctxt) + + from_stream.assert_called_once_with('credentials_file') + account.Credentials.from_service_account_file.assert_not_called() + build.assert_called_once_with('storage', 'v1', cache_discovery=False, + credentials=from_stream.return_value) + + @mock.patch('googleapiclient.__version__', '1.6.6') + @mock.patch.object(google_dr.client.GoogleCredentials, 'from_stream') + @mock.patch.object(google_dr.discovery, 'build') + @mock.patch.object(google_dr, 'service_account', None) + def test_no_httplib2_auth(self, build, from_stream): + # Google api client requires google-auth-httplib2 if not present we + # use legacy credentials + google_dr.CONF.set_override('backup_gcs_credential_file', + 'credentials_file') + + google_dr.GoogleBackupDriver(self.ctxt) + + from_stream.assert_called_once_with('credentials_file') + build.assert_called_once_with('storage', 'v1', cache_discovery=False, + credentials=from_stream.return_value) + + @mock.patch('googleapiclient.__version__', '1.6.6') + @mock.patch.object(google_dr, 'gexceptions', mock.Mock()) + @mock.patch.object(google_dr.client.GoogleCredentials, 'from_stream') + @mock.patch.object(google_dr.discovery, 'build') + @mock.patch.object(google_dr, 'service_account') + def test_google_auth_used(self, account, build, from_stream): + # Google api client requires google-auth-httplib2 if not present we + # use legacy credentials + google_dr.CONF.set_override('backup_gcs_credential_file', + 'credentials_file') + + google_dr.GoogleBackupDriver(self.ctxt) + + from_stream.assert_not_called() + create_creds = account.Credentials.from_service_account_file + create_creds.assert_called_once_with('credentials_file') + build.assert_called_once_with('storage', 'v1', cache_discovery=False, + credentials=create_creds.return_value) diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index 8ca0f1ac862..f290b670f98 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -1823,6 +1823,7 @@ class BackupTestCaseWithVerify(BaseBackupTest): service_name = ('cinder.tests.unit.backup.' 'fake_service_with_verify.FakeBackupServiceWithVerify') self.override_config('backup_driver', service_name) + self.backup_mgr.driver_name = service_name vol_id = self._create_volume_db_entry(status='available', size=1) backup = self._create_backup_db_entry(status=fields.BackupStatus.ERROR, diff --git a/releasenotes/notes/google-auth-for-gcs-backup-1642cd0e741fbdf9.yaml b/releasenotes/notes/google-auth-for-gcs-backup-1642cd0e741fbdf9.yaml new file mode 100644 index 00000000000..57f87965186 --- /dev/null +++ b/releasenotes/notes/google-auth-for-gcs-backup-1642cd0e741fbdf9.yaml @@ -0,0 +1,13 @@ +--- +features: + - Google backup driver now supports ``google-auth`` library, and is the + preferred library if both ``google-auth`` (together with + ``google-auth-httplib2``) and ``oauth2client`` libraries are present in the + system. +deprecations: + - Cinder's Google backup driver is now called gcs, so ``backup_driver`` + configuration for Google Cloud Storage should be updated from + ``cinder.backup.drivers.google`` to ``cinder.backup.driver.gcs``. +fixes: + - Google backup driver now works when using ``google-api-python-client`` + version 1.6.0 or higher.