Skip to content

Commit

Permalink
Better checks that static file routing works as expected
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
KevinMind committed Jan 7, 2025
1 parent 9bb6b68 commit ff0f48b
Show file tree
Hide file tree
Showing 10 changed files with 226 additions and 56 deletions.
27 changes: 1 addition & 26 deletions Makefile-docker
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down
17 changes: 11 additions & 6 deletions docker/nginx/addons.conf
Original file line number Diff line number Diff line change
Expand Up @@ -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/ {
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 9 additions & 6 deletions src/olympia/amo/management/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,15 +211,18 @@ def make_dir(self, name: str, force: bool = False) -> None:

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)
2 changes: 1 addition & 1 deletion src/olympia/amo/management/commands/data_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def handle(self, *args, **options):
raise CommandError(f'Storage backup not found: {storage_path}')

cache.clear()
self.clean_storage()
self.make_storage(clean=True)

call_command(
'mediarestore',
Expand Down
2 changes: 1 addition & 1 deletion src/olympia/amo/management/commands/data_seed.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def handle(self, *args, **options):
# 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')

Expand Down
3 changes: 3 additions & 0 deletions src/olympia/amo/management/commands/initialize.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ def handle(self, *args, **options):
# 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',
Expand Down
8 changes: 4 additions & 4 deletions src/olympia/amo/tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ def test_make_dir_non_existing_path(self, mock_makedirs, mock_exists):
@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():
Expand Down Expand Up @@ -650,8 +650,8 @@ def setUp(self):
'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',
),
)

Expand All @@ -668,7 +668,7 @@ def test_default(self):
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'],
Expand Down
71 changes: 62 additions & 9 deletions src/olympia/core/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
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


Expand Down Expand Up @@ -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}'
Expand All @@ -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(
Expand Down Expand Up @@ -151,6 +155,55 @@ 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_ROOT, 'http://nginx/static'),
(settings.STATIC_FILES_PATH, 'http://nginx/static'),
]

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:
open(file_path, 'w').close()
response = requests.get(file_url)

expected_config = (
(response.status_code, 200),
(response.text, ''),
(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',
)
)
finally:
if os.path.exists(file_path):
os.remove(file_path)

return errors


class CoreConfig(AppConfig):
name = 'olympia.core'
verbose_name = _('Core')
Expand Down
Loading

0 comments on commit ff0f48b

Please sign in to comment.