From 5cb7b39f7c981bb20bba966c38d47b30f5ff5a4c Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Tue, 10 Dec 2024 20:59:13 +0100 Subject: [PATCH] Include DOCKER_* build info statically in image: - Updated Dockerfile to create a build info file containing static build variables (commit, version, build, target) that are now read-only at runtime. - Modified get_version_json function to read build information from the newly created build info file instead of relying on environment variables. - Introduced REQUIRED_VERSION_KEYS to ensure all necessary keys are present in version.json during checks. - Enhanced tests to validate the presence of required version keys and ensure proper functionality of the get_version_json method. - Updated docker-bake.hcl to maintain consistency in build arguments. --- Dockerfile | 34 +++++-- Makefile-os | 3 + docker-bake.hcl | 9 +- src/olympia/core/apps.py | 10 +-- src/olympia/core/tests/test_apps.py | 76 ++++++---------- src/olympia/core/tests/test_utils.py | 129 ++++++++++++++++----------- src/olympia/core/utils.py | 31 +++++-- tests/make/make.spec.js | 30 +++++++ 8 files changed, 194 insertions(+), 128 deletions(-) diff --git a/Dockerfile b/Dockerfile index 4d06ab992915..258022e8a457 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,6 +4,8 @@ FROM python:3.11-slim-bookworm AS olympia +ENV BUILD_INFO=/build-info + # Set shell to bash with logs and errors for build SHELL ["/bin/bash", "-xue", "-c"] @@ -18,6 +20,29 @@ ENV HOME=/data/olympia WORKDIR ${HOME} RUN chown -R olympia:olympia ${HOME} +FROM olympia AS info + +# Build args that represent static build information +# These are passed to docker via the bake.hcl file and +# should not be overridden in the container environment. +ARG DOCKER_COMMIT +ARG DOCKER_VERSION +ARG DOCKER_BUILD +ARG DOCKER_TARGET + +# Create the build file hard coding build variables to the image +RUN < ${BUILD_INFO} +commit="${DOCKER_COMMIT}" +version="${DOCKER_VERSION}" +build="${DOCKER_BUILD}" +target="${DOCKER_TARGET}" +source="https://github.com/mozilla/addons-server" +INNEREOF +# Set permissions to make the file readable by all but only writable by root +chmod 644 ${BUILD_INFO} +EOF + FROM olympia AS base # Add keys and repos for node and mysql # TODO: replace this with a bind mount on the RUN command @@ -95,8 +120,8 @@ rm -rf /deps/build/* ${PIP_COMMAND} install --progress-bar=off --no-deps --exists-action=w -r requirements/pip.txt EOF -# Expose the DOCKER_TARGET variable to all subsequent stages -# This value is used to determine if we are building for production or development +# TODO: we should remove dependency on the environment variable +# and instead read from the /build-info file ARG DOCKER_TARGET ENV DOCKER_TARGET=${DOCKER_TARGET} @@ -174,11 +199,7 @@ EOF FROM base AS sources -ARG DOCKER_BUILD DOCKER_COMMIT DOCKER_VERSION -ENV DOCKER_BUILD=${DOCKER_BUILD} -ENV DOCKER_COMMIT=${DOCKER_COMMIT} -ENV DOCKER_VERSION=${DOCKER_VERSION} # Add our custom mime types (required for for ts/json/md files) COPY docker/etc/mime.types /etc/mime.types @@ -187,6 +208,7 @@ COPY --chown=olympia:olympia . ${HOME} # Copy assets from assets COPY --from=assets --chown=olympia:olympia ${HOME}/site-static ${HOME}/site-static COPY --from=assets --chown=olympia:olympia ${HOME}/static-build ${HOME}/static-build +COPY --from=info ${BUILD_INFO} ${BUILD_INFO} # Set shell back to sh until we can prove we can use bash at runtime SHELL ["/bin/sh", "-c"] diff --git a/Makefile-os b/Makefile-os index 0af7990eb867..c0c5f9930631 100644 --- a/Makefile-os +++ b/Makefile-os @@ -7,6 +7,9 @@ DOCKER_PROGRESS ?= auto DOCKER_METADATA_FILE ?= buildx-bake-metadata.json DOCKER_PUSH ?= +# Not in dot env saved, +# Docker needs these values set, +# Static, cache preserved. export DOCKER_COMMIT ?= export DOCKER_BUILD ?= export DOCKER_VERSION ?= diff --git a/docker-bake.hcl b/docker-bake.hcl index 746b9034c88e..c1944889b061 100644 --- a/docker-bake.hcl +++ b/docker-bake.hcl @@ -15,10 +15,11 @@ 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}" + DOCKER_SOURCE = "https://github.com/mozilla/addons-server" } pull = true diff --git a/src/olympia/core/apps.py b/src/olympia/core/apps.py index 7329d9bcfa39..0b45f3b63dcf 100644 --- a/src/olympia/core/apps.py +++ b/src/olympia/core/apps.py @@ -12,7 +12,7 @@ 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 +64,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 +83,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..4073387ade55 100644 --- a/src/olympia/core/tests/test_apps.py +++ b/src/olympia/core/tests/test_apps.py @@ -3,36 +3,26 @@ from django.core.management import call_command from django.core.management.base import SystemCheckError 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 +46,15 @@ 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'{broken_key} is missing from version.json', ): call_command('check') - - def test_version_missing_multiple_keys(self): - 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'} - with self.assertRaisesMessage( - SystemCheckError, 'commit, source is missing from version.json' - ): - 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..987487377893 100644 --- a/src/olympia/core/utils.py +++ b/src/olympia/core/utils.py @@ -5,15 +5,30 @@ import django -def get_version_json(): - contents = {} - root = os.path.join(os.path.dirname(os.path.dirname(__file__)), '..', '..') +# Keys required to be set in the version.json file. +REQUIRED_VERSION_KEYS = ['target', 'version', 'source', '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') + + +def get_version_json( + build_info_path=os.environ['BUILD_INFO'], + pkg_json_path=pkg_json_path, +): + contents = {key: '' for key in REQUIRED_VERSION_KEYS} + + # Read the build info from the docker image. + # This is static read only data that cannot + # be overridden at runtime. + if os.path.exists(build_info_path): + 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 py_info = sys.version_info contents['python'] = '{major}.{minor}'.format( @@ -23,8 +38,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()) diff --git a/tests/make/make.spec.js b/tests/make/make.spec.js index e2effe24248f..ea92798d9cb2 100644 --- a/tests/make/make.spec.js +++ b/tests/make/make.spec.js @@ -220,6 +220,36 @@ describe('docker-compose.yml', () => { } } }); + + describe.each(['development', 'production'])( + 'DOCKER_TARGET=%s', + (DOCKER_TARGET) => { + const FILTERED_KEYS = [ + 'DOCKER_COMMIT', + 'DOCKER_VERSION', + 'DOCKER_BUILD', + ]; + it('.services.(web|worker).environment excludes build info variables', () => { + const { + services: { web, worker }, + } = getConfig({ + COMPOSE_FILE, + DOCKER_TARGET, + ...Object.fromEntries( + FILTERED_KEYS.map((key) => [key, 'filtered']), + ), + }); + for (let service of [web, worker]) { + for (let key of FILTERED_KEYS) { + expect(service.environment).not.toHaveProperty(key); + } + expect(service.environment.DOCKER_TARGET).toStrictEqual( + DOCKER_TARGET, + ); + } + }); + }, + ); }); // these keys require special handling to prevent runtime errors in make setup