From fc63c1593c1a84152f96a72f11af72700449bebc Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Tue, 17 Dec 2024 16:28:28 +0100 Subject: [PATCH] Fix the mounts.... - Removed the deprecated docker-compose.ci.yml file, consolidating configurations into docker-compose.yml. - Updated environment variable mappings to use OLYMPIA_UID and OLYMPIA_MOUNT for improved clarity and consistency. - Enhanced entrypoint script to adjust user IDs based on the new environment variables. - Modified setup.py to determine the appropriate olympia mount based on the target environment. - Updated GitHub Actions workflows to reflect changes in environment variable usage and remove references to the old compose file. --- .github/actions/run-docker/action.yml | 21 ++- .github/workflows/_test.yml | 6 - .github/workflows/_test_check.yml | 14 +- .github/workflows/_test_main.yml | 2 +- .github/workflows/ci.yml | 4 +- docker-compose.ci.yml | 25 ---- docker-compose.yml | 39 +++++- docker/entrypoint.sh | 23 +++- scripts/setup.py | 40 ++++-- settings.py | 3 +- src/olympia/lib/settings_base.py | 3 +- tests/make/make.spec.js | 180 ++++++++++++-------------- 12 files changed, 186 insertions(+), 174 deletions(-) delete mode 100644 docker-compose.ci.yml diff --git a/.github/actions/run-docker/action.yml b/.github/actions/run-docker/action.yml index cf7bafaa3179..c658eecb6081 100644 --- a/.github/actions/run-docker/action.yml +++ b/.github/actions/run-docker/action.yml @@ -12,10 +12,6 @@ inputs: run: description: 'Run command in container' required: true - compose_file: - description: 'The docker-compose file to use' - required: false - default: 'docker-compose.yml:docker-compose.ci.yml' logs: description: 'Show logs' required: false @@ -23,6 +19,10 @@ inputs: description: 'Skip data backup' required: false default: 'true' + mount: + description: 'Mount olympia files from host' + required: false + default: 'production' runs: using: 'composite' @@ -34,15 +34,14 @@ runs: - name: Run Docker Container shell: bash - env: - DOCKER_VERSION: ${{ inputs.version }} - DOCKER_DIGEST: ${{ inputs.digest }} - COMPOSE_FILE: ${{ inputs.compose_file }} - HOST_UID: ${{ steps.id.outputs.id }} - DATA_BACKUP_SKIP: ${{ inputs.data_backup_skip }} run: | # Start the specified services - make up + make up \ + DOCKER_VERSION="${{ inputs.version }}" \ + DOCKER_DIGEST="${{ inputs.digest }}" \ + OLYMPIA_UID="${{ steps.id.outputs.id }}" \ + OLYMPIA_MOUNT="${{ inputs.mount }}" \ + DATA_BACKUP_SKIP="${{ inputs.data_backup_skip }}" # Exec the run command in the container # quoted 'EOF' to prevent variable expansion diff --git a/.github/workflows/_test.yml b/.github/workflows/_test.yml index 6aa6ee00b36e..80da00809c98 100644 --- a/.github/workflows/_test.yml +++ b/.github/workflows/_test.yml @@ -41,29 +41,24 @@ jobs: - name: Needs Locale Compilation services: '' - compose_file: docker-compose.yml:docker-compose.ci.yml run: | make compile_locales make test_needs_locales_compilation - name: Static Assets services: '' - compose_file: docker-compose.yml:docker-compose.ci.yml run: make test_static_assets - name: Internal Routes services: '' - compose_file: docker-compose.yml:docker-compose.ci.yml run: make test_internal_routes_allowed - name: Elastic Search services: '' - compose_file: docker-compose.yml:docker-compose.ci.yml run: make test_es_tests - name: Codestyle services: web - compose_file: docker-compose.yml:docker-compose.ci.yml run: make lint-codestyle steps: - uses: actions/checkout@v4 @@ -73,5 +68,4 @@ jobs: version: ${{ inputs.version }} digest: ${{ inputs.digest }} services: ${{ matrix.services }} - compose_file: ${{ matrix.compose_file }} run: ${{ matrix.run }} diff --git a/.github/workflows/_test_check.yml b/.github/workflows/_test_check.yml index 19c2d4bcc3a1..50e634405ffd 100644 --- a/.github/workflows/_test_check.yml +++ b/.github/workflows/_test_check.yml @@ -45,16 +45,16 @@ jobs: runs-on: ubuntu-latest name: | version: '${{ matrix.version }}' | - compose_file: '${{ matrix.compose_file }}' + mount: '${{ matrix.mount }}' strategy: fail-fast: false matrix: version: - local - ${{ inputs.version }} - compose_file: - - docker-compose.yml - - docker-compose.yml:docker-compose.ci.yml + mount: + - development + - production steps: - uses: actions/checkout@v4 - shell: bash @@ -63,7 +63,7 @@ jobs: cat < + # in the running docker container. + # If DATA_OLYMPIA_MOUNT_PREFIX matches (data_olympia) + # then we use the production volume mounts. Otherwise + # it will map to the current directory ./ + # (data_olympia):/ + data_olympia: + data_olympia_storage: # Volume for rabbitmq/redis to avoid anonymous volumes data_rabbitmq: data_redis: diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index a48da2312246..507af623aa41 100755 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -6,7 +6,7 @@ ### id of the olympia user sometimes should match the host user's id ### to avoid permission issues with mounted volumes. -set -ue +set -xue if [[ $(id -u) -ne 0 ]]; then echo "This script must be run as root" @@ -18,13 +18,26 @@ OLYMPIA_USER="olympia" function get_olympia_uid() { echo "$(id -u "$OLYMPIA_USER")"; } function get_olympia_gid() { echo "$(id -g "$OLYMPIA_USER")"; } -if [[ -n "${HOST_UID:-}" ]]; then - usermod -u ${HOST_UID} ${OLYMPIA_USER} - echo "${OLYMPIA_USER} UID: ${OLYMPIA_UID} -> ${HOST_UID}" +OLD_OLYMPIA_UID=$(get_olympia_uid) + +# If the olympia user's uid is different in the container than from the build, +# we need to update the olympia user's uid to match the new one. +if [[ "${OLYMPIA_UID}" != "${OLD_OLYMPIA_UID}" ]]; then + usermod -u ${OLYMPIA_UID} ${OLYMPIA_USER} + echo "${OLYMPIA_USER} UID: ${OLD_OLYMPIA_UID} -> ${OLYMPIA_UID}" +fi + +NEW_OLYMPIA_UID=$(get_olympia_uid) +OLYMPIA_ID_STRING="${NEW_OLYMPIA_UID}:$(get_olympia_gid)" + +# If we are on production mode, update the ownership of /data/olympia and /deps to match the new id +if [[ "${OLYMPIA_MOUNT}" == "production" ]]; then + echo "Updating ownership of /data/olympia and /deps to ${OLYMPIA_ID_STRING}" + chown -R ${OLYMPIA_ID_STRING} /data/olympia /deps fi cat < { }); describe.each([ - 'docker-compose.yml', - 'docker-compose.yml:docker-compose.ci.yml', - ])('COMPOSE_FILE=%s', (COMPOSE_FILE) => { - const isProd = COMPOSE_FILE.includes('docker-compose.ci.yml'); + ['development', 'development'], + ['development', 'production'], + ['production', 'development'], + ['production', 'production'], + ])('DOCKER_TARGET=%s, OLYMPIA_MOUNT=%s', (DOCKER_TARGET, OLYMPIA_MOUNT) => { + const isProdTarget = DOCKER_TARGET === 'production'; + const isProdMount = OLYMPIA_MOUNT === 'production'; + const isProdMountTarget = isProdMount && isProdTarget; + + const inputValues = { + DOCKER_TARGET, + OLYMPIA_MOUNT, + DOCKER_TAG: 'mozilla/addons-server:tag', + DEBUG: 'debug', + DATA_BACKUP_SKIP: 'skip', + }; + it('.services.(web|worker) should have the correct configuration', () => { - const values = { - COMPOSE_FILE, - DOCKER_TAG: 'mozilla/addons-server:tag', - // set docker target to production to ensure we are allowed - // to override the olympia mount - DOCKER_TARGET: 'production', - DEBUG: 'debug', - OLYMPIA_UID: '1', - DATA_BACKUP_SKIP: 'skip', - }; + const { HOST_UID, HOST_MOUNT } = runSetup(inputValues); const { services: { web, worker }, - } = getConfig(values); + } = getConfig(inputValues); for (let service of [web, worker]) { - expect(service.image).toStrictEqual(values.DOCKER_TAG); + expect(service.image).toStrictEqual(inputValues.DOCKER_TAG); expect(service.pull_policy).toStrictEqual('never'); expect(service.user).toStrictEqual('root'); expect(service.platform).toStrictEqual('linux/amd64'); @@ -93,26 +99,25 @@ describe('docker-compose.yml', () => { expect(service.volumes).toEqual( expect.arrayContaining([ expect.objectContaining({ - ...(isProd ? {} : { source: expect.any(String) }), + source: isProdMountTarget + ? 'data_olympia' + : expect.any(String), target: '/data/olympia', }), - expect.objectContaining( - isProd - ? { - source: 'storage', - target: '/data/olympia/storage', - } - : {}, - ), + expect.objectContaining({ + source: isProdMountTarget + ? 'data_olympia_storage' + : expect.any(String), + target: '/data/olympia/storage', + }), ]), ); expect(service.environment).toEqual( expect.objectContaining({ - COMPOSE_FILE: values.COMPOSE_FILE, - DEBUG: values.DEBUG, - // Should be set to the UID of the host user - HOST_UID: expect.any(String), - DATA_BACKUP_SKIP: values.DATA_BACKUP_SKIP, + ...inputValues, + // expect mapping of host_* values to olympia_* values + OLYMPIA_MOUNT: HOST_MOUNT, + OLYMPIA_UID: HOST_UID, }), ); } @@ -134,9 +139,7 @@ describe('docker-compose.yml', () => { it('.services.nginx should have the correct configuration', () => { const { services: { nginx }, - } = getConfig({ - COMPOSE_FILE, - }); + } = getConfig(inputValues); // nginx is mapped from http://olympia.test to port 80 in /etc/hosts on the host expect(nginx.ports).toStrictEqual([ expect.objectContaining({ @@ -153,27 +156,27 @@ describe('docker-compose.yml', () => { source: 'data_nginx', target: '/etc/nginx/conf.d', }), - // mapping for local host directory to /data/olympia - expect.objectContaining({ - source: expect.any(String), - target: '/srv', - }), - // mapping for local host directory to /data/olympia/storage - expect.objectContaining( - isProd - ? { - source: 'storage', - target: '/srv/storage', - } - : {}, - ), + // // mapping for local host directory to /data/olympia + // expect.objectContaining({ + // source: isProdMountTarget + // ? 'data_olymmpia' + // : expect.any(String), + // target: '/data/olympia', + // }), + // // mapping for local host directory to /data/olympia/storage + // expect.objectContaining({ + // source: isProdMountTarget + // ? 'data_olympia_storage' + // : expect.any(String), + // target: '/data/olympia/storage', + // }), ]), ); }); it('.services.*.volumes duplicate volumes should be defined on services.olympia_volumes.volumes', () => { const key = 'olympia_volumes'; - const { services } = getConfig({ COMPOSE_FILE }); + const { services } = getConfig(inputValues); // all volumes defined on any service other than olympia const volumesMap = new Map(); // volumes defined on the olympia service, any dupes in other services should be here also @@ -218,68 +221,53 @@ describe('docker-compose.yml', () => { }); it('.services.*.volumes does not contain anonymous or unnamed volumes', () => { - const { services } = getConfig({ COMPOSE_FILE }); + const { services } = getConfig(inputValues); for (let [name, config] of Object.entries(services)) { for (let volume of config.volumes ?? []) { - if (volume.bind) { - console.warn( - `'.services.${name}.volumes' contains anonymous bind mount: ` + - `'${volume.source}:${volume.target}'. Please use a named volume mount instead.` + - 'In the future, this will raise an error!', - ); - } else if (!volume.source) { - console.warn( + if (!volume.bind && !volume.source) { + throw new Error( `'.services.${name}.volumes' contains unnamed volume mount: ` + - `'${volume.target}'. Please use a named volume mount instead.` + - 'In the future, this will raise an error!', + `'${volume.target}'. Please use a named volume mount instead.`, ); } } } }); - describe.each(['development', 'production'])( - 'When DOCKER_TARGET=%s', - (DOCKER_TARGET) => { - const FILTERED_KEYS = [ - 'DOCKER_COMMIT', - 'DOCKER_VERSION', - 'DOCKER_BUILD', - ]; - // This test ensures that we do NOT include environment variables that are used - // at build time in the container. Cointainer environment variables are dynamic - // and should not be able to deviate from the state at build time. - 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, - ); - } - }); - }, - ); + const EXCLUDED_KEYS = ['DOCKER_COMMIT', 'DOCKER_VERSION', 'DOCKER_BUILD']; + // This test ensures that we do NOT include environment variables that are used + // at build time in the container. Cointainer environment variables are dynamic + // and should not be able to deviate from the state at build time. + it('.services.(web|worker).environment excludes build info variables', () => { + const { + services: { web, worker }, + } = getConfig({ + ...inputValues, + ...Object.fromEntries(EXCLUDED_KEYS.map((key) => [key, 'filtered'])), + }); + for (let service of [web, worker]) { + for (let key of EXCLUDED_KEYS) { + expect(service.environment).not.toHaveProperty(key); + } + } + }); }); // these keys require special handling to prevent runtime errors in make setup const failKeys = [ // Invalid docker tag leads to docker not parsing the image 'DOCKER_TAG', - // Invalid compose file leads to inability to create a compose config - 'COMPOSE_FILE', + // Value is read directly as the volume source for /data/olympia and must be valid + 'OLYMPIA_MOUNT_SOURCE', + ]; + const ignoreKeys = [ + // Ignored because these values are explicitly mapped to the host_* values + 'OLYMPIA_UID', + 'OLYMPIA_MOUNT', + // Ignored because the HOST_UID is always set to the host user's UID + 'HOST_UID', + 'HOST_MOUNT', ]; - const ignoreKeys = []; const defaultEnv = runSetup(); const customValue = 'custom';