From 0ee1c2f05a49dc3494f4327d690a24d751c7e7f9 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Dec 2019 11:27:13 -0500 Subject: [PATCH 1/2] Remove S3ReportStorage, because S3BotoStorage already handles a custom_domain argument --- lms/djangoapps/instructor/tests/test_api.py | 2 +- lms/djangoapps/instructor_task/models.py | 2 +- .../instructor_task/tests/test_models.py | 2 +- openedx/core/storage.py | 24 ------------------- 4 files changed, 3 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 6001cc3806..3408f52b4a 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -3305,7 +3305,7 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment ex_status = 503 ex_reason = 'Slow Down' url = reverse('list_report_downloads', kwargs={'course_id': text_type(self.course.id)}) - with patch('openedx.core.storage.S3ReportStorage.listdir', side_effect=BotoServerError(ex_status, ex_reason)): + with patch('storages.backends.s3boto.S3BotoStorage.listdir', side_effect=BotoServerError(ex_status, ex_reason)): response = self.client.post(url, {}) mock_error.assert_called_with( u'Fetching files failed for course: %s, status: %s, reason: %s', diff --git a/lms/djangoapps/instructor_task/models.py b/lms/djangoapps/instructor_task/models.py index 82ceea68a1..d4d0310614 100644 --- a/lms/djangoapps/instructor_task/models.py +++ b/lms/djangoapps/instructor_task/models.py @@ -212,7 +212,7 @@ class ReportStore(object): storage_type = config.get('STORAGE_TYPE', '').lower() if storage_type == 's3': return DjangoStorageReportStore( - storage_class='openedx.core.storage.S3ReportStorage', + storage_class='storages.backends.s3boto.S3BotoStorage', storage_kwargs={ 'bucket': config['BUCKET'], 'location': config['ROOT_PATH'], diff --git a/lms/djangoapps/instructor_task/tests/test_models.py b/lms/djangoapps/instructor_task/tests/test_models.py index 2d16d10454..5eab482b9f 100644 --- a/lms/djangoapps/instructor_task/tests/test_models.py +++ b/lms/djangoapps/instructor_task/tests/test_models.py @@ -125,7 +125,7 @@ class DjangoStorageReportStoreS3TestCase(MockS3Mixin, ReportStoreTestMixin, Test storage. """ test_settings = copy.deepcopy(settings.GRADES_DOWNLOAD) - test_settings['STORAGE_CLASS'] = 'openedx.core.storage.S3ReportStorage' + test_settings['STORAGE_CLASS'] = 'storages.backends.s3boto.S3BotoStorage' test_settings['STORAGE_KWARGS'] = { 'bucket': settings.GRADES_DOWNLOAD['BUCKET'], 'location': settings.GRADES_DOWNLOAD['ROOT_PATH'], diff --git a/openedx/core/storage.py b/openedx/core/storage.py index 3c60bb0949..262ff47580 100644 --- a/openedx/core/storage.py +++ b/openedx/core/storage.py @@ -9,7 +9,6 @@ from django.utils.deconstruct import deconstructible from django.utils.lru_cache import lru_cache from pipeline.storage import NonPackagingMixin, PipelineCachedStorage from require.storage import OptimizedFilesMixin -from storages.backends.s3boto import S3BotoStorage from openedx.core.djangoapps.theming.storage import ThemeCachedFilesMixin, ThemePipelineMixin, ThemeStorage @@ -68,29 +67,6 @@ class DevelopmentStorage( pass -class S3ReportStorage(S3BotoStorage): # pylint: disable=abstract-method - """ - Storage for reports. - """ - def __init__(self, acl=None, bucket=None, custom_domain=None, **settings): - """ - init method for S3ReportStorage, Note that we have added an extra key-word - argument named "custom_domain" and this argument should not be passed to the superclass's init. - - Args: - acl: content policy for the uploads i.e. private, public etc. - bucket: Name of S3 bucket to use for storing and/or retrieving content - custom_domain: custom domain to use for generating file urls - **settings: additional settings to be passed in to S3BotoStorage, - - Returns: - - """ - if custom_domain: - self.custom_domain = custom_domain - super(S3ReportStorage, self).__init__(acl=acl, bucket=bucket, **settings) - - @deconstructible class OverwriteStorage(FileSystemStorage): """ From 151b30922585654ffc193680286d2b6b21871c2f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 18 Dec 2019 14:14:12 -0500 Subject: [PATCH 2/2] Convert custom storage subclasses into mixins so that we can store to either the filesystem or S3 --- cms/envs/production.py | 6 ++++ lms/djangoapps/instructor/tests/test_api.py | 2 +- .../instructor_task/tests/test_models.py | 6 +++- lms/envs/production.py | 6 ++++ openedx/core/djangoapps/theming/finders.py | 2 +- openedx/core/djangoapps/theming/storage.py | 20 +++++------ .../core/lib/django_require/staticstorage.py | 6 ++-- openedx/core/storage.py | 33 ++++++++++++------- requirements/edx/base.in | 2 +- requirements/edx/base.txt | 6 ++-- requirements/edx/development.txt | 6 ++-- requirements/edx/testing.txt | 6 ++-- 12 files changed, 62 insertions(+), 39 deletions(-) diff --git a/cms/envs/production.py b/cms/envs/production.py index 018ecca4e8..b4604bf34e 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -208,6 +208,12 @@ if 'loc_cache' not in CACHES: if 'staticfiles' in CACHES: CACHES['staticfiles']['KEY_PREFIX'] = EDX_PLATFORM_REVISION +# In order to transition from local disk asset storage to S3 backed asset storage, +# we need to run asset collection twice, once for local disk and once for S3. +# Once we have migrated to service assets off S3, then we can convert this back to +# managed by the yaml file contents +STATICFILES_STORAGE = os.environ.get('STATICFILES_STORAGE', ENV_TOKENS.get('STATICFILES_STORAGE', STATICFILES_STORAGE)) + SESSION_COOKIE_DOMAIN = ENV_TOKENS.get('SESSION_COOKIE_DOMAIN') SESSION_COOKIE_HTTPONLY = ENV_TOKENS.get('SESSION_COOKIE_HTTPONLY', True) SESSION_ENGINE = ENV_TOKENS.get('SESSION_ENGINE', SESSION_ENGINE) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 3408f52b4a..b1d56fd423 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -3297,7 +3297,7 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment self.assertIn("attachment; filename=org", response['Content-Disposition']) @patch('lms.djangoapps.instructor_task.models.logger.error') - @patch.dict(settings.GRADES_DOWNLOAD, {'STORAGE_TYPE': 's3'}) + @patch.dict(settings.GRADES_DOWNLOAD, {'STORAGE_TYPE': 's3', 'ROOT_PATH': 'tmp/edx-s3/grades'}) def test_list_report_downloads_error(self, mock_error): """ Tests the Rate-Limit exceeded is handled and does not raise 500 error. diff --git a/lms/djangoapps/instructor_task/tests/test_models.py b/lms/djangoapps/instructor_task/tests/test_models.py index 5eab482b9f..770b234c6a 100644 --- a/lms/djangoapps/instructor_task/tests/test_models.py +++ b/lms/djangoapps/instructor_task/tests/test_models.py @@ -84,7 +84,11 @@ class LocalFSReportStoreTestCase(ReportStoreTestMixin, TestReportMixin, SimpleTe return ReportStore.from_config(config_name='GRADES_DOWNLOAD') -@patch.dict(settings.GRADES_DOWNLOAD, {'STORAGE_TYPE': 's3'}) +@patch.dict(settings.GRADES_DOWNLOAD, { + 'STORAGE_TYPE': 's3', + # Strip the leading `/`, because boto doesn't want it + 'ROOT_PATH': settings.GRADES_DOWNLOAD['ROOT_PATH'].lstrip('/') +}) class S3ReportStoreTestCase(MockS3Mixin, ReportStoreTestMixin, TestReportMixin, SimpleTestCase): """ Test the old S3ReportStore configuration. diff --git a/lms/envs/production.py b/lms/envs/production.py index 7cd00b77b9..8292f1a66e 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -244,6 +244,12 @@ if 'loc_cache' not in CACHES: if 'staticfiles' in CACHES: CACHES['staticfiles']['KEY_PREFIX'] = EDX_PLATFORM_REVISION +# In order to transition from local disk asset storage to S3 backed asset storage, +# we need to run asset collection twice, once for local disk and once for S3. +# Once we have migrated to service assets off S3, then we can convert this back to +# managed by the yaml file contents +STATICFILES_STORAGE = os.environ.get('STATICFILES_STORAGE', ENV_TOKENS.get('STATICFILES_STORAGE', STATICFILES_STORAGE)) + # Email overrides DEFAULT_FROM_EMAIL = ENV_TOKENS.get('DEFAULT_FROM_EMAIL', DEFAULT_FROM_EMAIL) DEFAULT_FEEDBACK_EMAIL = ENV_TOKENS.get('DEFAULT_FEEDBACK_EMAIL', DEFAULT_FEEDBACK_EMAIL) diff --git a/openedx/core/djangoapps/theming/finders.py b/openedx/core/djangoapps/theming/finders.py index ff3c1bc91f..c16082efc3 100644 --- a/openedx/core/djangoapps/theming/finders.py +++ b/openedx/core/djangoapps/theming/finders.py @@ -47,7 +47,7 @@ class ThemeFilesFinder(BaseFinder): themes = get_themes() for theme in themes: theme_storage = self.storage_class( - os.path.join(theme.path, self.source_dir), + location=os.path.join(theme.path, self.source_dir), prefix=theme.theme_dir_name, ) diff --git a/openedx/core/djangoapps/theming/storage.py b/openedx/core/djangoapps/theming/storage.py index a831e3cbbc..7d56081948 100644 --- a/openedx/core/djangoapps/theming/storage.py +++ b/openedx/core/djangoapps/theming/storage.py @@ -28,7 +28,7 @@ from openedx.core.djangoapps.theming.helpers import ( ) -class ThemeStorage(StaticFilesStorage): +class ThemeMixin(object): """ Comprehensive theme aware Static files storage. """ @@ -38,16 +38,10 @@ class ThemeStorage(StaticFilesStorage): # instead of "images/logo.png" prefix = None - def __init__(self, location=None, base_url=None, file_permissions_mode=None, - directory_permissions_mode=None, prefix=None): + def __init__(self, **kwargs): - self.prefix = prefix - super(ThemeStorage, self).__init__( - location=location, - base_url=base_url, - file_permissions_mode=file_permissions_mode, - directory_permissions_mode=directory_permissions_mode, - ) + self.prefix = kwargs.pop('prefix', None) + super(ThemeMixin, self).__init__(**kwargs) def url(self, name): """ @@ -76,7 +70,7 @@ class ThemeStorage(StaticFilesStorage): if prefix and self.themed(name, prefix): name = os.path.join(prefix, name) - return super(ThemeStorage, self).url(name) + return super(ThemeMixin, self).url(name) def themed(self, name, theme): """ @@ -112,6 +106,10 @@ class ThemeStorage(StaticFilesStorage): return self.exists(os.path.join(theme, name)) +class ThemeStorage(ThemeMixin, StaticFilesStorage): + pass + + class ThemeCachedFilesMixin(CachedFilesMixin): """ Comprehensive theme aware CachedFilesMixin. diff --git a/openedx/core/lib/django_require/staticstorage.py b/openedx/core/lib/django_require/staticstorage.py index ee2321e894..df0c543f50 100644 --- a/openedx/core/lib/django_require/staticstorage.py +++ b/openedx/core/lib/django_require/staticstorage.py @@ -2,12 +2,12 @@ :class:`~django_require.staticstorage.OptimizedCachedRequireJsStorage` """ - -from openedx.core.storage import PipelineForgivingStorage +from openedx.core.storage import PipelineForgivingMixin +from pipeline.storage import PipelineCachedStorage from require.storage import OptimizedFilesMixin -class OptimizedCachedRequireJsStorage(OptimizedFilesMixin, PipelineForgivingStorage): +class OptimizedCachedRequireJsStorage(OptimizedFilesMixin, PipelineForgivingMixin, PipelineCachedStorage): """ Custom storage backend that is used by Django-require. """ diff --git a/openedx/core/storage.py b/openedx/core/storage.py index 262ff47580..885e8a62ce 100644 --- a/openedx/core/storage.py +++ b/openedx/core/storage.py @@ -7,19 +7,20 @@ from django.contrib.staticfiles.storage import StaticFilesStorage from django.core.files.storage import get_storage_class, FileSystemStorage from django.utils.deconstruct import deconstructible from django.utils.lru_cache import lru_cache -from pipeline.storage import NonPackagingMixin, PipelineCachedStorage +from pipeline.storage import NonPackagingMixin from require.storage import OptimizedFilesMixin +from storages.backends.s3boto3 import S3Boto3Storage -from openedx.core.djangoapps.theming.storage import ThemeCachedFilesMixin, ThemePipelineMixin, ThemeStorage +from openedx.core.djangoapps.theming.storage import ThemeCachedFilesMixin, ThemePipelineMixin, ThemeMixin -class PipelineForgivingStorage(PipelineCachedStorage): +class PipelineForgivingMixin(object): """ An extension of the django-pipeline storage backend which forgives missing files. """ def hashed_name(self, name, content=None, **kwargs): try: - out = super(PipelineForgivingStorage, self).hashed_name(name, content, **kwargs) + out = super(PipelineForgivingMixin, self).hashed_name(name, content, **kwargs) except ValueError: # This means that a file could not be found, and normally this would # cause a fatal error, which seems rather excessive given that @@ -29,7 +30,7 @@ class PipelineForgivingStorage(PipelineCachedStorage): def stored_name(self, name): try: - out = super(PipelineForgivingStorage, self).stored_name(name) + out = super(PipelineForgivingMixin, self).stored_name(name) except ValueError: # This means that a file could not be found, and normally this would # cause a fatal error, which seems rather excessive given that @@ -38,25 +39,33 @@ class PipelineForgivingStorage(PipelineCachedStorage): return out -class ProductionStorage( - PipelineForgivingStorage, +class ProductionMixin( + PipelineForgivingMixin, OptimizedFilesMixin, ThemePipelineMixin, ThemeCachedFilesMixin, - ThemeStorage, - StaticFilesStorage + ThemeMixin, ): """ - This class combines Django's StaticFilesStorage class with several mixins - that provide additional functionality. We use this version on production. + This class combines several mixins that provide additional functionality, and + can be applied over an existing Storage. + We use this version on production. """ pass +class ProductionStorage(ProductionMixin, StaticFilesStorage): + pass + + +class ProductionS3Storage(ProductionMixin, S3Boto3Storage): # pylint: disable=abstract-method + pass + + class DevelopmentStorage( NonPackagingMixin, ThemePipelineMixin, - ThemeStorage, + ThemeMixin, StaticFilesStorage ): """ diff --git a/requirements/edx/base.in b/requirements/edx/base.in index 97d5192bfa..035f468ece 100644 --- a/requirements/edx/base.in +++ b/requirements/edx/base.in @@ -60,7 +60,7 @@ django-ses django-simple-history django-splash django-statici18n==1.4.0 -django-storages==1.4.1 +django-storages django-user-tasks django-waffle==0.12.0 django-webpack-loader # Used to wire webpack bundles into the django asset pipeline diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6ed1d427d2..b6b24add20 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -81,7 +81,7 @@ django-ses==0.8.13 django-simple-history==2.8.0 django-splash==0.2.5 django-statici18n==1.4.0 -django-storages==1.4.1 +django-storages==1.8 django-user-tasks==0.3.0 django-waffle==0.12.0 django-webpack-loader==0.6.0 @@ -104,7 +104,7 @@ edx-django-release-util==0.3.2 edx-django-sites-extensions==2.4.2 edx-django-utils==2.0.2 edx-drf-extensions==2.4.5 -edx-enterprise==2.0.36 +edx-enterprise==2.0.37 edx-i18n-tools==0.5.0 edx-milestones==0.2.6 edx-oauth2-provider==1.3.1 @@ -175,7 +175,7 @@ paver==1.3.4 pbr==5.4.4 pdfminer.six==20191110 piexif==1.0.2 -pillow==6.2.1 +pillow==7.0.0 pkgconfig==1.5.1 # via xmlsec polib==1.1.0 # via edx-i18n-tools psutil==1.2.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 0a2e64153f..b942a06c48 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -94,7 +94,7 @@ django-ses==0.8.13 django-simple-history==2.8.0 django-splash==0.2.5 django-statici18n==1.4.0 -django-storages==1.4.1 +django-storages==1.8 django-user-tasks==0.3.0 django-waffle==0.12.0 django-webpack-loader==0.6.0 @@ -118,7 +118,7 @@ edx-django-release-util==0.3.2 edx-django-sites-extensions==2.4.2 edx-django-utils==2.0.2 edx-drf-extensions==2.4.5 -edx-enterprise==2.0.36 +edx-enterprise==2.0.37 edx-i18n-tools==0.5.0 edx-lint==1.3.0 edx-milestones==0.2.6 @@ -217,7 +217,7 @@ paver==1.3.4 pbr==5.4.4 pdfminer.six==20191110 piexif==1.0.2 -pillow==6.2.1 +pillow==7.0.0 pip-tools==4.3.0 pkgconfig==1.5.1 pluggy==0.13.1 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index c5531c39f3..2e5bf1f60e 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -92,7 +92,7 @@ django-ses==0.8.13 django-simple-history==2.8.0 django-splash==0.2.5 django-statici18n==1.4.0 -django-storages==1.4.1 +django-storages==1.8 django-user-tasks==0.3.0 django-waffle==0.12.0 django-webpack-loader==0.6.0 @@ -115,7 +115,7 @@ edx-django-release-util==0.3.2 edx-django-sites-extensions==2.4.2 edx-django-utils==2.0.2 edx-drf-extensions==2.4.5 -edx-enterprise==2.0.36 +edx-enterprise==2.0.37 edx-i18n-tools==0.5.0 edx-lint==1.3.0 edx-milestones==0.2.6 @@ -209,7 +209,7 @@ paver==1.3.4 pbr==5.4.4 pdfminer.six==20191110 piexif==1.0.2 -pillow==6.2.1 +pillow==7.0.0 pkgconfig==1.5.1 pluggy==0.13.1 polib==1.1.0