Better checks that static file routing works as expected (#23014)

- Removed obsolete file checks and user validation from Makefile-docker.
- Updated Nginx configuration to improve static file handling and added headers for better traceability.
- Refactored storage management commands in the Python codebase:
  - Renamed `clean_storage` to `make_storage` for clarity and added a `clean` parameter.
  - Updated command implementations to use the new `make_storage` method.
- Introduced a new system check for Nginx configurations to ensure proper file accessibility and response validation.

.gitignore

@@ -55,5 +55,3 @@ tmp/*
 # do not ignore the following files
 !docker-compose.private.yml
 !private/README.md
-!storage/.gitignore
-!deps/.gitkeep

Makefile-docker

@@ -10,14 +10,6 @@ APP=src/olympia/
 
 NODE_MODULES := $(NPM_CONFIG_PREFIX)node_modules/
 
-REQUIRED_FILES := \
-	Makefile \
-	Makefile-os \
-	Makefile-docker \
-	/deps/package.json \
-	/deps/package-lock.json \
-	/addons-server-docker-container \
-
 # Build list of dependencies to install
 DEPS = pip prod
 # If we're running a development image, then we should install the development dependencies
@@ -39,29 +31,12 @@ check_pip_packages: ## check the existence of multiple python packages
 		./scripts/check_pip_packages.sh $$dep.txt; \
 	done
 
-.PHONY: check_files
-check_files: ## check the existence of multiple files
-	@for file in $(REQUIRED_FILES); do test -f "$$file" || (echo "$$file is missing." && exit 1); done
-	@echo "All required files are present."
-
-.PHONY: check_olympia_user
-check_olympia_user: ## check if the olympia user exists and is current user
-	@if [ "$$(id -u olympia)" != "$$(id -u)" ]; then echo "The current user is not the olympia user."; exit 1; fi
-	@echo "The current user is the olympia user."
-
 .PHONY: check_django
 check_django: ## check if the django app is configured properly
 	./manage.py check
 
-.PHONY: check_nginx
-check_nginx:  ## check if the nginx config for local development is configured properly
-	mkdir -p /data/olympia/storage/shared_storage/uploads
-	echo "OK" > /data/olympia/storage/shared_storage/uploads/.check
-	@if [ "$$(curl -sf http://nginx/user-media/.check)" != "OK" ]; then echo "Requesting http://nginx/user-media/.check failed"; exit 1; fi
-	@echo "Nginx user-media configuration looks correct."
-
 .PHONY: check
-check: check_nginx check_files check_olympia_user check_debian_packages check_pip_packages check_django
+check: check_debian_packages check_pip_packages check_django
 
 .PHONY: data_dump
 data_dump:

docker-compose.yml

@@ -47,8 +47,8 @@ services:
       # used by: web, worker, nginx
       - ${HOST_MOUNT_SOURCE:?}:/data/olympia
       - ${HOST_MOUNT_SOURCE:?}deps:/deps
-      - data_site_static:/data/olympia/site-static
-      - ${HOST_MOUNT_SOURCE:?}storage:/data/olympia/storage
+      - ./site-static:/data/olympia/site-static
+      - ./storage:/data/olympia/storage
   worker:
     <<: *olympia
     command: [
@@ -65,7 +65,7 @@ services:
     volumes:
       - ${HOST_MOUNT_SOURCE:?}:/data/olympia
       - ${HOST_MOUNT_SOURCE:?}deps:/deps
-      - ${HOST_MOUNT_SOURCE:?}storage:/data/olympia/storage
+      - ./storage:/data/olympia/storage
     extra_hosts:
      - "olympia.test:127.0.0.1"
     restart: on-failure:5
@@ -94,18 +94,16 @@ services:
       retries: 3
       start_interval: 1s
     volumes:
-      # Don't mount generated files. They only exist in the container
-      # and would otherwiser be deleted by mounting the cwd volume above
-      - data_static_build:/data/olympia/static-build
-      - data_site_static:/data/olympia/site-static
+      - ./static-build:/data/olympia/static-build
+      - ./site-static:/data/olympia/site-static
 
   nginx:
     image: nginx
     volumes:
       - data_nginx:/etc/nginx/conf.d
       - ${HOST_MOUNT_SOURCE:?}:/srv
-      - data_site_static:/srv/site-static
-      - ${HOST_MOUNT_SOURCE:?}storage:/srv/storage
+      - ./site-static:/srv/site-static
+      - ./storage:/srv/storage
     ports:
       - "80:80"
     networks:
@@ -204,10 +202,6 @@ networks:
     enable_ipv6: false
 
 volumes:
-  # Volumes for static files that should not be
-  # mounted from the host.
-  data_static_build:
-  data_site_static:
   # Volumes for the production olympia mounts
   # allowing to conditionally mount directories
   # from the host or from the image to <path>
@@ -218,7 +212,6 @@ volumes:
   # (data_olympia_)<name>:/<path>
   data_olympia_:
   data_olympia_deps:
-  data_olympia_storage:
   # Volume for rabbitmq/redis to avoid anonymous volumes
   data_rabbitmq:
   data_redis:

docker/nginx/addons.conf

@@ -10,17 +10,20 @@ server {
         alias /srv/storage/;
     }
 
-    location /static/ {
-         alias /srv/static/;
+    location ~ ^/static/(.*)$ {
+        add_header X-Served-By "nginx" always;
 
-         # Fallback to the uwsgi server if the file is not found in the static files directory.
-         # This will happen for vendor files from pytnon or npm dependencies that won't be available
-         # in the static files directory.
-         error_page 404 = @olympia;
+        # "root /srv" as we mount all files in this directory
+        root /srv;
+
+        # If it doesn't exist under /srv/site-static/$1,
+        # fall back to /srv/static/$1, else forward to @olympia
+        try_files /site-static/$1 /static/$1 @olympia;
     }
 
     location /user-media/ {
         alias /srv/storage/shared_storage/uploads/;
+        add_header X-Served-By "nginx" always;
     }
 
     location ~ ^/api/ {
@@ -50,6 +53,8 @@ server {
         uwsgi_param   X-Real-IP             $remote_addr;
         uwsgi_param   X-Forwarded-For       $proxy_add_x_forwarded_for;
         uwsgi_param   X-Forwarded-Protocol  ssl;
+
+        add_header X-Served-By "olympia" always;
     }
 
     location @frontendamo {

scripts/setup.py

@@ -3,8 +3,13 @@
 import os
 
 
+root = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))
+
+env_path = os.path.join(root, '.env')
+
+
 def set_env_file(values):
-    with open('.env', 'w') as f:
+    with open(env_path, 'w') as f:
         print('Environment:')
         for key, value in values.items():
             f.write(f'{key}="{value}"\n')
@@ -14,8 +19,8 @@ def set_env_file(values):
 def get_env_file():
     env = {}
 
-    if os.path.exists('.env'):
-        with open('.env', 'r') as f:
+    if os.path.exists(env_path):
+        with open(env_path, 'r') as f:
             for line in f:
                 key, value = line.strip().split('=', 1)
                 env[key] = value.strip('"')
@@ -143,6 +148,10 @@ def main():
         }
     )
 
+    # Create the directories that are expected to exist in the container.
+    for dir in ['deps', 'site-static', 'static-build', 'storage']:
+        os.makedirs(os.path.join(root, dir), exist_ok=True)
+
 
 if __name__ == '__main__':
     main()

src/olympia/amo/management/__init__.py

@@ -211,15 +211,18 @@ class BaseDataCommand(BaseCommand):
 
         os.makedirs(path, exist_ok=True)
 
-    def _clean_storage(self, root: str, dir_dict: dict[str, str | dict]) -> None:
+    def _clean_storage(
+        self, root: str, dir_dict: dict[str, str | dict], clean: bool = False
+    ) -> None:
         for key, value in dir_dict.items():
             curr_path = os.path.join(root, key)
             if isinstance(value, dict):
-                self._clean_storage(curr_path, value)
+                self._clean_storage(curr_path, value, clean=clean)
             else:
-                shutil.rmtree(curr_path, ignore_errors=True)
+                if clean:
+                    shutil.rmtree(curr_path, ignore_errors=True)
                 os.makedirs(curr_path, exist_ok=True)
 
-    def clean_storage(self):
-        self.logger.info('Cleaning storage...')
-        self._clean_storage(settings.STORAGE_ROOT, storage_structure)
+    def make_storage(self, clean: bool = False):
+        self.logger.info('Making storage...')
+        self._clean_storage(settings.STORAGE_ROOT, storage_structure, clean=clean)

src/olympia/amo/management/commands/data_load.py

@@ -38,7 +38,7 @@ class Command(BaseDataCommand):
             raise CommandError(f'Storage backup not found: {storage_path}')
 
         cache.clear()
-        self.clean_storage()
+        self.make_storage(clean=True)
 
         call_command(
             'mediarestore',

src/olympia/amo/management/commands/data_seed.py

@@ -22,7 +22,7 @@ class Command(BaseDataCommand):
         # Delete any local storage files
         # This should happen after we reset the database to ensure any records
         # relying on storage are deleted.
-        self.clean_storage()
+        self.make_storage(clean=True)
         # Migrate the database
         call_command('migrate', '--noinput')
 

src/olympia/amo/management/commands/initialize.py

@@ -73,6 +73,9 @@ class Command(BaseDataCommand):
             # so our database tables should be accessible
             call_command('monitors', services=['database'])
 
+        # Ensure that the storage directories exist.
+        self.make_storage(clean=False)
+
         # Ensure any additional required dependencies are available before proceeding.
         call_command(
             'monitors',

src/olympia/amo/tests/test_commands.py

@@ -489,7 +489,7 @@ class TestBaseDataCommand(BaseTestDataCommand):
     @mock.patch('olympia.amo.management.shutil.rmtree')
     @mock.patch('olympia.amo.management.os.makedirs')
     def test_clean_storage(self, mock_makedirs, mock_rmtree):
-        self.base_data_command.clean_storage()
+        self.base_data_command.make_storage(clean=True)
 
         def walk_keys(root, dir_dict):
             for key, value in dir_dict.items():
@@ -650,8 +650,8 @@ class TestSeedDataCommand(BaseTestDataCommand):
                 'olympia.amo.management.commands.data_seed.BaseDataCommand.clean_dir',
             ),
             (
-                'mock_clean_storage',
-                'olympia.amo.management.commands.data_seed.BaseDataCommand.clean_storage',
+                'mock_make_storage',
+                'olympia.amo.management.commands.data_seed.BaseDataCommand.make_storage',
             ),
         )
 
@@ -668,7 +668,7 @@ class TestSeedDataCommand(BaseTestDataCommand):
         self.mocks['mock_clean_dir'].assert_called_once_with(
             self.base_data_command.data_backup_init
         )
-        self.mocks['mock_clean_storage'].assert_called_once()
+        self.mocks['mock_make_storage'].assert_called_once()
 
         self._assert_commands_called_in_order(
             self.mocks['mock_call_command'],

src/olympia/core/apps.py

@@ -13,6 +13,8 @@ from django.core.management.base import CommandError
 from django.db import connection
 from django.utils.translation import gettext_lazy as _
 
+import requests
+
 from olympia.core.utils import REQUIRED_VERSION_KEYS, get_version_json
 
 
@@ -93,16 +95,10 @@ def static_check(app_configs, **kwargs):
 
     try:
         call_command('compress_assets', dry_run=True, stdout=output)
-        file_paths = output.getvalue().strip().split('\n')
+        stripped_output = output.getvalue().strip()
 
-        if not file_paths:
-            errors.append(
-                Error(
-                    'No compressed asset files were found.',
-                    id='setup.E003',
-                )
-            )
-        else:
+        if stripped_output:
+            file_paths = stripped_output.split('\n')
             for file_path in file_paths:
                 if not os.path.exists(file_path):
                     error = f'Compressed asset file does not exist: {file_path}'
@@ -112,6 +108,14 @@ def static_check(app_configs, **kwargs):
                             id='setup.E003',
                         )
                     )
+        else:
+            errors.append(
+                Error(
+                    'No compressed asset files were found.',
+                    id='setup.E003',
+                )
+            )
+
     except CommandError as e:
         errors.append(
             Error(
@@ -151,6 +155,61 @@ def db_charset_check(app_configs, **kwargs):
     return errors
 
 
+@register(CustomTags.custom_setup)
+def nginx_check(app_configs, **kwargs):
+    errors = []
+    version = get_version_json()
+
+    if version.get('target') == 'production':
+        return []
+
+    configs = [
+        (settings.MEDIA_ROOT, 'http://nginx/user-media'),
+        (settings.STATIC_FILES_PATH, 'http://nginx/static'),
+        (settings.STATIC_ROOT, 'http://nginx/static'),
+    ]
+
+    files_to_remove = []
+
+    for dir, base_url in configs:
+        file_path = os.path.join(dir, 'test.txt')
+        file_url = f'{base_url}/test.txt'
+
+        if not os.path.exists(dir):
+            errors.append(Error(f'{dir} does not exist', id='setup.E007'))
+
+        try:
+            with open(file_path, 'w') as f:
+                f.write(dir)
+
+            files_to_remove.append(file_path)
+            response = requests.get(file_url)
+
+            expected_config = (
+                (response.status_code, 200),
+                (response.text, dir),
+                (response.headers.get('X-Served-By'), 'nginx'),
+            )
+
+            if any(item[0] != item[1] for item in expected_config):
+                message = f'Failed to access {file_url}. {expected_config}'
+                errors.append(Error(message, id='setup.E008'))
+
+        except Exception as e:
+            errors.append(
+                Error(
+                    f'Unknown error accessing {file_path} via {file_url}: {e}',
+                    id='setup.E010',
+                )
+            )
+
+    # Always remove the files we created.
+    for file_path in files_to_remove:
+        os.remove(file_path)
+
+    return errors
+
+
 class CoreConfig(AppConfig):
     name = 'olympia.core'
     verbose_name = _('Core')

src/olympia/core/tests/test_apps.py

@@ -1,10 +1,14 @@
+import os
+import tempfile
 from unittest import mock
 
-from django.core.management import call_command
+from django.core.management import CommandError, call_command
 from django.core.management.base import SystemCheckError
 from django.test import TestCase
 from django.test.utils import override_settings
 
+import responses
+
 from olympia.core.utils import REQUIRED_VERSION_KEYS
 
 
@@ -20,11 +24,25 @@ class SystemCheckIntegrationTest(TestCase):
         }
         patch = mock.patch(
             'olympia.core.apps.get_version_json',
-            return_value=self.default_version_json,
         )
         self.mock_get_version_json = patch.start()
+        self.mock_get_version_json.return_value = self.default_version_json
         self.addCleanup(patch.stop)
 
+        mock_dir = tempfile.mkdtemp(prefix='static-root')
+        self.fake_css_file = os.path.join(mock_dir, 'foo.css')
+        with open(self.fake_css_file, 'w') as f:
+            f.write('body { background: red; }')
+
+        patch_command = mock.patch('olympia.core.apps.call_command')
+        self.mock_call_command = patch_command.start()
+        self.mock_call_command.side_effect = (
+            lambda command, dry_run, stdout: stdout.write(f'{self.fake_css_file}\n')
+        )
+        self.addCleanup(patch_command.stop)
+
+        self.media_root = tempfile.mkdtemp(prefix='media-root')
+
     @mock.patch('olympia.core.apps.connection.cursor')
     def test_db_charset_check(self, mock_cursor):
         mock_cursor.return_value.__enter__.return_value.fetchone.return_value = (
@@ -85,3 +103,115 @@ class SystemCheckIntegrationTest(TestCase):
 
         with override_settings(HOST_UID=1000):
             call_command('check')
+
+    def test_static_check_no_assets_found(self):
+        """
+        Test static_check fails if compress_assets reports no files.
+        """
+        self.mock_get_version_json.return_value['target'] = 'production'
+        # Simulate "compress_assets" returning no file paths.
+        self.mock_call_command.side_effect = (
+            lambda command, dry_run, stdout: stdout.write('')
+        )
+        with self.assertRaisesMessage(
+            SystemCheckError, 'No compressed asset files were found.'
+        ):
+            call_command('check')
+
+    @mock.patch('os.path.exists')
+    def test_static_check_missing_assets(self, mock_exists):
+        """
+        Test static_check fails if at least one specified compressed
+        asset file does not exist.
+        """
+        self.mock_get_version_json.return_value['target'] = 'production'
+        # Simulate "compress_assets" returning a couple of files.
+        self.mock_call_command.side_effect = (
+            lambda command, dry_run, stdout: stdout.write(
+                f'{self.fake_css_file}\nfoo.js\n'
+            )
+        )
+        # Pretend neither file exists on disk.
+        mock_exists.return_value = False
+
+        with self.assertRaisesMessage(
+            SystemCheckError,
+            # Only the first missing file triggers the AssertionError message check
+            'Compressed asset file does not exist: foo.js',
+        ):
+            call_command('check')
+
+    def test_static_check_command_error(self):
+        """
+        Test static_check fails if there's an error during compress_assets.
+        """
+        self.mock_get_version_json.return_value['target'] = 'production'
+        self.mock_call_command.side_effect = CommandError('Oops')
+        with self.assertRaisesMessage(
+            SystemCheckError, 'Error running compress_assets command: Oops'
+        ):
+            call_command('check')
+
+    def test_static_check_command_success(self):
+        """
+        Test static_check succeeds if compress_assets runs without errors.
+        """
+        self.mock_get_version_json.return_value['target'] = 'production'
+        self.mock_call_command.side_effect = (
+            lambda command, dry_run, stdout: stdout.write(f'{self.fake_css_file}\n')
+        )
+        call_command('check')
+
+    def test_nginx_skips_check_on_production_target(self):
+        fake_media_root = '/fake/not/real'
+        with override_settings(MEDIA_ROOT=fake_media_root):
+            call_command('check')
+
+    def test_nginx_raises_missing_directory(self):
+        self.mock_get_version_json.return_value['target'] = 'development'
+        fake_media_root = '/fake/not/real'
+        with override_settings(MEDIA_ROOT=fake_media_root):
+            with self.assertRaisesMessage(
+                SystemCheckError,
+                f'{fake_media_root} does not exist',
+            ):
+                call_command('check')
+
+    def _test_nginx_response(
+        self, base_url, status_code=200, response_text='', served_by='nginx'
+    ):
+        self.mock_get_version_json.return_value['target'] = 'development'
+        url = f'{base_url}/test.txt'
+
+        responses.add(
+            responses.GET,
+            url,
+            status=status_code,
+            body=response_text,
+            headers={'X-Served-By': served_by},
+        )
+
+        expected_config = (
+            (status_code, 200),
+            (response_text, self.media_root),
+            (served_by, 'nginx'),
+        )
+
+        with override_settings(MEDIA_ROOT=self.media_root):
+            with self.assertRaisesMessage(
+                SystemCheckError,
+                f'Failed to access {url}. {expected_config}',
+            ):
+                call_command('check')
+
+    def test_nginx_raises_non_200_status_code(self):
+        """Test that files return a 200 status code."""
+        self._test_nginx_response('http://nginx/user-media', status_code=404)
+
+    def test_nginx_raises_unexpected_content(self):
+        """Test that files return the expected content."""
+        self._test_nginx_response('http://nginx/user-media', response_text='foo')
+
+    def test_nginx_raises_unexpected_served_by(self):
+        """Test that files are served by nginx and not redirected elsewhere."""
+        self._test_nginx_response('http://nginx/user-media', served_by='wow')

src/olympia/lib/settings_base.py

@@ -1333,9 +1333,10 @@ NODE_PACKAGE_JSON = os.path.join('/', 'deps', 'package.json')
 NODE_PACKAGE_MANAGER_INSTALL_OPTIONS = ['--dry-run']
 
 STATIC_BUILD_PATH = path('static-build')
+STATIC_FILES_PATH = path('static')
 
 STATICFILES_DIRS = (
-    path('static'),
+    STATIC_FILES_PATH,
     STATIC_BUILD_PATH,
 )
 

tests/make/make.spec.js

@@ -105,9 +105,7 @@ describe('docker-compose.yml', () => {
               target: '/data/olympia',
             }),
             expect.objectContaining({
-              source: isProdMountTarget
-                ? 'data_olympia_storage'
-                : expect.any(String),
+              source: expect.any(String),
               target: '/data/olympia/storage',
             }),
           ]),
@@ -125,11 +123,11 @@ describe('docker-compose.yml', () => {
       expect(web.volumes).toEqual(
         expect.arrayContaining([
           expect.objectContaining({
-            source: 'data_static_build',
+            source: expect.any(String),
             target: '/data/olympia/static-build',
           }),
           expect.objectContaining({
-            source: 'data_site_static',
+            source: expect.any(String),
             target: '/data/olympia/site-static',
           }),
         ]),
@@ -162,14 +160,12 @@ describe('docker-compose.yml', () => {
             target: '/srv',
           }),
           expect.objectContaining({
-            source: 'data_site_static',
+            source: expect.any(String),
             target: '/srv/site-static',
           }),
           // mapping for local host directory to /data/olympia/storage
           expect.objectContaining({
-            source: isProdMountTarget
-              ? 'data_olympia_storage'
-              : expect.any(String),
+            source: expect.any(String),
             target: '/srv/storage',
           }),
         ]),

tests/make/test_setup.py

@@ -229,3 +229,15 @@ class TestOlympiaDeps(BaseTestClass):
     def test_olympia_deps_override(self):
         main()
         self.assert_set_env_file_called_with(OLYMPIA_DEPS='test')
+
+
+@override_env()
+@mock.patch('scripts.setup.os.makedirs')
+def test_make_dirs(mock_makedirs):
+    from scripts.setup import root
+
+    main()
+    assert mock_makedirs.call_args_list == [
+        mock.call(os.path.join(root, dir), exist_ok=True)
+        for dir in ['deps', 'site-static', 'static-build', 'storage']
+    ]