From 0608e297e398909668eabaddd5c89e70c27834e4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 31 Mar 2021 18:49:27 -0500 Subject: [PATCH] Build: allow to install packages with apt --- readthedocs/config/config.py | 63 ++++++++++++++-- readthedocs/config/models.py | 2 +- readthedocs/config/tests/test_config.py | 51 +++++++++++++ readthedocs/projects/tasks.py | 18 ++++- .../rtd_tests/fixtures/spec/v2/schema.yml | 4 ++ readthedocs/rtd_tests/tests/test_celery.py | 71 ++++++++++++++++++- readthedocs/settings/base.py | 1 + 7 files changed, 201 insertions(+), 9 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 9166d171fc8..ae6441e5a69 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -64,6 +64,7 @@ SUBMODULES_INVALID = 'submodules-invalid' INVALID_KEYS_COMBINATION = 'invalid-keys-combination' INVALID_KEY = 'invalid-key' +INVALID_NAME = 'invalid-name' LATEST_CONFIGURATION_VERSION = 2 @@ -124,7 +125,9 @@ def _get_display_key(self): # Checks for patterns similar to `python.install.0.requirements` # if matched change to `python.install[0].requirements` using backreference. return re.sub( - r'^(python\.install)(\.)(\d+)(\.\w+)$', r'\1[\3]\4', self.key + r'^([a-zA-Z_.-]+)\.(\d+)([a-zA-Z_.-]*)$', + r'\1[\2]\3', + self.key ) @@ -622,7 +625,10 @@ def conda(self): @property def build(self): """The docker image used by the builders.""" - return Build(**self._config['build']) + return Build( + apt_packages=[], + **self._config['build'], + ) @property def doctype(self): @@ -745,12 +751,57 @@ def validate_build(self): ), ) - # Allow to override specific project - config_image = self.defaults.get('build_image') - if config_image: - build['image'] = config_image + # Allow to override specific project + config_image = self.defaults.get('build_image') + if config_image: + build['image'] = config_image + + with self.catch_validation_error('build.apt_packages'): + raw_packages = self._raw_config.get('build', {}).get('apt_packages', []) + validate_list(raw_packages) + # Transform to a dict, so is easy to validate individual entries. + self._raw_config.setdefault('build', {})['apt_packages'] = ( + list_to_dict(raw_packages) + ) + + build['apt_packages'] = [ + self.validate_apt_package(index) + for index in range(len(raw_packages)) + ] + if not raw_packages: + self.pop_config('build.apt_packages') + return build + def validate_apt_package(self, index): + """ + Validate the package name to avoid injections of extra options. + + Packages names can be a regex pattern. + We just validate that they aren't interpreted as an option or file. + """ + key = f'build.apt_packages.{index}' + package = self.pop_config(key) + with self.catch_validation_error(key): + validate_string(package) + package = package.strip() + invalid_starts = [ + # Don't allow to inject extra options. + '-', + '\\', + # Don't allow to install from a path. + '/', + '.', + ] + for start in invalid_starts: + if package.startswith(start): + self.error( + key=key, + message='Invalid package name', + code=INVALID_NAME, + ) + return package + def validate_python(self): """ Validates the python key. diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 998da8498be..5fd09187d04 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -28,7 +28,7 @@ def as_dict(self): class Build(Base): - __slots__ = ('image',) + __slots__ = ('image', 'apt_packages') class Python(Base): diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index a2834c78305..63ed174f407 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -26,6 +26,7 @@ CONFIG_REQUIRED, CONFIG_SYNTAX_INVALID, INVALID_KEY, + INVALID_NAME, PYTHON_INVALID, VERSION_INVALID, ) @@ -748,6 +749,7 @@ def test_as_dict(tmpdir): }, 'build': { 'image': 'readthedocs/build:latest', + 'apt_packages': [], }, 'conda': None, 'sphinx': { @@ -935,6 +937,54 @@ def test_build_image_check_invalid_type(self, value): build.validate() assert excinfo.value.key == 'build.image' + @pytest.mark.parametrize( + 'value', + [ + [], + ['cmatrix'], + ['mysql', 'cmatrix', 'postgresql-dev'], + ['mysql', 'cmatrix', 'postgresql=1.2.3'], + ], + ) + def test_build_apt_packages_check_valid(self, value): + build = self.get_build_config({'build': {'apt_packages': value}}) + build.validate() + assert build.build.apt_packages == value + + @pytest.mark.parametrize( + 'value', + [3, 'string', {}], + ) + def test_build_apt_packages_invalid_type(self, value): + build = self.get_build_config({'build': {'apt_packages': value}}) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == 'build.apt_packages' + + @pytest.mark.parametrize( + 'error_index, value', + [ + (0, ['/', 'cmatrix']), + (1, ['cmatrix', '-q']), + (1, ['cmatrix', ' -q']), + (1, ['cmatrix', '\\-q']), + (1, ['cmatrix', '--quiet']), + (1, ['cmatrix', ' --quiet']), + (2, ['cmatrix', 'quiet', './package.deb']), + (2, ['cmatrix', 'quiet', ' ./package.deb ']), + (2, ['cmatrix', 'quiet', '/home/user/package.deb']), + (2, ['cmatrix', 'quiet', ' /home/user/package.deb']), + (2, ['cmatrix', 'quiet', '../package.deb']), + (2, ['cmatrix', 'quiet', ' ../package.deb']), + ], + ) + def test_build_apt_packages_invalid_value(self, error_index, value): + build = self.get_build_config({'build': {'apt_packages': value}}) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == f'build.apt_packages.{error_index}' + assert excinfo.value.code == INVALID_NAME + @pytest.mark.parametrize('value', [3, [], 'invalid']) def test_python_check_invalid_types(self, value): build = self.get_build_config({'python': value}) @@ -2072,6 +2122,7 @@ def test_as_dict(self, tmpdir): }, 'build': { 'image': 'readthedocs/build:latest', + 'apt_packages': [], }, 'conda': None, 'sphinx': { diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 64d52de6432..03495e755c0 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -794,7 +794,7 @@ def run_build(self, record): environment=self.build_env, ) with self.project.repo_nonblockinglock(version=self.version): - self.setup_python_environment() + self.setup_build() # TODO the build object should have an idea of these states, # extend the model to include an idea of these outcomes @@ -1152,6 +1152,10 @@ def update_app_instances( search_ignore=self.config.search.ignore, ) + def setup_build(self): + self.install_system_dependencies() + self.setup_python_environment() + def setup_python_environment(self): """ Build the virtualenv and install the project into it. @@ -1177,6 +1181,18 @@ def setup_python_environment(self): if self.project.has_feature(Feature.LIST_PACKAGES_INSTALLED_ENV): self.python_env.list_packages_installed() + def install_system_dependencies(self): + packages = self.config.build.apt_packages + if packages: + self.build_env.run( + 'apt-get', 'update', '-y', '-q', + user=settings.RTD_BUILD_SUPER_USER, + ) + self.build_env.run( + 'apt-get', 'install', '-y', '-q', *packages, + user=settings.RTD_BUILD_SUPER_USER, + ) + def build_docs(self): """ Wrapper to all build functions. diff --git a/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml b/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml index f88db8025d1..70907d34e14 100644 --- a/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml +++ b/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml @@ -50,6 +50,10 @@ build: # Note: it can be overridden by a project image: enum('stable', 'latest', required=False) + # List of packages to be installed with apt-get + # Default: [] + apt_packages: list(str(), required=False) + python: # The Python version (this depends on the build image) # Default: '3' diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 6f684f3b0d3..ba3229a51fb 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -2,6 +2,7 @@ import shutil from os.path import exists from tempfile import mkdtemp +from unittest import mock from unittest.mock import MagicMock, patch from allauth.socialaccount.models import SocialAccount @@ -17,7 +18,11 @@ LATEST, ) from readthedocs.builds.models import Build, Version -from readthedocs.doc_builder.environments import LocalBuildEnvironment +from readthedocs.config.config import BuildConfigV2 +from readthedocs.doc_builder.environments import ( + BuildEnvironment, + LocalBuildEnvironment, +) from readthedocs.doc_builder.exceptions import VersionLockedError from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation from readthedocs.projects import tasks @@ -469,3 +474,67 @@ def test_send_build_status_no_remote_repo_or_social_account_gitlab(self, send_bu send_build_status.assert_not_called() self.assertEqual(Message.objects.filter(user=self.eric).count(), 1) + + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock) + @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock) + @patch.object(BuildEnvironment, 'run') + @patch('readthedocs.doc_builder.config.load_config') + def test_install_apt_packages(self, load_config, run): + config = BuildConfigV2( + {}, + { + 'version': 2, + 'build': { + 'apt_packages': [ + 'clangd', + 'cmatrix', + ], + }, + }, + source_file='readthedocs.yml', + ) + config.validate() + load_config.return_value = config + + version = self.project.versions.first() + build = get( + Build, + project=self.project, + version=version, + ) + with mock_api(self.repo): + result = tasks.update_docs_task.delay( + version.pk, + build_pk=build.pk, + record=False, + intersphinx=False, + ) + self.assertTrue(result.successful()) + + self.assertEqual(run.call_count, 2) + apt_update = run.call_args_list[0] + apt_install = run.call_args_list[1] + self.assertEqual( + apt_update, + mock.call( + 'apt-get', + 'update', + '-y', + '-q', + user='root:root', + ) + ) + self.assertEqual( + apt_install, + mock.call( + 'apt-get', + 'install', + '-y', + '-q', + 'clangd', + 'cmatrix', + user='root:root', + ) + ) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 31c921a7361..f0e0269cb59 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -433,6 +433,7 @@ def TEMPLATES(self): # instance to avoid file permissions issues. # https://docs.docker.com/engine/reference/run/#user RTD_DOCKER_USER = 'docs:docs' + RTD_BUILD_SUPER_USER = 'root:root' RTD_DOCKER_COMPOSE = False