From 93e99d5b89bca273a65a5ed923400cb75827f911 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 14:42:51 +0100 Subject: [PATCH 01/74] Make packages easier to read --- docker/hasher-api/Dockerfile | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/docker/hasher-api/Dockerfile b/docker/hasher-api/Dockerfile index 1a32e1d9b..bbc638c94 100644 --- a/docker/hasher-api/Dockerfile +++ b/docker/hasher-api/Dockerfile @@ -19,8 +19,17 @@ ARG TEST="false" # OS setup RUN export DEBIAN_FRONTEND=noninteractive && \ apt-get update && \ - apt-get install --yes --no-install-recommends procps ca-certificates \ - iproute2 git curl libpq-dev curl gnupg g++ locales + apt-get install --yes --no-install-recommends \ + procps \ + ca-certificates \ + iproute2 \ + git \ + curl \ + libpq-dev \ + curl \ + gnupg \ + g++ \ + locales RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen # clean up From 4b1f8fa5bfbd696d588acf9e2c90d704ae011e25 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 14:44:51 +0100 Subject: [PATCH 02/74] Remove unneeded packages --- docker/hasher-api/Dockerfile | 3 --- 1 file changed, 3 deletions(-) diff --git a/docker/hasher-api/Dockerfile b/docker/hasher-api/Dockerfile index bbc638c94..a04efba49 100644 --- a/docker/hasher-api/Dockerfile +++ b/docker/hasher-api/Dockerfile @@ -23,12 +23,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ procps \ ca-certificates \ iproute2 \ - git \ - curl \ libpq-dev \ curl \ gnupg \ - g++ \ locales RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen From d087520ed62fe05bf25f18d5a03ffe0fcc62db58 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 15:03:28 +0100 Subject: [PATCH 03/74] Merge three pixl python docker images into one multi-stage dockerfile to avoid repetition. --- docker-compose.yml | 9 ++-- docker/export-api/Dockerfile | 48 ----------------- docker/hasher-api/Dockerfile | 52 ------------------- .../{imaging-api => pixl-python}/Dockerfile | 42 ++++++++++++++- 4 files changed, 47 insertions(+), 104 deletions(-) delete mode 100644 docker/export-api/Dockerfile delete mode 100644 docker/hasher-api/Dockerfile rename docker/{imaging-api => pixl-python}/Dockerfile (53%) diff --git a/docker-compose.yml b/docker-compose.yml index 6953ea9dd..71c05b72f 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,7 +81,8 @@ services: hasher-api: build: context: . - dockerfile: ./docker/hasher-api/Dockerfile + dockerfile: ./docker/pixl-python/Dockerfile + target: hasher_api args: <<: *build-args-common environment: @@ -253,7 +254,8 @@ services: export-api: build: context: . - dockerfile: ./docker/export-api/Dockerfile + dockerfile: ./docker/pixl-python/Dockerfile + target: export_api args: <<: *build-args-common environment: @@ -299,7 +301,8 @@ services: imaging-api: build: context: . - dockerfile: ./docker/imaging-api/Dockerfile + dockerfile: ./docker/pixl-python/Dockerfile + target: imaging_api args: <<: *build-args-common depends_on: diff --git a/docker/export-api/Dockerfile b/docker/export-api/Dockerfile deleted file mode 100644 index dfbd0324b..000000000 --- a/docker/export-api/Dockerfile +++ /dev/null @@ -1,48 +0,0 @@ -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -FROM python:3.11.7-slim-bullseye -SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] - -ARG TEST="false" - -RUN export DEBIAN_FRONTEND=noninteractive && \ - apt-get update && \ - apt-get install --yes --no-install-recommends \ - procps \ - ca-certificates \ - iproute2 \ - libpq-dev \ - curl \ - gnupg \ - locales -RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen -RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* - -WORKDIR /app - -# Install requirements before copying modules -COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./pixl_export/pyproject.toml ./pixl_export/pyproject.toml -RUN pip3 install --no-cache-dir pixl_core/ \ - && pip3 install --no-cache-dir pixl_export/ - -COPY ./pixl_core/ pixl_core/ -COPY ./pixl_export/ . -RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ - --no-cache-dir --force-reinstall --no-deps . && \ - if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi - -HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 - -ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/docker/hasher-api/Dockerfile b/docker/hasher-api/Dockerfile deleted file mode 100644 index a04efba49..000000000 --- a/docker/hasher-api/Dockerfile +++ /dev/null @@ -1,52 +0,0 @@ -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -FROM python:3.11.7-slim-bullseye -SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] - -ARG TEST="false" - -# OS setup -RUN export DEBIAN_FRONTEND=noninteractive && \ - apt-get update && \ - apt-get install --yes --no-install-recommends \ - procps \ - ca-certificates \ - iproute2 \ - libpq-dev \ - curl \ - gnupg \ - locales -RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen - -# clean up -RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* - -WORKDIR /app - -# Install requirements before copying modules -COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./hasher/pyproject.toml . -RUN --mount=type=cache,target=/root/.cache \ - pip3 install --no-cache-dir pixl_core/ \ - && pip3 install --no-cache-dir /app/ - -COPY ./pixl_core/ pixl_core/ -COPY ./hasher/ . -RUN --mount=type=cache,target=/root/.cache \ - pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ . && \ - if [ "$TEST" = "true" ]; \ - then pip install --no-cache-dir --force-reinstall --no-deps pixl_core/[test] \ - --no-cache-dir --force-reinstall --no-deps .[test]; fi - -ENTRYPOINT ["uvicorn", "hasher.main:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/docker/imaging-api/Dockerfile b/docker/pixl-python/Dockerfile similarity index 53% rename from docker/imaging-api/Dockerfile rename to docker/pixl-python/Dockerfile index 647771236..4a19ff331 100644 --- a/docker/imaging-api/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -FROM python:3.11.7-slim-bullseye +FROM python:3.11.7-slim-bullseye AS pixl_python_base SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] ARG TEST="false" @@ -30,6 +30,46 @@ RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* WORKDIR /app +# --- END COMMMON --- + +FROM pixl_python_base AS export_api + +# Install requirements before copying modules +COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml +COPY ./pixl_export/pyproject.toml ./pixl_export/pyproject.toml +RUN pip3 install --no-cache-dir pixl_core/ \ + && pip3 install --no-cache-dir pixl_export/ + +COPY ./pixl_core/ pixl_core/ +COPY ./pixl_export/ . +RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ + --no-cache-dir --force-reinstall --no-deps . && \ + if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi + +HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 + +ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] + +FROM pixl_python_base AS hasher_api + +# Install requirements before copying modules +COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml +COPY ./hasher/pyproject.toml . +RUN --mount=type=cache,target=/root/.cache \ + pip3 install --no-cache-dir pixl_core/ \ + && pip3 install --no-cache-dir /app/ + +COPY ./pixl_core/ pixl_core/ +COPY ./hasher/ . +RUN --mount=type=cache,target=/root/.cache \ + pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ . && \ + if [ "$TEST" = "true" ]; \ + then pip install --no-cache-dir --force-reinstall --no-deps pixl_core/[test] \ + --no-cache-dir --force-reinstall --no-deps .[test]; fi + +ENTRYPOINT ["uvicorn", "hasher.main:app", "--host", "0.0.0.0", "--port", "8000"] + +FROM pixl_python_base AS imaging_api # Install requirements before copying modules COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml From 1e1c18ecc61c94c6dc40ce9ff9e284f24572a704 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 15:18:05 +0100 Subject: [PATCH 04/74] Specify healthcheck command in only one place --- docker-compose.yml | 2 -- docker/pixl-python/Dockerfile | 5 ++--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 71c05b72f..40fcce784 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -101,7 +101,6 @@ services: networks: - pixl-net healthcheck: - test: ["CMD", "curl", "-f", "http://hasher-api:8000/heart-beat"] interval: 10s timeout: 30s retries: 5 @@ -311,7 +310,6 @@ services: orthanc-raw: condition: service_healthy healthcheck: - test: curl -f http://0.0.0.0:8000/heart-beat interval: 10s timeout: 30s retries: 5 diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index 4a19ff331..07bb72aa2 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -29,8 +29,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ RUN sed -i '/en_GB.UTF-8/s/^# //g' /etc/locale.gen && locale-gen RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* +HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 + WORKDIR /app -# --- END COMMMON --- FROM pixl_python_base AS export_api @@ -46,8 +47,6 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi -HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 - ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] FROM pixl_python_base AS hasher_api From 63d6edb3dd39ab080c7472ee945da67923d531c5 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 16:10:44 +0100 Subject: [PATCH 05/74] De-dupe the pre-reqs installation code using a build arg --- docker-compose.yml | 3 +++ docker/pixl-python/Dockerfile | 28 +++++++++------------------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 40fcce784..508223d27 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -84,6 +84,7 @@ services: dockerfile: ./docker/pixl-python/Dockerfile target: hasher_api args: + PIXL_PACKAGE_DIR: hasher <<: *build-args-common environment: <<: [*proxy-common, *pixl-common-env] @@ -256,6 +257,7 @@ services: dockerfile: ./docker/pixl-python/Dockerfile target: export_api args: + PIXL_PACKAGE_DIR: pixl_export <<: *build-args-common environment: <<: @@ -303,6 +305,7 @@ services: dockerfile: ./docker/pixl-python/Dockerfile target: imaging_api args: + PIXL_PACKAGE_DIR: pixl_imaging <<: *build-args-common depends_on: queue: diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index 07bb72aa2..f6b8a4603 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -32,14 +32,17 @@ RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app - -FROM pixl_python_base AS export_api +ARG PIXL_PACKAGE_DIR # Install requirements before copying modules COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./pixl_export/pyproject.toml ./pixl_export/pyproject.toml +COPY ./$PIXL_PACKAGE_DIR/pyproject.toml ./$PIXL_PACKAGE_DIR/pyproject.toml RUN pip3 install --no-cache-dir pixl_core/ \ - && pip3 install --no-cache-dir pixl_export/ + && pip3 install --no-cache-dir $PIXL_PACKAGE_DIR/ + + + +FROM pixl_python_base AS export_api COPY ./pixl_core/ pixl_core/ COPY ./pixl_export/ . @@ -49,14 +52,8 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] -FROM pixl_python_base AS hasher_api -# Install requirements before copying modules -COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./hasher/pyproject.toml . -RUN --mount=type=cache,target=/root/.cache \ - pip3 install --no-cache-dir pixl_core/ \ - && pip3 install --no-cache-dir /app/ +FROM pixl_python_base AS hasher_api COPY ./pixl_core/ pixl_core/ COPY ./hasher/ . @@ -65,17 +62,10 @@ RUN --mount=type=cache,target=/root/.cache \ if [ "$TEST" = "true" ]; \ then pip install --no-cache-dir --force-reinstall --no-deps pixl_core/[test] \ --no-cache-dir --force-reinstall --no-deps .[test]; fi - ENTRYPOINT ["uvicorn", "hasher.main:app", "--host", "0.0.0.0", "--port", "8000"] -FROM pixl_python_base AS imaging_api -# Install requirements before copying modules -COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./pixl_imaging/pyproject.toml ./pixl_imaging/pyproject.toml -RUN --mount=type=cache,target=/root/.cache \ - pip3 install --no-cache-dir pixl_core/ \ - && pip3 install --no-cache-dir pixl_imaging/ +FROM pixl_python_base AS imaging_api COPY ./pixl_core/ pixl_core/ COPY ./pixl_imaging/ . From 46a08e07e768ed53b3a77ce30cc3f8eaba5591d8 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 16:16:04 +0100 Subject: [PATCH 06/74] De-dupe the actual install as well --- docker/pixl-python/Dockerfile | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index f6b8a4603..f730168c3 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -32,6 +32,7 @@ RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app +# specify what we're installing using build time arg ARG PIXL_PACKAGE_DIR # Install requirements before copying modules @@ -40,39 +41,20 @@ COPY ./$PIXL_PACKAGE_DIR/pyproject.toml ./$PIXL_PACKAGE_DIR/pyproject.toml RUN pip3 install --no-cache-dir pixl_core/ \ && pip3 install --no-cache-dir $PIXL_PACKAGE_DIR/ - - -FROM pixl_python_base AS export_api - +# Install our code COPY ./pixl_core/ pixl_core/ -COPY ./pixl_export/ . +COPY ./$PIXL_PACKAGE_DIR/ . RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi -ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] +# Each container should be run with a different entry point +FROM pixl_python_base AS export_api +ENTRYPOINT ["uvicorn", "pixl_export.main:app", "--host", "0.0.0.0", "--port", "8000"] FROM pixl_python_base AS hasher_api - -COPY ./pixl_core/ pixl_core/ -COPY ./hasher/ . -RUN --mount=type=cache,target=/root/.cache \ - pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ . && \ - if [ "$TEST" = "true" ]; \ - then pip install --no-cache-dir --force-reinstall --no-deps pixl_core/[test] \ - --no-cache-dir --force-reinstall --no-deps .[test]; fi ENTRYPOINT ["uvicorn", "hasher.main:app", "--host", "0.0.0.0", "--port", "8000"] - FROM pixl_python_base AS imaging_api - -COPY ./pixl_core/ pixl_core/ -COPY ./pixl_imaging/ . -RUN --mount=type=cache,target=/root/.cache \ - pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ . && \ - if [ "$TEST" = "true" ]; \ - then pip install --no-cache-dir --force-reinstall --no-deps pixl_core/[test] \ - --no-cache-dir --force-reinstall --no-deps .[test]; fi - ENTRYPOINT ["/app/scripts/migrate_and_run.sh"] From 64fcbc57796c35eea7fa2a50368128b796940ab9 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 8 May 2024 17:31:08 +0100 Subject: [PATCH 07/74] Add (failing) test for the condition we are aiming for: containers to run as non-root users. --- test/test_docker_containers.py | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 test/test_docker_containers.py diff --git a/test/test_docker_containers.py b/test/test_docker_containers.py new file mode 100644 index 000000000..da50e4abb --- /dev/null +++ b/test/test_docker_containers.py @@ -0,0 +1,47 @@ +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Check requirements related to docker configuration.""" + +import pytest +from loguru import logger +from pytest_pixl.helpers import run_subprocess + + +@pytest.mark.usefixtures("_setup_pixl_cli") +@pytest.mark.parametrize( + "container", + [ + "export-api", + "imaging-api", + "hasher-api", + "postgres", + "queue", + # hopefully we can fix orthanc as well + ], +) +def test_non_root_uids(container: str) -> None: + """ + Test that all processes inside certain containers are not running as root. + This is easier for ones we have full control over, but + we may wish to expand the list of containers as we fix them. + It would also be good to check that they're running as a particular user, but + non-root will do for now. + """ + cp = run_subprocess(["docker", "top", f"system-test-{container}-1"]) + lines = cp.stdout.decode().splitlines() + logger.info(lines) + assert lines[0].startswith("UID") # sanity check title line + for proc in lines[1:]: + uid = proc.split(" ")[0] + assert uid != "root" From 0d5cb398e55344079f66da02a57bbd79a84109f4 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 11:21:47 +0100 Subject: [PATCH 08/74] Run as user pixl, which we have to create inside the image with a fixed user ID. --- .env.sample | 4 ++++ docker-compose.yml | 6 +++++- docker/pixl-python/Dockerfile | 6 +++++- test/.env | 4 ++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.env.sample b/.env.sample index 4b0f09c32..2ce0530a8 100644 --- a/.env.sample +++ b/.env.sample @@ -106,3 +106,7 @@ HASHER_API_AZ_CLIENT_ID= HASHER_API_AZ_CLIENT_PASSWORD= HASHER_API_AZ_TENANT_ID= HASHER_API_AZ_KEY_VAULT_NAME= + +# User and group IDs for pixl:pixl. Only needed at build time. +PIXL_USER_UID=7001 +PIXL_USER_GID=7001 diff --git a/docker-compose.yml b/docker-compose.yml index 508223d27..0cb60aacf 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -25,8 +25,12 @@ x-proxy-common: &proxy-common NO_PROXY: *no-proxy no_proxy: *no-proxy +x-pixl-uid-gid: &pixl-uid-gid + PIXL_USER_UID: ${PIXL_USER_UID} + PIXL_USER_GID: ${PIXL_USER_GID} + x-build-args-common: &build-args-common - <<: [*proxy-common] + <<: [*proxy-common, *pixl-uid-gid] x-pixl-common-env: &pixl-common-env DEBUG: ${DEBUG} diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index f730168c3..6f8645f21 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -32,7 +32,7 @@ RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app -# specify what we're installing using build time arg +# specify which of our packages we're installing using build time arg ARG PIXL_PACKAGE_DIR # Install requirements before copying modules @@ -48,6 +48,10 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi +ARG PIXL_USER_UID +ARG PIXL_USER_GID +RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl +USER pixl # Each container should be run with a different entry point FROM pixl_python_base AS export_api diff --git a/test/.env b/test/.env index e5ff8ec9f..1ebc33d13 100644 --- a/test/.env +++ b/test/.env @@ -83,3 +83,7 @@ FTP_USER_PASSWORD=longpassword # Project configs directory PROJECT_CONFIGS_DIR=projects/configs + +# User and group IDs for pixl:pixl. Only needed at build time. +PIXL_USER_UID=7001 +PIXL_USER_GID=7001 From 397cb68b0451fc3c6f32c72bb6e5a138fbc9886d Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 11:37:01 +0100 Subject: [PATCH 09/74] The system test already builds the required docker images, and passes the correct env files in. No need to do it here as well, and it was failing due to lack of build args. --- .github/workflows/main.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f0f592a2e..bf1208816 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -107,15 +107,6 @@ jobs: - name: Create .secrets.env run: touch test/.secrets.env - - name: Build test services - working-directory: test - run: | - docker compose build - - - name: Build services - run: | - docker compose build - - name: Run tests working-directory: test env: From d7fa46ef379d3479542197b3f45c4759bf2a1a1d Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 12:22:34 +0100 Subject: [PATCH 10/74] Semi-WIP: Create pixl user+group on the GHA test runner so that we can set file permissions correctly for the export directory, which is needed so that it can be deleted from inside the container. --- .github/workflows/main.yml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bf1208816..80dcc72cf 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -119,7 +119,23 @@ jobs: HASHER_API_AZ_TENANT_ID: ${{ secrets.EXPORT_AZ_TENANT_ID }} HASHER_API_AZ_KEY_VAULT_NAME: ${{ secrets.EXPORT_AZ_KEY_VAULT_NAME }} run: | - ./run-system-test.sh coverage + # These two variables are needed for creating the pixl user and group on the GHA runner. + # Don't worry about passing them into pytest if you're running pytest manually or through your + # IDE - they're only used inside an "if GITHUB_ACTIONS" block anyway. + # We could do this in the setup function, but we don't at that point know the location of the + # exports dir (how the exports dir is determined is due to change anyway) + source .env + export PIXL_USER_UID PIXL_USER_GID + pwd + ls -laR + sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl + sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl + host_export_root_dir=./exports + sudo --non-interactive chown -R pixl:docker host_export_root_dir + sudo --non-interactive chmod -R ug+rwx host_export_root_dir + ls -laR + + sudo --non-interactive -u pixl -g pixl ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From d8525b18f50caf0a04ddde0e93791b223eb3616c Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 12:34:31 +0100 Subject: [PATCH 11/74] Fix variable usage and exports dir location --- .github/workflows/main.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 80dcc72cf..8a02c71ff 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -130,9 +130,9 @@ jobs: ls -laR sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl - host_export_root_dir=./exports - sudo --non-interactive chown -R pixl:docker host_export_root_dir - sudo --non-interactive chmod -R ug+rwx host_export_root_dir + host_export_root_dir=../projects/exports + sudo --non-interactive chown -R pixl:docker $host_export_root_dir + sudo --non-interactive chmod -R ug+rwx $host_export_root_dir ls -laR sudo --non-interactive -u pixl -g pixl ./run-system-test.sh coverage From 6facef9f82fa8ce7373264a45b07586d4589caf1 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 14:05:56 +0100 Subject: [PATCH 12/74] More debugging --- .github/workflows/main.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8a02c71ff..b280926af 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -125,15 +125,18 @@ jobs: # We could do this in the setup function, but we don't at that point know the location of the # exports dir (how the exports dir is determined is due to change anyway) source .env + set -x export PIXL_USER_UID PIXL_USER_GID pwd ls -laR sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports - sudo --non-interactive chown -R pixl:docker $host_export_root_dir - sudo --non-interactive chmod -R ug+rwx $host_export_root_dir + echo JES1 $host_export_root_dir + sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" + sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" ls -laR + pwd sudo --non-interactive -u pixl -g pixl ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT From 55db93c0dd807ca546b22342485e70e9b7240caa Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 14:43:35 +0100 Subject: [PATCH 13/74] debug --- test/run-system-test.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/run-system-test.sh b/test/run-system-test.sh index 01cdf3645..b129974b8 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -49,6 +49,7 @@ else # setup flags used for pytest declare -a PYTEST_FLAGS + set -x # Add individual options to the array PYTEST_FLAGS+=("--verbose") PYTEST_FLAGS+=("--log-cli-level" "INFO") @@ -57,6 +58,8 @@ else fi # Run the tests setup + echo JES11 + pwd pytest "${PYTEST_FLAGS[@]}" echo FINISHED PYTEST COMMAND teardown From 6f7c8dd88581340dff50e3897f34fe77448a5f7c Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 14:51:53 +0100 Subject: [PATCH 14/74] debug --- test/run-system-test.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/run-system-test.sh b/test/run-system-test.sh index b129974b8..ae375e59a 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -18,14 +18,19 @@ PACKAGE_DIR="${BIN_DIR%/*}" cd "${PACKAGE_DIR}/test" setup() { + echo JES21 $PACKAGE_DIR docker compose --env-file .env -p system-test down --volumes + echo JES22 $(pwd) # # Note: cannot run as single docker compose command due to different build contexts docker compose --env-file .env -p system-test up --wait -d --build --remove-orphans + echo JES23 # Warning: Requires to be run from the project root ( cd "${PACKAGE_DIR}" + echo JES24 docker compose --env-file test/.env --env-file test/.secrets.env -p system-test up --wait -d --build + echo JES25 ) } From 0ff9148509bb4fe93a4234cc44ee7191bc0a93f9 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 15:00:59 +0100 Subject: [PATCH 15/74] split build and up --- test/run-system-test.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/run-system-test.sh b/test/run-system-test.sh index ae375e59a..c53bd8d35 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -27,9 +27,11 @@ setup() { echo JES23 # Warning: Requires to be run from the project root ( - cd "${PACKAGE_DIR}" + cd "${PACKAGE_DIR}" echo JES24 - docker compose --env-file test/.env --env-file test/.secrets.env -p system-test up --wait -d --build + docker compose --env-file test/.env --env-file test/.secrets.env -p system-test build + echo JES24a + docker compose --env-file test/.env --env-file test/.secrets.env -p system-test up --wait -d echo JES25 ) } From cce26a55f383b7bebafcc692b2715b6c5ee8c9d8 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 15:45:37 +0100 Subject: [PATCH 16/74] Create user home dir in the GHA script to avoid error from failed creation later on --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b280926af..68dbec9a0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -130,7 +130,7 @@ jobs: pwd ls -laR sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl - sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl + sudo --non-interactive useradd --create-home --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports echo JES1 $host_export_root_dir sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" From 1e3ed880d9e9ef487a1521797f021ed150f7237e Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:02:44 +0100 Subject: [PATCH 17/74] Why is pytest not found? Some kind of env change caused by sudo? --- .github/workflows/main.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 68dbec9a0..e53208306 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -130,6 +130,8 @@ jobs: pwd ls -laR sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl + # Create home dir now because otherwise the docker build will try to do + # it (unsuccessfully) for an unknown reason sudo --non-interactive useradd --create-home --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports echo JES1 $host_export_root_dir @@ -138,8 +140,14 @@ jobs: ls -laR pwd + echo JES00 + echo PATH = $PATH + echo JES01 + sudo --non-interactive -u pixl -g pixl env + echo JES02 sudo --non-interactive -u pixl -g pixl ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT + ls -laR /home/pixl - name: Dump export-api docker logs for debugging if: ${{ failure() }} From 8abcb5f597c08f224ac605979e11aedd73330a37 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:14:08 +0100 Subject: [PATCH 18/74] Preserve path when running sudo --- .github/workflows/main.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e53208306..d14e783d6 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -145,7 +145,9 @@ jobs: echo JES01 sudo --non-interactive -u pixl -g pixl env echo JES02 - sudo --non-interactive -u pixl -g pixl ./run-system-test.sh coverage + # --preserve-env needed to keep the installed python dir in the path etc + # do we need to run with sudo at all? + sudo --non-interactive --preserve-env -u pixl -g pixl ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT ls -laR /home/pixl From de4e1e9a6c76860934fc5235dffd24b1b85043a6 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:19:40 +0100 Subject: [PATCH 19/74] Run tests as normal runner user --- .github/workflows/main.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d14e783d6..c3d58dd36 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -145,9 +145,7 @@ jobs: echo JES01 sudo --non-interactive -u pixl -g pixl env echo JES02 - # --preserve-env needed to keep the installed python dir in the path etc - # do we need to run with sudo at all? - sudo --non-interactive --preserve-env -u pixl -g pixl ./run-system-test.sh coverage + ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT ls -laR /home/pixl From 5736f30a7c02787f3e46250a5b5a9b43db18cecc Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:40:30 +0100 Subject: [PATCH 20/74] Don't see why this needs to have docker group --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c3d58dd36..b06930d25 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -135,7 +135,7 @@ jobs: sudo --non-interactive useradd --create-home --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports echo JES1 $host_export_root_dir - sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" + sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" ls -laR pwd From 09f37eab201bc6c850117de79203f8cb42478755 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:40:56 +0100 Subject: [PATCH 21/74] Debugging for perm failure on file deletes --- test/conftest.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index a25b1b70b..63b87e765 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -135,12 +135,15 @@ def _upload_dicom_instance(dicom_dir: Path, **kwargs: Any) -> None: @pytest.fixture(scope="session") -def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None) -> Generator: +def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None, host_export_root_dir) -> Generator: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress wait_for_stable_orthanc_anon(121, 5, 15, min_instances=3) yield + run_subprocess( + ["ls", "-laR", host_export_root_dir] + ) run_subprocess( [ "docker", @@ -155,12 +158,26 @@ def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None) -> Generato @pytest.fixture(scope="session") -def _setup_pixl_cli_dicomweb(_populate_vna: None) -> Generator: +def _setup_pixl_cli_dicomweb(_populate_vna: None, host_export_root_dir) -> Generator: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DICOMWEB_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress wait_for_stable_orthanc_anon(121, 5, 15, min_instances=3) yield + run_subprocess( + ["ls", "-laR", host_export_root_dir] + ) + run_subprocess( + [ + "docker", + "exec", + "system-test-export-api-1", + "ls", + "-laR", + "/run/projects/exports/", + ], + TEST_DIR, + ) run_subprocess( [ "docker", From 0edcf17511a38e56ca93832c74ccd5e292ee3c8f Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 16:55:05 +0100 Subject: [PATCH 22/74] Fix scope error --- test/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/conftest.py b/test/conftest.py index 63b87e765..2fe1736d3 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -29,7 +29,7 @@ pytest_plugins = "pytest_pixl" -@pytest.fixture() +@pytest.fixture(scope="session") def host_export_root_dir() -> Path: """Intermediate export dir as seen from the host""" return Path(__file__).parents[1] / "projects" / "exports" From d0201d88b189cb2c0030af71fe4e00f7d5296dbd Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 17:07:26 +0100 Subject: [PATCH 23/74] OK maybe this was needed after all --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b06930d25..c3d58dd36 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -135,7 +135,7 @@ jobs: sudo --non-interactive useradd --create-home --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports echo JES1 $host_export_root_dir - sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" + sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" ls -laR pwd From 077a304815511cacbb92b363bc0b851d86e5dc40 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 17:48:57 +0100 Subject: [PATCH 24/74] Since CLI creates everything in exports dir these days, it makes more sense to delete stuff afterwards from the host side rather than the container side. --- test/conftest.py | 22 ---------------------- test/run-system-test.sh | 7 +++++-- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index 2fe1736d3..6e71e6da8 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -144,17 +144,6 @@ def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None, host_export run_subprocess( ["ls", "-laR", host_export_root_dir] ) - run_subprocess( - [ - "docker", - "exec", - "system-test-export-api-1", - "rm", - "-r", - "/run/projects/exports/test-extract-uclh-omop-cdm/", - ], - TEST_DIR, - ) @pytest.fixture(scope="session") @@ -178,17 +167,6 @@ def _setup_pixl_cli_dicomweb(_populate_vna: None, host_export_root_dir) -> Gener ], TEST_DIR, ) - run_subprocess( - [ - "docker", - "exec", - "system-test-export-api-1", - "rm", - "-r", - "/run/projects/exports/test-extract-uclh-omop-cdm-dicomweb/", - ], - TEST_DIR, - ) @pytest.fixture(scope="session") diff --git a/test/run-system-test.sh b/test/run-system-test.sh index c53bd8d35..2af093951 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -15,6 +15,7 @@ set -euxo pipefail BIN_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) PACKAGE_DIR="${BIN_DIR%/*}" +EXPORTS_DIR="${PACKAGE_DIR}/projects/exports" cd "${PACKAGE_DIR}/test" setup() { @@ -38,8 +39,10 @@ setup() { teardown() { ( - cd "${PACKAGE_DIR}" - docker compose -f docker-compose.yml -f test/docker-compose.yml -p system-test down --volumes + cd "${PACKAGE_DIR}" + rm -r "${EXPORTS_DIR}/test-extract-uclh-omop-cdm-dicomweb/" + rm -r "${EXPORTS_DIR}/test-extract-uclh-omop-cdm/" + docker compose -f docker-compose.yml -f test/docker-compose.yml -p system-test down --volumes ) } From cd93adc2259a1342716668b7c766a4b396890ec0 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 18:07:47 +0100 Subject: [PATCH 25/74] Try not creating home dir for pixl - what is even using this? --- .github/workflows/main.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c3d58dd36..5eb4bffdc 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -132,7 +132,10 @@ jobs: sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl # Create home dir now because otherwise the docker build will try to do # it (unsuccessfully) for an unknown reason - sudo --non-interactive useradd --create-home --uid $PIXL_USER_UID --gid pixl --groups docker pixl + # How much of this is even needed now that we're deleting host side? + # pixl needs to be able to *read* files in the export dir, but it isn't + # creating them any more + sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports echo JES1 $host_export_root_dir sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" From c0c0d96145489edb71f8381b505d2671e599c5cf Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Thu, 9 May 2024 19:33:16 +0100 Subject: [PATCH 26/74] and therefore I can't do this any more --- .github/workflows/main.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5eb4bffdc..31c501c85 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -150,7 +150,6 @@ jobs: echo JES02 ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - ls -laR /home/pixl - name: Dump export-api docker logs for debugging if: ${{ failure() }} From 9507816a7bd0f75b1fe590623a0b6c289fe4ab81 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 11:41:45 +0100 Subject: [PATCH 27/74] Remove debugging --- .github/workflows/main.yml | 13 ------------- test/conftest.py | 26 +++----------------------- test/run-system-test.sh | 12 +----------- 3 files changed, 4 insertions(+), 47 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 31c501c85..b3e20f29f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -125,29 +125,16 @@ jobs: # We could do this in the setup function, but we don't at that point know the location of the # exports dir (how the exports dir is determined is due to change anyway) source .env - set -x export PIXL_USER_UID PIXL_USER_GID - pwd - ls -laR sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl - # Create home dir now because otherwise the docker build will try to do - # it (unsuccessfully) for an unknown reason # How much of this is even needed now that we're deleting host side? # pixl needs to be able to *read* files in the export dir, but it isn't # creating them any more sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports - echo JES1 $host_export_root_dir sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" - ls -laR - pwd - echo JES00 - echo PATH = $PATH - echo JES01 - sudo --non-interactive -u pixl -g pixl env - echo JES02 ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT diff --git a/test/conftest.py b/test/conftest.py index 6e71e6da8..271fadc98 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -14,7 +14,6 @@ """System/E2E test setup""" # ruff: noqa: C408 dict() makes test data easier to read and write -from collections.abc import Generator from pathlib import Path from typing import Any @@ -29,7 +28,7 @@ pytest_plugins = "pytest_pixl" -@pytest.fixture(scope="session") +@pytest.fixture() def host_export_root_dir() -> Path: """Intermediate export dir as seen from the host""" return Path(__file__).parents[1] / "projects" / "exports" @@ -135,38 +134,19 @@ def _upload_dicom_instance(dicom_dir: Path, **kwargs: Any) -> None: @pytest.fixture(scope="session") -def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None, host_export_root_dir) -> Generator: +def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None) -> None: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress wait_for_stable_orthanc_anon(121, 5, 15, min_instances=3) - yield - run_subprocess( - ["ls", "-laR", host_export_root_dir] - ) @pytest.fixture(scope="session") -def _setup_pixl_cli_dicomweb(_populate_vna: None, host_export_root_dir) -> Generator: +def _setup_pixl_cli_dicomweb(_populate_vna: None) -> None: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DICOMWEB_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress wait_for_stable_orthanc_anon(121, 5, 15, min_instances=3) - yield - run_subprocess( - ["ls", "-laR", host_export_root_dir] - ) - run_subprocess( - [ - "docker", - "exec", - "system-test-export-api-1", - "ls", - "-laR", - "/run/projects/exports/", - ], - TEST_DIR, - ) @pytest.fixture(scope="session") diff --git a/test/run-system-test.sh b/test/run-system-test.sh index 2af093951..2652ed513 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -19,21 +19,14 @@ EXPORTS_DIR="${PACKAGE_DIR}/projects/exports" cd "${PACKAGE_DIR}/test" setup() { - echo JES21 $PACKAGE_DIR docker compose --env-file .env -p system-test down --volumes - echo JES22 $(pwd) # # Note: cannot run as single docker compose command due to different build contexts docker compose --env-file .env -p system-test up --wait -d --build --remove-orphans - echo JES23 # Warning: Requires to be run from the project root ( cd "${PACKAGE_DIR}" - echo JES24 - docker compose --env-file test/.env --env-file test/.secrets.env -p system-test build - echo JES24a - docker compose --env-file test/.env --env-file test/.secrets.env -p system-test up --wait -d - echo JES25 + docker compose --env-file test/.env --env-file test/.secrets.env -p system-test up --wait -d --build ) } @@ -59,7 +52,6 @@ else # setup flags used for pytest declare -a PYTEST_FLAGS - set -x # Add individual options to the array PYTEST_FLAGS+=("--verbose") PYTEST_FLAGS+=("--log-cli-level" "INFO") @@ -68,8 +60,6 @@ else fi # Run the tests setup - echo JES11 - pwd pytest "${PYTEST_FLAGS[@]}" echo FINISHED PYTEST COMMAND teardown From bd5cf84859be535a0809d4fc7f052834a280afe8 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 14:22:30 +0100 Subject: [PATCH 28/74] Document the reason for the permissions setup better. Don't need to export variables any more. --- .github/workflows/main.yml | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b3e20f29f..7767e9e59 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -119,17 +119,12 @@ jobs: HASHER_API_AZ_TENANT_ID: ${{ secrets.EXPORT_AZ_TENANT_ID }} HASHER_API_AZ_KEY_VAULT_NAME: ${{ secrets.EXPORT_AZ_KEY_VAULT_NAME }} run: | - # These two variables are needed for creating the pixl user and group on the GHA runner. - # Don't worry about passing them into pytest if you're running pytest manually or through your - # IDE - they're only used inside an "if GITHUB_ACTIONS" block anyway. - # We could do this in the setup function, but we don't at that point know the location of the - # exports dir (how the exports dir is determined is due to change anyway) + # We need PIXL_USER_UID and PIXL_USER_GID from the test env file. source .env - export PIXL_USER_UID PIXL_USER_GID + # Export API (running as pixl) needs to be able to *read* files in + # the export dir, but it doesn't create them. + # So, set ownership/permissions accordingly. sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl - # How much of this is even needed now that we're deleting host side? - # pixl needs to be able to *read* files in the export dir, but it isn't - # creating them any more sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" From de2ea9db180ac2acbdb251f3fa738100a5d56836 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 14:25:44 +0100 Subject: [PATCH 29/74] If Export API is believed to only read from the export dir, then let's enforce this to avoid any weird surprises. --- .github/workflows/main.yml | 4 +++- docker-compose.yml | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7767e9e59..afcc7e72e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -124,11 +124,13 @@ jobs: # Export API (running as pixl) needs to be able to *read* files in # the export dir, but it doesn't create them. # So, set ownership/permissions accordingly. + # Bear in mind that ownership/perms of newly created files will + # depend on the CLI process. I think we can avoid setting the ACL for now. sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" - sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" + sudo --non-interactive chmod -R ug+rx "$host_export_root_dir" ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT diff --git a/docker-compose.yml b/docker-compose.yml index 0cb60aacf..0291e4c71 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -300,7 +300,7 @@ services: extra_hosts: - "host.docker.internal:host-gateway" volumes: - - ${PWD}/projects/exports:/run/projects/exports + - ${PWD}/projects/exports:/run/projects/exports:ro - ${PWD}/projects/configs:/${PROJECT_CONFIGS_DIR:-/projects/configs}:ro imaging-api: From 9aaf2c765b6260a44d7cfd6fd317d3ce6181ecbe Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 15:01:07 +0100 Subject: [PATCH 30/74] First attempt at documentation on docker permissions. --- README.md | 8 ++++++- docs/design/docker_perms.md | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 docs/design/docker_perms.md diff --git a/README.md b/README.md index 597e63b53..1c056c745 100644 --- a/README.md +++ b/README.md @@ -277,10 +277,16 @@ These files will subsequently be uploaded to the `parquet` destination specified [project config](#3-configure-a-new-project). ``` -EXPORT_ROOT/PROJECT_SLUG/all_extracts/EXTRACT_DATETIME/radiology/radiology.parquet +EXPORT_ROOT/PROJECT_SLUG/all_extracts/EXTRACT_DATETIME/radiology/IMAGE_LINKER.parquet ....................................................../omop/public/*.parquet ``` +This directory is current located in `projects/exports` relative to the source code root. + +The unix user that the Export API (and other containers) runs as is configurable, and +the (human) user must ensure that Export API can read the export dir given this configuration. +[Further discussion about docker permissions is found here](./docs/design/docker_perms.md) + ### Destination #### FTP server diff --git a/docs/design/docker_perms.md b/docs/design/docker_perms.md new file mode 100644 index 000000000..798e18aa8 --- /dev/null +++ b/docs/design/docker_perms.md @@ -0,0 +1,44 @@ +# Docker setup + +Setup of user IDs, file permissions, and other related things in docker. + +## Requirements and design considerations + +### Docker containers should not run as root + +Security. Different docker configs; see issue # for more + +### Export temp dir needs to be readable by Export API container + +The PIXL CLI is the only thing that writes into the Export temp dir, so it's up to the user +to make sure that it does so in such a way that it can be read by the Export API. +The Export API runs as a configurable user and group (see below), so it will depend +on which user is running the CLI, its umask settings, file permissions/ACLs, and perhaps +more as to whether it will work. + + +### PIXL_USER_UID and PIXL_USER_GID + +During docker image build, a user called `pixl` and a group called `pixl` are created. +These variables control the numerical UID and GID that will be used. + +This needs to be controlled because on the GAEs, docker is set up so that user IDs +on containers are the same as on the host. If you're UID 7001 on the container, then +you're 7001 on the host! So you will want to change these values to match which user +you want the PIXL containers to run as on the host. +Ideally you'd create a service account especially for pixl, and then set its UID/GID +in these variables. The PIXL containers will still use the name `pixl`, but as long +as the numerical IDs match, it'll consider it to be the same user. + +The variables are also used by the GHA system test to set up this user and group on +the GHA test runner, simulating how an admin would have set this up in production. + +If you are running the system test on Windows or Mac, you should find it works fine. +On linux, you may have to ensure you have installed Docker in rootless mode, otherwise +you may have to do annoying things like modify your values of these variables to match +the UID/GID of your current username, or create a new user/group (called pixl) on your +machine with UID/GID 7001. + + + + From 0bd75fbd7cd27e0ea768f2c0aedd0b2eda561a9a Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 17:05:44 +0100 Subject: [PATCH 31/74] CLI user needs to be able to write to export dir. --- .github/workflows/main.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index afcc7e72e..14ece28ff 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -130,8 +130,7 @@ jobs: sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" - sudo --non-interactive chmod -R ug+rx "$host_export_root_dir" - + sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT From db201d9b930a6de982feaa038369dd72f91cf7c8 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Fri, 10 May 2024 18:59:17 +0100 Subject: [PATCH 32/74] Clarify docs --- .github/workflows/main.yml | 6 +-- docs/design/docker_perms.md | 91 ++++++++++++++++++++++++++----------- 2 files changed, 68 insertions(+), 29 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 14ece28ff..7335ba177 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -122,10 +122,10 @@ jobs: # We need PIXL_USER_UID and PIXL_USER_GID from the test env file. source .env # Export API (running as pixl) needs to be able to *read* files in - # the export dir, but it doesn't create them. + # the export dir, but it doesn't write there. # So, set ownership/permissions accordingly. - # Bear in mind that ownership/perms of newly created files will - # depend on the CLI process. I think we can avoid setting the ACL for now. + # Bear in mind that ownership/perms of files created below this dir will + # depend on the CLI process. I think we don't need to set ACLs for now. sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl host_export_root_dir=../projects/exports diff --git a/docs/design/docker_perms.md b/docs/design/docker_perms.md index 798e18aa8..c44f116e8 100644 --- a/docs/design/docker_perms.md +++ b/docs/design/docker_perms.md @@ -1,44 +1,83 @@ # Docker setup -Setup of user IDs, file permissions, and other related things in docker. +Setup of user IDs, file permissions, ACLs, and other related things in docker. ## Requirements and design considerations -### Docker containers should not run as root +### The aim - Docker containers should not run as root -Security. Different docker configs; see issue # for more +Generally it's considered a bad idea to run anything as root that doesn't need it. +See "Principle of Least Privilege". -### Export temp dir needs to be readable by Export API container +Bear in mind that if you're running as a user in the `docker` group, you might as well +be running as root, because you can create docker containers that run as root. -The PIXL CLI is the only thing that writes into the Export temp dir, so it's up to the user -to make sure that it does so in such a way that it can be read by the Export API. -The Export API runs as a configurable user and group (see below), so it will depend -on which user is running the CLI, its umask settings, file permissions/ACLs, and perhaps -more as to whether it will work. +This page is mostly about how to not run containers as root, and the workarounds needed +to make it work. +### How do we run as non-root? -### PIXL_USER_UID and PIXL_USER_GID +During the docker image build, a user called `pixl` and a group called `pixl` are created +*on the container*. +The build variables `PIXL_USER_UID` and `PIXL_USER_GID` control the numerical UID and GID that +will be mapped to those names. When the container is started, it runs as this user. -During docker image build, a user called `pixl` and a group called `pixl` are created. -These variables control the numerical UID and GID that will be used. +The numerical IDs need to be configurable because on the GAEs, docker is set up so that +there is no remapping between user IDs on containers and user IDs on the host. +If you're UID 7001 on the container, then +you're 7001 on the host! Although containers may interpret them as `pixl:pixl`, the host may +interpret them as another user/group, or as a missing user/group. +So you will want to change these values so PIXL can play nicely with your host. -This needs to be controlled because on the GAEs, docker is set up so that user IDs -on containers are the same as on the host. If you're UID 7001 on the container, then -you're 7001 on the host! So you will want to change these values to match which user -you want the PIXL containers to run as on the host. -Ideally you'd create a service account especially for pixl, and then set its UID/GID -in these variables. The PIXL containers will still use the name `pixl`, but as long -as the numerical IDs match, it'll consider it to be the same user. +### Doesn't this have some annoying side effects? -The variables are also used by the GHA system test to set up this user and group on -the GHA test runner, simulating how an admin would have set this up in production. +Yes, it means that files created on a mounted directory by a container will have the ownership +of (eg) 7001:7001. -If you are running the system test on Windows or Mac, you should find it works fine. -On linux, you may have to ensure you have installed Docker in rootless mode, otherwise -you may have to do annoying things like modify your values of these variables to match -the UID/GID of your current username, or create a new user/group (called pixl) on your -machine with UID/GID 7001. +And similarly the container will only be able to read (or write) files on a mounted directory +according to its UID/GID. +The host will map these numerical IDs according to its view of users and groups, as defined +in the usual way (`/etc/passwd` and `/etc/group`). +In the case of the Export API, it reads files from the export dir that were written by the CLI. +So, all files in the export temp dir need to be readable by the Export API container, and therefore +it matters as what user/group the CLI is run. + +### Example + +Imagine you are a user on the GAE with username `fred`, primary group `fred`, and supplementary +group `docker`. + +When you run the CLI, it will run as fred:fred, and normally any files it creates would also have +this ownership. (ACLs defined in `/gae` will affect this however, so the files may have +a different group ownership, such as fred:docker) + +There is a problem here in that the export API will likely be running as some other user/group, +so it will not be able to read the files. + +Options for fixing: +* The CLI (or an ACL on the exports dir) makes the files world readable. Not ideal from a security + pov. +* Relying on the existing ACL for `/gae` which gives files group ownership of `docker`, we run the + Export API as the `docker` group, thus giving it read access to these files. However, this is + very similar to running the container as root, which is what this whole thing is trying to avoid. +* Create a new group (`pixl`) on the GAE and add all pixl devs to it. + Set an ACL on the exports dir that sets group ownership for all created files as `pixl`. + Then we could set its GID in the env file so that the container runs as group `pixl`. +* Always build and run the containers as the same user that is running the CLI. You set the + build vars `PIXL_USER_[UG]ID` to the numerical value of fred:fred. PIXL runs as you. When you run + CLI, the files are owned by fred:docker, so export api can read them fine. + + +# What about system testing? + +The build args are also used by the GHA system test workflow to set up this user and group on +the GHA test runner, simulating how a user might set this up in production. + +Running the system test on Windows or Mac should be fine, but on linux, it might only work if Docker +is installed in rootless mode, otherwise you may have to do annoying things like modify your values +of these variables to match the UID/GID of your current username, or create a new user/group +(called pixl) on your machine with UID/GID 7001. From 928a0926d0a2f7e3133b01a280285a6d43139d6a Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 15:05:46 +0100 Subject: [PATCH 33/74] Go with the assumption that the host machine will have a "pixl" user and group which we run our containers as, and human users will be added to the pixl group. --- .github/workflows/main.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7335ba177..e848a3010 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -121,16 +121,17 @@ jobs: run: | # We need PIXL_USER_UID and PIXL_USER_GID from the test env file. source .env - # Export API (running as pixl) needs to be able to *read* files in - # the export dir, but it doesn't write there. - # So, set ownership/permissions accordingly. - # Bear in mind that ownership/perms of files created below this dir will - # depend on the CLI process. I think we don't need to set ACLs for now. + # Set up as per docker_perms.md + # Most importantly, the pixl user is *not* in the docker group, and + # the default "runner" user is added to the pixl group. sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl - sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups docker pixl + sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups pixl + sudo --non-interactive usermod --append --groups pixl runner host_export_root_dir=../projects/exports - sudo --non-interactive chown -R pixl:docker "$host_export_root_dir" + sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" + # The top-level commands are still run as the GHA runner user, which + # is analogous to a human user on the GAE bringing up a PIXL instance. ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT From e13fe0133b77e5429bf78b1b0b9a3da751227b2b Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 15:16:29 +0100 Subject: [PATCH 34/74] oops left in empty arg --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e848a3010..e9390aa76 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -125,7 +125,7 @@ jobs: # Most importantly, the pixl user is *not* in the docker group, and # the default "runner" user is added to the pixl group. sudo --non-interactive groupadd --gid $PIXL_USER_GID pixl - sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl --groups pixl + sudo --non-interactive useradd --uid $PIXL_USER_UID --gid pixl pixl sudo --non-interactive usermod --append --groups pixl runner host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" From 017a0a93ff0cba1c7229a8141ec57e8cdf331465 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 15:44:46 +0100 Subject: [PATCH 35/74] Set the setgid bit so that subdirs/files in exports will be owned by the pixl group --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e9390aa76..8e7fcd648 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -129,7 +129,7 @@ jobs: sudo --non-interactive usermod --append --groups pixl runner host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" - sudo --non-interactive chmod -R ug+rwx "$host_export_root_dir" + sudo --non-interactive chmod -R u=rwx,g=rwxs,o= "$host_export_root_dir" # The top-level commands are still run as the GHA runner user, which # is analogous to a human user on the GAE bringing up a PIXL instance. ./run-system-test.sh coverage From 9eb831b4041afedc34aed620e2bce8e37371a765 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 16:02:09 +0100 Subject: [PATCH 36/74] debugging --- pixl_core/src/core/exports.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pixl_core/src/core/exports.py b/pixl_core/src/core/exports.py index c884b6d99..d8e92b389 100644 --- a/pixl_core/src/core/exports.py +++ b/pixl_core/src/core/exports.py @@ -15,10 +15,12 @@ from __future__ import annotations +import shlex import shutil from typing import TYPE_CHECKING import slugify +from pytest_pixl.helpers import run_subprocess from core.project_config import load_project_config from core.uploader import get_uploader @@ -117,6 +119,8 @@ def export_radiology_linker(self, linker_data: pd.DataFrame) -> pathlib.Path: @staticmethod def _mkdir(directory: pathlib.Path) -> pathlib.Path: + logger.warning("About to create directory {}", directory) + run_subprocess(shlex.split("ls -laR /home/runner/work/PIXL/PIXL/projects/exports")) directory.mkdir(parents=True, exist_ok=True) return directory From 8e9771bbcaa2894f35221e3ceb757d4dcd0eb144 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 16:44:42 +0100 Subject: [PATCH 37/74] fix debugging --- README.md | 4 +- docs/design/docker_perms.md | 83 ----------------------------------- docs/setup/docker_perms.md | 52 ++++++++++++++++++++++ pixl_core/src/core/exports.py | 9 +++- 4 files changed, 60 insertions(+), 88 deletions(-) delete mode 100644 docs/design/docker_perms.md create mode 100644 docs/setup/docker_perms.md diff --git a/README.md b/README.md index 1c056c745..3c05d51dd 100644 --- a/README.md +++ b/README.md @@ -283,9 +283,7 @@ EXPORT_ROOT/PROJECT_SLUG/all_extracts/EXTRACT_DATETIME/radiology/IMAGE_LINKER.pa This directory is current located in `projects/exports` relative to the source code root. -The unix user that the Export API (and other containers) runs as is configurable, and -the (human) user must ensure that Export API can read the export dir given this configuration. -[Further discussion about docker permissions is found here](./docs/design/docker_perms.md) +[Further discussion about docker permissions and the exports directory is found here](docs/setup/docker_perms.md) ### Destination diff --git a/docs/design/docker_perms.md b/docs/design/docker_perms.md deleted file mode 100644 index c44f116e8..000000000 --- a/docs/design/docker_perms.md +++ /dev/null @@ -1,83 +0,0 @@ -# Docker setup - -Setup of user IDs, file permissions, ACLs, and other related things in docker. - -## Requirements and design considerations - -### The aim - Docker containers should not run as root - -Generally it's considered a bad idea to run anything as root that doesn't need it. -See "Principle of Least Privilege". - -Bear in mind that if you're running as a user in the `docker` group, you might as well -be running as root, because you can create docker containers that run as root. - -This page is mostly about how to not run containers as root, and the workarounds needed -to make it work. - -### How do we run as non-root? - -During the docker image build, a user called `pixl` and a group called `pixl` are created -*on the container*. -The build variables `PIXL_USER_UID` and `PIXL_USER_GID` control the numerical UID and GID that -will be mapped to those names. When the container is started, it runs as this user. - -The numerical IDs need to be configurable because on the GAEs, docker is set up so that -there is no remapping between user IDs on containers and user IDs on the host. -If you're UID 7001 on the container, then -you're 7001 on the host! Although containers may interpret them as `pixl:pixl`, the host may -interpret them as another user/group, or as a missing user/group. -So you will want to change these values so PIXL can play nicely with your host. - -### Doesn't this have some annoying side effects? - -Yes, it means that files created on a mounted directory by a container will have the ownership -of (eg) 7001:7001. - -And similarly the container will only be able to read (or write) files on a mounted directory -according to its UID/GID. - -The host will map these numerical IDs according to its view of users and groups, as defined -in the usual way (`/etc/passwd` and `/etc/group`). - -In the case of the Export API, it reads files from the export dir that were written by the CLI. -So, all files in the export temp dir need to be readable by the Export API container, and therefore -it matters as what user/group the CLI is run. - -### Example - -Imagine you are a user on the GAE with username `fred`, primary group `fred`, and supplementary -group `docker`. - -When you run the CLI, it will run as fred:fred, and normally any files it creates would also have -this ownership. (ACLs defined in `/gae` will affect this however, so the files may have -a different group ownership, such as fred:docker) - -There is a problem here in that the export API will likely be running as some other user/group, -so it will not be able to read the files. - -Options for fixing: -* The CLI (or an ACL on the exports dir) makes the files world readable. Not ideal from a security - pov. -* Relying on the existing ACL for `/gae` which gives files group ownership of `docker`, we run the - Export API as the `docker` group, thus giving it read access to these files. However, this is - very similar to running the container as root, which is what this whole thing is trying to avoid. -* Create a new group (`pixl`) on the GAE and add all pixl devs to it. - Set an ACL on the exports dir that sets group ownership for all created files as `pixl`. - Then we could set its GID in the env file so that the container runs as group `pixl`. -* Always build and run the containers as the same user that is running the CLI. You set the - build vars `PIXL_USER_[UG]ID` to the numerical value of fred:fred. PIXL runs as you. When you run - CLI, the files are owned by fred:docker, so export api can read them fine. - - -# What about system testing? - -The build args are also used by the GHA system test workflow to set up this user and group on -the GHA test runner, simulating how a user might set this up in production. - -Running the system test on Windows or Mac should be fine, but on linux, it might only work if Docker -is installed in rootless mode, otherwise you may have to do annoying things like modify your values -of these variables to match the UID/GID of your current username, or create a new user/group -(called pixl) on your machine with UID/GID 7001. - - diff --git a/docs/setup/docker_perms.md b/docs/setup/docker_perms.md new file mode 100644 index 000000000..04444312b --- /dev/null +++ b/docs/setup/docker_perms.md @@ -0,0 +1,52 @@ +# Docker setup + +Setup of user IDs, file permissions, ACLs, and other related things in docker. + +## Requirements and design considerations + +### TL;DR - what do I do? +Change these values in your production `.env` file: +``` +PIXL_USER_UID=??? +PIXL_USER_GID=??? +``` +so that they match the UID and GID of the pixl user/group on +your host. + +Set the permissions/ACLs of the "export" directory as follows: + +chgrp -R pixl "$MY_EXPORT_DIR" +chmod -R g+rwxs "$MY_EXPORT_DIR" +setfacl -R -m d:g::rwX "$MY_EXPORT_DIR" + + +### The aim - Docker containers should not run as root + +Generally it's considered a bad idea to run anything as root that doesn't need it. +See "Principle of Least Privilege". + +Bear in mind that if you're running as a user in the `docker` group, you might as well +be running as root, because you can create docker containers that run as root. + +### How to run as non-root + +During the docker image build, a user called `pixl` with UID `PIXL_USER_UID` and +a group called `pixl` with GID `PIXL_USER_GID` are created *on the container*. +When the container is started, it runs as this user. + +The numerical IDs need to be configurable because on the GAEs, +docker is set up so that there is no remapping between user IDs on containers and +user IDs on the host. +If you're UID 7001 on the container, then +you're 7001 on the host! + + +# System testing + +The build args are also used by the GHA system test workflow to set up this user and group on +the GHA test runner, simulating how a user should set this up in production. + +If you're running the system test on linux, you will probably have better luck if Docker +is installed in rootless mode. + + diff --git a/pixl_core/src/core/exports.py b/pixl_core/src/core/exports.py index d8e92b389..aa6f2def9 100644 --- a/pixl_core/src/core/exports.py +++ b/pixl_core/src/core/exports.py @@ -17,10 +17,10 @@ import shlex import shutil +import subprocess from typing import TYPE_CHECKING import slugify -from pytest_pixl.helpers import run_subprocess from core.project_config import load_project_config from core.uploader import get_uploader @@ -120,7 +120,12 @@ def export_radiology_linker(self, linker_data: pd.DataFrame) -> pathlib.Path: @staticmethod def _mkdir(directory: pathlib.Path) -> pathlib.Path: logger.warning("About to create directory {}", directory) - run_subprocess(shlex.split("ls -laR /home/runner/work/PIXL/PIXL/projects/exports")) + cp = subprocess.run( + shlex.split("ls -laR /home/runner/work/PIXL/PIXL/projects/exports"), + check=True, + capture_output=True, + ) + logger.warning("ls rc {}", str(cp)) directory.mkdir(parents=True, exist_ok=True) return directory From 15b98c8da2d19ac07e07ad5ce0ea13f0eda02ce7 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 17:05:25 +0100 Subject: [PATCH 38/74] doesn't exist? --- pixl_core/src/core/exports.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/src/core/exports.py b/pixl_core/src/core/exports.py index aa6f2def9..b088df909 100644 --- a/pixl_core/src/core/exports.py +++ b/pixl_core/src/core/exports.py @@ -121,7 +121,7 @@ def export_radiology_linker(self, linker_data: pd.DataFrame) -> pathlib.Path: def _mkdir(directory: pathlib.Path) -> pathlib.Path: logger.warning("About to create directory {}", directory) cp = subprocess.run( - shlex.split("ls -laR /home/runner/work/PIXL/PIXL/projects/exports"), + shlex.split("ls -laR .."), check=True, capture_output=True, ) From 572144d744e33e3d538e499ce5eb43d82e32b1cd Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 17:21:45 +0100 Subject: [PATCH 39/74] Make debugging more useful and permanent. More docs --- .github/workflows/main.yml | 5 +++++ docs/setup/docker_perms.md | 16 ---------------- docs/setup/uclh-infrastructure-setup.md | 20 ++++++++++++++++++++ pixl_core/src/core/exports.py | 9 --------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8e7fcd648..0834ec277 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -150,6 +150,11 @@ jobs: run: | docker logs -t system-test-imaging-api-1 2>&1 + - name: View file structure for debugging + if: ${{ failure() }} + run: | + ls -laR + - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v4.2.0 with: diff --git a/docs/setup/docker_perms.md b/docs/setup/docker_perms.md index 04444312b..67e04cb73 100644 --- a/docs/setup/docker_perms.md +++ b/docs/setup/docker_perms.md @@ -4,22 +4,6 @@ Setup of user IDs, file permissions, ACLs, and other related things in docker. ## Requirements and design considerations -### TL;DR - what do I do? -Change these values in your production `.env` file: -``` -PIXL_USER_UID=??? -PIXL_USER_GID=??? -``` -so that they match the UID and GID of the pixl user/group on -your host. - -Set the permissions/ACLs of the "export" directory as follows: - -chgrp -R pixl "$MY_EXPORT_DIR" -chmod -R g+rwxs "$MY_EXPORT_DIR" -setfacl -R -m d:g::rwX "$MY_EXPORT_DIR" - - ### The aim - Docker containers should not run as root Generally it's considered a bad idea to run anything as root that doesn't need it. diff --git a/docs/setup/uclh-infrastructure-setup.md b/docs/setup/uclh-infrastructure-setup.md index 9f1fa92fc..22686f8d8 100644 --- a/docs/setup/uclh-infrastructure-setup.md +++ b/docs/setup/uclh-infrastructure-setup.md @@ -77,3 +77,23 @@ chmod -R g+rwxs /gae/pixl_dev # inherit group when new directories or files are setfacl -R -m d:g::rwX /gae/pixl_dev # now clone the repository or copy an existing deployment ``` + +Following clone, the exports directory needs to be permissioned appropriately: + +Change these values in your production `.env` file: +``` +PIXL_USER_UID=??? +PIXL_USER_GID=??? +``` +so that they match the UID and GID of the pixl user/group on +your host. + +Set the ownership of the "exports" directory as follows: + +``` +MY_EXPORT_DIR=/gae/pixl_dev/PIXL/projects/exports +chgrp -R pixl "$MY_EXPORT_DIR" +``` + +See [docker permissions setup](./docker_perms.md) for further discussion about +why the docker export directory needs to be set up this way. \ No newline at end of file diff --git a/pixl_core/src/core/exports.py b/pixl_core/src/core/exports.py index b088df909..c884b6d99 100644 --- a/pixl_core/src/core/exports.py +++ b/pixl_core/src/core/exports.py @@ -15,9 +15,7 @@ from __future__ import annotations -import shlex import shutil -import subprocess from typing import TYPE_CHECKING import slugify @@ -119,13 +117,6 @@ def export_radiology_linker(self, linker_data: pd.DataFrame) -> pathlib.Path: @staticmethod def _mkdir(directory: pathlib.Path) -> pathlib.Path: - logger.warning("About to create directory {}", directory) - cp = subprocess.run( - shlex.split("ls -laR .."), - check=True, - capture_output=True, - ) - logger.warning("ls rc {}", str(cp)) directory.mkdir(parents=True, exist_ok=True) return directory From 2d7c9024206a96bfda6a9e1b8072699c4eb901d0 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 17:37:02 +0100 Subject: [PATCH 40/74] more debugging --- .github/workflows/main.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0834ec277..cef8edd5a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -153,7 +153,16 @@ jobs: - name: View file structure for debugging if: ${{ failure() }} run: | - ls -laR + # just in case the error is caused by permissions, make sure all files are seen + sudo --non-interactive ls -laR + + - name: View user setup for debugging + if: ${{ failure() }} + run: | + set -x + groups + id runner + id pixl - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v4.2.0 From cf1900ed458ca4ac59a8373413cc3e88253c2b6b Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 17:50:17 +0100 Subject: [PATCH 41/74] Set the FACL --- .github/workflows/main.yml | 1 + docs/setup/uclh-infrastructure-setup.md | 3 +++ 2 files changed, 4 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index cef8edd5a..7674f5c9c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -130,6 +130,7 @@ jobs: host_export_root_dir=../projects/exports sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" sudo --non-interactive chmod -R u=rwx,g=rwxs,o= "$host_export_root_dir" + sudo --non-interactive setfacl -R -m d:g::rwX "$host_export_root_dir" # The top-level commands are still run as the GHA runner user, which # is analogous to a human user on the GAE bringing up a PIXL instance. ./run-system-test.sh coverage diff --git a/docs/setup/uclh-infrastructure-setup.md b/docs/setup/uclh-infrastructure-setup.md index 22686f8d8..3b2d1c93f 100644 --- a/docs/setup/uclh-infrastructure-setup.md +++ b/docs/setup/uclh-infrastructure-setup.md @@ -95,5 +95,8 @@ MY_EXPORT_DIR=/gae/pixl_dev/PIXL/projects/exports chgrp -R pixl "$MY_EXPORT_DIR" ``` +See Issue https://github.com/UCLH-Foundry/PIXL/issues/401 where I suggest setting +the whole PIXL repo as group `pixl` rather than `docker`, for simplicity. + See [docker permissions setup](./docker_perms.md) for further discussion about why the docker export directory needs to be set up this way. \ No newline at end of file From eb5da97e9705d1353398a8aa9977c27bda434f6e Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 18:10:06 +0100 Subject: [PATCH 42/74] usermod doesn't change groups of existing shells. Need to run the system test in a new shell. --- .github/workflows/main.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7674f5c9c..a5a7aaa12 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -133,7 +133,8 @@ jobs: sudo --non-interactive setfacl -R -m d:g::rwX "$host_export_root_dir" # The top-level commands are still run as the GHA runner user, which # is analogous to a human user on the GAE bringing up a PIXL instance. - ./run-system-test.sh coverage + # Run in a new shell so that the new group (see usermod cmd) is picked up. + bash ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From 995b35b8f936ad3bccf26d6668c8c5c957b6a4f7 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 18:36:47 +0100 Subject: [PATCH 43/74] Invoking a new shell with "bash" doesn't re-read groups, but using sudo does. --- .github/workflows/main.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a5a7aaa12..b78cc03b2 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -133,8 +133,9 @@ jobs: sudo --non-interactive setfacl -R -m d:g::rwX "$host_export_root_dir" # The top-level commands are still run as the GHA runner user, which # is analogous to a human user on the GAE bringing up a PIXL instance. - # Run in a new shell so that the new group (see usermod cmd) is picked up. - bash ./run-system-test.sh coverage + # Although we are already running as "runner", the sudo is needed + # to run in a new shell so that the new group (see usermod above) is picked up. + sudo --non-interactive -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From 303374c69bc44cd47dbc707e6ae8d4bdb13fc597 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 18:47:43 +0100 Subject: [PATCH 44/74] Need to preserve envs --- .github/workflows/main.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b78cc03b2..ff6323dcc 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -131,11 +131,11 @@ jobs: sudo --non-interactive chown -R pixl:pixl "$host_export_root_dir" sudo --non-interactive chmod -R u=rwx,g=rwxs,o= "$host_export_root_dir" sudo --non-interactive setfacl -R -m d:g::rwX "$host_export_root_dir" - # The top-level commands are still run as the GHA runner user, which + # The top-level commands are still run as the GHA "runner" user, which # is analogous to a human user on the GAE bringing up a PIXL instance. - # Although we are already running as "runner", the sudo is needed - # to run in a new shell so that the new group (see usermod above) is picked up. - sudo --non-interactive -u runner ./run-system-test.sh coverage + # The sudo command is needed to make it run in a new shell + # so that the new group (see usermod above) is picked up. + sudo --non-interactive --preserve-env -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From edde878770329bfd58fb80aa788665dee8d5b09b Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 19:08:41 +0100 Subject: [PATCH 45/74] more debugging --- .github/workflows/main.yml | 5 +++++ test/run-system-test.sh | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ff6323dcc..b99a35516 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -135,6 +135,11 @@ jobs: # is analogous to a human user on the GAE bringing up a PIXL instance. # The sudo command is needed to make it run in a new shell # so that the new group (see usermod above) is picked up. + set -x + echo $PATH + which pytest || true + sudo --non-interactive --preserve-env -u runner env + sudo --non-interactive --preserve-env -u runner which pytest || true sudo --non-interactive --preserve-env -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT diff --git a/test/run-system-test.sh b/test/run-system-test.sh index 2652ed513..3231241fa 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -60,6 +60,10 @@ else fi # Run the tests setup + set -x + which pytest || true + echo $PATH + pytest "${PYTEST_FLAGS[@]}" echo FINISHED PYTEST COMMAND teardown From ef1b113f70268d91b736fface2a5f0834a02ece6 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 20:14:54 +0100 Subject: [PATCH 46/74] Unlikely to work because su will prompt --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b99a35516..8b5baa8b7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -140,7 +140,7 @@ jobs: which pytest || true sudo --non-interactive --preserve-env -u runner env sudo --non-interactive --preserve-env -u runner which pytest || true - sudo --non-interactive --preserve-env -u runner ./run-system-test.sh coverage + su -c "./run-system-test.sh coverage" runner echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From 806449d0d019e72f409123435da9b40d9d8d051a Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 20:21:08 +0100 Subject: [PATCH 47/74] Will just passing PATH be enough? --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8b5baa8b7..9e38e331d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -140,7 +140,7 @@ jobs: which pytest || true sudo --non-interactive --preserve-env -u runner env sudo --non-interactive --preserve-env -u runner which pytest || true - su -c "./run-system-test.sh coverage" runner + sudo --non-interactive --preserve-env=PATH -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From 629a5e44554f2244c64c71bbd9bf5efc730d721b Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 20:31:35 +0100 Subject: [PATCH 48/74] Pass through all env as well as PATH --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9e38e331d..bed9f87b9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -140,7 +140,7 @@ jobs: which pytest || true sudo --non-interactive --preserve-env -u runner env sudo --non-interactive --preserve-env -u runner which pytest || true - sudo --non-interactive --preserve-env=PATH -u runner ./run-system-test.sh coverage + sudo --non-interactive --preserve-env --preserve-env=PATH -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT - name: Dump export-api docker logs for debugging From 8bbe27351f0af6f1a68ff81622ee053b937f5d23 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 15 May 2024 20:33:43 +0100 Subject: [PATCH 49/74] Remove debugging --- .github/workflows/main.yml | 16 ++-------------- test/run-system-test.sh | 4 ---- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bed9f87b9..649947aea 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -135,11 +135,7 @@ jobs: # is analogous to a human user on the GAE bringing up a PIXL instance. # The sudo command is needed to make it run in a new shell # so that the new group (see usermod above) is picked up. - set -x - echo $PATH - which pytest || true - sudo --non-interactive --preserve-env -u runner env - sudo --non-interactive --preserve-env -u runner which pytest || true + # PATH needs to be explicitly mentioned to be passed through. sudo --non-interactive --preserve-env --preserve-env=PATH -u runner ./run-system-test.sh coverage echo FINISHED SYSTEM TEST SCRIPT @@ -158,20 +154,12 @@ jobs: run: | docker logs -t system-test-imaging-api-1 2>&1 - - name: View file structure for debugging + - name: View filesystem for debugging if: ${{ failure() }} run: | # just in case the error is caused by permissions, make sure all files are seen sudo --non-interactive ls -laR - - name: View user setup for debugging - if: ${{ failure() }} - run: | - set -x - groups - id runner - id pixl - - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v4.2.0 with: diff --git a/test/run-system-test.sh b/test/run-system-test.sh index 3231241fa..2652ed513 100755 --- a/test/run-system-test.sh +++ b/test/run-system-test.sh @@ -60,10 +60,6 @@ else fi # Run the tests setup - set -x - which pytest || true - echo $PATH - pytest "${PYTEST_FLAGS[@]}" echo FINISHED PYTEST COMMAND teardown From 4c6f6d195d876d5bbbb7326657c27ffed4bd21fb Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Tue, 21 May 2024 17:01:37 +0100 Subject: [PATCH 50/74] Unite orthanc Dockerfiles --- docker-compose.yml | 6 ++- docker/orthanc-anon/Dockerfile | 46 ---------------------- docker/{orthanc-raw => orthanc}/Dockerfile | 12 +++++- pixl_imaging/tests/docker-compose.yml | 3 +- 4 files changed, 17 insertions(+), 50 deletions(-) delete mode 100644 docker/orthanc-anon/Dockerfile rename docker/{orthanc-raw => orthanc}/Dockerfile (86%) diff --git a/docker-compose.yml b/docker-compose.yml index 0291e4c71..0eb9c865d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -114,7 +114,8 @@ services: orthanc-anon: build: context: . - dockerfile: ./docker/orthanc-anon/Dockerfile + dockerfile: ./docker/orthanc/Dockerfile + target: pixl_orthanc_anon args: <<: *build-args-common platform: linux/amd64 @@ -184,7 +185,8 @@ services: orthanc-raw: build: context: . - dockerfile: ./docker/orthanc-raw/Dockerfile + dockerfile: ./docker/orthanc/Dockerfile + target: pixl_orthanc_raw args: <<: *build-args-common ORTHANC_RAW_MAXIMUM_STORAGE_SIZE: ${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE} diff --git a/docker/orthanc-anon/Dockerfile b/docker/orthanc-anon/Dockerfile deleted file mode 100644 index fc5fe000e..000000000 --- a/docker/orthanc-anon/Dockerfile +++ /dev/null @@ -1,46 +0,0 @@ -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -FROM orthancteam/orthanc:24.3.3 -SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] - -# Create a virtual environment, recommended since python 3.11 and Debian bookworm based images -# where you get a warning when installing system-wide packages. -RUN export DEBIAN_FRONTEND=noninteractive && \ - apt-get update && \ - apt-get install --yes --no-install-recommends python3-venv -RUN python3 -m venv /.venv - -# Install curl, used in system tests -RUN apt-get --assume-yes install curl - -# Install requirements before copying modules -COPY ./pixl_core/pyproject.toml /pixl_core/pyproject.toml -COPY ./pixl_dcmd/pyproject.toml /pixl_dcmd/pyproject.toml - -RUN --mount=type=cache,target=/root/.cache \ - /.venv/bin/pip install pixl_core/ \ - && /.venv/bin/pip install pixl_dcmd/ - -COPY ./orthanc/orthanc-anon/plugin/pixl.py /etc/orthanc/pixl.py - -COPY ./pixl_core/ /pixl_core -RUN --mount=type=cache,target=/root/.cache \ - /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_core - -COPY ./pixl_dcmd/ /pixl_dcmd -RUN --mount=type=cache,target=/root/.cache \ - /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd - -ENV PYTHONPATH=/.venv/lib64/python3.11/site-packages/ - diff --git a/docker/orthanc-raw/Dockerfile b/docker/orthanc/Dockerfile similarity index 86% rename from docker/orthanc-raw/Dockerfile rename to docker/orthanc/Dockerfile index b558520d8..1d85f8e1b 100644 --- a/docker/orthanc-raw/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -FROM orthancteam/orthanc:24.3.3 +FROM orthancteam/orthanc:24.3.3 AS pixl_orthanc_base SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] @@ -22,6 +22,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ apt-get install --yes --no-install-recommends python3-venv RUN python3 -m venv /.venv +# Install curl for now, but try to remove this dependency +RUN apt-get --assume-yes install curl + # Install requirements before copying modules COPY ./pixl_core/pyproject.toml /pixl_core/pyproject.toml COPY ./pixl_dcmd/pyproject.toml /pixl_dcmd/pyproject.toml @@ -38,6 +41,8 @@ COPY ./pixl_dcmd/ /pixl_dcmd RUN --mount=type=cache,target=/root/.cache \ /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd +FROM pixl_orthanc_base AS pixl_orthanc_raw + COPY ./orthanc/orthanc-raw/plugin/pixl.py /etc/orthanc/pixl.py # Orthanc can't substitute environment veriables as integers so copy and replace before running @@ -50,3 +55,8 @@ RUN sed -i "s/\${ORTHANC_RAW_JOB_HISTORY_SIZE}/${ORTHANC_RAW_JOB_HISTORY_SIZE:-1 RUN sed -i "s/\${ORTHANC_RAW_CONCURRENT_JOBS}/${ORTHANC_RAW_CONCURRENT_JOBS:-5}/g" /run/secrets/orthanc.json ENV PYTHONPATH=/.venv/lib64/python3.11/site-packages/ + +FROM pixl_orthanc_base AS pixl_orthanc_anon +COPY ./orthanc/orthanc-anon/plugin/pixl.py /etc/orthanc/pixl.py + +ENV PYTHONPATH=/.venv/lib64/python3.11/site-packages/ diff --git a/pixl_imaging/tests/docker-compose.yml b/pixl_imaging/tests/docker-compose.yml index 56c8a776d..bf318b794 100644 --- a/pixl_imaging/tests/docker-compose.yml +++ b/pixl_imaging/tests/docker-compose.yml @@ -50,7 +50,8 @@ services: orthanc-raw: build: context: ../../ - dockerfile: ./docker/orthanc-raw/Dockerfile + dockerfile: ./docker/orthanc/Dockerfile + target: pixl_orthanc_raw environment: ORTHANC_NAME: "PIXL: Raw" ORTHANC_USERNAME: "orthanc" From 1b46c836093edfd81e11fb0776422f6c18ee5e7e Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Tue, 21 May 2024 18:02:00 +0100 Subject: [PATCH 51/74] Add orthanc containers to test --- test/test_docker_containers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test_docker_containers.py b/test/test_docker_containers.py index da50e4abb..3fb89f875 100644 --- a/test/test_docker_containers.py +++ b/test/test_docker_containers.py @@ -27,7 +27,8 @@ "hasher-api", "postgres", "queue", - # hopefully we can fix orthanc as well + "orthanc-raw", + "orthanc-anon", ], ) def test_non_root_uids(container: str) -> None: From 468d9080afcaeeb4efe53c83a3cfb147d54da2f4 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 12:51:12 +0100 Subject: [PATCH 52/74] Make orthanc run as orthanc user instead of root --- docker/orthanc/Dockerfile | 12 +++++++++++- docs/setup/docker_perms.md | 8 ++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 1d85f8e1b..194ff605a 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -41,6 +41,15 @@ COPY ./pixl_dcmd/ /pixl_dcmd RUN --mount=type=cache,target=/root/.cache \ /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd +USER orthanc + +# Because the volume contents inherit the ownership of the mountpoint if it already exists, +# creating and chowning at this point ensures that this happens. +# See https://devops.stackexchange.com/a/4542 although the docs no longer seem to mention this. +# The VOLUME instruction is not needed because this is specified by the docker compose file. +ARG ORTHANC_DATA_VOLUME=/var/lib/orthanc/db/ +RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:orthanc "$ORTHANC_DATA_VOLUME" + FROM pixl_orthanc_base AS pixl_orthanc_raw COPY ./orthanc/orthanc-raw/plugin/pixl.py /etc/orthanc/pixl.py @@ -49,7 +58,8 @@ COPY ./orthanc/orthanc-raw/plugin/pixl.py /etc/orthanc/pixl.py ARG ORTHANC_RAW_MAXIMUM_STORAGE_SIZE ARG ORTHANC_RAW_JOB_HISTORY_SIZE ARG ORTHANC_RAW_CONCURRENT_JOBS -COPY ./orthanc/orthanc-raw/config /run/secrets +COPY --chown=orthanc:orthanc ./orthanc/orthanc-raw/config /run/secrets +RUN ls -la /run/secrets RUN sed -i "s/\${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}/${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE:-0}/g" /run/secrets/orthanc.json RUN sed -i "s/\${ORTHANC_RAW_JOB_HISTORY_SIZE}/${ORTHANC_RAW_JOB_HISTORY_SIZE:-100}/g" /run/secrets/orthanc.json RUN sed -i "s/\${ORTHANC_RAW_CONCURRENT_JOBS}/${ORTHANC_RAW_CONCURRENT_JOBS:-5}/g" /run/secrets/orthanc.json diff --git a/docs/setup/docker_perms.md b/docs/setup/docker_perms.md index 67e04cb73..ff8fac1d9 100644 --- a/docs/setup/docker_perms.md +++ b/docs/setup/docker_perms.md @@ -14,16 +14,20 @@ be running as root, because you can create docker containers that run as root. ### How to run as non-root -During the docker image build, a user called `pixl` with UID `PIXL_USER_UID` and +During the build process for the PIXL images that we build from scratch (export, imaging, hasher), +a user called `pixl` with UID `PIXL_USER_UID` and a group called `pixl` with GID `PIXL_USER_GID` are created *on the container*. When the container is started, it runs as this user. The numerical IDs need to be configurable because on the GAEs, docker is set up so that there is no remapping between user IDs on containers and user IDs on the host. -If you're UID 7001 on the container, then +Eg. if you're UID 7001 on the container, then you're 7001 on the host! +So you should set these arguments to match the GAE user/group that you want the containers to run as. +The images for orthanc, postgres and rabbitmq already come with an existing non-root user. +It seems to be easier to use this, rather than trying to them run as `pixl`. # System testing From 1004681a925ad2d82f859c0f7a8e7dea72696e53 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 13:34:40 +0100 Subject: [PATCH 53/74] Can also use non-root for fakes in system test --- test/docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/docker-compose.yml b/test/docker-compose.yml index 71f186b72..42b4c9fe4 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -20,6 +20,7 @@ networks: services: vna-qr: image: orthancteam/orthanc:24.3.3 + user: orthanc platform: linux/amd64 environment: ORTHANC_NAME: ${VNAQR_AE_TITLE} @@ -44,6 +45,7 @@ services: pixl-net: dicomweb-server: image: orthancteam/orthanc:24.4.0 + user: orthanc platform: linux/amd64 environment: ORTHANC_NAME: "dicomweb" From 668ebc07bbfee75aad0652d99913be7bb8af48a5 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 13:38:39 +0100 Subject: [PATCH 54/74] Non-root orthanc works for core and imaging tests too --- pixl_core/tests/docker-compose.yml | 2 ++ pixl_imaging/tests/docker-compose.yml | 1 + 2 files changed, 3 insertions(+) diff --git a/pixl_core/tests/docker-compose.yml b/pixl_core/tests/docker-compose.yml index e6d81fa97..ed6182d65 100644 --- a/pixl_core/tests/docker-compose.yml +++ b/pixl_core/tests/docker-compose.yml @@ -32,6 +32,7 @@ services: orthanc: image: orthancteam/orthanc:24.4.0 + user: orthanc platform: linux/amd64 environment: ORTHANC_NAME: "orthanc" @@ -56,6 +57,7 @@ services: dicomweb-server: image: orthancteam/orthanc:24.4.0 + user: orthanc platform: linux/amd64 environment: ORTHANC_NAME: "dicomweb" diff --git a/pixl_imaging/tests/docker-compose.yml b/pixl_imaging/tests/docker-compose.yml index bf318b794..f1e237d20 100644 --- a/pixl_imaging/tests/docker-compose.yml +++ b/pixl_imaging/tests/docker-compose.yml @@ -32,6 +32,7 @@ x-orthanc-healthcheck: &orthanc-healthcheck services: vna-qr: image: orthancteam/orthanc:24.3.3 + user: orthanc environment: ORTHANC_NAME: "VNAQR" ORTHANC_USERNAME: "orthanc" From 695f64f20afc0a4bf78d77ae403beadb2e628f95 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 13:39:09 +0100 Subject: [PATCH 55/74] Add fakes to system test container user check --- test/test_docker_containers.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/test_docker_containers.py b/test/test_docker_containers.py index 3fb89f875..9ae853a68 100644 --- a/test/test_docker_containers.py +++ b/test/test_docker_containers.py @@ -29,6 +29,8 @@ "queue", "orthanc-raw", "orthanc-anon", + "vna-qr", + "dicomweb-server", ], ) def test_non_root_uids(container: str) -> None: From b34abbc1f1a8da695cb330dba40be50a459b4544 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 15:10:20 +0100 Subject: [PATCH 56/74] Remove debugging --- docker/orthanc/Dockerfile | 1 - 1 file changed, 1 deletion(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 194ff605a..effe3d385 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -59,7 +59,6 @@ ARG ORTHANC_RAW_MAXIMUM_STORAGE_SIZE ARG ORTHANC_RAW_JOB_HISTORY_SIZE ARG ORTHANC_RAW_CONCURRENT_JOBS COPY --chown=orthanc:orthanc ./orthanc/orthanc-raw/config /run/secrets -RUN ls -la /run/secrets RUN sed -i "s/\${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}/${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE:-0}/g" /run/secrets/orthanc.json RUN sed -i "s/\${ORTHANC_RAW_JOB_HISTORY_SIZE}/${ORTHANC_RAW_JOB_HISTORY_SIZE:-100}/g" /run/secrets/orthanc.json RUN sed -i "s/\${ORTHANC_RAW_CONCURRENT_JOBS}/${ORTHANC_RAW_CONCURRENT_JOBS:-5}/g" /run/secrets/orthanc.json From 02e3b79de5768061d293796dd060eb7be7f2de05 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 15:16:12 +0100 Subject: [PATCH 57/74] Use same orthanc version everywhere --- docker/orthanc/Dockerfile | 2 +- pixl_imaging/tests/docker-compose.yml | 2 +- test/docker-compose.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index effe3d385..71ea80ec5 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -FROM orthancteam/orthanc:24.3.3 AS pixl_orthanc_base +FROM orthancteam/orthanc:24.4.0 AS pixl_orthanc_base SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"] diff --git a/pixl_imaging/tests/docker-compose.yml b/pixl_imaging/tests/docker-compose.yml index f1e237d20..29317781b 100644 --- a/pixl_imaging/tests/docker-compose.yml +++ b/pixl_imaging/tests/docker-compose.yml @@ -31,7 +31,7 @@ x-orthanc-healthcheck: &orthanc-healthcheck services: vna-qr: - image: orthancteam/orthanc:24.3.3 + image: orthancteam/orthanc:24.4.0 user: orthanc environment: ORTHANC_NAME: "VNAQR" diff --git a/test/docker-compose.yml b/test/docker-compose.yml index 42b4c9fe4..23e143111 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -19,7 +19,7 @@ networks: services: vna-qr: - image: orthancteam/orthanc:24.3.3 + image: orthancteam/orthanc:24.4.0 user: orthanc platform: linux/amd64 environment: From 420fb7f4508af84c3d14caecdf2493f4f096cb43 Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 15:33:19 +0100 Subject: [PATCH 58/74] Use existing YAML anchors --- docker-compose.yml | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 0eb9c865d..c7c3d898b 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -121,7 +121,7 @@ services: platform: linux/amd64 command: /run/secrets environment: - <<: [*proxy-common, *pixl-common-env, *azure-keyvault] + <<: [*proxy-common, *pixl-common-env, *azure-keyvault, *pixl-db] ORTHANC_NAME: "PIXL: Anon" ORTHANC_USERNAME: ${ORTHANC_ANON_USERNAME} ORTHANC_PASSWORD: ${ORTHANC_ANON_PASSWORD} @@ -134,11 +134,6 @@ services: ORTHANC_ANON_URL: "http://localhost:8042" ORTHANC_ANON_USERNAME: ${ORTHANC_ANON_USERNAME} ORTHANC_ANON_PASSWORD: ${ORTHANC_ANON_PASSWORD} - PIXL_DB_HOST: ${PIXL_DB_HOST} - PIXL_DB_PORT: ${PIXL_DB_PORT} - PIXL_DB_NAME: ${PIXL_DB_NAME} - PIXL_DB_USER: ${PIXL_DB_USER} - PIXL_DB_PASSWORD: ${PIXL_DB_PASSWORD} DICOM_WEB_PLUGIN_ENABLED: ${ENABLE_DICOM_WEB} HASHER_API_AZ_NAME: "hasher-api" HASHER_API_PORT: 8000 @@ -325,18 +320,13 @@ services: networks: - pixl-net environment: - <<: [*pixl-rabbit-mq, *proxy-common, *pixl-common-env] + <<: [*pixl-rabbit-mq, *proxy-common, *pixl-common-env, *pixl-db] ORTHANC_RAW_URL: ${ORTHANC_RAW_URL} ORTHANC_RAW_USERNAME: ${ORTHANC_RAW_USERNAME} ORTHANC_RAW_PASSWORD: ${ORTHANC_RAW_PASSWORD} ORTHANC_RAW_AE_TITLE: ${ORTHANC_RAW_AE_TITLE} VNAQR_MODALITY: ${VNAQR_MODALITY} SKIP_ALEMBIC: ${SKIP_ALEMBIC} - PIXL_DB_HOST: ${PIXL_DB_HOST} - PIXL_DB_PORT: ${PIXL_DB_PORT} - PIXL_DB_NAME: ${PIXL_DB_NAME} - PIXL_DB_USER: ${PIXL_DB_USER} - PIXL_DB_PASSWORD: ${PIXL_DB_PASSWORD} PIXL_DICOM_TRANSFER_TIMEOUT: ${PIXL_DICOM_TRANSFER_TIMEOUT} PIXL_QUERY_TIMEOUT: ${PIXL_QUERY_TIMEOUT} PIXL_MAX_MESSAGES_IN_FLIGHT: ${PIXL_MAX_MESSAGES_IN_FLIGHT} From b4d2526c24c593e89a66870d74b66a2aa7c288ec Mon Sep 17 00:00:00 2001 From: Jeremy Stein Date: Wed, 22 May 2024 15:39:58 +0100 Subject: [PATCH 59/74] Don't think this is needed since we moved this code to the export-api --- docker-compose.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index c7c3d898b..355d4ddad 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -130,10 +130,6 @@ services: ORTHANC_RAW_AE_TITLE: ${ORTHANC_RAW_AE_TITLE} ORTHANC_RAW_DICOM_PORT: "4242" ORTHANC_RAW_HOSTNAME: "orthanc-raw" - # For the export API - ORTHANC_ANON_URL: "http://localhost:8042" - ORTHANC_ANON_USERNAME: ${ORTHANC_ANON_USERNAME} - ORTHANC_ANON_PASSWORD: ${ORTHANC_ANON_PASSWORD} DICOM_WEB_PLUGIN_ENABLED: ${ENABLE_DICOM_WEB} HASHER_API_AZ_NAME: "hasher-api" HASHER_API_PORT: 8000 From cca7b93beecf5e3464e60982ea0f28fec4228aea Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:27:37 +0100 Subject: [PATCH 60/74] Try adding orthanc to PIXL group --- docker/orthanc/Dockerfile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 71ea80ec5..0c6e6a900 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -41,6 +41,8 @@ COPY ./pixl_dcmd/ /pixl_dcmd RUN --mount=type=cache,target=/root/.cache \ /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd +ARG PIXL_USER_GID +RUN groupadd --gid $PIXL_USER_GID orthanc USER orthanc # Because the volume contents inherit the ownership of the mountpoint if it already exists, @@ -48,7 +50,7 @@ USER orthanc # See https://devops.stackexchange.com/a/4542 although the docs no longer seem to mention this. # The VOLUME instruction is not needed because this is specified by the docker compose file. ARG ORTHANC_DATA_VOLUME=/var/lib/orthanc/db/ -RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:orthanc "$ORTHANC_DATA_VOLUME" +RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:$PIXL_USER_GID "$ORTHANC_DATA_VOLUME" FROM pixl_orthanc_base AS pixl_orthanc_raw From 5925520c8abd380f80657f43a3d60f321b95947c Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:28:23 +0100 Subject: [PATCH 61/74] Try adding orthanc to PIXL group --- docker/orthanc/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 0c6e6a900..050a763c4 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -42,7 +42,7 @@ RUN --mount=type=cache,target=/root/.cache \ /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd ARG PIXL_USER_GID -RUN groupadd --gid $PIXL_USER_GID orthanc +RUN groupadd --gid $PIXL_USER_GID pixl USER orthanc # Because the volume contents inherit the ownership of the mountpoint if it already exists, @@ -50,7 +50,7 @@ USER orthanc # See https://devops.stackexchange.com/a/4542 although the docs no longer seem to mention this. # The VOLUME instruction is not needed because this is specified by the docker compose file. ARG ORTHANC_DATA_VOLUME=/var/lib/orthanc/db/ -RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:$PIXL_USER_GID "$ORTHANC_DATA_VOLUME" +RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:pixl "$ORTHANC_DATA_VOLUME" FROM pixl_orthanc_base AS pixl_orthanc_raw From 34370052487a27fe704e5c3e2b232fc3f07f22a0 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:28:59 +0100 Subject: [PATCH 62/74] Try adding orthanc to PIXL group --- docker/orthanc/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 050a763c4..34145e2c3 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -50,7 +50,7 @@ USER orthanc # See https://devops.stackexchange.com/a/4542 although the docs no longer seem to mention this. # The VOLUME instruction is not needed because this is specified by the docker compose file. ARG ORTHANC_DATA_VOLUME=/var/lib/orthanc/db/ -RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:pixl "$ORTHANC_DATA_VOLUME" +RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:orthanc "$ORTHANC_DATA_VOLUME" FROM pixl_orthanc_base AS pixl_orthanc_raw From 905e741f74e6ef97e905b5e34982ab466f2391a6 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:37:34 +0100 Subject: [PATCH 63/74] Try adding orthanc to PIXL group --- docker/orthanc/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 34145e2c3..9bb345ddd 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -42,7 +42,7 @@ RUN --mount=type=cache,target=/root/.cache \ /.venv/bin/pip install --no-cache-dir --force-reinstall --no-deps ./pixl_dcmd ARG PIXL_USER_GID -RUN groupadd --gid $PIXL_USER_GID pixl +RUN groupadd --gid $PIXL_USER_GID pixl && usermod -a -G pixl orthanc USER orthanc # Because the volume contents inherit the ownership of the mountpoint if it already exists, From 19be6c7153fa4c094a960dcac9a4a76c71f5db2d Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:47:22 +0100 Subject: [PATCH 64/74] Change owner of pixl file --- docker/orthanc/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 9bb345ddd..7af8a045d 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -54,7 +54,7 @@ RUN mkdir "$ORTHANC_DATA_VOLUME" && chown orthanc:orthanc "$ORTHANC_DATA_VOLUME" FROM pixl_orthanc_base AS pixl_orthanc_raw -COPY ./orthanc/orthanc-raw/plugin/pixl.py /etc/orthanc/pixl.py +COPY --chown=orthanc:orthanc ./orthanc/orthanc-raw/plugin/pixl.py /etc/orthanc/pixl.py # Orthanc can't substitute environment veriables as integers so copy and replace before running ARG ORTHANC_RAW_MAXIMUM_STORAGE_SIZE @@ -68,6 +68,6 @@ RUN sed -i "s/\${ORTHANC_RAW_CONCURRENT_JOBS}/${ORTHANC_RAW_CONCURRENT_JOBS:-5}/ ENV PYTHONPATH=/.venv/lib64/python3.11/site-packages/ FROM pixl_orthanc_base AS pixl_orthanc_anon -COPY ./orthanc/orthanc-anon/plugin/pixl.py /etc/orthanc/pixl.py +COPY --chown=orthanc:orthanc ./orthanc/orthanc-anon/plugin/pixl.py /etc/orthanc/pixl.py ENV PYTHONPATH=/.venv/lib64/python3.11/site-packages/ From 4c39072451a817d1d5ce2f16320c6fb43b5280c4 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:55:43 +0100 Subject: [PATCH 65/74] Try setting pixl user before copying --- docker/pixl-python/Dockerfile | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index 6f8645f21..eef46d9e6 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -32,6 +32,12 @@ RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app +# Add pixl user +ARG PIXL_USER_UID +ARG PIXL_USER_GID +RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl +USER pixl + # specify which of our packages we're installing using build time arg ARG PIXL_PACKAGE_DIR @@ -48,10 +54,6 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi -ARG PIXL_USER_UID -ARG PIXL_USER_GID -RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl -USER pixl # Each container should be run with a different entry point FROM pixl_python_base AS export_api From 32ca6bbceb8df18cb6fd8ed3be6677daff837a47 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 14:57:36 +0100 Subject: [PATCH 66/74] Change owner of python projects on copy --- docker/pixl-python/Dockerfile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index eef46d9e6..e0ca60fc0 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -42,14 +42,14 @@ USER pixl ARG PIXL_PACKAGE_DIR # Install requirements before copying modules -COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml -COPY ./$PIXL_PACKAGE_DIR/pyproject.toml ./$PIXL_PACKAGE_DIR/pyproject.toml +COPY --chown=pixl:pixl ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml +COPY --chown=pixl:pixl ./$PIXL_PACKAGE_DIR/pyproject.toml ./$PIXL_PACKAGE_DIR/pyproject.toml RUN pip3 install --no-cache-dir pixl_core/ \ && pip3 install --no-cache-dir $PIXL_PACKAGE_DIR/ # Install our code -COPY ./pixl_core/ pixl_core/ -COPY ./$PIXL_PACKAGE_DIR/ . +COPY --chown=pixl:pixl ./pixl_core/ pixl_core/ +COPY --chown=pixl:pixl ./$PIXL_PACKAGE_DIR/ . RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi From 81eabb7570708ded044d2a6412d7f4f305e4a709 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 15:04:31 +0100 Subject: [PATCH 67/74] Change owner of python projects on copy --- docker/pixl-python/Dockerfile | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index e0ca60fc0..f52593d96 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -32,11 +32,6 @@ RUN apt-get autoremove && apt-get clean && rm -rf /var/lib/apt/lists/* HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app -# Add pixl user -ARG PIXL_USER_UID -ARG PIXL_USER_GID -RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl -USER pixl # specify which of our packages we're installing using build time arg ARG PIXL_PACKAGE_DIR @@ -54,6 +49,11 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi +# Add pixl user +ARG PIXL_USER_UID +ARG PIXL_USER_GID +RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl +USER pixl # Each container should be run with a different entry point FROM pixl_python_base AS export_api From 21ccb1361961b083925cdead79f54c2b1b60ba5c Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 15:04:54 +0100 Subject: [PATCH 68/74] Change owner of python projects on copy --- docker/pixl-python/Dockerfile | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docker/pixl-python/Dockerfile b/docker/pixl-python/Dockerfile index f52593d96..392599171 100644 --- a/docker/pixl-python/Dockerfile +++ b/docker/pixl-python/Dockerfile @@ -33,6 +33,11 @@ HEALTHCHECK CMD /usr/bin/curl -f http://0.0.0.0:8000/heart-beat || exit 1 WORKDIR /app +# Add pixl user +ARG PIXL_USER_UID +ARG PIXL_USER_GID +RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl + # specify which of our packages we're installing using build time arg ARG PIXL_PACKAGE_DIR @@ -49,10 +54,6 @@ RUN pip install --no-cache-dir --force-reinstall --no-deps pixl_core/ \ --no-cache-dir --force-reinstall --no-deps . && \ if [ "$TEST" = "true" ]; then pip install --no-cache-dir pixl_core/[test] .[test]; fi -# Add pixl user -ARG PIXL_USER_UID -ARG PIXL_USER_GID -RUN groupadd --gid $PIXL_USER_GID pixl && useradd --uid $PIXL_USER_UID --gid $PIXL_USER_GID pixl USER pixl # Each container should be run with a different entry point From ee1b97cabb65466ea2de7a93203ed4929d1f6a8b Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 17:19:41 +0100 Subject: [PATCH 69/74] Define wait for images to be exported in conftest --- test/conftest.py | 52 ++++++++++++++++++++++++++++++++++++---- test/utils.py | 62 ------------------------------------------------ 2 files changed, 47 insertions(+), 67 deletions(-) delete mode 100644 test/utils.py diff --git a/test/conftest.py b/test/conftest.py index 0f66a8c7d..68efbe401 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -15,10 +15,15 @@ # ruff: noqa: C408 dict() makes test data easier to read and write import os -from collections.abc import Generator +from functools import partial, update_wrapper from pathlib import Path from typing import Any +from core.db.models import Image +from core.db.queries import engine +from sqlalchemy import cast, not_ +from sqlalchemy.orm import sessionmaker + # Setting env variables before loading modules os.environ["PIXL_DB_HOST"] = "localhost" os.environ["PIXL_DB_PORT"] = "7001" @@ -30,9 +35,8 @@ import requests from pytest_pixl.dicom import generate_dicom_dataset from pytest_pixl.ftpserver import PixlFTPServer -from pytest_pixl.helpers import run_subprocess +from pytest_pixl.helpers import run_subprocess, wait_for_condition from pytest_pixl.plugin import FtpHostAddress -from utils import wait_for_images_to_be_exported pytest_plugins = "pytest_pixl" @@ -142,12 +146,50 @@ def _upload_dicom_instance(dicom_dir: Path, **kwargs: Any) -> None: _upload_to_vna(test_dcm_file) +def wait_for_images_to_be_exported( + seconds_max: int, + seconds_interval: int, + seconds_condition_stays_true_for: int, + min_studies: int = 3, +) -> None: + """ + Query pixl DB to ensure that images have been processed and exported. + If they haven't within the time limit, raise a TimeoutError + """ + studies: list[Image] = [] + + def at_least_n_studies_exported(n_studies: int) -> bool: + nonlocal studies + + PixlSession = sessionmaker(engine) + with PixlSession() as session: + studies = cast( + list[Image], + session.query(Image).filter(not_(Image.exported_at.is_(None))).all(), + ) + return len(studies) >= n_studies + + condition = partial(at_least_n_studies_exported, min_studies) + update_wrapper(condition, at_least_n_studies_exported) + + def list_studies() -> str: + return f"Expecting at least {min_studies} studies.\nexported studies: {studies}" + + wait_for_condition( + condition, + seconds_max=seconds_max, + seconds_interval=seconds_interval, + progress_string_fn=list_studies, + seconds_condition_stays_true_for=seconds_condition_stays_true_for, + ) + + @pytest.fixture(scope="session") def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None) -> None: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress - wait_for_stable_orthanc_anon(181, 5, 15, min_instances=3) + wait_for_images_to_be_exported(181, 5, 15) @pytest.fixture(scope="session") @@ -155,7 +197,7 @@ def _setup_pixl_cli_dicomweb(_populate_vna: None) -> None: """Run pixl populate/start. Cleanup intermediate export dir on exit.""" run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DICOMWEB_DIR.absolute())], TEST_DIR) # poll here for two minutes to check for imaging to be processed, printing progress - wait_for_stable_orthanc_anon(181, 5, 15, min_instances=3) + wait_for_images_to_be_exported(181, 5, 15) @pytest.fixture(scope="session") diff --git a/test/utils.py b/test/utils.py deleted file mode 100644 index b512d8d46..000000000 --- a/test/utils.py +++ /dev/null @@ -1,62 +0,0 @@ -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""Utilities for the system test""" - -from functools import partial, update_wrapper -from typing import cast - -from core.db.models import Image -from core.db.queries import engine -from pytest_pixl.helpers import wait_for_condition -from sqlalchemy import not_ -from sqlalchemy.orm import sessionmaker - - -def wait_for_images_to_be_exported( - seconds_max: int, - seconds_interval: int, - seconds_condition_stays_true_for: int, - min_studies: int = 2, -) -> None: - """ - Query pixl DB to ensure that images have been processed and exported. - If they haven't within the time limit, raise a TimeoutError - """ - studies: list[Image] = [] - - def at_least_n_studies_exported(n_studies: int) -> bool: - nonlocal studies - - PixlSession = sessionmaker(engine) - with PixlSession() as session: - studies = cast( - list[Image], - session.query(Image).filter(not_(Image.exported_at.is_(None))).all(), - ) - return len(studies) >= n_studies - - condition = partial(at_least_n_studies_exported, min_studies) - update_wrapper(condition, at_least_n_studies_exported) - - def list_studies() -> str: - return f"Expecting at least {min_studies} studies.\nexported studies: {studies}" - - wait_for_condition( - condition, - seconds_max=seconds_max, - seconds_interval=seconds_interval, - progress_string_fn=list_studies, - seconds_condition_stays_true_for=seconds_condition_stays_true_for, - ) From 47822a94116742a6e9e0032e4f44aefec8c9c1d9 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Fri, 14 Jun 2024 17:25:27 +0100 Subject: [PATCH 70/74] Add group ID to test build args --- pixl_imaging/tests/docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pixl_imaging/tests/docker-compose.yml b/pixl_imaging/tests/docker-compose.yml index 29317781b..20d15373a 100644 --- a/pixl_imaging/tests/docker-compose.yml +++ b/pixl_imaging/tests/docker-compose.yml @@ -53,6 +53,8 @@ services: context: ../../ dockerfile: ./docker/orthanc/Dockerfile target: pixl_orthanc_raw + args: + PIXL_USER_GID: 9999 environment: ORTHANC_NAME: "PIXL: Raw" ORTHANC_USERNAME: "orthanc" From 6e945c3e9748f8e798af2cc9adad06da5cdb1f65 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Thu, 18 Jul 2024 11:23:30 +0100 Subject: [PATCH 71/74] Update README.md Co-authored-by: Milan Malfait --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e5a3fbaaa..41502a5e4 100644 --- a/README.md +++ b/README.md @@ -299,7 +299,7 @@ EXPORT_ROOT/PROJECT_SLUG/all_extracts/EXTRACT_DATETIME/radiology/IMAGE_LINKER.pa ....................................................../omop/public/*.parquet ``` -This directory is current located in `projects/exports` relative to the source code root. +This directory is currently located in `projects/exports` relative to the source code root. [Further discussion about docker permissions and the exports directory is found here](docs/setup/docker_perms.md) From 521b1cdd424dcb3bfd684def1b9785a191d775cf Mon Sep 17 00:00:00 2001 From: Milan Malfait Date: Tue, 30 Jul 2024 12:15:34 +0100 Subject: [PATCH 72/74] Consolidate `RUN` commands Following good practice: https://github.com/hadolint/hadolint/wiki/DL3059 --- docker/orthanc/Dockerfile | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index 033f8e889..d5b363a96 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -67,10 +67,12 @@ ARG PIXL_DICOM_TRANSFER_TIMEOUT # Orthanc can't substitute environment veriables as integers so copy and replace before running COPY --chown=orthanc:orthanc ./orthanc/orthanc-raw/config /run/secrets -RUN sed -i "s/\${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}/${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE:-0}/g" /run/secrets/orthanc.json -RUN sed -i "s/\${ORTHANC_RAW_JOB_HISTORY_SIZE}/${ORTHANC_RAW_JOB_HISTORY_SIZE:-100}/g" /run/secrets/orthanc.json -RUN sed -i "s/\${ORTHANC_CONCURRENT_JOBS}/${ORTHANC_CONCURRENT_JOBS:-5}/g" /run/secrets/orthanc.json -RUN sed -i "s/\${ORTHANC_RAW_STABLE_SECONDS}/$((PIXL_DICOM_TRANSFER_TIMEOUT + ${ORTHANC_RAW_EXTRA_STABLE_SECONDS:-60}))/g" /run/secrets/orthanc.json +RUN < Date: Wed, 31 Jul 2024 17:56:38 +0100 Subject: [PATCH 73/74] Add @dram1964's instructions for setting up a PIXL user on GAE --- docs/setup/uclh-infrastructure-setup.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/setup/uclh-infrastructure-setup.md b/docs/setup/uclh-infrastructure-setup.md index 3b2d1c93f..43d27cd73 100644 --- a/docs/setup/uclh-infrastructure-setup.md +++ b/docs/setup/uclh-infrastructure-setup.md @@ -80,7 +80,20 @@ setfacl -R -m d:g::rwX /gae/pixl_dev Following clone, the exports directory needs to be permissioned appropriately: +## Create a PIXL system account + +To avoid [running Docker containers as root](https://github.com/UCLH-Foundry/PIXL/issues/234), we set up a service account on the GAE as follows + +```shell +groupadd -r -g 250 PIXL +useradd -r -s /usr/bin/false -c "PIXL Service Account" -u 250 -g 250 PIXL +usermod -a -G PIXL $user_to_add +``` + +Ask [@dram1964](https://github.com/dram1964) for help with this. + Change these values in your production `.env` file: + ``` PIXL_USER_UID=??? PIXL_USER_GID=??? @@ -99,4 +112,4 @@ See Issue https://github.com/UCLH-Foundry/PIXL/issues/401 where I suggest settin the whole PIXL repo as group `pixl` rather than `docker`, for simplicity. See [docker permissions setup](./docker_perms.md) for further discussion about -why the docker export directory needs to be set up this way. \ No newline at end of file +why the docker export directory needs to be set up this way. From 75af5265cb0ed35b4fd73f970569bf70ffea57e8 Mon Sep 17 00:00:00 2001 From: Stef Piatek Date: Mon, 7 Oct 2024 17:12:59 +0100 Subject: [PATCH 74/74] Merge remote-tracking branch 'origin/main' into jeremy/docker-uid-new # Conflicts: # .github/workflows/main.yml # docker/hasher-api/Dockerfile # docker/imaging-api/Dockerfile # docker/orthanc-anon/Dockerfile # docker/orthanc/Dockerfile # docker/pixl-python/Dockerfile # pixl_imaging/tests/docker-compose.yml # test/conftest.py --- test/conftest.py | 47 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index fdcf148ac..b44275518 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -16,11 +16,13 @@ # ruff: noqa: C408 dict() makes test data easier to read and write import os from collections.abc import Generator +from functools import partial, update_wrapper from pathlib import Path from typing import Any -from core.db.models import Base -from sqlalchemy import URL, create_engine +from core.db.models import Base, Image +from core.db.queries import engine +from sqlalchemy import URL, cast, create_engine, not_ from sqlalchemy.orm import sessionmaker # Setting env variables before loading modules @@ -34,9 +36,8 @@ import requests from pytest_pixl.dicom import generate_dicom_dataset from pytest_pixl.ftpserver import PixlFTPServer -from pytest_pixl.helpers import run_subprocess +from pytest_pixl.helpers import run_subprocess, wait_for_condition from pytest_pixl.plugin import FtpHostAddress -from utils import wait_for_images_to_be_exported pytest_plugins = "pytest_pixl" @@ -140,6 +141,44 @@ def _upload_dicom_instance(dicom_dir: Path, **kwargs: Any) -> None: _upload_to_vna(test_dcm_file) +def wait_for_images_to_be_exported( + seconds_max: int, + seconds_interval: int, + seconds_condition_stays_true_for: int, + min_studies: int = 2, +) -> None: + """ + Query pixl DB to ensure that images have been processed and exported. + If they haven't within the time limit, raise a TimeoutError + """ + studies: list[Image] = [] + + def at_least_n_studies_exported(n_studies: int) -> bool: + nonlocal studies + + PixlSession = sessionmaker(engine) + with PixlSession() as session: + studies = cast( + list[Image], + session.query(Image).filter(not_(Image.exported_at.is_(None))).all(), + ) + return len(studies) >= n_studies + + condition = partial(at_least_n_studies_exported, min_studies) + update_wrapper(condition, at_least_n_studies_exported) + + def list_studies() -> str: + return f"Expecting at least {min_studies} studies.\nexported studies: {studies}" + + wait_for_condition( + condition, + seconds_max=seconds_max, + seconds_interval=seconds_interval, + progress_string_fn=list_studies, + seconds_condition_stays_true_for=seconds_condition_stays_true_for, + ) + + @pytest.fixture(scope="session") def _setup_pixl_cli(ftps_server: PixlFTPServer, _populate_vna: None) -> Generator: """Run pixl populate/start. Cleanup intermediate export dir on exit."""