From ea0dc7bbcdbd61749bc313d043151e62029bf1bc Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Mon, 9 Dec 2024 23:18:15 +0100 Subject: [PATCH] Include DOCKER_* build info statically in image - Modified `Dockerfile` to expose build arguments and create a static build info file, ensuring that build variables are hard-coded and not overridden at runtime. - Refactored `get_version_json` function in `utils.py` to read build information from the newly created static file, ensuring required keys are validated. - Updated tests in `test_apps.py` and `test_utils.py` to check for required version keys and validate the build info retrieval process. --- Dockerfile | 21 +++++ Makefile-os | 4 + docker-bake.hcl | 8 +- src/olympia/core/apps.py | 13 +-- src/olympia/core/tests/test_apps.py | 127 +++++++++++++++++--------- src/olympia/core/tests/test_utils.py | 129 ++++++++++++++++----------- src/olympia/core/utils.py | 39 ++++++-- 7 files changed, 225 insertions(+), 116 deletions(-) diff --git a/Dockerfile b/Dockerfile index 4d06ab992915..217b91975a6e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -46,7 +46,28 @@ RUN localedef -i en_US -f UTF-8 en_US.UTF-8 ENV LANG=en_US.UTF-8 ENV LC_ALL=en_US.UTF-8 +# Build args that determine the tag and stage of the build +# These are passed to docker via the bake.hcl file +ARG DOCKER_COMMIT +ARG DOCKER_VERSION +ARG DOCKER_BUILD +ARG DOCKER_TARGET + +# Expose these variables via hard coded file to prevent them from being +# overridden by the environment when running the container. These +# values represent the build and should not be changable afterwards. +ENV BUILD_INFO=/build-info RUN < ${BUILD_INFO} +commit="${DOCKER_COMMIT}" +version="${DOCKER_VERSION}" +build="${DOCKER_BUILD}" +target="${DOCKER_TARGET}" +INNEREOF +# Set permissions to make the file readable by all but only writable by root +chmod 644 ${BUILD_INFO} + # Create directory for dependencies mkdir /deps chown -R olympia:olympia /deps diff --git a/Makefile-os b/Makefile-os index 0af7990eb867..48960bf73030 100644 --- a/Makefile-os +++ b/Makefile-os @@ -7,6 +7,10 @@ DOCKER_PROGRESS ?= auto DOCKER_METADATA_FILE ?= buildx-bake-metadata.json DOCKER_PUSH ?= +# Values that are not saved to .env +# but should be set in the docker image +# default to static values to prevent +# invalidating docker build cache export DOCKER_COMMIT ?= export DOCKER_BUILD ?= export DOCKER_VERSION ?= diff --git a/docker-bake.hcl b/docker-bake.hcl index 746b9034c88e..3820ebdae43e 100644 --- a/docker-bake.hcl +++ b/docker-bake.hcl @@ -15,10 +15,10 @@ target "web" { tags = ["${DOCKER_TAG}"] platforms = ["linux/amd64"] args = { - DOCKER_COMMIT = "${DOCKER_COMMIT}" - DOCKER_VERSION = "${DOCKER_VERSION}" - DOCKER_BUILD = "${DOCKER_BUILD}" - DOCKER_TARGET = "${DOCKER_TARGET}" + DOCKER_COMMIT = "${DOCKER_COMMIT}" + DOCKER_VERSION = "${DOCKER_VERSION}" + DOCKER_BUILD = "${DOCKER_BUILD}" + DOCKER_TARGET = "${DOCKER_TARGET}" } pull = true diff --git a/src/olympia/core/apps.py b/src/olympia/core/apps.py index 7329d9bcfa39..848fc4b67d98 100644 --- a/src/olympia/core/apps.py +++ b/src/olympia/core/apps.py @@ -12,7 +12,10 @@ from django.db import connection from django.utils.translation import gettext_lazy as _ -from olympia.core.utils import get_version_json +from olympia.core.utils import ( + REQUIRED_VERSION_KEYS, + get_version_json, +) log = logging.getLogger('z.startup') @@ -64,11 +67,9 @@ def host_check(app_configs, **kwargs): @register(CustomTags.custom_setup) def version_check(app_configs, **kwargs): """Check the (virtual) version.json file exists and has the correct keys.""" - required_keys = ['version', 'build', 'commit', 'source'] - version = get_version_json() - missing_keys = [key for key in required_keys if key not in version] + missing_keys = [key for key in REQUIRED_VERSION_KEYS if key not in version] if missing_keys: return [ @@ -85,8 +86,10 @@ def version_check(app_configs, **kwargs): def static_check(app_configs, **kwargs): errors = [] output = StringIO() + version = get_version_json() - if settings.DEV_MODE: + # We only run this check in production images. + if version.get('target') != 'production': return [] try: diff --git a/src/olympia/core/tests/test_apps.py b/src/olympia/core/tests/test_apps.py index 13cd7d4cf8c4..b9a4106850b7 100644 --- a/src/olympia/core/tests/test_apps.py +++ b/src/olympia/core/tests/test_apps.py @@ -1,3 +1,4 @@ +import os from unittest import mock from django.core.management import call_command @@ -5,34 +6,25 @@ from django.test import TestCase from django.test.utils import override_settings +from olympia.core.utils import REQUIRED_VERSION_KEYS -class SystemCheckIntegrationTest(TestCase): - @mock.patch('olympia.core.apps.os.getuid') - def test_illegal_override_uid_check(self, mock_getuid): - """ - If the HOST_UID is not set or if it is not set to the - olympia user actual uid, then file ownership is probably - incorrect and we should fail the check. - """ - dummy_uid = '1000' - olympia_uid = '9500' - for host_uid in [None, olympia_uid]: - with override_settings(HOST_UID=host_uid): - with self.subTest(host_uid=host_uid): - mock_getuid.return_value = int(dummy_uid) - with self.assertRaisesMessage( - SystemCheckError, - f'Expected user uid to be {olympia_uid}, received {dummy_uid}', - ): - call_command('check') - with override_settings(HOST_UID=dummy_uid): - mock_getuid.return_value = int(olympia_uid) - with self.assertRaisesMessage( - SystemCheckError, - f'Expected user uid to be {dummy_uid}, received {olympia_uid}', - ): - call_command('check') +class SystemCheckIntegrationTest(TestCase): + def setUp(self): + self.default_version_json = { + 'tag': 'mozilla/addons-server:1.0', + 'target': 'production', + 'commit': 'abc', + 'version': '1.0', + 'build': 'http://example.com/build', + 'source': 'https://github.com/mozilla/addons-server', + } + patch = mock.patch( + 'olympia.core.apps.get_version_json', + return_value=self.default_version_json, + ) + self.mock_get_version_json = patch.start() + self.addCleanup(patch.stop) @mock.patch('olympia.core.apps.connection.cursor') def test_db_charset_check(self, mock_cursor): @@ -56,29 +48,76 @@ def test_uwsgi_check(self): ): call_command('check') - def test_version_missing_key(self): - call_command('check') - - with mock.patch('olympia.core.apps.get_version_json') as get_version_json: - keys = ['version', 'build', 'commit', 'source'] - version_mock = {key: 'test' for key in keys} - - for key in keys: - version = version_mock.copy() - version.pop(key) - get_version_json.return_value = version - + def test_missing_version_keys_check(self): + """ + We expect all required version keys to be set during the docker build. + """ + for broken_key in REQUIRED_VERSION_KEYS: + with self.subTest(broken_key=broken_key): + del self.mock_get_version_json.return_value[broken_key] with self.assertRaisesMessage( - SystemCheckError, f'{key} is missing from version.json' + SystemCheckError, + f'Expected key: {broken_key} to exist', ): call_command('check') - def test_version_missing_multiple_keys(self): - call_command('check') + def test_dockerignore_file_exists_check(self): + """ + In production, or when the host mount is set to production, we expect + not to find docker ignored files like Makefile-os in the file system. + """ + original_exists = os.path.exists + + def mock_exists(path): + return path == '/data/olympia/Makefile-os' or original_exists(path) + + for host_mount in (None, 'production'): + with self.subTest(host_mount=host_mount): + with override_settings(OLYMPIA_MOUNT=host_mount): + with mock.patch('os.path.exists', side_effect=mock_exists): + with self.assertRaisesMessage( + SystemCheckError, + 'Makefile-os should be excluded by dockerignore', + ): + call_command('check') + + @override_settings(OLYMPIA_UID=None) + @mock.patch('olympia.core.apps.os.getuid') + def test_illegal_override_uid_check(self, mock_getuid): + """ + In production, or when OLYMPIA_UID is not set, we expect to not override + the default uid of 9500 for the olympia user. + """ + mock_getuid.return_value = 1000 + with self.assertRaisesMessage( + SystemCheckError, + 'Expected user uid to be 9500', + ): + call_command('check') - with mock.patch('olympia.core.apps.get_version_json') as get_version_json: - get_version_json.return_value = {'version': 'test', 'build': 'test'} + @mock.patch('olympia.core.apps.os.getuid') + def test_illegal_override_uid_check(self, mock_getuid): + """ + If the HOST_UID is not set or if it is not set to the + olympia user actual uid, then file ownership is probably + incorrect and we should fail the check. + """ + dummy_uid = '1000' + olympia_uid = '9500' + for host_uid in [None, olympia_uid]: + with override_settings(HOST_UID=host_uid): + with self.subTest(host_uid=host_uid): + mock_getuid.return_value = int(dummy_uid) + with self.assertRaisesMessage( + SystemCheckError, + f'Expected user uid to be {olympia_uid}, received {dummy_uid}', + ): + call_command('check') + + with override_settings(HOST_UID=dummy_uid): + mock_getuid.return_value = int(olympia_uid) with self.assertRaisesMessage( - SystemCheckError, 'commit, source is missing from version.json' + SystemCheckError, + f'Expected user uid to be {dummy_uid}, received {olympia_uid}', ): call_command('check') diff --git a/src/olympia/core/tests/test_utils.py b/src/olympia/core/tests/test_utils.py index 1fc6719ef0be..e427bc7a3f67 100644 --- a/src/olympia/core/tests/test_utils.py +++ b/src/olympia/core/tests/test_utils.py @@ -1,88 +1,109 @@ import json +import os +import shutil import sys -from unittest import mock +import tempfile +from unittest import TestCase, mock from olympia.core.utils import get_version_json default_version = { - 'commit': 'commit', + 'commit': '', 'version': 'local', - 'build': 'build', + 'build': '', + 'target': 'development', 'source': 'https://github.com/mozilla/addons-server', } -@mock.patch.dict('os.environ', clear=True) -def test_get_version_json_defaults(): - result = get_version_json() +class TestGetVersionJson(TestCase): + def setUp(self): + self.tmp_dir = tempfile.mkdtemp() + self.build_info_path = os.path.join(self.tmp_dir, 'build-info') + self.pkg_json_path = os.path.join(self.tmp_dir, 'package.json') - assert result['commit'] == default_version['commit'] - assert result['version'] == default_version['version'] - assert result['build'] == default_version['build'] - assert result['source'] == default_version['source'] + self.with_build_info() + self.with_pkg_json({}) + def tearDown(self): + shutil.rmtree(self.tmp_dir) -def test_get_version_json_commit(): - with mock.patch.dict('os.environ', {'DOCKER_COMMIT': 'new_commit'}): - result = get_version_json() + def with_build_info(self, **kwargs): + data = '\n'.join( + f'{key}="{value}"' for key, value in {**default_version, **kwargs}.items() + ) + with open(self.build_info_path, 'w') as f: + f.write(data) + f.close() - assert result['commit'] == 'new_commit' + def with_pkg_json(self, data): + with open(self.pkg_json_path, 'w') as f: + f.write(json.dumps(data)) + f.close() + def test_get_version_json_defaults(self): + result = get_version_json(build_info_path=self.build_info_path) -def test_get_version_json_version(): - with mock.patch.dict('os.environ', {'DOCKER_VERSION': 'new_version'}): - result = get_version_json() + assert result['commit'] == default_version['commit'] + assert result['version'] == default_version['version'] + assert result['build'] == default_version['build'] + assert result['source'] == default_version['source'] - assert result['version'] == 'new_version' + def test_get_version_json_commit(self): + self.with_build_info(commit='new_commit') + result = get_version_json(build_info_path=self.build_info_path) + assert result['commit'] == 'new_commit' -def test_get_version_json_build(): - with mock.patch.dict('os.environ', {'DOCKER_BUILD': 'new_build'}): - result = get_version_json() + def test_get_version_json_version(self): + self.with_build_info(version='new_version') + result = get_version_json(build_info_path=self.build_info_path) - assert result['build'] == 'new_build' + assert result['version'] == 'new_version' + def test_get_version_json_build(self): + self.with_build_info(build='new_build') + result = get_version_json(build_info_path=self.build_info_path) -def test_get_version_json_python(): - with mock.patch.object(sys, 'version_info') as v_info: - v_info.major = 3 - v_info.minor = 9 - result = get_version_json() + assert result['build'] == 'new_build' - assert result['python'] == '3.9' + def test_get_version_json_python(self): + with mock.patch.object(sys, 'version_info') as v_info: + v_info.major = 3 + v_info.minor = 9 + result = get_version_json(build_info_path=self.build_info_path) + assert result['python'] == '3.9' -def test_get_version_json_django(): - with mock.patch('django.VERSION', (3, 2)): - result = get_version_json() + def test_get_version_json_django(self): + with mock.patch('django.VERSION', (3, 2)): + result = get_version_json(build_info_path=self.build_info_path) - assert result['django'] == '3.2' + assert result['django'] == '3.2' + def test_get_version_json_addons_linter(self): + self.with_pkg_json({'dependencies': {'addons-linter': '1.2.3'}}) + result = get_version_json( + build_info_path=self.build_info_path, + pkg_json_path=self.pkg_json_path, + ) -def test_get_version_json_addons_linter(): - with mock.patch('os.path.exists', return_value=True): - with mock.patch( - 'builtins.open', - mock.mock_open(read_data='{"dependencies": {"addons-linter": "1.2.3"}}'), - ): - result = get_version_json() + assert result['addons-linter'] == '1.2.3' - assert result['addons-linter'] == '1.2.3' + def test_get_version_json_addons_linter_missing_package(self): + self.with_pkg_json({'dependencies': {}}) + result = get_version_json( + build_info_path=self.build_info_path, + pkg_json_path=self.pkg_json_path, + ) + assert result['addons-linter'] == '' -def test_get_version_json_addons_linter_missing_package(): - with mock.patch('os.path.exists', return_value=True): - with mock.patch( - 'builtins.open', mock.mock_open(read_data=json.dumps({'dependencies': {}})) - ): - result = get_version_json() + def test_get_version_json_addons_linter_missing_file(self): + result = get_version_json( + build_info_path=self.build_info_path, + pkg_json_path=self.pkg_json_path, + ) - assert result['addons-linter'] == '' - - -def test_get_version_json_addons_linter_missing_file(): - with mock.patch('os.path.exists', return_value=False): - result = get_version_json() - - assert result['addons-linter'] == '' + assert result['addons-linter'] == '' diff --git a/src/olympia/core/utils.py b/src/olympia/core/utils.py index f294026904e4..936e81ea13fd 100644 --- a/src/olympia/core/utils.py +++ b/src/olympia/core/utils.py @@ -1,19 +1,42 @@ import json import os import sys +from functools import cache import django -def get_version_json(): - contents = {} +# Keys exempt from inspection in local images as they must be set. +EXEMPT_REQUIRED_KEYS = ['target', 'version', 'source'] - root = os.path.join(os.path.dirname(os.path.dirname(__file__)), '..', '..') +# Keys required to be set in the version.json file. +REQUIRED_VERSION_KEYS = [*EXEMPT_REQUIRED_KEYS, 'commit', 'build'] - contents['commit'] = os.environ.get('DOCKER_COMMIT', 'commit') - contents['version'] = os.environ.get('DOCKER_VERSION', 'local') - contents['source'] = 'https://github.com/mozilla/addons-server' - contents['build'] = os.environ.get('DOCKER_BUILD', 'build') +root = os.path.join(os.path.dirname(os.path.dirname(__file__)), '..', '..') +pkg_json_path = os.path.join(root, 'package.json') + + +@cache +def get_version_json( + build_info_path=os.environ['BUILD_INFO'], + pkg_json_path=pkg_json_path, +): + contents = {'source': 'https://github.com/mozilla/addons-server'} + + # Read the build info from the docker image. + # This is static read only data that cannot + # be overridden at runtime. + with open(build_info_path) as f: + for line in f: + key, value = line.strip().split('=', 1) + if value.startswith('"') and value.endswith('"'): + value = value[1:-1] + contents[key] = value + + # Ensure all the expected keys are present. + for key in REQUIRED_VERSION_KEYS: + if key not in contents: + raise ValueError(f'{key} is not set in the docker image') py_info = sys.version_info contents['python'] = '{major}.{minor}'.format( @@ -23,8 +46,6 @@ def get_version_json(): major=django.VERSION[0], minor=django.VERSION[1] ) - pkg_json_path = os.path.join(root, 'package.json') - if os.path.exists(pkg_json_path): with open(pkg_json_path) as f: data = json.loads(f.read())