diff --git a/.github/workflows/test_helm_chart.yml b/.github/workflows/test_helm_chart.yml index 541b9c65e..3284added 100644 --- a/.github/workflows/test_helm_chart.yml +++ b/.github/workflows/test_helm_chart.yml @@ -106,6 +106,115 @@ jobs: --set omopDb.external.host=test.example.com \ > /dev/null + # The real-PACS path is entirely opt-in: with the defaults the chart deploys the mocked + # Orthanc, keeps the Service ClusterIP and leaves ingress default-denied. None of the + # NodePort or NetworkPolicy-ingress bodies render at all unless configured, so without this + # step a broken .Values path in either would merge green and only fail for the first trust + # that connects a real PACS (FLIP#993). + - name: Render template (real PACS) + run: | + helm template trust-release deploy/providers/kubernetes/ \ + --set xnat.web.service.type=NodePort \ + --set xnat.web.dicomNodePort=8104 \ + --set xnat.web.dicomAet=FLIPXNAT \ + --set pacs.host=10.0.0.10 \ + --set pacs.aeTitle=SECTRA_QR \ + --set pacs.qrPort=8059 \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].port=8104' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].port=8059' > /tmp/real-pacs.yaml + for want in "nodePort: 8104" "allow-pacs-ingress" "cidr: \"10.0.0.10/32\"" \ + "value: \"SECTRA_QR\"" "value: \"10.0.0.10\"" "value: \"8059\"" \ + "value: \"FLIPXNAT\""; do + if ! grep -q "$want" /tmp/real-pacs.yaml; then + echo "::error::real-PACS render is missing: $want" + exit 1 + fi + done + # imaging-api builds the C-MOVE destination from these, and DQR matches it against a + # registered receiver by exact AE title and port. Grepping the whole document is not + # enough — the init job's own copy satisfies that while imaging-api silently falls back + # to its code default, which is how this shipped broken once (FLIP#993). + python3 - <<'PY' + import re, sys, pathlib + doc = pathlib.Path("/tmp/real-pacs.yaml").read_text() + cm = next((d for d in doc.split("---") + if "kind: ConfigMap" in d and "component: imaging-api" in d), None) + if cm is None: + sys.exit("::error::imaging-api ConfigMap not found in the real-PACS render") + for key, val in (("XNAT_AETITLE", "FLIPXNAT"), ("XNAT_PORT", "8104")): + if not re.search(rf'^\s*{key}:\s*"{val}"\s*$', cm, re.M): + sys.exit(f'::error::imaging-api ConfigMap missing {key}: "{val}" ' + f'— the C-MOVE destination will not match the registered receiver') + PY + + # A real PACS with no egress rule cannot be queried at all — C-FIND never leaves the cluster. + # The chart refuses to render that, so assert the refusal: this configuration was previously + # rendered and asserted as correct (FLIP#993). + - name: Render template (real PACS without egress is refused) + run: | + if helm template trust-release deploy/providers/kubernetes/ \ + --set pacs.host=10.0.0.10 --set pacs.qrPort=8059 > /dev/null 2>&1; then + echo "::error::a PACS with no egress rule rendered successfully" + exit 1 + fi + + # The return leg. Egress alone gets C-FIND and C-MOVE out; the studies come back on a *new* + # association the PACS opens to XNAT, which the default-deny drops unless the ingress + # allowance names the receiver's port. That configuration used to render clean and fail only + # at retrieval time — queries succeed, retrievals silently time out (FLIP#993). The second + # case is the near miss the guard exists for: an operator who lists the NodePort instead of + # the pod's containerPort. Both must be refused, or the render guard is decorative. + - name: Render template (real PACS without matching ingress is refused) + run: | + for port_args in "" "--set networkPolicies.allowedIngressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32 --set networkPolicies.allowedIngressCIDRsWithPorts[0].port=31104"; do + # shellcheck disable=SC2086 # deliberate word splitting: $port_args carries several flags + if helm template trust-release deploy/providers/kubernetes/ \ + --set pacs.host=10.0.0.10 --set pacs.qrPort=8059 \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].port=8059' \ + $port_args > /dev/null 2>&1; then + echo "::error::a PACS with no ingress rule on the receiver's port rendered successfully (${port_args:-no ingress entry})" + exit 1 + fi + done + + # One layer up from the NetworkPolicy: with every egress/ingress rule in place but the + # Service left at its ClusterIP default, xnat-web.yaml's NodePort blocks silently no-op and + # the receiver is unreachable from outside the cluster — a render that satisfies both guards + # above yet gives the PACS's C-STORE leg no path to take. The guard refuses ClusterIP + # specifically (not "anything but NodePort") so a LoadBalancer receiver stays valid. + - name: Render template (real PACS on a ClusterIP service is refused) + run: | + if helm template trust-release deploy/providers/kubernetes/ \ + --set pacs.host=10.0.0.10 --set pacs.qrPort=8059 \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].port=8104' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].port=8059' > /dev/null 2>&1; then + echo "::error::a real PACS with the Service left ClusterIP rendered successfully — the receiver is unreachable" + exit 1 + fi + # And the LoadBalancer allowance must keep rendering, or on-prem MetalLB trusts break. + helm template trust-release deploy/providers/kubernetes/ \ + --set xnat.web.service.type=LoadBalancer \ + --set pacs.host=10.0.0.10 --set pacs.qrPort=8059 \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedIngressCIDRsWithPorts[0].port=8104' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].cidrs[0]=10.0.0.10/32' \ + --set 'networkPolicies.allowedEgressCIDRsWithPorts[0].port=8059' > /dev/null + + # Default-deny must survive: the ingress allowance is the one inbound path into a trust, so a + # chart that opened it without being asked would silently widen every existing deployment. + - name: Render template (default keeps ingress denied) + run: | + helm template trust-release deploy/providers/kubernetes/ > /tmp/default.yaml + if grep -q "allow-pacs-ingress" /tmp/default.yaml; then + echo "::error::the PACS ingress NetworkPolicy rendered without being configured" + exit 1 + fi + # omopDb.vocabLoad.s3Bucket defaults to "" (the licensed bundle has no public # mirror — FLIP#842/843), so every other render in this job skips the # vocab-load Job. Without this step its ~110-line body is never rendered in diff --git a/.github/workflows/test_trust_xnat.yml b/.github/workflows/test_trust_xnat.yml index e0ddd1d0d..7c8ffaf52 100644 --- a/.github/workflows/test_trust_xnat.yml +++ b/.github/workflows/test_trust_xnat.yml @@ -12,10 +12,11 @@ name: Trust - XNAT CI -# The suite covers the anonymization script and the weak-password guards. The -# guard parity tests read their inputs from outside trust/xnat/tests/, so every -# file they assert on has to trigger this workflow — otherwise a change to a -# guard (or to the credential minter) lands with the test that pins it unrun. +# The suite covers the anonymization script, the weak-password guards, and the deployment wiring +# that carries configuration into configure-xnat.sh. Those tests read their inputs from outside +# trust/xnat/tests/, so every file they assert on has to trigger this workflow — otherwise a change +# to a guard, to the credential minter, or to either deployment path's environment block lands with +# the test that pins it unrun. # The dcm2niix-pin-sync job below likewise reads four files scattered across the # repo, so each of those triggers the workflow too. on: @@ -28,6 +29,8 @@ on: - "trust/xnat/scripts/ensure_plugins.sh" - "trust/xnat/tests/**" - "trust/xnat/Makefile" + - "trust/xnat/docker-compose-stack.yml" + - "deploy/providers/kubernetes/templates/xnat-init-job.yaml" - "trust/Makefile" - "Makefile" - "flip-api/Makefile" @@ -50,6 +53,8 @@ on: - "trust/xnat/scripts/ensure_plugins.sh" - "trust/xnat/tests/**" - "trust/xnat/Makefile" + - "trust/xnat/docker-compose-stack.yml" + - "deploy/providers/kubernetes/templates/xnat-init-job.yaml" - "trust/Makefile" - "Makefile" - "flip-api/Makefile" diff --git a/AGENTS.md b/AGENTS.md index 642db8539..12759feb1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -411,6 +411,29 @@ After changes, evaluate if docs need updating: - `FL_PROVISIONED_DIR` — path to the in-tree provisioned FL artifacts, derived per-backend by `deploy/fl_backend.mk` from `FL_BACKEND`: `fl-services/nvflare/provision/workspace-dev` (nvflare startup kits) or `fl-services/flower/provision/creds` (flower per-net TLS certs + SuperNode keys). Both gitignored. Read only by the dev compose overlays for the cert/workspace volume mounts; override at the CLI for a one-off (`make up FL_PROVISIONED_DIR=...`). FL Makefiles are **per-backend** — each `fl-services//Makefile` owns that backend's `build`/`provision`/`up`/`down`/`submit` (flower also `up-secure`); the root Makefile forwards only `build-fl` by `FL_BACKEND`. Each backend's `fl-services//Makefile` also owns its network provisioning (NVFLARE adds `provision`/`provision-2-nets`/`provision-stag`/`provision-prod`/`upload-kits-to-s3`; the project YAMLs, `scripts/`, and gitignored `workspace-{dev,stag,prod}/` output live under `provision/`). Provision with `make -C fl-services/nvflare provision-2-nets` (nvflare) or `make -C fl-services/flower provision NET_NUMBER=` (flower). To run a backend standalone + submit without the full stack: `make -C fl-services/ up` (or `up-secure`) then `make -C fl-services/ submit APP=`. - `FL_APP_BASE_DIR` — Local directory holding the base FL application templates (the repo's `fl-apps/` tree), baked into the flip-api image and bind-mounted in dev. flip-api walks `///` to bundle an application (uploading those files into `FL_APP_DESTINATION_BUCKET/`) and reads each backend's manifest from `//required_files.json`. Default `/app/fl-apps`; override to mount operator-provided templates. Replaces the removed `FL_APP_BASE_BUCKET` S3 dependency (FLIP#724): base templates are no longer published to S3 (the `fl-apps-push-s3-*` sync workflows are gone), so a template hotfix now ships by rebuilding + redeploying the flip-api image rather than syncing S3. `fl-apps/` is baked into the image via a BuildKit named build context (`fl_apps=../fl-apps`) since it sits outside flip-api's build context. For the Flower backend, the template pyprojects also steer Flower's **per-run dependency install** (`uv sync` on every app launch; SuperNodes opt in via `--allow-runtime-dependency-installation` in the composes): `[tool.uv.sources]` pins `flip-utils` to the source kept at `/opt/flip-utils` inside the FL images (never PyPI — FLIP#767; a flip-utils change ships by rebuilding the FL images, `make build-fl FL_BACKEND=flower`) and torch/torchvision to the cu128 index (PyPI's default cu130 wheels need driver >=580). - `FL_KIT_SLOT_NAMES` — JSON list (e.g. `["Trust_1", "Trust_2"]`) of FL kit-slot names for the hub's `fl_kit_slot` pool that `register_trust` claims from; each name must match a provisioned participant kit (in-tree workspace for dev, `s3:///fl-flare-participant-kits//net-/services//` for stag/prod; slot names are global across nets — every net carries a kit per name). The pool is seeded at flip-api boot and **reconciled on demand** when a registration finds it exhausted (`resolve_fl_kit_slot_names`, additive — never deletes or re-assigns rows); only then does `NoFreeKitSlotError` surface. Single source per env: dev = this env var (a `DevSettings`-only field; restart to change, settings load once); stag/prod = the `/flip/fl_kit_slot_names` SSM parameter (Terraform-rendered from this var — the list is plain config, not a secret; deliberately **no env fallback**, so a broken/missing parameter means the pool can't grow, loudly, never masked by stale task-def env). Growing the pool is an env-file edit + `make -C deploy/providers/AWS apply-fl-kit-slots` (targeted plan/apply of just the parameter, plain-text diff) — **no restart, no task-definition change**. One-command workflow: `make -C deploy/providers/AWS add-fl-kits N= PROD=stag|true` (N = "ensure N more live slots": activate spares toward N first, mint only the shortfall on every net → additive S3 upload → env edit → parameter apply); full runbook in `fl-services/nvflare/README.md` ("Onboarding a new client onto an existing network"). NVFLARE-only dynamics — Flower's SuperNode key labelling reads the list at net startup. +- `XNAT_PORT` / `XNAT_WEB_PORT` / `XNAT_AETITLE` — XNAT's DICOM SCP receiver port, its host-published + web-UI port, and its AE title. `XNAT_PORT` was historically one variable doing both jobs, which is + why host 8104 served Tomcat while the DICOM receiver's 8104 was an unpublished container port + (FLIP#993). `XNAT_AETITLE` is applied to the SCP receiver, `dqrCallingAe`, and the C-MOVE + destination in `ImportStudyRequest` — DQR matches that destination against a registered receiver by + exact `AE:port`, so all three must agree and no translation is possible on that leg. Both ports are + host-published — the receiver so a real PACS can complete the C-STORE return leg of a retrieval, + and dev keeps the same wiring — so they must differ; the Makefile refuses to deploy if they + collide. Dev allocation: 8104/8105 (GSTT), 8106/8107 (KCH). +- `PACS_HOST` / `PACS_AETITLE` / `PACS_QR_PORT` / `PACS_LABEL` — the upstream PACS, defaulting to the + mocked Orthanc (`orthanc` / `ORTHANC` / `4242`). `PACS_QR_PORT` must be reachable *from the XNAT + container*, not a host-published port — conflating the two is what the retired `PACS_DICOM_PORT` + did (FLIP#822/#862). `configure-xnat.sh` updates an existing registration in place when the host or + port drift, and imaging-api reads the PACS id from XNAT at runtime rather than assuming 1 — + `configure-xnat.sh` keeps exactly one registration, so it is the sole one XNAT reports. +- `PACS_SUPPORTS_EXTENDED_NEGOTIATIONS` — whether the PACS supports relational queries / extended + negotiation (default `true`). A capability of the PACS rather than a preference: one that does not + support it rejects the association outright. Validated as literally `true` or `false` before it + reaches jq, so a `yes` or a bare `1` fails naming the variable instead of registering the number 1. +- `PACS_AVAILABILITY_DAYS` / `_START` / `_END` / `PACS_THREADS` / `PACS_UTILIZATION_PERCENT` / + `DQR_MAX_PACS_REQUEST_ATTEMPTS` / `DQR_RETRY_WAIT_SECONDS` — the retrieval throttle. A production + PACS may refuse further associations after a certain volume, so the window and thread count are + agreed with the trust's PACS manager. Defaults are all week, all day, one thread. - `PROD` — `true` (production), `stag` (staging), unset (development) - `FLIP_INSTANCE` — names a **second dev hub** so two stacks can run on one host (FLIP#957). Unset (the norm) every derived name is exactly what it was before the knob existed. When set it prefixes four things, all of them names that are global to the docker daemon and so cannot be scoped by `-p`: the hub compose project name (`COMPOSE_PROJECT`), all six hub-side Docker networks, each trust's compose project (`TRUST_PROJECT`, `-trust`) and each trust's XNAT swarm stack (`XNAT_STACK`, `-xnat`). The last two carry it for the same reason the trust overlays do — FL kit slots are handed out per hub, so a second hub restarts its numbering at 1 and its first trust would otherwise adopt or deploy over the default stack's `trust1`/`xnat1`. Those six follow **one** name rule — `${FLIP_INSTANCE:+$FLIP_INSTANCE-}deploy_`, i.e. `deploy_central-hub-network`, `deploy_central-hub-trust-apis-network`, `deploy_fl-net-{1,2}` **and** `deploy_trust-network-{1,2}` — where `deploy_` names the hub compose project that owns the network, which is also exactly what compose would generate (`_`) if it still created them. It stopped generating anything once they became `external: true` ("the `name` field is used as is and is not scoped with the project name"), which is why the prefix is written out by hand in the composes, both Makefiles and `scripts/check_local_status.py`. FLIP#957 renamed **four** of them onto that rule: `central-hub-network` and `central-hub-trust-apis-network` had been left bare, and `shared-net-{1,2}` became `fl-net-{1,2}` — "shared" described who happened to be attached rather than what the network is for, and stopped being true the moment flip-api came off it. The two numbering axes are **not** parallel, which is the trap the new name defuses: `fl-net-` is numbered by **FL net** (the `fl_nets` table, `NET_NUMBER`, `fl-server-net-`) while `trust-network-` is numbered by **trust slot**, so one trust routinely sits on `trust-network-2` and `fl-net-1` at once. `fl-net-` is the FL **data plane**, and its membership is exactly two services — the hub's `fl-server-net-` and every trust's `fl-client-net-`, making it the FL twin of `central-hub-trust-apis-network` (flip-api ↔ trust-api). Everything else hub-side stays on `default` and reaches the FL server there over its control ports: **`flip-api` is deliberately not on it**, and neither are `fl-api-net-` or flower's `register-supernode-keys-net-`: it fronts the database, nothing on the FL data plane calls it (an fl-client carries no hub URL and no hub credential, only `TRUST_INTERNAL_SERVICE_KEY`), and the one callback that exists — fl-server's `FLIP_API_INTERNAL_URL` — goes over `default`, the hub-internal network, which both fl-servers now join. Because these networks are external and pre-created, all four renames are **not** transparent — an existing dev host must re-run `make create-networks`, recreate whatever was attached to the old ones, and `docker network rm` the four leftovers. The project name matters because compose derives it from the directory of the first `-f` file, always `deploy/`, so without it both stacks land in project `deploy` and `up` on one tears down the other. The networks are prefixed *separately* rather than left to `-p` because they are a cross-project contract: the hub project creates them and each trust — its own compose project — joins them `external: true` by literal name, which `-p` cannot scope. The trust overlays carry the prefix like everything else because a trust number alone does not isolate them: slots are handed out per-hub, so a second hub restarts its numbering at 1 and its first trust would otherwise land on the default stack's `deploy_trust-network-1`. Since every network belongs to exactly one instance, `make remove-networks FLIP_INSTANCE=` is instance-scoped and removes that instance's networks *including* its trust overlays. Both values are derived once in [`deploy/instance.mk`](deploy/instance.mk) (`INSTANCE_PREFIX` and `COMPOSE_PROJECT`), included by the root, trust, `trust/xnat`, flip-api and flip-ui Makefiles, which also exports `FLIP_INSTANCE` so the compose files see it; the compose files interpolate `${FLIP_INSTANCE}` rather than the make variable because they are also invoked directly, without make. **Container names are not prefixed and are not set at all** — no hub service declares `container_name`, in the development composes or the production ones, so compose names them from the project. On the default stack that is `deploy-flip-api-1`, `deploy-flip-db-1`, `deploy-flip-ui-1`, `deploy-pgadmin-1`, `deploy-fl-api-net-1-1`, `deploy-fl-server-net-1-1`; on a second stack `-deploy-…`. Since FLIP normally runs a single hub those default-stack names are deterministic, so **docs spell them out literally** — a concrete name is easier to read, copy and grep than a `docker compose -p deploy exec ` form — and only a doc explicitly about a second stack needs the `-deploy-…` variant. **Tooling must not**: a Makefile or script has to work on either instance, so it addresses the service through compose (`$(DOCKER_COMMAND) logs flip-api`) or resolves the container from the `com.docker.compose.project` / `.service` labels, as [`scripts/check_local_status.py`](scripts/check_local_status.py) does. (None of this reaches AWS: the production composes are a local prod-image harness, never a deployment target — on ECS the container names come from the task definitions in `deploy/providers/AWS/ecs_tasks.tf` and discovery from Cloud Map.) The compose **service keys** are the stable identity instead, and are load-bearing: docker registers each as a network alias on every network the container joins, which is how the FL kits keep resolving `fl-server-net-1` (`fed_client.json`'s `target`, and the SuperLink certificate's SAN) and how `NET_ENDPOINTS` reaches `fl-api-net-1`. Renaming a service key breaks TLS and the provisioned kits; renaming a container name breaks nothing. A second stack also needs its own value for every host port (`UI_PORT`, `API_PORT`, `DB_PORT`, `PGADMIN_PORT`, `FL_API_PORT`, `API_DEBUG_PORT`, `FL_API_DEBUG_PORT`, and on flower `FLOWER_SUPERLINK_NET_{1,2}_PORT`), its own `CENTRAL_HUB_API_URL` (it embeds `API_PORT`, so a copied env file silently points the second stack's UI and trust-api at the *first* stack's API — the one misconfiguration here that still starts cleanly and looks right), its own `XNAT_PORT` per trust kit, and its own trust numbers — all host-global and not covered by the prefix. Two traps when running one: `NET_ENDPOINTS` (which the hub seeds into `fl_nets` at boot) must carry the service name `fl-api-net-1`, never the pre-FLIP#957 `flip-fl-api-net-1`, which is now a DNS name in no stack at all; and `FL_PROVISIONED_DIR` may be given as either a relative or an absolute path (relative resolves against the repo root). - `MAIN_ENV_FILE` — which repo-root env file the Makefiles load, as a bare **filename** (never a path). Defaults by `PROD` to `.env.production` / `.env.stag` / `.env.development`; override it to run a second stack from the *same checkout* — `make up MAIN_ENV_FILE=.env.b.development FLIP_INSTANCE=b` — instead of needing a second clone. The root Makefile `export`s it, and the flip-api, flip-ui and trust Makefiles each `include ../$(MAIN_ENV_FILE)`, which is why the value must stay a bare filename: each one prepends its own `../`. Every include is wildcard-guarded, so a value that resolves to nothing is **skipped silently** and the service builds with no environment at all rather than failing — check the `Using MAIN_ENV_FILE:` line each Makefile prints if a second stack comes up with empty config. `scripts/check_local_status.py` honours it too (via the environment), so a status check reports on the stack you are actually running. diff --git a/CLAUDE.md b/CLAUDE.md index 4cbd34632..2385180c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -411,6 +411,29 @@ After changes, evaluate if docs need updating: - `FL_PROVISIONED_DIR` — path to the in-tree provisioned FL artifacts, derived per-backend by `deploy/fl_backend.mk` from `FL_BACKEND`: `fl-services/nvflare/provision/workspace-dev` (nvflare startup kits) or `fl-services/flower/provision/creds` (flower per-net TLS certs + SuperNode keys). Both gitignored. Read only by the dev compose overlays for the cert/workspace volume mounts; override at the CLI for a one-off (`make up FL_PROVISIONED_DIR=...`). FL Makefiles are **per-backend** — each `fl-services//Makefile` owns that backend's `build`/`provision`/`up`/`down`/`submit` (flower also `up-secure`); the root Makefile forwards only `build-fl` by `FL_BACKEND`. Each backend's `fl-services//Makefile` also owns its network provisioning (NVFLARE adds `provision`/`provision-2-nets`/`provision-stag`/`provision-prod`/`upload-kits-to-s3`; the project YAMLs, `scripts/`, and gitignored `workspace-{dev,stag,prod}/` output live under `provision/`). Provision with `make -C fl-services/nvflare provision-2-nets` (nvflare) or `make -C fl-services/flower provision NET_NUMBER=` (flower). To run a backend standalone + submit without the full stack: `make -C fl-services/ up` (or `up-secure`) then `make -C fl-services/ submit APP=`. - `FL_APP_BASE_DIR` — Local directory holding the base FL application templates (the repo's `fl-apps/` tree), baked into the flip-api image and bind-mounted in dev. flip-api walks `///` to bundle an application (uploading those files into `FL_APP_DESTINATION_BUCKET/`) and reads each backend's manifest from `//required_files.json`. Default `/app/fl-apps`; override to mount operator-provided templates. Replaces the removed `FL_APP_BASE_BUCKET` S3 dependency (FLIP#724): base templates are no longer published to S3 (the `fl-apps-push-s3-*` sync workflows are gone), so a template hotfix now ships by rebuilding + redeploying the flip-api image rather than syncing S3. `fl-apps/` is baked into the image via a BuildKit named build context (`fl_apps=../fl-apps`) since it sits outside flip-api's build context. For the Flower backend, the template pyprojects also steer Flower's **per-run dependency install** (`uv sync` on every app launch; SuperNodes opt in via `--allow-runtime-dependency-installation` in the composes): `[tool.uv.sources]` pins `flip-utils` to the source kept at `/opt/flip-utils` inside the FL images (never PyPI — FLIP#767; a flip-utils change ships by rebuilding the FL images, `make build-fl FL_BACKEND=flower`) and torch/torchvision to the cu128 index (PyPI's default cu130 wheels need driver >=580). - `FL_KIT_SLOT_NAMES` — JSON list (e.g. `["Trust_1", "Trust_2"]`) of FL kit-slot names for the hub's `fl_kit_slot` pool that `register_trust` claims from; each name must match a provisioned participant kit (in-tree workspace for dev, `s3:///fl-flare-participant-kits//net-/services//` for stag/prod; slot names are global across nets — every net carries a kit per name). The pool is seeded at flip-api boot and **reconciled on demand** when a registration finds it exhausted (`resolve_fl_kit_slot_names`, additive — never deletes or re-assigns rows); only then does `NoFreeKitSlotError` surface. Single source per env: dev = this env var (a `DevSettings`-only field; restart to change, settings load once); stag/prod = the `/flip/fl_kit_slot_names` SSM parameter (Terraform-rendered from this var — the list is plain config, not a secret; deliberately **no env fallback**, so a broken/missing parameter means the pool can't grow, loudly, never masked by stale task-def env). Growing the pool is an env-file edit + `make -C deploy/providers/AWS apply-fl-kit-slots` (targeted plan/apply of just the parameter, plain-text diff) — **no restart, no task-definition change**. One-command workflow: `make -C deploy/providers/AWS add-fl-kits N= PROD=stag|true` (N = "ensure N more live slots": activate spares toward N first, mint only the shortfall on every net → additive S3 upload → env edit → parameter apply); full runbook in `fl-services/nvflare/README.md` ("Onboarding a new client onto an existing network"). NVFLARE-only dynamics — Flower's SuperNode key labelling reads the list at net startup. +- `XNAT_PORT` / `XNAT_WEB_PORT` / `XNAT_AETITLE` — XNAT's DICOM SCP receiver port, its host-published + web-UI port, and its AE title. `XNAT_PORT` was historically one variable doing both jobs, which is + why host 8104 served Tomcat while the DICOM receiver's 8104 was an unpublished container port + (FLIP#993). `XNAT_AETITLE` is applied to the SCP receiver, `dqrCallingAe`, and the C-MOVE + destination in `ImportStudyRequest` — DQR matches that destination against a registered receiver by + exact `AE:port`, so all three must agree and no translation is possible on that leg. Both ports are + host-published — the receiver so a real PACS can complete the C-STORE return leg of a retrieval, + and dev keeps the same wiring — so they must differ; the Makefile refuses to deploy if they + collide. Dev allocation: 8104/8105 (GSTT), 8106/8107 (KCH). +- `PACS_HOST` / `PACS_AETITLE` / `PACS_QR_PORT` / `PACS_LABEL` — the upstream PACS, defaulting to the + mocked Orthanc (`orthanc` / `ORTHANC` / `4242`). `PACS_QR_PORT` must be reachable *from the XNAT + container*, not a host-published port — conflating the two is what the retired `PACS_DICOM_PORT` + did (FLIP#822/#862). `configure-xnat.sh` updates an existing registration in place when the host or + port drift, and imaging-api reads the PACS id from XNAT at runtime rather than assuming 1 — + `configure-xnat.sh` keeps exactly one registration, so it is the sole one XNAT reports. +- `PACS_SUPPORTS_EXTENDED_NEGOTIATIONS` — whether the PACS supports relational queries / extended + negotiation (default `true`). A capability of the PACS rather than a preference: one that does not + support it rejects the association outright. Validated as literally `true` or `false` before it + reaches jq, so a `yes` or a bare `1` fails naming the variable instead of registering the number 1. +- `PACS_AVAILABILITY_DAYS` / `_START` / `_END` / `PACS_THREADS` / `PACS_UTILIZATION_PERCENT` / + `DQR_MAX_PACS_REQUEST_ATTEMPTS` / `DQR_RETRY_WAIT_SECONDS` — the retrieval throttle. A production + PACS may refuse further associations after a certain volume, so the window and thread count are + agreed with the trust's PACS manager. Defaults are all week, all day, one thread. - `PROD` — `true` (production), `stag` (staging), unset (development) - `FLIP_INSTANCE` — names a **second dev hub** so two stacks can run on one host (FLIP#957). Unset (the norm) every derived name is exactly what it was before the knob existed. When set it prefixes four things, all of them names that are global to the docker daemon and so cannot be scoped by `-p`: the hub compose project name (`COMPOSE_PROJECT`), all six hub-side Docker networks, each trust's compose project (`TRUST_PROJECT`, `-trust`) and each trust's XNAT swarm stack (`XNAT_STACK`, `-xnat`). The last two carry it for the same reason the trust overlays do — FL kit slots are handed out per hub, so a second hub restarts its numbering at 1 and its first trust would otherwise adopt or deploy over the default stack's `trust1`/`xnat1`. Those six follow **one** name rule — `${FLIP_INSTANCE:+$FLIP_INSTANCE-}deploy_`, i.e. `deploy_central-hub-network`, `deploy_central-hub-trust-apis-network`, `deploy_fl-net-{1,2}` **and** `deploy_trust-network-{1,2}` — where `deploy_` names the hub compose project that owns the network, which is also exactly what compose would generate (`_`) if it still created them. It stopped generating anything once they became `external: true` ("the `name` field is used as is and is not scoped with the project name"), which is why the prefix is written out by hand in the composes, both Makefiles and `scripts/check_local_status.py`. FLIP#957 renamed **four** of them onto that rule: `central-hub-network` and `central-hub-trust-apis-network` had been left bare, and `shared-net-{1,2}` became `fl-net-{1,2}` — "shared" described who happened to be attached rather than what the network is for, and stopped being true the moment flip-api came off it. The two numbering axes are **not** parallel, which is the trap the new name defuses: `fl-net-` is numbered by **FL net** (the `fl_nets` table, `NET_NUMBER`, `fl-server-net-`) while `trust-network-` is numbered by **trust slot**, so one trust routinely sits on `trust-network-2` and `fl-net-1` at once. `fl-net-` is the FL **data plane**, and its membership is exactly two services — the hub's `fl-server-net-` and every trust's `fl-client-net-`, making it the FL twin of `central-hub-trust-apis-network` (flip-api ↔ trust-api). Everything else hub-side stays on `default` and reaches the FL server there over its control ports: **`flip-api` is deliberately not on it**, and neither are `fl-api-net-` or flower's `register-supernode-keys-net-`: it fronts the database, nothing on the FL data plane calls it (an fl-client carries no hub URL and no hub credential, only `TRUST_INTERNAL_SERVICE_KEY`), and the one callback that exists — fl-server's `FLIP_API_INTERNAL_URL` — goes over `default`, the hub-internal network, which both fl-servers now join. Because these networks are external and pre-created, all four renames are **not** transparent — an existing dev host must re-run `make create-networks`, recreate whatever was attached to the old ones, and `docker network rm` the four leftovers. The project name matters because compose derives it from the directory of the first `-f` file, always `deploy/`, so without it both stacks land in project `deploy` and `up` on one tears down the other. The networks are prefixed *separately* rather than left to `-p` because they are a cross-project contract: the hub project creates them and each trust — its own compose project — joins them `external: true` by literal name, which `-p` cannot scope. The trust overlays carry the prefix like everything else because a trust number alone does not isolate them: slots are handed out per-hub, so a second hub restarts its numbering at 1 and its first trust would otherwise land on the default stack's `deploy_trust-network-1`. Since every network belongs to exactly one instance, `make remove-networks FLIP_INSTANCE=` is instance-scoped and removes that instance's networks *including* its trust overlays. Both values are derived once in [`deploy/instance.mk`](deploy/instance.mk) (`INSTANCE_PREFIX` and `COMPOSE_PROJECT`), included by the root, trust, `trust/xnat`, flip-api and flip-ui Makefiles, which also exports `FLIP_INSTANCE` so the compose files see it; the compose files interpolate `${FLIP_INSTANCE}` rather than the make variable because they are also invoked directly, without make. **Container names are not prefixed and are not set at all** — no hub service declares `container_name`, in the development composes or the production ones, so compose names them from the project. On the default stack that is `deploy-flip-api-1`, `deploy-flip-db-1`, `deploy-flip-ui-1`, `deploy-pgadmin-1`, `deploy-fl-api-net-1-1`, `deploy-fl-server-net-1-1`; on a second stack `-deploy-…`. Since FLIP normally runs a single hub those default-stack names are deterministic, so **docs spell them out literally** — a concrete name is easier to read, copy and grep than a `docker compose -p deploy exec ` form — and only a doc explicitly about a second stack needs the `-deploy-…` variant. **Tooling must not**: a Makefile or script has to work on either instance, so it addresses the service through compose (`$(DOCKER_COMMAND) logs flip-api`) or resolves the container from the `com.docker.compose.project` / `.service` labels, as [`scripts/check_local_status.py`](scripts/check_local_status.py) does. (None of this reaches AWS: the production composes are a local prod-image harness, never a deployment target — on ECS the container names come from the task definitions in `deploy/providers/AWS/ecs_tasks.tf` and discovery from Cloud Map.) The compose **service keys** are the stable identity instead, and are load-bearing: docker registers each as a network alias on every network the container joins, which is how the FL kits keep resolving `fl-server-net-1` (`fed_client.json`'s `target`, and the SuperLink certificate's SAN) and how `NET_ENDPOINTS` reaches `fl-api-net-1`. Renaming a service key breaks TLS and the provisioned kits; renaming a container name breaks nothing. A second stack also needs its own value for every host port (`UI_PORT`, `API_PORT`, `DB_PORT`, `PGADMIN_PORT`, `FL_API_PORT`, `API_DEBUG_PORT`, `FL_API_DEBUG_PORT`, and on flower `FLOWER_SUPERLINK_NET_{1,2}_PORT`), its own `CENTRAL_HUB_API_URL` (it embeds `API_PORT`, so a copied env file silently points the second stack's UI and trust-api at the *first* stack's API — the one misconfiguration here that still starts cleanly and looks right), its own `XNAT_PORT` per trust kit, and its own trust numbers — all host-global and not covered by the prefix. Two traps when running one: `NET_ENDPOINTS` (which the hub seeds into `fl_nets` at boot) must carry the service name `fl-api-net-1`, never the pre-FLIP#957 `flip-fl-api-net-1`, which is now a DNS name in no stack at all; and `FL_PROVISIONED_DIR` may be given as either a relative or an absolute path (relative resolves against the repo root). - `MAIN_ENV_FILE` — which repo-root env file the Makefiles load, as a bare **filename** (never a path). Defaults by `PROD` to `.env.production` / `.env.stag` / `.env.development`; override it to run a second stack from the *same checkout* — `make up MAIN_ENV_FILE=.env.b.development FLIP_INSTANCE=b` — instead of needing a second clone. The root Makefile `export`s it, and the flip-api, flip-ui and trust Makefiles each `include ../$(MAIN_ENV_FILE)`, which is why the value must stay a bare filename: each one prepends its own `../`. Every include is wildcard-guarded, so a value that resolves to nothing is **skipped silently** and the service builds with no environment at all rather than failing — check the `Using MAIN_ENV_FILE:` line each Makefile prints if a second stack comes up with empty config. `scripts/check_local_status.py` honours it too (via the environment), so a status check reports on the stack you are actually running. diff --git a/deploy/providers/AWS/README.md b/deploy/providers/AWS/README.md index a105442a3..8580ce5b1 100644 --- a/deploy/providers/AWS/README.md +++ b/deploy/providers/AWS/README.md @@ -768,7 +768,7 @@ This prints a list of URLs you can paste into your browser: | Service | Local URL | Purpose | | --- | --- | --- | -| XNAT | `http://localhost:8104` | Neuroimaging platform UI | +| XNAT | `http://localhost:8105` | Neuroimaging platform UI | | Orthanc | `http://localhost:8042` | DICOM server UI (basic auth: the kit file's `ORTHANC_USERNAME`/`ORTHANC_PASSWORD`) | | trust-api swagger | `http://localhost:8020/docs` | Trust API documentation | | imaging-api swagger | `http://localhost:8001/docs` | Imaging API documentation | diff --git a/deploy/providers/AWS/check_status.py b/deploy/providers/AWS/check_status.py index d4e8d831d..77264a306 100755 --- a/deploy/providers/AWS/check_status.py +++ b/deploy/providers/AWS/check_status.py @@ -1363,7 +1363,7 @@ def main( # Grafana is part of the optional observability stack — treat its # absence as WARN, not FAIL. trust_endpoints = [ - ("XNAT", "http://127.0.0.1:8104/", ["200", "302"], "FAIL"), + ("XNAT", "http://127.0.0.1:8105/", ["200", "302"], "FAIL"), # Auth is always enforced (FLIP-PT-091): a 200 without # credentials means an unauthenticated PACS — fail. ("Orthanc", "http://127.0.0.1:8042/", ["401"], "FAIL"), diff --git a/deploy/providers/AWS/scripts/forward-trust-all.sh b/deploy/providers/AWS/scripts/forward-trust-all.sh index 42d8beb97..df5ce8319 100755 --- a/deploy/providers/AWS/scripts/forward-trust-all.sh +++ b/deploy/providers/AWS/scripts/forward-trust-all.sh @@ -56,7 +56,7 @@ trap cleanup EXIT INT TERM echo "🔀 Opening SSM port forwards to Trust EC2 ($INSTANCE_ID)..." echo "" -forward 8104 8104 "XNAT" "http://localhost:8104" +forward 8105 8105 "XNAT" "http://localhost:8105" forward 8042 8042 "Orthanc" "http://localhost:8042" forward 8020 8020 "trust-api" "http://localhost:8020/docs" forward 8001 8001 "imaging-api" "http://localhost:8001/docs" diff --git a/deploy/providers/kubernetes/NETWORK-POLICY.md b/deploy/providers/kubernetes/NETWORK-POLICY.md index fd91c28b6..7f4f3d047 100644 --- a/deploy/providers/kubernetes/NETWORK-POLICY.md +++ b/deploy/providers/kubernetes/NETWORK-POLICY.md @@ -45,6 +45,7 @@ boundary — see [Residual risk](#residual-risk). |---|---|---|---| | `allowedEgressPorts` (default 53/UDP, 53/TCP, 80/TCP, 443/TCP) | **any IP** | DNS resolution; 443 for the hub poll (CloudFront), S3 (kit/results), Cognito, GHCR/ECR image pulls; 80 for redirects/package metadata. `sync-kit` appends `FL_SERVER_PORT` here for the fl-client → fl-server gRPC (#593 pt.3, port-only). | **Primary residual risk: 443/80 to any IP is an exfiltration channel.** A compromised fl-client could POST data anywhere on 443. The added FL-server port widens egress on that one port to any IP — accepted because the FL server is behind an internet-facing NLB with rotating AWS-managed IPs that a `/32` pin cannot track. | | intra-namespace | same namespace | trust-api → imaging/data-access/fl-client, etc. | Low — intra-trust only. | +| `allowedIngressCIDRsWithPorts` (default `[]`) | listed CIDRs, one port, **inbound** to xnat-web | The DICOM C-STORE return leg. FLIP pulls, so after XNAT issues C-MOVE the PACS opens a new association back to XNAT; without this it is dropped and retrievals silently time out (FLIP#993). | Scope to the PACS itself, never the whole trust network. Default-deny is unchanged while the list is empty. | | `allowedEgressCIDRs` (default `[]`) | listed CIDRs, **all ports** | Operator escape hatch to reach an external OMOP/PACS/XNAT on arbitrary ports. | Scoped to listed CIDRs; all-ports is broad — keep the list tight. | | `allowedEgressCIDRsWithPorts` (default `[]`) | listed CIDRs, one port | Operator escape hatch for CIDR+port egress (e.g. an on-prem service on a fixed IP). Not populated by `sync-kit` — the FL-server allowance is port-only (see `allowedEgressPorts`). | Scoped CIDR+port — tightest rule. | | AWS IMDS | `169.254.169.254/32` | EC2 metadata / IAM-role credentials for the fl-client S3 kit sync. | IMDS is a known SSRF/cred-theft target — see hardening note. | diff --git a/deploy/providers/kubernetes/README.md b/deploy/providers/kubernetes/README.md index 67d922c61..85b215e6e 100644 --- a/deploy/providers/kubernetes/README.md +++ b/deploy/providers/kubernetes/README.md @@ -460,7 +460,9 @@ old install on the previous chart version. - **NetworkPolicies**: Default-deny-ingress, allow-intra-namespace, allow-egress to Central Hub and FL server only (audit and threat model: [NETWORK-POLICY.md](NETWORK-POLICY.md)) -- **No LoadBalancer or NodePort** for application services (all ClusterIP) +- **No LoadBalancer or NodePort** for application services (all ClusterIP), with one opt-in + exception: `xnat.web.dicomNodePort` with `service.type: NodePort` exposes XNAT's DICOM SCP + receiver so a trust PACS can complete the C-STORE leg of a retrieval. Off by default. - **Secrets**: Separate from ConfigMaps; recommend External Secrets Operator - **FL clients**: No Central Hub credentials; connect outbound to FL server only - **ServiceAccounts**: each stateless service runs under its own ServiceAccount diff --git a/deploy/providers/kubernetes/TROUBLESHOOTING.md b/deploy/providers/kubernetes/TROUBLESHOOTING.md index 6d7b491f3..b5f24ad0b 100644 --- a/deploy/providers/kubernetes/TROUBLESHOOTING.md +++ b/deploy/providers/kubernetes/TROUBLESHOOTING.md @@ -455,11 +455,20 @@ The study data is sent to XNAT's prearchive. #### DICOM Port Map -| Service | AE Title | Host | Port | Purpose | -|---------|---------|------|------|---------| -| XNAT SCP | `XNAT` | xnat-web | 8104 | Receives C-STORE from PACS | -| Orthanc | `ORTHANC` | orthanc | 4242 | PACS — stores DICOM studies | -| Imaging Worker | `FLIPIMPORT` | (any) | — | C-MOVE source AE | +Values below are the shipped defaults for the mocked Orthanc; a trust PACS overrides them +through the Helm values named in each cell. + +| Service | AE title | Host | Port | Purpose | +|---------|----------|------|------|---------| +| XNAT SCP receiver | `XNAT` (`xnat.web.dicomAet`) | xnat-web (`pacs.host` dials it back) | 8104 (`xnat.web.dicomPort`) | Receives the C-STORE the PACS opens after a C-MOVE | +| PACS | `ORTHANC` (`pacs.aeTitle`) | orthanc (`pacs.host`) | 4242 (`pacs.qrPort`) | Serves C-FIND and C-MOVE | +| DQR calling AE | same as the SCP receiver | — | — | The AE XNAT presents when it queries the PACS | + +There is no separate AE for the imaging worker: imaging-api drives retrieval through XNAT's DQR +REST API, so every association on the wire is between XNAT and the PACS. The SCP receiver's AE +title and the DQR calling AE are both `xnat.web.dicomAet` and must stay equal — DQR matches the +C-MOVE destination against a registered receiver by exact `AE:port`, so a mismatch means the PACS +sends the studies to an address XNAT is not listening on. #### XNAT SCP Receiver Configuration @@ -470,7 +479,8 @@ kubectl exec -n flip-trust trust-release-flip-trust-xnat-db-0 -- psql -U xnat -d "SELECT id, ae_title, port, direct_archive, custom_processing, identifier FROM xhbm_dicomscpinstance;" ``` -Expected output: +Expected output (`ae_title` and `port` follow `xnat.web.dicomAet` / `xnat.web.dicomPort`; the +rest are fixed by `configure-xnat.sh`): `ae_title=XNAT, port=8104, direct_archive=t, custom_processing=t, identifier=dqrObjectIdentifier` If missing or wrong, recreate it via the REST API (see §2.2 — prefer the API diff --git a/deploy/providers/kubernetes/templates/imaging-api.yaml b/deploy/providers/kubernetes/templates/imaging-api.yaml index 460a015c2..b51627abe 100644 --- a/deploy/providers/kubernetes/templates/imaging-api.yaml +++ b/deploy/providers/kubernetes/templates/imaging-api.yaml @@ -30,7 +30,12 @@ metadata: app.kubernetes.io/component: imaging-api data: XNAT_URL: {{ .Values.imagingApi.env.XNAT_URL | quote }} - XNAT_PORT: {{ .Values.imagingApi.env.XNAT_PORT | quote }} + # Both of these describe XNAT's DICOM SCP receiver, and imaging-api uses them to build the C-MOVE + # destination it hands to the PACS. DQR matches that destination against a registered receiver by + # exact AE title and port, so they must equal what xnat-init-job.yaml registered — they are + # sourced from the same xnat.web.* values it uses rather than restated here (FLIP#993). + XNAT_PORT: {{ .Values.xnat.web.dicomPort | quote }} + XNAT_AETITLE: {{ .Values.xnat.web.dicomAet | quote }} PACS_ID: {{ .Values.imagingApi.env.PACS_ID | quote }} XNAT_DATABASE_URL: {{ .Values.imagingApi.env.XNAT_DATABASE_URL | quote }} DATA_ACCESS_API_URL: {{ .Values.imagingApi.env.DATA_ACCESS_API_URL | quote }} diff --git a/deploy/providers/kubernetes/templates/network-policy.yaml b/deploy/providers/kubernetes/templates/network-policy.yaml index 10d5307c8..6fcc7245b 100644 --- a/deploy/providers/kubernetes/templates/network-policy.yaml +++ b/deploy/providers/kubernetes/templates/network-policy.yaml @@ -49,6 +49,87 @@ spec: policyTypes: - Ingress --- +{{- if and .Values.networkPolicies.enabled (ne .Values.pacs.host "orthanc") }} +{{- $pacsPort := .Values.pacs.qrPort | int }} +{{- $egressOk := false }} +{{- range .Values.networkPolicies.allowedEgressCIDRsWithPorts }} + {{- if eq (.port | int) $pacsPort }}{{ $egressOk = true }}{{ end }} +{{- end }} +{{- range .Values.networkPolicies.allowedEgressPorts }} + {{- if eq (.port | int) $pacsPort }}{{ $egressOk = true }}{{ end }} +{{- end }} +{{- if and (not $egressOk) (not .Values.networkPolicies.allowedEgressCIDRs) }} +{{- fail (printf "pacs.host is %s but no egress rule reaches its query/retrieve port %v. XNAT could not issue C-FIND or C-MOVE, so retrieval would fail before it began. Add the PACS to networkPolicies.allowedEgressCIDRsWithPorts." .Values.pacs.host .Values.pacs.qrPort) }} +{{- end }} +{{- $dicomPort := .Values.xnat.web.dicomPort | int }} +{{- $ingressOk := false }} +{{- range .Values.networkPolicies.allowedIngressCIDRsWithPorts }} + {{- if eq (.port | int) $dicomPort }}{{ $ingressOk = true }}{{ end }} +{{- end }} +{{- /* +The return leg, guarded to the same standard as the outbound one. Retrieval is two connections in +opposite directions: XNAT dials the PACS to C-FIND and C-MOVE, then the PACS opens a *new* +association back to XNAT to C-STORE the studies. Omitting the egress rule already fails this render; +omitting the inbound one used to render clean and produce the failure this chart's comments call the +hardest to diagnose — queries succeed, retrievals silently time out with nothing logged on either +side. Both directions now fail equally loudly, at render, naming the values to set. +*/ -}} +{{- if not $ingressOk }} +{{- fail (printf "pacs.host is %s but networkPolicies.allowedIngressCIDRsWithPorts has no entry on port %v. After XNAT issues C-MOVE the PACS opens a new association back to XNAT to C-STORE the studies; without that allowance it is dropped, so queries succeed and retrievals silently time out. Add the PACS CIDRs on port %v, and make the receiver reachable from outside the cluster with xnat.web.service.type: NodePort plus xnat.web.dicomNodePort. Note the port here is the pod's containerPort (xnat.web.dicomPort, default 8104) and not dicomNodePort: a NetworkPolicy matches the port the pod listens on, after the node has undone the NodePort translation." .Values.pacs.host $dicomPort $dicomPort) }} +{{- end }} +{{- /* +The NetworkPolicy admits the C-STORE leg only as far as the pod; the packet still needs a route from +outside the cluster to that pod. templates/xnat-web.yaml publishes the DICOM receiver beyond the +cluster only when service.type is NodePort AND dicomNodePort is set — on ClusterIP both blocks +silently no-op, so a chart could satisfy every guard above and still render with no path a PACS +packet can take: the same queries-succeed/retrievals-time-out failure, one layer up. Refuse +ClusterIP specifically rather than demanding NodePort, because a LoadBalancer service (MetalLB and +friends) is an equally valid way to make the receiver reachable and must not be rejected. +*/ -}} +{{- if eq .Values.xnat.web.service.type "ClusterIP" }} +{{- fail (printf "pacs.host is %s but xnat.web.service.type is ClusterIP, so the DICOM receiver is unreachable from outside the cluster and the C-STORE return leg the PACS opens after C-MOVE can never arrive. Set xnat.web.service.type: NodePort plus xnat.web.dicomNodePort (equal to xnat.web.dicomPort), or expose the receiver through a LoadBalancer service." .Values.pacs.host) }} +{{- end }} +{{- end }} +{{- if .Values.networkPolicies.allowedIngressCIDRsWithPorts }} +# Allow inbound DICOM from the trust PACS. +# +# The default-deny above models FLIP's zero-inbound posture, which holds for the internet and for +# the Central Hub: neither can open a connection to a trust. Retrieval from a trust PACS is the one +# exception, and it is inbound by protocol rather than by choice — FLIP pulls, so after XNAT issues +# C-MOVE the PACS opens a *new* association back to XNAT to C-STORE the studies (FLIP#993). +# +# This stays default-deny until an operator lists the PACS CIDRs, and the allowance is scoped to +# those CIDRs on that port only. It does not open anything to the internet or to the hub. +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: {{ include "flip-trust.fullname" . }}-allow-pacs-ingress + namespace: {{ include "flip-trust.namespace" . }} + labels: + {{- include "flip-trust.labels" . | nindent 4 }} +spec: + podSelector: + matchLabels: + {{- include "flip-trust.selectorLabels" . | nindent 6 }} + app.kubernetes.io/component: xnat-web + ingress: +{{- range .Values.networkPolicies.allowedIngressCIDRsWithPorts }} + - from: + {{- range .cidrs }} + - ipBlock: + cidr: {{ . | quote }} + {{- end }} + ports: + {{- if not .port }} + {{- fail "networkPolicies.allowedIngressCIDRsWithPorts: each entry needs an explicit `port`. Kubernetes reads an absent port as *all* ports, which would widen the one inbound path into the trust from DICOM to everything." }} + {{- end }} + - port: {{ .port }} + protocol: {{ .protocol | default "TCP" }} +{{- end }} + policyTypes: + - Ingress +{{- end }} +--- # Allow egress to specific destinations (Central Hub, FL server, DNS, IMDS) # All other egress is denied apiVersion: networking.k8s.io/v1 diff --git a/deploy/providers/kubernetes/templates/orthanc.yaml b/deploy/providers/kubernetes/templates/orthanc.yaml index 57823ad9c..a31beb8dc 100644 --- a/deploy/providers/kubernetes/templates/orthanc.yaml +++ b/deploy/providers/kubernetes/templates/orthanc.yaml @@ -23,7 +23,7 @@ data: # Orthanc configuration is built from values; users may supply additional # configuration via extra config maps in the future. ORTHANC__DICOM_MODALITIES: | - {"XNAT": {"AET": "XNAT", "Host": "xnat-web", "Port": "8104"}} + {"XNAT": {"AET": {{ .Values.xnat.web.dicomAet | default "XNAT" | quote }}, "Host": "xnat-web", "Port": {{ .Values.xnat.web.dicomPort | default 8104 | quote }}}} --- apiVersion: v1 kind: Service diff --git a/deploy/providers/kubernetes/templates/xnat-init-job.yaml b/deploy/providers/kubernetes/templates/xnat-init-job.yaml index f874d7824..c996e8497 100644 --- a/deploy/providers/kubernetes/templates/xnat-init-job.yaml +++ b/deploy/providers/kubernetes/templates/xnat-init-job.yaml @@ -40,362 +40,49 @@ spec: # POST /xapi/users instead — see the comment at that call. containers: - name: configure-xnat-web - image: alpine:3.20 + # Runs the same configure-xnat.sh the Compose deployment runs, from the same image that + # ships it (trust/xnat/xnat/Dockerfile ADDs config/ to ${XNAT_ROOT}/config). This used to + # be ~380 lines of shell inlined here plus a third copy in a ConfigMap, and the three had + # drifted: the inlined copy hardcoded the mocked Orthanc and ignored the chart's own + # orthanc.dicomHost / dicomPort / dicomAet values, which is why the external-PACS override + # never actually redirected DQR (FLIP#993). One script, one place, covered by + # trust/xnat/tests/. + # + # configure-xnat.sh is self-contained: it waits for XNAT, waits for the DQR plugin routes + # via wait-for-xnat-plugins.sh, then applies site, user, DQR, SCP receiver and PACS + # configuration. It short-circuits when the site is already initialised, so re-running on + # helm upgrade is safe. + # + # The `cd /data/xnat/config` below is a literal on purpose. XNAT_ROOT is a build ARG only — + # the image never exports it (the Dockerfile exports XNAT_HOME, not XNAT_ROOT), so there is + # no image-provided variable to read it back from at runtime. And /data/xnat is pinned + # chart-wide, not just here: xnat-web's six persistent-volume mountPaths, its XNAT_HOME, its + # -Dxnat.home, and the Container Service ConfigMap's "combined-path-translation" below all + # spell it out. An image rebuilt with a different XNAT_ROOT breaks every one of those with + # or without this line, so a value here would advertise a configurability the chart does not + # have. If XNAT_ROOT ever needs to move, it moves in all of those places at once. + image: "{{ .Values.xnat.web.image.repository }}:{{ .Values.xnat.web.image.tag }}" + imagePullPolicy: {{ .Values.xnat.web.image.pullPolicy }} command: - - /bin/sh + - /bin/bash - -c - | set -euo pipefail - apk add --no-cache curl jq >/dev/null 2>&1 - XNAT_URL="http://xnat-web:8080" - ADMIN_USER="{{ .Values.xnat.web.adminUser }}" - ADMIN_PASS="${XNAT_ADMIN_PASS}" - - # Scrub secrets out of anything echoed on failure: the value - # following -u (credentials) or -d/--data-binary (payloads carry - # the admin and service passwords) must never reach the pod log, - # and neither must a live JSESSIONID. - scrub_args() { - local redact_next="" - local arg - local out="" - for arg in "$@"; do - if [ -n "$redact_next" ]; then - out="$out " - redact_next="" - elif [ "$arg" = "-u" ] || [ "$arg" = "-d" ] || [ "$arg" = "--data-binary" ]; then - out="$out $arg" - redact_next=1 - else - out="$out $arg" - fi - done - printf '%s' "${out# }" | sed 's/JSESSIONID=[^ ]*/JSESSIONID=/g' - } - - # Fail-loud wrapper for XNAT's REST API — the Kubernetes - # counterpart of xnat_curl in trust/xnat/xnat/config/configure-xnat.sh - # (FLIP#862). Bare `curl -s` exits 0 on HTTP errors, so every - # configuration call below used to be swallowed by `|| true` and - # this Job reported success on a half-configured XNAT — the same - # silent-failure class as the unregistered-PACS bug (FLIP#822). - # On 2xx, emits the response body on stdout. On anything else — - # or a curl transport failure — reports the (scrubbed) request and - # the response body on stderr and returns non-zero, which `set -e` - # turns into an abort at the call site. - xnat_curl() { - local response - local status - local body - local curl_exit=0 - response=$(curl -sS --connect-timeout 10 --max-time 120 -w '\n%{http_code}' "$@") || curl_exit=$? - if [ "$curl_exit" -ne 0 ]; then - echo "ERROR: curl transport failure (exit $curl_exit)" >&2 - echo " args: $(scrub_args "$@")" >&2 - return 1 - fi - # Strip CRs so a middlebox emitting \r\n line endings cannot - # make the status guard fail on an invisible character. - status=$(printf '%s' "$response" | tail -n1 | tr -d '\r') - body=$(printf '%s' "$response" | sed '$d' | tr -d '\r') - # Fail closed: anything other than a literal 2xx status line - # (including an empty or non-numeric one) is an error. - case "$status" in - 2[0-9][0-9]) ;; - *) - echo "ERROR: XNAT request failed with HTTP $status" >&2 - echo " args: $(scrub_args "$@")" >&2 - echo " body: $body" >&2 - return 1 - ;; - esac - if [ -n "$body" ]; then - printf '%s\n' "$body" - fi - } - - echo "Waiting for XNAT to be ready..." - XNAT_READY=false - HTTP_CODE=000 - for i in $(seq 1 60); do - # `|| HTTP_CODE=000` is load-bearing: `set -e` aborts on a - # failing command substitution, so without it the first probe - # against a still-booting Tomcat (curl exit 7) kills this - # container and the loop can never wait. - HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" \ - --connect-timeout 5 --max-time 10 "${XNAT_URL}/xapi/siteConfig" 2>/dev/null) || HTTP_CODE=000 - if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "401" ]; then - echo "XNAT is ready (HTTP ${HTTP_CODE})!" - XNAT_READY=true - break - fi - sleep 10 - done - if [ "$XNAT_READY" != "true" ]; then - echo "ERROR: XNAT did not become ready within 600s (last HTTP ${HTTP_CODE})" >&2 - exit 1 - fi - - # Login: try configured password first, fall back to XNAT default (admin:admin pre-init) - _xnat_login() { - local PASS="$1" - local RESP - RESP=$(curl -s -D - -X POST "${XNAT_URL}/data/JSESSION" -u "${ADMIN_USER}:${PASS}" 2>/dev/null) - local SID - SID=$(echo "$RESP" | tail -1 | tr -d '[:space:]') - if [ -z "$SID" ] || echo "$SID" | grep -qi '/dev/null) || HTTP_CODE=000 - case "${HTTP_CODE}" in - 2*) - echo "XNAT plugin routes are ready (HTTP ${HTTP_CODE})!" - PLUGINS_READY=true - break - ;; - esac - sleep 10 - done - if [ "$PLUGINS_READY" != "true" ]; then - echo "ERROR: XNAT plugin routes did not register within $(( $(date +%s) - PLUGIN_WAIT_START ))s (last HTTP ${HTTP_CODE})" >&2 - echo " endpoint: ${XNAT_URL}/xapi/dqr/settings" >&2 - exit 1 - fi - - # Activate XNAT site (idempotent — safe to call even if already initialized). - # siteUrl must be non-empty on XNAT >= 1.10.0: the Restlet create paths NPE on a - # null siteUrl while building the response (entity created, request 500s) — see - # trust/xnat/xnat/config/configure-xnat.sh for the full explanation. - echo "Activating XNAT site..." - xnat_curl -X POST "${XNAT_URL}/xapi/siteConfig" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d "{\"initialized\": true, \"siteUrl\": \"${XNAT_URL}\"}" >/dev/null - - # Set admin password to configured value if it differs from default - echo "Setting admin password..." - xnat_curl -X PUT "${XNAT_URL}/xapi/users/${ADMIN_USER}" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d "{\"password\": \"${ADMIN_PASS}\"}" >/dev/null - - # Grant the admin account the ContainerManager role. Mirrors - # trust/xnat/xnat/config/configure-xnat.sh — the sibling - # configure-dcm2niix container authenticates as admin, and since - # Container Service 3.7.0 both /xapi/docker/server and - # /xapi/commands require this role. Without it those calls fail - # with 401/403 and, because they were `|| true`, the Job still - # reported success with dcm2niix silently unregistered. - echo "Assigning role 'ContainerManager' to ${ADMIN_USER}..." - xnat_curl -X PUT "${XNAT_URL}/xapi/users/${ADMIN_USER}/roles/ContainerManager" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "accept: application/json" >/dev/null - - # Assign roles to service account - SERVICE_USER="${XNAT_SERVICE_USER:-flipServiceAccount}" - - # Create the service account through the REST API, never with a raw - # INSERT into xdat_user: XNAT only provisions the account's matching - # xhbm_xdat_user_auth "localdb" record on this path, and without that - # record every login is rejected 401 no matter what - # xdat_user.primary_password holds. XNAT 1.9.3 masked a DB-layer - # insert by back-filling the record on the password PUT below; 1.10.0 - # does not, and since the PUT still answers 200 the job would report - # success on an XNAT whose service account can never authenticate — - # imaging-api then 401s on every call (TROUBLESHOOTING.md §2.3). - echo "Ensuring service account ${SERVICE_USER} exists..." - existing_users=$(xnat_curl "${XNAT_URL}/xapi/users" \ - -H "Cookie: JSESSIONID=${JSESSION}") - if printf '%s' "$existing_users" | jq -e --arg u "${SERVICE_USER}" 'index($u)' >/dev/null; then - echo " ${SERVICE_USER} already exists — leaving as-is." - else - xnat_curl -X POST "${XNAT_URL}/xapi/users" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d "{\"username\": \"${SERVICE_USER}\", - \"password\": \"${SERVICE_PASS}\", - \"firstName\": \"FLIP\", - \"lastName\": \"Service\", - \"email\": \"flip@gstt.nhs.uk\", - \"enabled\": true, - \"verified\": true}" >/dev/null - echo " ${SERVICE_USER} created." - fi - - # Sync service-account password from the chart secret so it always - # matches XNAT_SERVICE_PASSWORD in imaging-api, even after a backup - # restore that carries an older password hash. - echo "Setting ${SERVICE_USER} password..." - xnat_curl -X PUT "${XNAT_URL}/xapi/users/${SERVICE_USER}" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d "{\"password\": \"${SERVICE_PASS}\"}" >/dev/null - echo "Assigning roles to ${SERVICE_USER}..." - xnat_curl -X PUT "${XNAT_URL}/xapi/users/${SERVICE_USER}/groups/" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d '["ALL_DATA_ADMIN"]' >/dev/null - xnat_curl -X PUT "${XNAT_URL}/xapi/users/${SERVICE_USER}/roles/" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d '["ContainerManager","DataManager","SiteUser","Administrator","Dqr","non_expiring"]' >/dev/null - - # Apply + enable the site-wide anonymization script (mirrors - # trust/xnat/xnat/config/configure-xnat.sh in the Compose deploy). - echo "Applying site-wide anonymization script..." - xnat_curl -X PUT "${XNAT_URL}/xapi/anonymize/site" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: text/plain" \ - --data-binary @/cs-config/anon_script.das >/dev/null - xnat_curl -X PUT "${XNAT_URL}/xapi/anonymize/site/enabled" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d 'true' >/dev/null - - # Replace the default DICOM SCP receiver with the DQR-aware one. - # The stock receiver uses identifier=dicomObjectIdentifier with - # customProcessing=false, which leaves DQR-pulled studies in the - # Unassigned prearchive ("Cannot build session. 0 prearchive - # sessions found"). DQR needs identifier=dqrObjectIdentifier + - # customProcessing so received studies are routed to the - # requesting project and the relabel map (Subject UUID / - # Session=accession) is applied. - echo "Replacing DICOM SCP receiver with DQR-aware config..." - scp_list=$(xnat_curl "${XNAT_URL}/xapi/dicomscp" \ - -H "Cookie: JSESSIONID=${JSESSION}") - for SCP_ID in $(printf '%s' "$scp_list" | jq -r '.[] | select(.aeTitle == "XNAT") | .id'); do - echo " Removing existing SCP receiver id=${SCP_ID}..." - xnat_curl -X DELETE "${XNAT_URL}/xapi/dicomscp/${SCP_ID}" \ - -H "Cookie: JSESSIONID=${JSESSION}" >/dev/null - done - xnat_curl -X POST "${XNAT_URL}/xapi/dicomscp" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d '{ - "aeTitle": "XNAT", - "port": 8104, - "enabled": true, - "customProcessing": true, - "directArchive": true, - "identifier": "dqrObjectIdentifier", - "anonymizationEnabled": true, - "whitelistEnabled": false, - "whitelistText": "", - "routingExpressionsEnabled": false, - "projectRoutingExpression": "", - "subjectRoutingExpression": "", - "sessionRoutingExpression": "" - }' >/dev/null - - # Register PACS. Check-then-create by aeTitle: a duplicate - # registration surfaces as an unspecific 500 (DB unique-constraint - # violation), so re-run idempotency has to be a lookup rather than - # a tolerated status code (FLIP#862). - existing_pacs=$(xnat_curl "${XNAT_URL}/xapi/pacs" \ - -H "Cookie: JSESSIONID=${JSESSION}") - if printf '%s' "$existing_pacs" | grep -q '"aeTitle":"ORTHANC"'; then - echo "PACS 'ORTHANC' already registered — leaving as-is." - else - echo "Registering PACS..." - xnat_curl -X POST "${XNAT_URL}/xapi/pacs" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d '{ - "aeTitle": "ORTHANC", - "defaultQueryRetrievePacs": true, - "defaultStoragePacs": true, - "host": "orthanc", - "label": "Orthanc PACS", - "ormStrategySpringBeanId": "dicomOrmStrategy", - "queryRetrievePort": 4242, - "queryable": true, - "storable": true, - "supportsExtendedNegotiations": true - }' >/dev/null - fi - - # Configure DQR settings - echo "Configuring DQR settings..." - xnat_curl -X POST "${XNAT_URL}/xapi/dqr/settings" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d '{ - "pacsAvailabilityCheckFrequency": "1 minute", - "dqrWaitToRetryRequestInSeconds": "300", - "assumeSameSessionIfArrivedWithin": "30 minutes", - "allowAllUsersToUseDqr": false, - "dqrCallingAe": "XNAT", - "notifyAdminOnImport": false, - "allowAllProjectsToUseDqr": true, - "leavePacsAuditTrail": false, - "dqrMaxPacsRequestAttempts": "100" - }' >/dev/null - - # Configure PACS availability for all days. DQR pre-creates - # availability intervals when the PACS is registered, so this POST - # returns 400 "probable overlap with existing interval" for an - # already-scheduled day — treated as "already configured" rather - # than a failure. Anything else non-2xx is a real error. - echo "Configuring PACS availability..." - for DAY in MONDAY TUESDAY WEDNESDAY THURSDAY FRIDAY SATURDAY SUNDAY; do - avail_status=$(curl -s -o /tmp/pacs-availability-response.json \ - --connect-timeout 10 --max-time 120 -w '%{http_code}' \ - -X POST "${XNAT_URL}/xapi/pacs/1/availability" \ - -H "Cookie: JSESSIONID=${JSESSION}" \ - -H "Content-Type: application/json" \ - -d "{\"availabilityEnd\":\"23:59\",\"availabilityStart\":\"00:00\",\"availableNow\":true,\"dayOfWeek\":\"$DAY\",\"enabled\":true,\"pacsId\":1,\"threads\":4,\"utilizationPercent\":100}") || avail_status="000" - case "$avail_status" in - 2[0-9][0-9]) ;; - 400) - echo " Availability interval for $DAY already exists (HTTP 400 overlap) — leaving as-is." - ;; - *) - echo "ERROR: setting PACS availability for $DAY failed (HTTP $avail_status)" >&2 - cat /tmp/pacs-availability-response.json >&2 || true - exit 1 - ;; - esac - done - - echo "XNAT web configuration complete." + cd /data/xnat/config + bash configure-xnat.sh env: - - name: XNAT_ADMIN_PASS + - name: XNAT_URL + value: "http://xnat-web:8080" + - name: XNAT_ADMIN_USER + value: {{ .Values.xnat.web.adminUser | quote }} + # Both map to the same secret key, matching the xnat-web deployment: on a fresh install + # XNAT is seeded with this password, and configure-xnat.sh rotates from it to itself. + - name: XNAT_ADMIN_INITIAL_PASSWORD + valueFrom: + secretKeyRef: + name: {{ if .Values.secrets.create }}{{ include "flip-trust.fullname" . }}-secrets{{ else }}{{ .Values.secrets.existingName }}{{ end }} + key: xnat-admin-password + - name: XNAT_ADMIN_PASSWORD valueFrom: secretKeyRef: name: {{ if .Values.secrets.create }}{{ include "flip-trust.fullname" . }}-secrets{{ else }}{{ .Values.secrets.existingName }}{{ end }} @@ -405,27 +92,53 @@ spec: secretKeyRef: name: {{ if .Values.secrets.create }}{{ include "flip-trust.fullname" . }}-secrets{{ else }}{{ .Values.secrets.existingName }}{{ end }} key: xnat-service-user - - name: SERVICE_PASS + - name: XNAT_SERVICE_PASSWORD valueFrom: secretKeyRef: name: {{ if .Values.secrets.create }}{{ include "flip-trust.fullname" . }}-secrets{{ else }}{{ .Values.secrets.existingName }}{{ end }} key: xnat-service-password - volumeMounts: - - name: cs-config - mountPath: /cs-config + # DICOM SCP receiver: the port XNAT binds and registers, and the AE title the PACS + # addresses its C-STORE association to. Must match what the PACS has registered. + # No `| default` here: Helm substitutes a default for an empty string too, which would + # hand the script a value it considers configured and cancel its fail-loud guard. The + # chart's own defaults live in values.yaml; an operator who blanks one gets an error. + - name: XNAT_PORT + value: {{ .Values.xnat.web.dicomPort | quote }} + - name: XNAT_AETITLE + value: {{ .Values.xnat.web.dicomAet | quote }} + # Upstream PACS. Defaults are the mocked Orthanc; a real trust overrides them. + - name: PACS_HOST + value: {{ .Values.pacs.host | quote }} + - name: PACS_AETITLE + value: {{ .Values.pacs.aeTitle | quote }} + - name: PACS_QR_PORT + value: {{ .Values.pacs.qrPort | quote }} + - name: PACS_LABEL + value: {{ .Values.pacs.label | quote }} + - name: PACS_SUPPORTS_EXTENDED_NEGOTIATIONS + value: {{ .Values.pacs.supportsExtendedNegotiations | quote }} + # Throttle: a production PACS may refuse further associations after a certain volume. + - name: PACS_AVAILABILITY_DAYS + value: {{ .Values.pacs.availability.days | quote }} + - name: PACS_AVAILABILITY_START + value: {{ .Values.pacs.availability.start | quote }} + - name: PACS_AVAILABILITY_END + value: {{ .Values.pacs.availability.end | quote }} + - name: PACS_THREADS + value: {{ .Values.pacs.availability.threads | quote }} + - name: PACS_UTILIZATION_PERCENT + value: {{ .Values.pacs.availability.utilizationPercent | quote }} + - name: DQR_MAX_PACS_REQUEST_ATTEMPTS + value: {{ .Values.pacs.dqr.maxRequestAttempts | quote }} + - name: DQR_RETRY_WAIT_SECONDS + value: {{ .Values.pacs.dqr.retryWaitSeconds | quote }} resources: requests: - memory: "64Mi" + memory: "128Mi" cpu: "100m" limits: - memory: "128Mi" + memory: "256Mi" cpu: "200m" - # Configures the XNAT Container Service plugin to use the native - # Kubernetes compute backend (available since container-service 3.2.0), - # registers the dcm2niix command, and enables the Event Service so - # imaging-api can create per-project event subscriptions. Mirrors what - # configure-dcm2niix.sh does in the Compose deployment, but speaks to - # the K8s backend instead of the Docker socket. - name: configure-dcm2niix image: alpine:3.20 command: @@ -848,142 +561,4 @@ data: "generic-resources": {}, "ulimits": {} } ---- -# ConfigMap with helper scripts for XNAT configuration -apiVersion: v1 -kind: ConfigMap -metadata: - name: {{ include "flip-trust.fullname" . }}-xnat-scripts - namespace: {{ include "flip-trust.namespace" . }} - labels: - {{- include "flip-trust.labels" . | nindent 4 }} - app.kubernetes.io/component: xnat-init -data: - configure-xnat.sh: | - #!/bin/sh - # XNAT Configuration Script - # This script configures XNAT after initial deployment. - # It can be run manually if the automated init job is disabled. - set -euo pipefail - - XNAT_URL="${1:-http://xnat-web:8080}" - ADMIN_USER="${2:-admin}" - ADMIN_PASS="${3:-}" - SERVICE_USER="${4:-flipServiceAccount}" - SERVICE_PASS="${5:-}" - ORTHANC_HOST="${6:-orthanc}" - PACS_DICOM_PORT="${7:-4242}" - - if [ -z "$ADMIN_PASS" ] || [ -z "$SERVICE_PASS" ]; then - echo "Usage: configure-xnat.sh [service_user] [service_pass] [orthanc_host] [pacs_port]" - exit 1 - fi - - echo "Configuring XNAT at ${XNAT_URL}..." - - wait_for_xnat() { - echo "Waiting for XNAT to be available..." - for i in $(seq 1 120); do - if wget -q --spider "${XNAT_URL}/app/template/Login.vm" 2>/dev/null; then - echo "XNAT is up!" - return 0 - fi - sleep 5 - done - echo "XNAT did not become available within timeout" - return 1 - } - - wait_for_xnat - - # The session id is written to disk on the way through, so remove it whenever this script - # exits. It is a live XNAT admin session and the file would otherwise outlive its use inside - # the container's filesystem. - trap 'rm -f /tmp/jsession.txt' EXIT - - # Login and get JSESSIONID - login() { - local u="$1" p="$2" - wget -q -O /tmp/jsession.txt --post-data="" \ - --header="Authorization: Basic $(printf '%s:%s' "$u" "$p" | base64)" \ - "${XNAT_URL}/data/JSESSION" 2>/dev/null || true - cat /tmp/jsession.txt 2>/dev/null || echo "" - } - - JSESSION=$(login "${ADMIN_USER}" "${ADMIN_PASS}") - - if [ -z "$JSESSION" ]; then - echo "Trying initial admin password..." - JSESSION=$(login "${ADMIN_USER}" "${ADMIN_PASS}") - if [ -z "$JSESSION" ]; then - echo "Failed to authenticate with XNAT" - exit 1 - fi - fi - - COOKIE="Cookie: JSESSIONID=${JSESSION}" - - # Activate XNAT site. siteUrl must be non-empty on XNAT >= 1.10.0 (Restlet create paths - # NPE on null siteUrl while building the response) — see configure-xnat.sh. - echo "Activating XNAT site..." - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data="{\"initialized\": true, \"siteUrl\": \"${XNAT_URL}\"}" "${XNAT_URL}/xapi/siteConfig" - - # Register PACS (Orthanc) - echo "Registering PACS..." - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data="{ - \"aeTitle\": \"ORTHANC\", - \"defaultQueryRetrievePacs\": true, - \"defaultStoragePacs\": true, - \"host\": \"${ORTHANC_HOST}\", - \"label\": \"Orthanc PACS\", - \"ormStrategySpringBeanId\": \"dicomOrmStrategy\", - \"queryRetrievePort\": ${PACS_DICOM_PORT}, - \"queryable\": true, - \"storable\": true, - \"supportsExtendedNegotiations\": true - }" "${XNAT_URL}/xapi/pacs" - - # Assign roles to service account - echo "Assigning roles to service account..." - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data='["ALL_DATA_ADMIN"]' \ - "${XNAT_URL}/xapi/users/${SERVICE_USER}/groups/" - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data='["ContainerManager","DataManager","SiteUser","Administrator","Dqr","non_expiring"]' \ - "${XNAT_URL}/xapi/users/${SERVICE_USER}/roles/" - - # Configure DQR settings - echo "Configuring DQR settings..." - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data='{ - "pacsAvailabilityCheckFrequency": "1 minute", - "dqrWaitToRetryRequestInSeconds": "300", - "assumeSameSessionIfArrivedWithin": "30 minutes", - "allowAllUsersToUseDqr": false, - "dqrCallingAe": "XNAT", - "notifyAdminOnImport": false, - "allowAllProjectsToUseDqr": true, - "leavePacsAuditTrail": false, - "dqrMaxPacsRequestAttempts": "100" - }' "${XNAT_URL}/xapi/dqr/settings" - - # Configure PACS availability for all days - echo "Configuring PACS availability..." - for DAY in MONDAY TUESDAY WEDNESDAY THURSDAY FRIDAY SATURDAY SUNDAY; do - wget -q -O /dev/null --header="${COOKIE}" --header="Content-Type: application/json" \ - --post-data="{ - \"availabilityEnd\": \"23:59\", - \"availabilityStart\": \"00:00\", - \"availableNow\": true, - \"dayOfWeek\": \"$DAY\", - \"enabled\": true, - \"pacsId\": 1, - \"threads\": 4, - \"utilizationPercent\": 100 - }" "${XNAT_URL}/xapi/pacs/1/availability" - done - - echo "XNAT configuration complete." {{- end }} diff --git a/deploy/providers/kubernetes/templates/xnat-web.yaml b/deploy/providers/kubernetes/templates/xnat-web.yaml index c2fe3b84c..1b1b9d756 100644 --- a/deploy/providers/kubernetes/templates/xnat-web.yaml +++ b/deploy/providers/kubernetes/templates/xnat-web.yaml @@ -22,7 +22,11 @@ metadata: data: XNAT_HOME: /data/xnat/home XNAT_ADMIN_USER: {{ .Values.xnat.web.adminUser | quote }} - XNAT_PORT: "8080" + # The DICOM SCP receiver port. Sourced from the same value the init job registers the receiver + # with, so a hand-run of configure-xnat.sh inside this pod (initJob.enabled: false) configures the + # same port rather than a stale literal. It read "8080" — Tomcat's port — while XNAT_PORT was + # still ambiguous; it now unambiguously means DICOM (FLIP#993). + XNAT_PORT: {{ .Values.xnat.web.dicomPort | quote }} XNAT_DATASOURCE_DRIVER: "org.postgresql.Driver" XNAT_DATASOURCE_NAME: "xnat" XNAT_DATASOURCE_USERNAME: {{ .Values.xnat.db.datasourceUsername | quote }} @@ -33,7 +37,6 @@ data: XNAT_MIN_HEAP: {{ .Values.xnat.web.env.XNAT_MIN_HEAP | quote }} XNAT_MAX_HEAP: {{ .Values.xnat.web.env.XNAT_MAX_HEAP | quote }} CATALINA_OPTS: "-Xms{{ .Values.xnat.web.env.XNAT_MIN_HEAP }} -Xmx{{ .Values.xnat.web.env.XNAT_MAX_HEAP }} -Dxnat.home=/data/xnat/home -XX:+UseG1GC -Djava.awt.headless=true" - PACS_DICOM_PORT: "4242" --- apiVersion: v1 kind: Service @@ -45,6 +48,15 @@ metadata: app.kubernetes.io/component: xnat-web spec: type: {{ .Values.xnat.web.service.type }} + {{- if and (eq .Values.xnat.web.service.type "NodePort") .Values.xnat.web.dicomNodePort }} + # Preserve the PACS's source IP. Under the default `Cluster` policy kube-proxy SNATs NodePort + # traffic to the node address before it reaches the pod, so the ingress NetworkPolicy's PACS CIDR + # would never match and the C-STORE return leg would be dropped by default-deny — queries + # succeeding while retrievals silently time out, which is the bug this exists to prevent + # (FLIP#993). `Local` also means only nodes running the pod answer, which suits the single-node + # trust deployment this targets. + externalTrafficPolicy: Local + {{- end }} ports: - port: {{ .Values.xnat.web.service.port }} targetPort: tomcat @@ -54,6 +66,11 @@ spec: targetPort: {{ .Values.xnat.web.dicomPort | default 8104 }} protocol: TCP name: dicom-scp + {{- if and .Values.xnat.web.dicomNodePort (eq .Values.xnat.web.service.type "NodePort") }} + # Pinned so the PACS has a stable destination port. Without this Kubernetes allocates one at + # random, and the C-MOVE destination XNAT advertises would not match what the PACS can reach. + nodePort: {{ .Values.xnat.web.dicomNodePort }} + {{- end }} selector: {{- include "flip-trust.selectorLabels" . | nindent 4 }} app.kubernetes.io/component: xnat-web diff --git a/deploy/providers/kubernetes/values.yaml b/deploy/providers/kubernetes/values.yaml index 12a9d7776..4aae263c5 100644 --- a/deploy/providers/kubernetes/values.yaml +++ b/deploy/providers/kubernetes/values.yaml @@ -206,11 +206,10 @@ imagingApi: topologyKey: kubernetes.io/hostname env: XNAT_URL: "http://xnat-web:8080" - # DICOM SCP receiver port — NOT the web port. DQR import requests are - # queued with destination AE "XNAT:{XNAT_PORT}"; XNAT only dequeues them - # if a DICOM SCP receiver with that exact AE:port exists (8104, matching - # the xnat-web Service dicom-scp port and the Orthanc modality config). - XNAT_PORT: "8104" + # XNAT_PORT and XNAT_AETITLE are deliberately absent here. They describe XNAT's DICOM SCP + # receiver, so they come from xnat.web.dicomPort / xnat.web.dicomAet — the same values the init + # job registers the receiver with. Restating them here made two sources for one number, which is + # the defect FLIP#993 set out to remove. PACS_ID: "1" # Topology only — no password here (FLIP-PT-056): imaging-api splices in the # xnat-datasource-password secret at startup (see templates/imaging-api.yaml). @@ -504,9 +503,6 @@ omopDb: orthanc: enabled: true host: orthanc - dicomHost: orthanc - dicomPort: 4242 - dicomAet: ORTHANC external: host: "" port: 8042 @@ -566,10 +562,20 @@ xnat: web: enabled: true adminUser: "admin" - # DICOM SCP (Service Class Provider) configuration - # XNAT listens as AE 'XNAT' on this port to receive C-STORE requests + # DICOM SCP (Service Class Provider) configuration. + # XNAT binds this port, registers it on its dicomscp receiver, and advertises it as the C-MOVE + # destination. DQR matches that destination against a registered receiver by exact AE title and + # port, so these must equal what the PACS has registered for us — no translation is possible on + # that leg (FLIP#993). dicomAet: XNAT dicomPort: 8104 + # Fixed NodePort for the DICOM receiver, used with service.type: NodePort. Required when the + # PACS is outside the cluster: after XNAT issues C-MOVE the PACS opens a *new* association back + # to XNAT to C-STORE the studies, so that port must be reachable. Leave empty for the mocked + # Orthanc, which reaches the receiver over the cluster network. + # Must be inside the API server's --service-node-port-range (default 30000-32767), or the range + # widened to admit the DICOM port. Set it equal to dicomPort so one number is true end to end. + dicomNodePort: "" image: repository: ghcr.io/londonaicentre/xnat-web tag: stag @@ -753,12 +759,48 @@ podDisruptionBudget: # --------------------------------------------------------------------------- # Network policies — zero inbound trust model # --------------------------------------------------------------------------- +# Upstream PACS that XNAT retrieves imaging from, via the DQR plugin. +# +# Defaults describe the mocked Orthanc deployed by this chart. A trust points these at its own PACS; +# see docs/source/components/component-pacs.rst. These replace the +# former orthanc.dicomHost / dicomPort / dicomAet values, which no template ever read — which is why +# setting orthanc.enabled: false with an external.host never actually redirected DQR. +pacs: + host: orthanc + aeTitle: ORTHANC + qrPort: 4242 + label: Test PACS instance + # Relational queries / extended negotiation. A per-PACS capability, not a preference: a PACS that + # does not support it rejects the association unless this is false. Ask the PACS team. + supportsExtendedNegotiations: true + # Throttle. A production PACS may refuse further associations after a certain volume, and a trust + # may want retrieval confined to out-of-hours. Agree the window with the PACS manager. + availability: + days: MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY + start: "00:00" + end: "24:00" + threads: 1 + utilizationPercent: 100 + dqr: + maxRequestAttempts: 100 + retryWaitSeconds: 300 + networkPolicies: enabled: true # When true, allows ingress from kube-system namespace (kubelet health checks, # metrics scraping). Set to true if your CNI requires explicit carve-outs for # kubelet probes or cluster-internal monitoring. allowKubeSystemIngress: false + # CIDRs allowed inbound on a specific port, in addition to the default-deny above. + # The only intended use is the DICOM C-STORE return leg: FLIP retrieves by pull, so after XNAT + # issues C-MOVE the PACS opens a new association back to XNAT. That is inbound, and without an + # allowance here it is dropped — queries succeed and retrievals silently time out (FLIP#993). + # Scope this to the PACS itself, not the whole trust network. + # Each entry: cidrs (list of CIDR strings) + port (int) + optional protocol (default TCP). + allowedIngressCIDRsWithPorts: [] + # - cidrs: + # - 10.0.0.10/32 + # port: 8104 # CIDRs allowed for all-ports egress (in addition to intra-namespace) allowedEgressCIDRs: [] # - 10.0.0.0/8 diff --git a/docs/source/assets/xnat/credentials_email.png b/docs/source/assets/xnat/credentials_email.png index f6bd72bff..906949374 100644 Binary files a/docs/source/assets/xnat/credentials_email.png and b/docs/source/assets/xnat/credentials_email.png differ diff --git a/docs/source/components.rst b/docs/source/components.rst index 6b57edf11..9df770746 100644 --- a/docs/source/components.rst +++ b/docs/source/components.rst @@ -9,6 +9,7 @@ FLIP components components/component-fl-nodes components/component-omop-database components/component-xnat + components/component-pacs components/component-logging-stack .. note:: diff --git a/docs/source/components/component-pacs.rst b/docs/source/components/component-pacs.rst new file mode 100644 index 000000000..44ce67371 --- /dev/null +++ b/docs/source/components/component-pacs.rst @@ -0,0 +1,382 @@ +.. _flip-pacs: + +#### +PACS +#### + +The Picture Archiving and Communication System (PACS) is the clinical system that stores a trust's +imaging. FLIP does not hold a copy of it: imaging is retrieved from the trust's own PACS, on demand, +for the studies belonging to an approved project cohort, and lands in that trust's +:doc:`XNAT ` instance. + +This page describes how that retrieval works, what a trust's PACS and network teams need to +configure, and how to verify the connection. + +DICOM Networking in Brief +========================= + +Three ideas are enough to follow the rest of this section. + +**AE Title.** A DICOM system's *name* on the network — like a hostname, but specific to DICOM, and +at most 16 characters. When one system connects to another it announces "I am *X*, calling *Y*". The +receiver checks that *Y* is its own name and that *X* is one it has been told to accept. Names are +separate from addresses: the IP and port are configured alongside the AE title, not derived from it. + +**SCU and SCP.** Client and server. An SCU (Service Class *User*) opens connections; an SCP (Service +Class *Provider*) listens for them. XNAT is both, at different moments — an SCU when it queries the +PACS, an SCP when it receives the images. + +**The four operations**, in the order FLIP uses them: + +.. list-table:: + :widths: 15 55 30 + :header-rows: 1 + + * - Operation + - What it does + - Direction + * - ``C-ECHO`` + - A DICOM ping. Confirms two systems can reach and accept each other + - either way, for testing + * - ``C-FIND`` + - Search — "which study has accession number ABC123?" + - XNAT to PACS + * - ``C-MOVE`` + - "Send that study to the system called ``FLIPXNAT``" + - XNAT to PACS + * - ``C-STORE`` + - The image transfer itself + - **PACS to XNAT** + +.. important:: + + C-MOVE does not return the images on the connection that asked for them. XNAT names a + destination, the PACS looks that name up in its *own* table to find an address, and opens a + **new connection in the opposite direction** to deliver the study. + + Three consequences follow, and each has caused a failed integration in practice: + + * The destination must be registered on the PACS in advance — a name it does not know cannot be + delivered to. + * The AE title and port XNAT advertises must match that registration exactly. XNAT rejects an + association addressed to a different name, and the DQR plugin refuses to issue a C-MOVE whose + destination does not correspond to one of its own configured receivers. + * The return connection needs its own firewall rule. Every other FLIP connection is outbound, so + this is the one reviewers overlook. + +How Retrieval Works +=================== + +FLIP **pulls**; the PACS is never configured to push into XNAT. XNAT's +`DICOM Query-Retrieve (DQR) plugin `_ +performs the retrieval, driven over REST by the imaging API: + +1. The cohort query is re-run against the trust's OMOP database and returns **accession numbers + only**. +2. For each accession number, XNAT issues a **C-FIND** to the PACS at STUDY level, matching on + Accession Number ``(0008,0050)``, to resolve the Study Instance UID. +3. XNAT issues a **C-MOVE** to the PACS, naming itself as the move destination. +4. The PACS **C-STOREs** the study back to XNAT's DICOM SCP receiver, where the site-wide + anonymisation script runs on receipt, before the session is archived — see + :ref:`DICOM Anonymization `. + +Only accession numbers belonging to an approved project cohort are ever requested. There is no +standing forward rule and no bulk transfer. + +.. important:: + + Step 4 is a connection **from the PACS to XNAT**. It is easy to overlook when specifying firewall + rules, because every other FLIP connection is outbound. Without it, queries succeed and + retrievals silently time out. + +What the PACS Team Must Register +================================ + +The FLIP XNAT instance must be registered on the PACS as a DICOM node, permitted to: + +* issue **C-FIND** (Study Root query/retrieve) as an SCU; +* act as a **C-MOVE destination**, so retrieved studies can be returned to it; +* issue and answer **C-ECHO**, for verification in both directions. + +FLIP supplies its own AE title, host and DICOM port for that registration. In return, the trust +needs to confirm the following — the first three are required to configure anything at all, the rest +are the questions that most often turn out to matter: + +.. list-table:: + :widths: 32 68 + :header-rows: 1 + + * - What to ask for + - Why it matters + * - AE title, host/IP and query/retrieve port of the PACS + - The details XNAT dials. The port must be reachable from the XNAT container + * - Whether C-MOVE is served by a **different AE** to the query service + - Some PACS separate the query and retrieve roles; if so, both must be known + * - Confirmation that STUDY-level matching on Accession Number ``(0008,0050)`` is supported + - This is the only key FLIP queries by. Without it, nothing resolves + * - Whether relational queries / extended negotiation are supported + - Enabled by default; a PACS that does not support it rejects the association, and needs + ``PACS_SUPPORTS_EXTENDED_NEGOTIATIONS=false`` + * - The transfer syntax studies will be sent in + - Compressed or transcoded data still has to be readable by XNAT once archived + * - Any per-connection or per-session limit on how much may be retrieved + - A production PACS may refuse further associations after a certain volume; see + `Scheduling and Throughput`_ + * - Whether DICOM TLS is required + - FLIP does not currently support TLS on the DIMSE connection, so this needs to be established + before committing to a design + * - Which port they want used for bulk retrieval + - A port already used for teleradiology may be configured for single studies rather than the + sustained retrieval FLIP performs + +.. note:: + + The AE title FLIP presents is one of many configured on a trust PACS, so it should identify the + platform — for example ``FLIPXNAT``. It must match on both sides: the PACS opens the C-STORE + association using the AE title it has registered, and XNAT's SCP receiver rejects an association + addressed to a different AE title. + +.. note:: + + Registering a new node is often not something the trust's PACS team can do unaided. Depending on + how the PACS is managed, they may need to raise a call with the supplier to have the connection + enabled and any credentials issued — worth starting early, as it tends to set the lead time. + + Where FLIP has been connected to this PACS before, ask for any previous FLIP or XNAT node entries + to be removed at the same time. Stale registrations pointing at decommissioned hosts are + confusing at best, and a retrieval addressed to one silently goes nowhere. + +Configuring FLIP +================ + +These are FLIP's own settings, applied by the operator deploying the trust node — in the trust's kit +file for a Compose deployment, or in the Helm values for Kubernetes. They are not something the +trust's PACS team supplies; what they supply is covered above. + +.. important:: + + The defaults below describe the **mocked PACS that ships with FLIP for development**, not values + a trust should use. Every one of them is replaced with the real details when connecting to a + trust PACS. + +.. list-table:: + :widths: 30 45 25 + :header-rows: 1 + + * - Setting + - Description + - Development default + * - ``XNAT_AETITLE`` + - XNAT's own AE title, used for the DICOM SCP receiver, the DQR calling AE, and the C-MOVE + destination + - ``XNAT`` + * - ``XNAT_PORT`` + - DICOM SCP receiver port + - ``8104`` + * - ``PACS_AETITLE`` + - AE title of the trust PACS + - ``ORTHANC`` + * - ``PACS_HOST`` + - Hostname or IP of the trust PACS + - ``orthanc`` + * - ``PACS_QR_PORT`` + - Query/retrieve port on the trust PACS. Must be reachable *from the XNAT container* — this is + not a host-published port + - ``4242`` + * - ``XNAT_WEB_PORT`` + - Host-published port for XNAT's web UI and REST API. Unrelated to DICOM; the receiver is + host-published too, so this must differ from ``XNAT_PORT`` — the deploy refuses a collision + - ``8105`` + * - ``PACS_SUPPORTS_EXTENDED_NEGOTIATIONS`` + - Whether the PACS supports relational queries / extended negotiation. A capability of the + PACS, not a preference — see the table above + - ``true`` + * - ``PACS_AVAILABILITY_DAYS`` / ``_START`` / ``_END`` + - When retrieval may run, as a comma-separated day list and a daily window + - all week, ``00:00``–``24:00`` + * - ``PACS_THREADS`` / ``PACS_UTILIZATION_PERCENT`` + - How hard to drive the PACS during that window + - ``1`` / ``100`` + * - ``DQR_MAX_PACS_REQUEST_ATTEMPTS`` / ``DQR_RETRY_WAIT_SECONDS`` + - How many times, and how far apart, to retry a study the PACS did not deliver + - ``100`` / ``300`` + +The mocked PACS those defaults describe is covered below. + +Exposing the DICOM Receiver +=========================== + +The page has said several times that the receiver must be reachable from the PACS. How it is +exposed differs by deployment: + +**Compose.** The receiver is always published on the host, next to the web UI, so a development +deployment runs the same wiring a real-PACS trust relies on. ``XNAT_WEB_PORT`` and ``XNAT_PORT`` +must therefore differ; the Makefile refuses to deploy if they collide. The mocked Orthanc does not +itself need the publication — it reaches the receiver over the container network — and the mock +deployments expose nothing by it: local development binds on the developer's machine, and the +AWS-hosted mock trusts sit behind security groups with no ingress rules. + +**Kubernetes.** Three values, all off by default: + +.. code-block:: yaml + + xnat: + web: + service: + type: NodePort + # Pin the port so the PACS has a stable destination. Must be inside the API server's + # --service-node-port-range, or that range widened to admit the DICOM port. + dicomNodePort: 8104 + + networkPolicies: + # The C-STORE return leg. Scope to the PACS itself, never the whole trust network. + allowedIngressCIDRsWithPorts: + - cidrs: ["10.0.0.10/32"] + port: 8104 + # The outbound query. Without this, C-FIND never leaves the cluster. + allowedEgressCIDRsWithPorts: + - cidrs: ["10.0.0.10/32"] + port: 8059 + +The chart refuses to render a configured PACS with no egress rule, and equally one with no ingress +entry on ``xnat.web.dicomPort``: both directions of the retrieval have to be open, and an omitted +return leg is the failure mode where queries succeed and retrievals silently time out. The port in +the ingress entry is the pod's container port (``xnat.web.dicomPort``, default 8104), not +``dicomNodePort`` — a NetworkPolicy matches after the node has undone the NodePort translation. An +ingress entry with no ``port`` at all is refused too: Kubernetes reads an absent port as *all* +ports, which would widen the one inbound path into the trust from DICOM to everything. + +.. note:: + + Setting ``service.type: NodePort`` also sets ``externalTrafficPolicy: Local`` on that Service. + Under the default ``Cluster`` policy the PACS's source address is rewritten to the node's before + the pod sees it, so the ingress rule above would never match and the C-STORE would be dropped. + +Development: the Mocked PACS +============================ + +FLIP ships an `Orthanc `_ DICOM server that stands in for the trust +PACS during development and testing. It is seeded with synthetic DICOM studies whose accession +numbers match the mocked OMOP database, so a cohort query resolves to real studies and the full +retrieval path can be exercised without a hospital PACS. + +Orthanc is a genuine DICOM node, so it answers C-FIND and C-MOVE exactly as a production PACS would +and returns studies by C-STORE. The retrieval path under test is therefore the same one used against +a trust PACS — only the peer differs. It is why the configuration defaults above name ``orthanc``: + +* ``PACS_HOST=orthanc`` — the container name on the FLIP network +* ``PACS_AETITLE=ORTHANC`` — Orthanc's AE title +* ``PACS_QR_PORT=4242`` — Orthanc's DICOM port + +Because both sit on the same container network, Orthanc reaches XNAT's SCP receiver directly and +would not itself need the receiver's host port. A real PACS is outside that network — the one +material difference between the two setups, and the reason the Compose deployment publishes the +receiver everywhere (see above): the mocked setup then runs the identical wiring, rather than a +private variant of it. + +.. note:: + + The mocked PACS is for development and testing only. In a trust deployment the imaging comes from + the trust's own PACS, and Orthanc is either absent or confined to the FLIP node — its DICOM port + is deliberately not published to the wider network. + +.. warning:: + + The DICOM port must be **the same number everywhere** — the port XNAT binds, the port recorded on + its SCP receiver, the port advertised as the C-MOVE destination, and the port the PACS connects + to. DQR matches the C-MOVE destination against a registered SCP receiver by exact AE title and + port, so any translation between these layers causes retrieval to fail. + +Example: Sectra PACS +==================== + +The values below illustrate the shape of the exchange with a Sectra PACS. **They are examples, not +defaults** — each trust supplies its own. + +Provided by the trust's PACS team: + +.. code-block:: text + + Query/Retrieve node (PACS) + AE Title: QR_SCP_EXAMPLE + IP: 10.0.0.10 + Port: 8059 + +Provided by FLIP, to be registered on the PACS as a destination: + +.. code-block:: text + + Destination node (FLIP XNAT SCP receiver) + AE Title: FLIPXNAT + IP: 10.0.0.20 + Port: 8104 + +Firewall rules required, in both directions: + +.. list-table:: + :widths: 40 20 40 + :header-rows: 1 + + * - Connection + - Direction + - Purpose + * - XNAT host → PACS query/retrieve port + - Outbound + - C-ECHO, C-FIND, C-MOVE requests + * - PACS → XNAT host DICOM port + - **Inbound** + - C-STORE of the retrieved studies + +.. important:: + + Where the PACS is vendor-managed, opening the trust's own firewall may not be sufficient. The + vendor may operate a separate firewall that must also whitelist the connection, raised through + the trust's service desk as a request to the PACS supplier. + +Scheduling and Throughput +========================= + +A production PACS may limit how much can be retrieved before it refuses further connections, and +bulk retrieval competes with clinical use. Agree a retrieval schedule with the trust's PACS manager, +then configure XNAT to match: the DQR settings control retry behaviour, and each registered PACS +carries an availability schedule with a per-day window, a thread count and a utilisation percentage. + +Where a trust has a test or pre-production PACS, connecting FLIP to that first is recommended, and +is usually raised as a separate service request. + +.. warning:: + + **Verify the window that is actually in force rather than assuming the configured one applied.** + + DQR pre-creates availability intervals when a PACS is registered, and rejects a write to a day + that already has one. On a first deployment the configured values are applied; on any XNAT where + intervals already exist — including one where the PACS has been re-registered, which leaves the + previous intervals behind — the write is rejected and the existing window stands. Changing it + then requires deleting the intervals through XNAT's administration UI. + + Confirm with ``GET /xapi/pacs/{id}/availability`` after configuring. Note also that XNAT + normalises an end time of ``24:00`` to ``00:00`` when it stores it. + + This matters because the window is usually something a trust's PACS manager has agreed to. If it + has not applied, retrieval runs outside it. + +Verification +============ + +Work outwards from the network layer: + +1. **C-ECHO in both directions** — from the PACS to the FLIP XNAT AE title and port, and from XNAT + to the PACS. This confirms both firewall directions before any DICOM data moves. +2. **Ping the PACS from XNAT** — ``GET /xapi/pacs/{id}/status`` should report the PACS as reachable + and enabled. +3. **Query a single accession number** — ``POST /xapi/dqr/query/studies`` should return the matching + study. +4. **Import a single study**, and confirm it archives into the expected project with anonymisation + applied. +5. **Run a full project import**, monitoring the import status counts. + +.. note:: + + XNAT must be restarted for changes to its DICOM configuration to take effect. XNAT also holds the + DICOM port while running, so command-line testing with a tool such as ``storescp`` on the same + port requires stopping XNAT first. diff --git a/docs/source/components/component-xnat.rst b/docs/source/components/component-xnat.rst index c209b605e..919824d37 100644 --- a/docs/source/components/component-xnat.rst +++ b/docs/source/components/component-xnat.rst @@ -8,7 +8,7 @@ This page provides a quick-reference guide to both interactions with the XNAT UI `XNAT `_ is an open-source imaging informatics software platform dedicated to imaging-based research. XNAT's core functions manage importing, archiving, processing and securely distributing imaging and related study data. Detailed documentation on how to use XNAT is `provided on their wiki `_. -Upon FLIP project approval, XNAT project creation tasks are queued for each trust. Trusts poll for these tasks and create the XNAT projects locally. Relevant imaging data is then imported from trust PACS systems. Model developers are granted access to the XNAT project at each trust in order to perform any data preparation and enrichment activities which may be necessary for the running & training of AI models. +Upon FLIP project approval, XNAT project creation tasks are queued for each trust. Trusts poll for these tasks and create the XNAT projects locally. Relevant imaging data is then retrieved from the trust's PACS (see :doc:`PACS `). Model developers are granted access to the XNAT project at each trust in order to perform any data preparation and enrichment activities which may be necessary for the running & training of AI models. ******* XNAT UI @@ -25,7 +25,7 @@ On approval of a FLIP project, any associated users will be granted access to th :width: 500 :align: center - Email sent with XNAT account credentials. + Email sent with XNAT account credentials (password masked). Access ====== @@ -125,10 +125,27 @@ A CT session is derived from an imported PACS DICOM Study. The CT session page c Downloading and Uploading Imaging Data ======================================= -Imaging data will be automatically imported from trust PACS systems on XNAT project generation. Model developers may wish to download this data or upload new or amended imaging data to support model development. +Imaging data is retrieved automatically from the trust's :doc:`PACS ` when the XNAT project is generated. Model developers may wish to download this data or upload new or amended imaging data to support model development. Information on how to download and upload imaging data the XNAT UI can be found `here `_. +***************************** +Retrieval from the Trust PACS +***************************** + +Imaging reaches XNAT by retrieval from the trust's PACS, performed by XNAT's DICOM Query-Retrieve +(DQR) plugin: XNAT queries the PACS for the studies in an approved cohort, and the PACS returns them +for XNAT to archive. + +That connection — what the trust's PACS and network teams must configure, the AE titles and ports +involved, and how to verify it — is described on its own page. + +.. seealso:: + + :doc:`PACS ` — connecting XNAT to a trust PACS. + +.. _dicom-anonymization: + **************************** DICOM Anonymization **************************** diff --git a/docs/source/deploy-flip/deploy-flip-node-on-prem.rst b/docs/source/deploy-flip/deploy-flip-node-on-prem.rst index 044e6bbbe..bbe468ebf 100644 --- a/docs/source/deploy-flip/deploy-flip-node-on-prem.rst +++ b/docs/source/deploy-flip/deploy-flip-node-on-prem.rst @@ -7,7 +7,9 @@ Deploy a FLIP node on-prem An on-prem FLIP node runs the trust-side stack (trust-api, imaging-api, data-access-api, FL client, optional XNAT/Orthanc) on an Ubuntu host owned by the Trust. The node polls the Central Hub for tasks over HTTPS — all -communication is outbound, no inbound ports are opened. This is the deployment +communication with the hub is outbound and no inbound ports are opened to the internet. +Retrieval from the trust's own PACS is the exception — see +:doc:`../components/component-pacs`. This is the deployment model used when the Trust has direct, governed access to its own OMOP database and PACS. For deployment inside a TRE see :doc:`deploy-flip-node-in-tre`; for the Central Hub side see :doc:`deploy-central-hub`. @@ -275,7 +277,8 @@ the operator brings the stack up. Network requirements *********************** -**No inbound port forwarding is needed.** Trusts poll the hub outbound for +**No inbound port forwarding from the internet is needed.** (Retrieval from a trust PACS needs +one rule inside the trust's own network — see :doc:`../components/component-pacs`.) Trusts poll the hub outbound for tasks, and FL clients connect outbound to the FL server via the NLB. All communication is trust-initiated. diff --git a/docs/source/glossary.rst b/docs/source/glossary.rst index 372b867c6..e02170f84 100644 --- a/docs/source/glossary.rst +++ b/docs/source/glossary.rst @@ -31,6 +31,21 @@ Glossary **PACS** Picture Archiving and Communication System (PACS), the clinical system used to store and retrieve medical imaging studies (such as DICOM series). + **AE Title** + Application Entity Title. A DICOM system's name on the network, at most 16 characters. When one system connects to another it announces which AE title it is calling and which it is calling from, and the receiver accepts the connection only if the called title is its own. AE titles are names rather than addresses: the IP and port are configured alongside them. See :doc:`components/component-pacs`. + + **SCU / SCP** + Service Class User and Service Class Provider — DICOM's terms for the two sides of a service. The SCU requests it; the SCP provides it. The roles are per operation, not per system, and can swap mid-exchange: a PACS is the SCP for C-MOVE, then becomes the SCU of the C-STORE it opens back to the destination. XNAT is likewise an SCU when it queries a PACS and an SCP when it receives the images. + + **DIMSE** + DICOM Message Service Element, the classic DICOM network protocol (as opposed to the newer HTTP-based DICOMweb). FLIP retrieves imaging over DIMSE. + + **C-ECHO / C-FIND / C-MOVE / C-STORE** + The DIMSE operations FLIP uses. ``C-ECHO`` is a connectivity check. ``C-FIND`` searches a PACS, in FLIP's case by accession number. ``C-MOVE`` asks the PACS to send a study to a named destination. ``C-STORE`` is the image transfer itself — and because C-MOVE names a destination rather than returning data inline, the C-STORE arrives on a *new* connection opened by the PACS back to that destination. + + **DQR** + DICOM Query-Retrieve, the XNAT plugin that performs the C-FIND and C-MOVE operations against a trust PACS on FLIP's behalf. + **RBAC** Role Based Access Control (RBAC) defines what users are able to access within the FLIP platform. diff --git a/docs/source/governance-and-compliance.rst b/docs/source/governance-and-compliance.rst index 6359516ac..062e8f027 100644 --- a/docs/source/governance-and-compliance.rst +++ b/docs/source/governance-and-compliance.rst @@ -75,11 +75,13 @@ Network architecture as a governance guarantee ********************************************** The network design is why the guarantees above are structural rather than procedural. -Trust systems accept no inbound connections: each trust polls the Central Hub outbound, -there are no inbound firewall rules to open, and there is no route from the internet — or +Each trust polls the Central Hub outbound, and there is no route from the internet — or from the hub — into a trust's network. For a trust's own network team, onboarding FLIP -requires no inbound exposure at all. A site-to-site VPN can be provisioned on request -where a trust's policy calls for network-layer separation as well. +requires no inbound exposure to the outside world. Where FLIP is connected to the trust's +own PACS, one inbound rule is needed *inside* the trust: FLIP asks the PACS for a study, +and the PACS opens a connection back to XNAT to deliver it, on the DICOM port alone. +A site-to-site VPN can be provisioned on request where a trust's policy calls for +network-layer separation as well. The practical governance point: a trust does not have to rely on the Central Hub's access controls to be confident its systems are unreachable. There is no path. diff --git a/docs/source/security.rst b/docs/source/security.rst index 66613cade..4bee86a26 100644 --- a/docs/source/security.rst +++ b/docs/source/security.rst @@ -24,13 +24,19 @@ automated checks that run against every change are publicly inspectable. Network and perimeter ********************* -**Trust systems accept no inbound connections.** Each participating trust runs FLIP -services that reach *out* to the Central Hub to collect work and report results. -Nothing on the internet can open a connection to a trust's FLIP services. This is -enforced in the infrastructure definitions themselves — the security groups permit no -inbound traffic at all — rather than depending on configuration discipline. -Operator access is via AWS Systems Manager Session Manager, so port 22 is never -opened. +**Trust systems accept no inbound connections from the internet or the Central Hub.** +Each participating trust runs FLIP services that reach *out* to the Central Hub to +collect work and report results. This is enforced in the infrastructure definitions +themselves — the AWS trust security groups define no ingress rules at all — rather +than depending on configuration discipline. Operator access is via AWS Systems Manager +Session Manager, so port 22 is never opened. + +The one inbound connection in the design applies only where FLIP is connected to a +trust's own PACS, which today means an on-premises deployment. FLIP asks the PACS for a +study, and the PACS opens a connection back to XNAT to deliver it, on the DICOM port +alone. It stays inside the trust's own network. AWS-hosted trusts have no such path: +they run FLIP's own bundled Orthanc, which XNAT reaches over the container network, so +their security groups keep no ingress rules at all. **Only the Central Hub is internet-facing.** It sits behind CloudFront with modern TLS, HSTS, AWS WAF managed rules, and an internal-only Application Load Balancer. Nothing diff --git a/docs/source/sys-admin/admin-platform-support.rst b/docs/source/sys-admin/admin-platform-support.rst index 61003cb23..116b5c318 100644 --- a/docs/source/sys-admin/admin-platform-support.rst +++ b/docs/source/sys-admin/admin-platform-support.rst @@ -8,7 +8,8 @@ Networking All trust communication is **outbound** — trusts poll the Central Hub for tasks over HTTPS (via the ALB), and FL clients connect outbound to the FL server via the NLB. The hub never -makes inbound connections to trusts, so no inbound firewall rules or port forwarding are +makes inbound connections to trusts, so no inbound firewall rules or port forwarding from the +internet are required on trust hosts. Operator access is via AWS Systems Manager Session Manager (SSH-over-SSM); XNAT, Orthanc, and the trust-api Swagger docs are reachable only through SSM port forwarding (``make forward-trust``). Orthanc additionally requires HTTP basic auth — log in @@ -36,9 +37,12 @@ opened. * - Description - Inbound - Outbound - * - DICOM ingestion from local PACS into the trust XNAT + * - DICOM query/retrieve from the local PACS (XNAT → PACS) + - + - local PACS query/retrieve port + * - DICOM C-STORE return leg (PACS → XNAT) + - XNAT DICOM port (``XNAT_PORT``, 8104 by default) - - - local PACS DICOM ports * - Trust → Central Hub task polling (HTTPS) - - 443 diff --git a/flip-api/tests/demo_video.py b/flip-api/tests/demo_video.py index a9f6b3fdd..9be99d8a7 100644 --- a/flip-api/tests/demo_video.py +++ b/flip-api/tests/demo_video.py @@ -158,7 +158,7 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: parser.add_argument("--skip-xnat", action="store_true", help="Skip the XNAT/OHIF segment") parser.add_argument( "--xnat-url", - default="http://127.0.0.1:8104", + default="http://127.0.0.1:8105", help=( "Trust XNAT base URL for segment 3. Keep the IPv4 literal: the XNAT ports are published by " "Docker Swarm ingress, which accepts but never answers ::1 connections — python-requests " @@ -358,7 +358,7 @@ def resolve_xnat_ids( ``?subjectId=&projectId=&experimentId=&experimentLabel=``). Args: - xnat_url (str): Base URL of the trust's XNAT (e.g. http://localhost:8104). + xnat_url (str): Base URL of the trust's XNAT (e.g. http://localhost:8105). username (str): XNAT login. password (str): XNAT password. flip_project_id (str): FLIP project UUID to match against secondary_ID. diff --git a/flip-api/tests/xnat_seg_upload.py b/flip-api/tests/xnat_seg_upload.py index b15472dbe..30d8de5b8 100644 --- a/flip-api/tests/xnat_seg_upload.py +++ b/flip-api/tests/xnat_seg_upload.py @@ -47,7 +47,7 @@ # Both dev trust XNATs. IPv4 literals on purpose: the XNAT ports are published by Docker # Swarm ingress, which accepts but never answers ::1, and localhost resolves there first. -DEFAULT_XNAT_URLS = ("http://127.0.0.1:8104", "http://127.0.0.1:8106") +DEFAULT_XNAT_URLS = ("http://127.0.0.1:8105", "http://127.0.0.1:8107") DCMQI_IMAGE = "qiicr/dcmqi:v1.5.6" ROI_COLLECTION_TYPE = "icr:roiCollectionData" diff --git a/flip-ui/test/cypress/demo/03-xnat-ohif.spec.ts b/flip-ui/test/cypress/demo/03-xnat-ohif.spec.ts index f569a53b5..122f08e0b 100644 --- a/flip-ui/test/cypress/demo/03-xnat-ohif.spec.ts +++ b/flip-ui/test/cypress/demo/03-xnat-ohif.spec.ts @@ -13,7 +13,7 @@ // Demo segment 3 — inside one trust: the imported cohort has landed in XNAT, // and a study is opened in the OHIF DICOM viewer. This segment runs with -// CYPRESS_BASE_URL pointed at the trust's XNAT (e.g. http://localhost:8104); +// CYPRESS_BASE_URL pointed at the trust's XNAT (e.g. http://localhost:8105); // the orchestrator resolves the XNAT project (matched on secondary_ID == // FLIP project id) and an experiment id before launching it. Navigating // straight to /VIEWER/ keeps OHIF in the recorded tab — the XNAT UI's own diff --git a/scripts/check_local_status.py b/scripts/check_local_status.py index e99baa25b..bbadb11ea 100755 --- a/scripts/check_local_status.py +++ b/scripts/check_local_status.py @@ -441,7 +441,10 @@ class TrustKit: name (str): TRUST_NAME for display; falls back to the CODE when absent. slot_number (int | None): Assigned FL kit slot number (FL_KIT_SLOT_NUMBER). Drives the XNAT stack name (xnat) and which trust exposes host APIs. - xnat_port (str | None): Host port for the trust's XNAT web UI. + xnat_web_port (str | None): Host port for the trust's XNAT web UI. Read from + XNAT_WEB_PORT, falling back to XNAT_PORT — the same precedence trust/xnat/Makefile + applies. Those were one variable until FLIP#993 split the DICOM listener off; a kit + that sets only XNAT_PORT still publishes the web UI there. pacs_ui_port (str | None): Host port for the trust's Orthanc PACS UI. trust_api_port (str | None): Host port for trust-api (slot-1 trust only). imaging_api_port (str | None): Host port for imaging-api (slot-1 trust only). @@ -451,7 +454,7 @@ class TrustKit: code: str name: str slot_number: int | None - xnat_port: str | None + xnat_web_port: str | None pacs_ui_port: str | None trust_api_port: str | None imaging_api_port: str | None @@ -502,7 +505,7 @@ def discover_trust_kits(trust_dir: Path, env: str) -> list[TrustKit]: code=code, name=env_vars.get("TRUST_NAME") or code, slot_number=slot_number, - xnat_port=env_vars.get("XNAT_PORT"), + xnat_web_port=env_vars.get("XNAT_WEB_PORT") or env_vars.get("XNAT_PORT"), pacs_ui_port=env_vars.get("PACS_UI_PORT"), trust_api_port=env_vars.get("TRUST_API_PORT"), imaging_api_port=env_vars.get("IMAGING_API_PORT"), @@ -926,8 +929,8 @@ def main( for kit in trust_kits: slot = f" slot {kit.slot_number}" if kit.slot_number else "" # 127.0.0.1 (not localhost) avoids IPv6 routing issues with Docker Swarm. - if kit.xnat_port: - xnat_url = f"http://127.0.0.1:{kit.xnat_port}" + if kit.xnat_web_port: + xnat_url = f"http://127.0.0.1:{kit.xnat_web_port}" check_http_endpoint(xnat_url, f"XNAT {kit.name}{slot} Web UI", [200, 302]) if kit.pacs_ui_port: pacs_url = f"http://localhost:{kit.pacs_ui_port}" diff --git a/scripts/tests/test_check_local_status.py b/scripts/tests/test_check_local_status.py index f871cf27a..fa2eb935c 100644 --- a/scripts/tests/test_check_local_status.py +++ b/scripts/tests/test_check_local_status.py @@ -98,10 +98,26 @@ def test_discovers_code_kits() -> None: _assert("KCH" in by_code and "GSTT" in by_code, "keyed by CODE", f"got {sorted(by_code)}") kch = by_code.get("KCH", TrustKit("", "", None, None, None, None, None, None)) _assert(kch.slot_number == 1, "reads FL_KIT_SLOT_NUMBER", f"got {kch.slot_number!r}") - _assert(kch.xnat_port == "8106" and kch.pacs_ui_port == "8044", "reads XNAT/PACS ports") + _assert(kch.xnat_web_port == "8106" and kch.pacs_ui_port == "8044", "reads XNAT/PACS ports") _assert(kch.trust_api_port == "8020", "reads TRUST_API_PORT", f"got {kch.trust_api_port!r}") +def test_web_port_prefers_xnat_web_port() -> None: + """The web UI moved off XNAT_PORT in FLIP#993; probing the DICOM port would report it down.""" + trust = _trust_dir() + _write_kit(trust, "GSTT", "development", TRUST_NAME="GSTT", XNAT_PORT=8104, XNAT_WEB_PORT=8080) + kits = discover_trust_kits(trust, "development") + _assert(kits[0].xnat_web_port == "8080", "XNAT_WEB_PORT wins over XNAT_PORT", f"got {kits[0].xnat_web_port!r}") + + +def test_web_port_falls_back_to_xnat_port() -> None: + """A kit predating the split sets only XNAT_PORT, and still publishes the web UI there.""" + trust = _trust_dir() + _write_kit(trust, "GSTT", "development", TRUST_NAME="GSTT", XNAT_PORT=8104) + kits = discover_trust_kits(trust, "development") + _assert(kits[0].xnat_web_port == "8104", "falls back to XNAT_PORT", f"got {kits[0].xnat_web_port!r}") + + def test_ignores_examples_and_other_envs() -> None: trust = _trust_dir() _write_kit(trust, "KCH", "development", TRUST_NAME="KCH", FL_KIT_SLOT_NUMBER=1) @@ -227,15 +243,13 @@ def test_parse_compose_containers() -> None: def main() -> None: - for test in ( - test_discovers_code_kits, - test_ignores_examples_and_other_envs, - test_missing_slot_and_name_fallback, - test_missing_dir_returns_empty, - test_instance_prefix, - test_compose_project, - test_parse_compose_containers, - ): + # Discovered rather than listed: the hand-maintained tuple this replaced meant a new test + # function ran nowhere and the suite still reported green. + tests = [v for k, v in list(globals().items()) if k.startswith("test_") and callable(v)] + if not tests: + print("no tests discovered") + sys.exit(1) + for test in tests: print(f"\n{test.__name__}") test() print(f"\n{PASS} passed, {FAIL} failed") diff --git a/trust/.env.GSTT.development.example b/trust/.env.GSTT.development.example index 18165e5a2..d8428d0e3 100644 --- a/trust/.env.GSTT.development.example +++ b/trust/.env.GSTT.development.example @@ -12,7 +12,13 @@ TRUST_REGION=London # ── Host-local profile ──────────────────────────────────────────────────── OMOP_DB_PORT=5434 PACS_UI_PORT=8042 +# XNAT_PORT is the DICOM SCP receiver port; XNAT_WEB_PORT is the host-published web UI. They were +# one variable until FLIP#993. Both are host-published — the receiver so a real PACS can complete +# the C-STORE return leg of a retrieval, and dev keeps the same wiring — so they must differ; the +# deploy refuses if they collide. XNAT_PORT is the number the PACS dials and DQR matches exactly, +# so it keeps the canonical 8104 and the web UI takes the next port up. XNAT_PORT=8104 +XNAT_WEB_PORT=8105 TRUST_DEBUG_PORT=5682 IMAGING_DEBUG_PORT=5681 DATA_ACCESS_DEBUG_PORT=5680 @@ -71,7 +77,36 @@ TRUST_INTERNAL_SERVICE_KEY_HEADER=X-Trust-Internal-Service-Key GRAFANA_ADMIN_PASSWORD=admin -PACS_DICOM_PORT=4242 +# ── Upstream PACS ───────────────────────────────────────────────────────── +# Defaults describe the mocked Orthanc that ships for development. A real trust points these at its +# PACS; see docs/source/components/component-pacs.rst. +# +# XNAT_AETITLE is XNAT's own AE title, applied to its DICOM SCP receiver, the DQR calling AE, and the +# C-MOVE destination handed to the PACS. It must match the AE title the PACS has registered for us, +# because the PACS opens the C-STORE association addressed to that title. +# +# PACS_QR_PORT is the port XNAT dials, and must be reachable *from the XNAT container* — not a +# host-published port. Getting that wrong is what the retired PACS_DICOM_PORT variable did +# (FLIP#822 / FLIP#862). (PACS_ID is read from XNAT at runtime as the sole registration; it remains only as the +# fallback for when XNAT is unreachable, and is not a kit field.) +XNAT_AETITLE=XNAT +PACS_HOST=orthanc +PACS_AETITLE=ORTHANC +PACS_QR_PORT=4242 +PACS_LABEL=Test PACS instance +# Relational queries / extended negotiation. A per-PACS capability, not a preference: a PACS that +# does not support it rejects the association unless this is false. Ask the PACS team. +PACS_SUPPORTS_EXTENDED_NEGOTIATIONS=true + +# PACS throttle. A production PACS may refuse further associations after a certain volume, and a +# trust may want retrieval confined to out-of-hours. Agree the window with the PACS manager. +PACS_AVAILABILITY_DAYS=MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY +PACS_AVAILABILITY_START=00:00 +PACS_AVAILABILITY_END=24:00 +PACS_THREADS=1 +PACS_UTILIZATION_PERCENT=100 +DQR_MAX_PACS_REQUEST_ATTEMPTS=100 +DQR_RETRY_WAIT_SECONDS=300 POLL_INTERVAL_SECONDS=5 diff --git a/trust/.env.KCH.development.example b/trust/.env.KCH.development.example index 154239b0a..12a9e074e 100644 --- a/trust/.env.KCH.development.example +++ b/trust/.env.KCH.development.example @@ -12,7 +12,12 @@ TRUST_REGION=London # ── Host-local profile ──────────────────────────────────────────────────── OMOP_DB_PORT=5436 PACS_UI_PORT=8044 +# XNAT_PORT is the DICOM SCP receiver port; XNAT_WEB_PORT is the host-published web UI. They were +# one variable until FLIP#993. Both are host-published, so they must differ; the deploy refuses if +# they collide. The DICOM receiver keeps this trust's established 8106 and the web UI takes the +# next port up. XNAT_PORT=8106 +XNAT_WEB_PORT=8107 TRUST_DEBUG_PORT=5685 IMAGING_DEBUG_PORT=5684 DATA_ACCESS_DEBUG_PORT=5683 @@ -71,7 +76,36 @@ TRUST_INTERNAL_SERVICE_KEY_HEADER=X-Trust-Internal-Service-Key GRAFANA_ADMIN_PASSWORD=admin -PACS_DICOM_PORT=4242 +# ── Upstream PACS ───────────────────────────────────────────────────────── +# Defaults describe the mocked Orthanc that ships for development. A real trust points these at its +# PACS; see docs/source/components/component-pacs.rst. +# +# XNAT_AETITLE is XNAT's own AE title, applied to its DICOM SCP receiver, the DQR calling AE, and the +# C-MOVE destination handed to the PACS. It must match the AE title the PACS has registered for us, +# because the PACS opens the C-STORE association addressed to that title. +# +# PACS_QR_PORT is the port XNAT dials, and must be reachable *from the XNAT container* — not a +# host-published port. Getting that wrong is what the retired PACS_DICOM_PORT variable did +# (FLIP#822 / FLIP#862). (PACS_ID is read from XNAT at runtime as the sole registration; it remains only as the +# fallback for when XNAT is unreachable, and is not a kit field.) +XNAT_AETITLE=XNAT +PACS_HOST=orthanc +PACS_AETITLE=ORTHANC +PACS_QR_PORT=4242 +PACS_LABEL=Test PACS instance +# Relational queries / extended negotiation. A per-PACS capability, not a preference: a PACS that +# does not support it rejects the association unless this is false. Ask the PACS team. +PACS_SUPPORTS_EXTENDED_NEGOTIATIONS=true + +# PACS throttle. A production PACS may refuse further associations after a certain volume, and a +# trust may want retrieval confined to out-of-hours. Agree the window with the PACS manager. +PACS_AVAILABILITY_DAYS=MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY +PACS_AVAILABILITY_START=00:00 +PACS_AVAILABILITY_END=24:00 +PACS_THREADS=1 +PACS_UTILIZATION_PERCENT=100 +DQR_MAX_PACS_REQUEST_ATTEMPTS=100 +DQR_RETRY_WAIT_SECONDS=300 POLL_INTERVAL_SECONDS=5 diff --git a/trust/.env.example b/trust/.env.example index 21f69cc1b..71cf7af52 100644 --- a/trust/.env.example +++ b/trust/.env.example @@ -22,7 +22,13 @@ # ── Host-local profile ──────────────────────────────────────────────────── OMOP_DB_PORT=5434 PACS_UI_PORT=8042 +# XNAT_PORT is the DICOM SCP receiver port; XNAT_WEB_PORT is the host-published web UI. They were +# one variable until FLIP#993. Both are host-published — the receiver so a real PACS can complete +# the C-STORE return leg of a retrieval, and dev keeps the same wiring — so they must differ; the +# deploy refuses if they collide. XNAT_PORT is the number the PACS dials and DQR matches exactly, +# so it keeps the canonical 8104 and the web UI takes the next port up. XNAT_PORT=8104 +XNAT_WEB_PORT=8105 TRUST_DEBUG_PORT=5682 IMAGING_DEBUG_PORT=5681 DATA_ACCESS_DEBUG_PORT=5680 @@ -101,9 +107,36 @@ TRUST_INTERNAL_SERVICE_KEY_HEADER=X-Trust-Internal-Service-Key # Grafana GRAFANA_ADMIN_PASSWORD=admin -# PACS DICOM listening port. (PACS_ID — the id of the single XNAT-registered -# PACS — is always 1 and now defaults in imaging-api config; not a kit field.) -PACS_DICOM_PORT=4242 +# ── Upstream PACS ───────────────────────────────────────────────────────── +# Defaults describe the mocked Orthanc that ships for development. A real trust points these at its +# PACS; see docs/source/components/component-pacs.rst. +# +# XNAT_AETITLE is XNAT's own AE title, applied to its DICOM SCP receiver, the DQR calling AE, and the +# C-MOVE destination handed to the PACS. It must match the AE title the PACS has registered for us, +# because the PACS opens the C-STORE association addressed to that title. +# +# PACS_QR_PORT is the port XNAT dials, and must be reachable *from the XNAT container* — not a +# host-published port. Getting that wrong is what the retired PACS_DICOM_PORT variable did +# (FLIP#822 / FLIP#862). (PACS_ID is read from XNAT at runtime as the sole registration; it remains only as the +# fallback for when XNAT is unreachable, and is not a kit field.) +XNAT_AETITLE=XNAT +PACS_HOST=orthanc +PACS_AETITLE=ORTHANC +PACS_QR_PORT=4242 +PACS_LABEL=Test PACS instance +# Relational queries / extended negotiation. A per-PACS capability, not a preference: a PACS that +# does not support it rejects the association unless this is false. Ask the PACS team. +PACS_SUPPORTS_EXTENDED_NEGOTIATIONS=true + +# PACS throttle. A production PACS may refuse further associations after a certain volume, and a +# trust may want retrieval confined to out-of-hours. Agree the window with the PACS manager. +PACS_AVAILABILITY_DAYS=MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY +PACS_AVAILABILITY_START=00:00 +PACS_AVAILABILITY_END=24:00 +PACS_THREADS=1 +PACS_UTILIZATION_PERCENT=100 +DQR_MAX_PACS_REQUEST_ATTEMPTS=100 +DQR_RETRY_WAIT_SECONDS=300 # trust-api -> hub poll cadence (seconds). POLL_INTERVAL_SECONDS=5 diff --git a/trust/AGENTS.md b/trust/AGENTS.md index 0052fd57c..513a97ece 100644 --- a/trust/AGENTS.md +++ b/trust/AGENTS.md @@ -12,7 +12,7 @@ Trust services run at each healthcare institution (cloud EC2 or on-prem). All tr | fl-client | — | FL participant (connects outbound to FL server via NLB) | | omop-db | 5432 | Mocked OMOP patient database (PostgreSQL); dir also holds the image build source + populate tooling (#834, see `omop-db/AGENTS.md`) | | orthanc | 8042 | Mocked DICOM PACS server (UI/REST behind HTTP basic auth — kit file's `ORTHANC_USERNAME`/`ORTHANC_PASSWORD`; DICOM port 4242 is internal to the trust network and not bound to the host) | -| xnat | 8104 | Mocked neuroimaging platform | +| xnat | 8104/8105 | Mocked neuroimaging platform. `XNAT_PORT` (8104) is the **DICOM SCP receiver** port; `XNAT_WEB_PORT` (8105) is the web UI. Both are host-published — the receiver so a real PACS can C-STORE back in, dev included so it runs the same wiring — so the two must differ; the deploy refuses a collision (FLIP#993) | | observability | 3000/3100 | Grafana + Loki monitoring stack | ## Kit file structure diff --git a/trust/CLAUDE.md b/trust/CLAUDE.md index 28710d842..e110be24e 100644 --- a/trust/CLAUDE.md +++ b/trust/CLAUDE.md @@ -12,7 +12,7 @@ Trust services run at each healthcare institution (cloud EC2 or on-prem). All tr | fl-client | — | FL participant (connects outbound to FL server via NLB) | | omop-db | 5432 | Mocked OMOP patient database (PostgreSQL); dir also holds the image build source + populate tooling (#834, see `omop-db/CLAUDE.md`) | | orthanc | 8042 | Mocked DICOM PACS server (UI/REST behind HTTP basic auth — kit file's `ORTHANC_USERNAME`/`ORTHANC_PASSWORD`; DICOM port 4242 is internal to the trust network and not bound to the host) | -| xnat | 8104 | Mocked neuroimaging platform | +| xnat | 8104/8105 | Mocked neuroimaging platform. `XNAT_PORT` (8104) is the **DICOM SCP receiver** port; `XNAT_WEB_PORT` (8105) is the web UI. Both are host-published — the receiver so a real PACS can C-STORE back in, dev included so it runs the same wiring — so the two must differ; the deploy refuses a collision (FLIP#993) | | observability | 3000/3100 | Grafana + Loki monitoring stack | ## Kit file structure diff --git a/trust/deploy/compose_trust.development.yml b/trust/deploy/compose_trust.development.yml index 439af77ad..cf0f7c1e9 100644 --- a/trust/deploy/compose_trust.development.yml +++ b/trust/deploy/compose_trust.development.yml @@ -58,7 +58,6 @@ services: dockerfile: Dockerfile restart: always ports: - # - "${PACS_DICOM_PORT}:4242" - "${PACS_UI_PORT}:8042" security_opt: - no-new-privileges:true @@ -72,7 +71,7 @@ services: ORTHANC__REGISTERED_USERS: | {"${ORTHANC_USERNAME}": "${ORTHANC_PASSWORD}"} ORTHANC__DICOM_MODALITIES: | - {"XNAT": {"AET": "XNAT", "Host": "xnat-web", "Port": "${XNAT_PORT}"}} + {"XNAT": {"AET": "${XNAT_AETITLE:-XNAT}", "Host": "xnat-web", "Port": "${XNAT_PORT}"}} imaging-api: # Pull from GHCR by default; local imaging_api/ bind-mount overlays it for @@ -106,6 +105,9 @@ services: # default in imaging_api/config.py — do not inject them (an empty ${VAR} # from a kit that omits them would override the code default). XNAT_PORT: ${XNAT_PORT} + # Must match what configure-xnat.sh registered: this becomes the C-MOVE destination, + # which DQR matches against a registered SCP receiver by exact AE title and port. + XNAT_AETITLE: ${XNAT_AETITLE:-XNAT} XNAT_SERVICE_USER: ${XNAT_SERVICE_USER} XNAT_SERVICE_PASSWORD: ${XNAT_SERVICE_PASSWORD} # Minted per-trust XNAT DB password (FLIP-PT-056): imaging_api/config.py diff --git a/trust/deploy/compose_trust.production.yml b/trust/deploy/compose_trust.production.yml index 1a62ac6a8..398448465 100644 --- a/trust/deploy/compose_trust.production.yml +++ b/trust/deploy/compose_trust.production.yml @@ -50,7 +50,6 @@ services: image: ghcr.io/londonaicentre/orthanc:${DOCKER_TAG} restart: always ports: - # - "${PACS_DICOM_PORT}:4242" - "${PACS_UI_PORT}:8042" security_opt: - no-new-privileges:true @@ -71,7 +70,7 @@ services: ORTHANC__REGISTERED_USERS: | {"${ORTHANC_USERNAME}": "${ORTHANC_PASSWORD}"} ORTHANC__DICOM_MODALITIES: | - {"XNAT": {"AET": "XNAT", "Host": "xnat-web", "Port": "${XNAT_PORT}"}} + {"XNAT": {"AET": "${XNAT_AETITLE:-XNAT}", "Host": "xnat-web", "Port": "${XNAT_PORT}"}} imaging-api: image: ghcr.io/londonaicentre/imaging-api:${DOCKER_TAG} @@ -94,6 +93,9 @@ services: # default in imaging_api/config.py — do not inject them (an empty ${VAR} # from a kit that omits them would override the code default). XNAT_PORT: ${XNAT_PORT} + # Must match what configure-xnat.sh registered: this becomes the C-MOVE destination, + # which DQR matches against a registered SCP receiver by exact AE title and port. + XNAT_AETITLE: ${XNAT_AETITLE:-XNAT} XNAT_SERVICE_USER: ${XNAT_SERVICE_USER} XNAT_SERVICE_PASSWORD: ${XNAT_SERVICE_PASSWORD} # Minted per-trust XNAT DB password (FLIP-PT-056): imaging_api/config.py diff --git a/trust/imaging-api/README.md b/trust/imaging-api/README.md index 2a462e8ae..956a64873 100644 --- a/trust/imaging-api/README.md +++ b/trust/imaging-api/README.md @@ -60,7 +60,7 @@ Download and unzip a XNAT dataset to a local folder. ### Imaging Interfaces with XNAT's DICOM Query-Retrieve (DQR) plugin. Full DQR API docs available at -`http://127.0.0.1:8104/xapi/swagger-ui.html#/dicom-query-retrieve-api`. +`http://127.0.0.1:8105/xapi/swagger-ui.html#/dicom-query-retrieve-api`. - Query PACS with an accession number - Queue image retrieval from PACS to an XNAT project diff --git a/trust/imaging-api/imaging_api/config.py b/trust/imaging-api/imaging_api/config.py index 85cb1cb38..c714e7808 100644 --- a/trust/imaging-api/imaging_api/config.py +++ b/trust/imaging-api/imaging_api/config.py @@ -40,8 +40,13 @@ def coerce_empty_env(cls, v: str) -> str: # XNAT_PORT: int - # XNAT registers exactly one PACS (configure-xnat.sh), which it assigns id 1, - # so this defaults to 1 rather than being a required, mis-settable kit field. + # XNAT's own AE title. Must match XNAT_AETITLE in configure-xnat.sh, because this value becomes + # the C-MOVE destination handed to the PACS and DQR matches it against a registered SCP receiver + # by exact AE title and port (FLIP#993). + XNAT_AETITLE: str = "XNAT" + # A trust XNAT retrieves from exactly one PACS, and configure-xnat.sh enforces that, so the id + # is resolved at runtime as "the registered one" rather than assumed. This value is only the + # fallback for when XNAT cannot be reached: historically XNAT registered one PACS as id 1. PACS_ID: int = 1 # Internal trust-network URLs: docker service name + the service's container diff --git a/trust/imaging-api/imaging_api/routers/imaging.py b/trust/imaging-api/imaging_api/routers/imaging.py index 3003bdf85..d36eead73 100644 --- a/trust/imaging-api/imaging_api/routers/imaging.py +++ b/trust/imaging-api/imaging_api/routers/imaging.py @@ -17,6 +17,7 @@ from imaging_api.routers.schemas import ImportStudyRequest, ImportStudyResponse, PacsStatus, Study from imaging_api.services.imaging import ( ping_pacs, + ping_registered_pacs, query_by_accession_number, queue_image_import_request, ) @@ -29,14 +30,21 @@ XNATAuthHeaders = Annotated[dict[str, str], Depends(get_xnat_auth_headers)] +@router.get("/ping_pacs", summary="Ping the registered Imaging Provider (PACS)") @router.get("/ping_pacs/{pacs_id}", summary="Ping Imaging Provider (PACS) by ID") -def ping_pacs_endpoint(pacs_id: int, headers: XNATAuthHeaders) -> PacsStatus: - """ - Pings the imaging provider (PACS) to check if it is reachable. +def ping_pacs_endpoint(headers: XNATAuthHeaders, pacs_id: int | None = None) -> PacsStatus: + """Pings the imaging provider (PACS) to check if it is reachable. + + Two routes, one handler. Without an id the PACS is resolved from XNAT the same way the import + path resolves it — the id XNAT assigns at registration is not knowable in advance, so a caller + that only wants to know whether the trust's PACS answers should not have to guess one. A stale + cached id (the PACS was re-registered while imaging-api stayed up) is dropped and re-resolved + once rather than pinning the probe to a dead registration. The by-id route stays for callers + that genuinely mean a specific registration. Args: - pacs_id (int): PACS ID to ping. headers (XNATAuthHeaders): XNAT authentication headers. + pacs_id (int | None): PACS ID to ping. Resolved from XNAT when omitted. Returns: PacsStatus: Status of the PACS system. @@ -45,7 +53,7 @@ def ping_pacs_endpoint(pacs_id: int, headers: XNATAuthHeaders) -> PacsStatus: HTTPException: If PACS is not found or if there is an error during the ping operation. """ try: - return ping_pacs(pacs_id, headers) + return ping_registered_pacs(headers) if pacs_id is None else ping_pacs(pacs_id, headers) except NotFoundError as e: raise HTTPException(status_code=404, detail=str(e)) except Exception as e: diff --git a/trust/imaging-api/imaging_api/routers/schemas.py b/trust/imaging-api/imaging_api/routers/schemas.py index 4b094e407..8e94ecc73 100644 --- a/trust/imaging-api/imaging_api/routers/schemas.py +++ b/trust/imaging-api/imaging_api/routers/schemas.py @@ -20,6 +20,7 @@ PACS_ID = get_settings().PACS_ID XNAT_PORT = get_settings().XNAT_PORT +XNAT_AETITLE = get_settings().XNAT_AETITLE # Blocks the three characters most likely to enable structural XML injection # in the XNAT projectData payload built by imaging_api.services.projects. This @@ -236,7 +237,9 @@ class ImportStudyRequest(BaseModel): """Represents an image import request for DQR.""" pacs_id: int = Field(default=PACS_ID, alias="pacsId") - ae_title: str = Field(default="XNAT", alias="aeTitle") # XNAT + # The C-MOVE destination the PACS is told to send to. DQR matches this against a registered + # SCP receiver by exact AE title and port, so it must equal what configure-xnat.sh registered. + ae_title: str = Field(default=XNAT_AETITLE, alias="aeTitle") port: int = Field(default=XNAT_PORT, alias="port") project_id: str = Field(..., alias="projectId") force_import: bool = Field(default=True, alias="forceImport") diff --git a/trust/imaging-api/imaging_api/services/imaging.py b/trust/imaging-api/imaging_api/services/imaging.py index f4c1054f2..bc495a71b 100644 --- a/trust/imaging-api/imaging_api/services/imaging.py +++ b/trust/imaging-api/imaging_api/services/imaging.py @@ -28,6 +28,136 @@ PACS_ID = get_settings().PACS_ID XNAT_URL = get_settings().XNAT_URL +# Bound every XNAT call: an unbounded request to a wedged XNAT would hang the import (and the +# health probe) rather than failing. The DQR data-path calls get a far larger bound because they +# proxy live DIMSE operations against the upstream PACS — a throttled or slow real PACS can hold +# a C-FIND well past what any metadata round-trip to XNAT itself would need. +XNAT_REQUEST_TIMEOUT = 30 +XNAT_DQR_REQUEST_TIMEOUT = 300 + +# Cache for resolve_pacs_id(). XNAT assigns a PACS its id at registration time, so the id is fixed +# for the life of that registration and re-resolving on every query would add an XNAT round-trip per +# accession number. Note the cache outlives the registration: if the PACS is re-registered while +# imaging-api stays up — configure-xnat.sh deletes and recreates it when the AE title changes — the +# cached id points at a deleted entry, so it is cleared when XNAT reports the PACS missing. +# Deliberately unlocked: routes here are sync defs dispatched to a threadpool, so two cold-cache +# threads can race the read-check-set — but both resolve the same fixed id from the same XNAT +# roster, so the loser merely repeats one metadata round-trip. A lock would serialise every request +# to save that one call. +_resolved_pacs_id: int | None = None + + +def resolve_pacs_id(headers: dict[str, str]) -> int: + """ + Resolves the id of the PACS registered in XNAT. + + A trust XNAT retrieves from exactly one PACS, and ``configure-xnat.sh`` enforces that by removing + any other registration. The id itself cannot be assumed: XNAT numbers registrations in creation + order, so an XNAT that carried the mocked Orthanc before the real PACS was configured does not + have it at id 1 (FLIP#993). + + Falls back to the configured ``PACS_ID`` when XNAT cannot be reached or reports no PACS, so a + transient XNAT failure degrades to the previous behaviour rather than failing the import. + + Args: + headers (dict[str, str]): XNAT authentication headers. + + Returns: + int: The id of the registered PACS, or the configured ``PACS_ID`` fallback. + """ + global _resolved_pacs_id + if _resolved_pacs_id is not None: + return _resolved_pacs_id + + try: + response = requests.get(f"{XNAT_URL}/xapi/pacs", headers=headers, timeout=XNAT_REQUEST_TIMEOUT) + response.raise_for_status() + registrations = response.json() + except Exception as e: + # Deliberately not cached: a transient failure must not pin the fallback for the life of the + # process. Logged at error because the fallback is a guess — on an XNAT that carried the + # mocked Orthanc before the real PACS, id 1 is the known-wrong answer, and the symptom is + # "no study found" for every accession, which reads as a data problem rather than a + # misconfiguration (FLIP#993). + logger.error( + f"Could not resolve the PACS id from XNAT ({e}); falling back to id {PACS_ID}. " + f"Retrieval will target that id, which may not be the configured PACS." + ) + return PACS_ID + + if not isinstance(registrations, list) or not registrations: + logger.error( + f"XNAT reports no registered PACS; falling back to id {PACS_ID}. " + f"Retrieval will target that id, which may not be the configured PACS." + ) + return PACS_ID + + # configure-xnat.sh sets defaultQueryRetrievePacs on the one it owns and removes any other, so + # prefer that flag over list order — an extra registration added through the XNAT UI would + # otherwise be picked purely because XNAT happened to list it first. + default_qr = [p for p in registrations if p.get("defaultQueryRetrievePacs")] + chosen = (default_qr or registrations)[0] + + if len(registrations) > 1: + logger.warning( + f"XNAT has {len(registrations)} PACS registrations; expected one. " + f"Using '{chosen.get('aeTitle')}'" + f"{' (the default query/retrieve PACS)' if default_qr else ' (first listed)'}." + ) + + try: + _resolved_pacs_id = int(chosen["id"]) + except (KeyError, TypeError, ValueError) as e: + logger.error(f"PACS registration from XNAT has no usable id ({e}); falling back to id {PACS_ID}.") + return PACS_ID + + logger.info(f"Resolved PACS '{chosen.get('aeTitle')}' to id {_resolved_pacs_id}") + return _resolved_pacs_id + + +def forget_resolved_pacs_id() -> None: + """ + Clears the cached PACS id so the next call re-reads it from XNAT. + + Called when XNAT reports the resolved PACS missing. ``configure-xnat.sh`` deletes and recreates + the registration when its AE title changes, which gives it a new id, and nothing restarts + imaging-api when XNAT is reconfigured — so without this the cache would point at a deleted + registration until the container was restarted (FLIP#993). + """ + global _resolved_pacs_id + _resolved_pacs_id = None + + +def ping_registered_pacs(headers: dict[str, str]) -> PacsStatus: + """ + Pings the PACS registered in XNAT, resolving its id first. + + On a 404 the resolved id is presumed stale — the PACS was re-registered under a new id while + this process stayed up — so the cache is dropped and the ping retried once against a freshly + resolved id. Without that, a caller that only ever pings (the trust-api health probe) would + keep failing on the dead id until the container restarted; ``check_pacs`` drops the cache the + same way on the import path (FLIP#993). + + Args: + headers (dict[str, str]): XNAT authentication headers. + + Returns: + PacsStatus: Status of the PACS system. + + Raises: + imaging_api.utils.exceptions.NotFoundError: If the PACS is not found under the freshly + resolved id either. + Exception: If there is an error during the ping request. + """ + pacs_id = resolve_pacs_id(headers) + try: + return ping_pacs(pacs_id, headers) + except NotFoundError: + forget_resolved_pacs_id() + fresh_id = resolve_pacs_id(headers) + if fresh_id == pacs_id: + raise + return ping_pacs(fresh_id, headers) def ping_pacs(pacs_id: int, headers: dict[str, str]) -> PacsStatus: @@ -48,6 +178,7 @@ def ping_pacs(pacs_id: int, headers: dict[str, str]) -> PacsStatus: response = requests.get( f"{XNAT_URL}/xapi/pacs/{pacs_id}/status", headers=headers, + timeout=XNAT_REQUEST_TIMEOUT, ) if response.status_code == 200: return PacsStatus(**response.json()) @@ -57,13 +188,15 @@ def ping_pacs(pacs_id: int, headers: dict[str, str]) -> PacsStatus: raise Exception(f"Failed to ping PACS: {response.text}") -def check_pacs(headers: dict[str, str], pacs_id: int = PACS_ID) -> None: +def check_pacs(headers: dict[str, str], pacs_id: int | None = None) -> None: """ Checks if the PACS system is reachable by pinging it. Args: headers (dict[str, str]): XNAT authentication headers. - pacs_id (int): PACS ID to check. Default is the PACS_ID from settings. + pacs_id (int | None): PACS ID to check. Resolved from XNAT when omitted, rather than + defaulting to the configured ``PACS_ID`` — that setting is the unreachable-XNAT + fallback, not a description of what is registered. Returns: None @@ -72,9 +205,15 @@ def check_pacs(headers: dict[str, str], pacs_id: int = PACS_ID) -> None: imaging_api.utils.exceptions.NotFoundError: If the PACS with the given ID is not found. Exception: If there is an error during the ping request or if the PACS is not reachable or is disabled. """ + if pacs_id is None: + pacs_id = resolve_pacs_id(headers) try: pacs_status = ping_pacs(pacs_id, headers) except NotFoundError: + # The id we hold no longer exists in XNAT. If it came from the cache it is stale — the PACS + # was re-registered under a new id while this process stayed up — so drop it and let the + # next call re-read, rather than failing every import until the container restarts. + forget_resolved_pacs_id() raise NotFoundError(f"PACS with ID '{pacs_id}' not found.") except Exception: raise Exception(f"Failed to ping PACS with ID '{pacs_id}'.") @@ -100,12 +239,13 @@ def query_by_accession_number(accession_number: str, headers: dict[str, str]) -> Exception: If there is an error during the query request. """ # Construct DQR Query - study_query = StudyQuery(accessionNumber=accession_number, pacsId=PACS_ID) + study_query = StudyQuery(accessionNumber=accession_number, pacsId=resolve_pacs_id(headers)) response = requests.post( f"{XNAT_URL}/xapi/dqr/query/studies", headers=headers, json=study_query.model_dump(by_alias=True), + timeout=XNAT_DQR_REQUEST_TIMEOUT, ) logger.debug(f"Query response: {response.text} - {response.status_code} - {response.reason}") @@ -160,6 +300,11 @@ def queue_image_import_request( # Check if project exists get_project(import_request.project_id, headers=headers) + # Retrieve against the same PACS the C-FIND queried. The model default is the static fallback, + # so resolve here rather than leaving the import pointing at a different registration than the + # query used (FLIP#993). + import_request.pacs_id = resolve_pacs_id(headers) + # Check PACS check_pacs(headers=headers, pacs_id=import_request.pacs_id) @@ -168,6 +313,7 @@ def queue_image_import_request( f"{XNAT_URL}/xapi/dqr/import", headers=headers, json=import_request.model_dump(by_alias=True), + timeout=XNAT_DQR_REQUEST_TIMEOUT, ) if response.status_code == 200: diff --git a/trust/imaging-api/tests/routers/test_imaging.py b/trust/imaging-api/tests/routers/test_imaging.py index 4950c0382..05d09dce2 100644 --- a/trust/imaging-api/tests/routers/test_imaging.py +++ b/trust/imaging-api/tests/routers/test_imaging.py @@ -54,6 +54,59 @@ def test_ping_pacs_failure(client): assert error_message in response.json()["detail"] +def test_ping_pacs_without_id_resolves_the_registration(client): + """trust-api's health probe calls this route: it must not have to know the id XNAT assigned.""" + mock_response = { + "pacsId": 7, + "successful": True, + "pingTime": 123, + "created": 1610000000, + "enabled": True, + "timestamp": 1610001234, + "id": 7, + "disabled": 0, + } + + with ( + patch("imaging_api.routers.imaging.ping_registered_pacs") as mock_ping_registered, + patch("imaging_api.routers.imaging.ping_pacs") as mock_ping_pacs, + ): + mock_ping_registered.return_value = PacsStatus(**mock_response) + + response = client.get("/imaging/ping_pacs") + + assert response.status_code == 200 + assert mock_ping_registered.called, "the id was not resolved from XNAT" + assert not mock_ping_pacs.called, "bypassed the resolving (stale-cache-aware) ping" + assert response.json()["pacsId"] == 7 + + +def test_ping_pacs_with_explicit_id_does_not_resolve(client): + """The by-id route still means that specific registration.""" + mock_response = { + "pacsId": 3, + "successful": True, + "pingTime": 123, + "created": 1610000000, + "enabled": True, + "timestamp": 1610001234, + "id": 3, + "disabled": 0, + } + + with ( + patch("imaging_api.routers.imaging.ping_registered_pacs") as mock_ping_registered, + patch("imaging_api.routers.imaging.ping_pacs") as mock_ping_pacs, + ): + mock_ping_pacs.return_value = PacsStatus(**mock_response) + + response = client.get("/imaging/ping_pacs/3") + + assert response.status_code == 200 + assert not mock_ping_registered.called, "an explicit id was overridden by the resolved one" + assert mock_ping_pacs.call_args[0][0] == 3 + + def test_query_by_accession_number_success(client): mock_study = Study( studyInstanceUid="1.2.3", diff --git a/trust/imaging-api/tests/services/test_imaging.py b/trust/imaging-api/tests/services/test_imaging.py index 0e8effe8f..b6842204f 100644 --- a/trust/imaging-api/tests/services/test_imaging.py +++ b/trust/imaging-api/tests/services/test_imaging.py @@ -16,7 +16,14 @@ import pytest from imaging_api.routers.schemas import ImportStudyRequest -from imaging_api.services.imaging import check_pacs, ping_pacs, query_by_accession_number, queue_image_import_request +from imaging_api.services.imaging import ( + XNAT_DQR_REQUEST_TIMEOUT, + XNAT_REQUEST_TIMEOUT, + check_pacs, + ping_pacs, + query_by_accession_number, + queue_image_import_request, +) from imaging_api.utils.exceptions import NotFoundError @@ -57,6 +64,8 @@ def test_ping_pacs(mock_get): # Assertions assert pacs_status.successful is True assert pacs_status.enabled is True + # Bounded, so a wedged XNAT fails the health probe instead of hanging it + assert mock_get.call_args.kwargs["timeout"] == XNAT_REQUEST_TIMEOUT # Test for ping_pacs function with 404 error @@ -111,6 +120,8 @@ def test_query_by_accession_number(mock_post, headers): # Assertions assert len(studies) == 1 assert studies[0].accession_number == accession_number + # Bounded, so a wedged XNAT fails the query instead of hanging the import path + assert mock_post.call_args.kwargs["timeout"] == XNAT_DQR_REQUEST_TIMEOUT @patch("imaging_api.services.imaging.check_pacs") @@ -156,6 +167,8 @@ def test_queue_image_import_request(mock_check_pacs, mock_requests_post, mock_ge # Assertions assert response[0].status == "QUEUED" assert response[0].pacs_id == 1 + # Bounded, so a wedged XNAT fails the import instead of hanging it + assert mock_requests_post.call_args.kwargs["timeout"] == XNAT_DQR_REQUEST_TIMEOUT # --------------------------------------------------------------------------- @@ -181,6 +194,20 @@ def test_check_pacs_success(mock_ping): check_pacs({}, pacs_id=1) # should not raise +# --------------------------------------------------------------------------- +# check_pacs — no id supplied +# --------------------------------------------------------------------------- +@patch("imaging_api.services.imaging.resolve_pacs_id", return_value=7) +@patch("imaging_api.services.imaging.ping_pacs") +def test_check_pacs_without_id_resolves_rather_than_assuming_the_configured_one(mock_ping, mock_resolve): + """The configured PACS_ID is the unreachable-XNAT fallback, not a description of what exists.""" + mock_ping.return_value = MagicMock(successful=True, enabled=True) + + check_pacs({}) + + assert mock_ping.call_args[0][0] == 7 + + # --------------------------------------------------------------------------- # check_pacs — not found # --------------------------------------------------------------------------- @@ -383,3 +410,143 @@ def test_queue_image_import_request_partial_failure(mock_get_project, mock_post, response = queue_image_import_request(import_request, headers) assert response[0].status == "QUEUED" assert response[1].status == "FAILED" + + +# --- PACS id resolution (FLIP#993) --------------------------------------------------- + + +@pytest.fixture(autouse=True) +def _clear_resolved_pacs_id(): + """Resets the module-level PACS id cache so each test resolves independently.""" + import imaging_api.services.imaging as imaging_module + + imaging_module._resolved_pacs_id = None + yield + imaging_module._resolved_pacs_id = None + + +@patch("imaging_api.services.imaging.requests.get") +def test_resolve_pacs_id_uses_the_registered_pacs(mock_get, headers): + """The id is read from XNAT, not assumed to be 1: an XNAT that carried the mock first won't be.""" + from imaging_api.services.imaging import resolve_pacs_id + + mock_get.return_value = MagicMock(status_code=200, json=lambda: [{"id": 7, "aeTitle": "SECTRA_QR"}]) + + assert resolve_pacs_id(headers) == 7 + + +@patch("imaging_api.services.imaging.requests.get") +def test_resolve_pacs_id_is_cached(mock_get, headers): + """XNAT is queried once; ids are fixed for the life of a registration.""" + from imaging_api.services.imaging import resolve_pacs_id + + mock_get.return_value = MagicMock(status_code=200, json=lambda: [{"id": 4, "aeTitle": "ORTHANC"}]) + + assert resolve_pacs_id(headers) == 4 + assert resolve_pacs_id(headers) == 4 + assert mock_get.call_count == 1 + + +@patch("imaging_api.services.imaging.requests.get") +def test_resolve_pacs_id_falls_back_when_none_registered(mock_get, headers): + """No registration degrades to the configured fallback rather than failing the import.""" + from imaging_api.services.imaging import PACS_ID, resolve_pacs_id + + mock_get.return_value = MagicMock(status_code=200, json=lambda: []) + + assert resolve_pacs_id(headers) == PACS_ID + + +@patch("imaging_api.services.imaging.requests.get", side_effect=Exception("XNAT unreachable")) +def test_resolve_pacs_id_falls_back_when_xnat_unreachable(mock_get, headers): + """A transient XNAT failure must not take out retrieval; it falls back to the configured id.""" + from imaging_api.services.imaging import PACS_ID, resolve_pacs_id + + assert resolve_pacs_id(headers) == PACS_ID + + +@patch("imaging_api.services.imaging.requests.get") +def test_resolve_pacs_id_prefers_the_default_query_retrieve_pacs(mock_get, headers): + """With a stray extra registration, the flagged default wins over list order.""" + from imaging_api.services.imaging import resolve_pacs_id + + mock_get.return_value = MagicMock( + status_code=200, + json=lambda: [ + {"id": 3, "aeTitle": "STRAY"}, + {"id": 9, "aeTitle": "SECTRA_QR", "defaultQueryRetrievePacs": True}, + ], + ) + + assert resolve_pacs_id(headers) == 9 + + +@patch("imaging_api.services.imaging.requests.get") +def test_resolve_pacs_id_falls_back_when_the_id_is_unusable(mock_get, headers): + """A registration without a usable id degrades to the fallback, which must not be cached.""" + import imaging_api.services.imaging as imaging_module + from imaging_api.services.imaging import PACS_ID, resolve_pacs_id + + mock_get.return_value = MagicMock(status_code=200, json=lambda: [{"aeTitle": "SECTRA_QR"}]) + + assert resolve_pacs_id(headers) == PACS_ID + assert imaging_module._resolved_pacs_id is None, "a fallback pinned in the cache would outlive the failure" + + +PACS_STATUS_OK = { + "pacsId": 9, + "successful": True, + "pingTime": 123, + "created": 1610000000, + "enabled": True, + "timestamp": 1610001234, + "id": 9, + "disabled": 0, +} + + +@patch("imaging_api.services.imaging.requests.get") +def test_ping_registered_pacs_drops_a_stale_cached_id_and_retries(mock_get, headers): + """A re-registration under a new id must heal on the next probe, not after a restart.""" + import imaging_api.services.imaging as imaging_module + from imaging_api.services.imaging import ping_registered_pacs + + imaging_module._resolved_pacs_id = 7 # stale: the registration it came from is gone + mock_get.side_effect = [ + MagicMock(status_code=404), # ping of the stale id + MagicMock(status_code=200, json=lambda: [{"id": 9, "aeTitle": "SECTRA_QR"}]), # re-resolve + MagicMock(status_code=200, json=lambda: PACS_STATUS_OK), # ping of the fresh id + ] + + status = ping_registered_pacs(headers) + + assert status.successful is True + assert imaging_module._resolved_pacs_id == 9, "the fresh id was not cached" + + +@patch("imaging_api.services.imaging.requests.get") +def test_ping_registered_pacs_raises_when_the_fresh_id_is_no_better(mock_get, headers): + """When re-resolving lands on the same missing id (e.g. the fallback), the 404 must surface.""" + import imaging_api.services.imaging as imaging_module + from imaging_api.services.imaging import ping_registered_pacs + + imaging_module._resolved_pacs_id = 7 + mock_get.side_effect = [ + MagicMock(status_code=404), # ping of the stale id + MagicMock(status_code=200, json=lambda: [{"id": 7, "aeTitle": "SECTRA_QR"}]), # same id again + ] + + with pytest.raises(NotFoundError): + ping_registered_pacs(headers) + + +def test_import_request_ae_title_follows_settings(): + """The C-MOVE destination AE title is configuration, not a hardcoded literal.""" + from imaging_api.config import get_settings + + request = ImportStudyRequest( + projectId="project-1", + studies=[{"studyInstanceUid": "1.2.3", "accessionNumber": "ACC1"}], + ) + + assert request.ae_title == get_settings().XNAT_AETITLE diff --git a/trust/trust-api/README.md b/trust/trust-api/README.md index 580c223a6..789ec8190 100644 --- a/trust/trust-api/README.md +++ b/trust/trust-api/README.md @@ -74,7 +74,6 @@ trust's kit file (`trust/.env..`); hub-shared values (`AES_KEY_BASE64 | `HEALTH_COLLECT_INTERVAL_SECONDS` | How often the health collector probes the trust services (default: 30) | | `HEALTH_PROBE_DEGRADED_MS` | A successful probe slower than this reports `degraded` (default: 1000) | | `XNAT_URL` | Internal URL of XNAT for the health probe (default `http://xnat-web:8080`) | -| `PACS_ID` | XNAT DQR PACS id used for the `ping_pacs` deep probe (default: 1) | | `OMOP_DB_HOST` / `OMOP_DB_PORT` | OMOP PostgreSQL address for the TCP health probe (defaults `omop-db` / 5432) | | `TRUST_INTERNAL_SERVICE_KEY_HEADER` | Header name for trust-internal service auth (default `X-Trust-Internal-Service-Key`) | | `TRUST_INTERNAL_SERVICE_KEY` | Per-trust plaintext key. Forwarded outbound on every call to imaging-api and data-access-api so those services can authenticate the caller. Minted by `register_trust` (`make register-trust KIT=`) into this trust's kit file (`trust/.env..`). | diff --git a/trust/trust-api/tests/services/test_health_collector.py b/trust/trust-api/tests/services/test_health_collector.py index fa40984da..f2135a36d 100644 --- a/trust/trust-api/tests/services/test_health_collector.py +++ b/trust/trust-api/tests/services/test_health_collector.py @@ -241,7 +241,6 @@ async def test_probe_dicom_healthy_measures_own_round_trip_and_sends_internal_ke mock_client.get.return_value = _response(200, {"successful": True, "pingTime": 1_786_029_453_834}) with ( - patch.object(health_collector, "PACS_ID", 1), patch( "trust_api.services.health_collector.trust_internal_headers", return_value={"X-Trust-Internal-Service-Key": "secret"}, @@ -252,7 +251,9 @@ async def test_probe_dicom_healthy_measures_own_round_trip_and_sends_internal_ke assert result == {"status": "healthy", "version": None, "response_ms": 250} call_args = mock_client.get.call_args - assert call_args[0][0].endswith("/imaging/ping_pacs/1") + # No trailing id: imaging-api resolves the registered PACS. A pinned id here was a second + # source of truth for something XNAT assigns at registration (FLIP#993). + assert call_args[0][0].endswith("/imaging/ping_pacs") assert call_args[1]["headers"] == {"X-Trust-Internal-Service-Key": "secret"} diff --git a/trust/trust-api/trust_api/config.py b/trust/trust-api/trust_api/config.py index d457e0995..8cf16e6b0 100644 --- a/trust/trust-api/trust_api/config.py +++ b/trust/trust-api/trust_api/config.py @@ -79,7 +79,6 @@ def coerce_empty_env(cls, v: str) -> str: HEALTH_COLLECT_INTERVAL_SECONDS: int = 30 # How often to probe the trust services (seconds) HEALTH_PROBE_DEGRADED_MS: int = 1000 # Successful probe slower than this reports "degraded" XNAT_URL: str = "http://xnat-web:8080" - PACS_ID: int = 1 # XNAT DQR PACS id used for the ping_pacs deep probe (matches imaging-api's default) OMOP_DB_HOST: str = "omop-db" OMOP_DB_PORT: int = 5432 diff --git a/trust/trust-api/trust_api/services/health_collector.py b/trust/trust-api/trust_api/services/health_collector.py index ca2ec8f91..f7089b76e 100644 --- a/trust/trust-api/trust_api/services/health_collector.py +++ b/trust/trust-api/trust_api/services/health_collector.py @@ -47,7 +47,6 @@ DATA_ACCESS_API_URL = get_settings().DATA_ACCESS_API_URL IMAGING_API_URL = get_settings().IMAGING_API_URL XNAT_URL = get_settings().XNAT_URL -PACS_ID = get_settings().PACS_ID OMOP_DB_HOST = get_settings().OMOP_DB_HOST OMOP_DB_PORT = get_settings().OMOP_DB_PORT HEALTH_COLLECT_INTERVAL_SECONDS = get_settings().HEALTH_COLLECT_INTERVAL_SECONDS @@ -204,7 +203,10 @@ async def _probe_dicom(client: httpx.AsyncClient) -> dict: dict: Wire-shaped entry. ``unknown`` when the prober chain (imaging-api → XNAT) itself fails, ``down`` only when XNAT reports the DIMSE echo failed. """ - url = f"{IMAGING_API_URL}/imaging/ping_pacs/{PACS_ID}" + # No id in the path: imaging-api resolves the registered PACS from XNAT. Pinning one here + # made trust-api a second source of truth for an id XNAT assigns at registration — a + # re-registered PACS reported permanently down while imports kept working (FLIP#993). + url = f"{IMAGING_API_URL}/imaging/ping_pacs" start = time.monotonic() try: response = await client.get(url, headers=trust_internal_headers()) diff --git a/trust/xnat/Makefile b/trust/xnat/Makefile index 43e7f7fd2..483afa3fb 100644 --- a/trust/xnat/Makefile +++ b/trust/xnat/Makefile @@ -68,7 +68,7 @@ KIT_FILE := $(if $(wildcard ../.env.$(KIT).$(ENV)),../.env.$(KIT).$(ENV),../.env -include $(KIT_FILE) # Kit-file values must reach the shell env, not just Make vars. docker stack # deploy (unlike docker compose) has no --env-file flag; it substitutes -# ${XNAT_ADMIN_USER}, ${PACS_DICOM_PORT}, ${DOCKER_REGISTRY}, etc. in the +# ${XNAT_ADMIN_USER}, ${PACS_UI_PORT}, ${DOCKER_REGISTRY}, etc. in the # compose YAML from the calling shell env only. Without this export, the # stack came up with empty admin creds and configure-xnat.sh would 401/403 # on every auth call, leaving "Site has not yet been configured" forever. @@ -98,9 +98,23 @@ XNAT_NETWORK := $(INSTANCE_PREFIX)deploy_trust-network-$(TRUST_NUM) # TRUST_PROJECT. Instance-scoped: unprefixed, the guard below inspects whichever hub owns # plain `trust` and reports on a trust that is not ours. TRUST_PROJECT := $(INSTANCE_PREFIX)trust$(TRUST_NUM) +# XNAT_PORT is the DICOM SCP receiver port: the port XNAT binds, registers on its dicomscp +# receiver, advertises as the C-MOVE destination, and publishes on the host so a PACS outside the +# host can complete the C-STORE return leg of a DQR retrieval (FLIP#993). XNAT_WEB_PORT is the +# host-published web UI port. Both are host-published, so every kit must give them distinct values +# (the guard in xnat-reset refuses a collision); the shipped dev allocation is 8104/8105 (GSTT) and +# 8106/8107 (KCH). The web port deliberately defaults to XNAT_PORT rather than to its own literal: +# the two were one variable until the FLIP#993 split, so a kit that predates it sets only XNAT_PORT +# — deriving the web port from it routes that kit into the collision guard, a loud instruction to +# allocate a second port, instead of silently moving its web UI to a number nothing else expects. # Defaults are the Trust_1 allocation, so a single-trust host (EC2 / on-prem) # needs no XNAT port entries in its kit file. XNAT_PORT_EFFECTIVE := $(or $(XNAT_PORT),8104) +XNAT_WEB_PORT_EFFECTIVE := $(or $(XNAT_WEB_PORT),$(XNAT_PORT_EFFECTIVE)) +# XNAT's AE title, applied to the SCP receiver, dqrCallingAe and the C-MOVE destination. +# $(if $(filter undefined,...)) rather than $(or): $(or) treats an empty value as false and +# would substitute the default, cancelling configure-xnat.sh's guard on an empty kit value. +XNAT_AETITLE_EFFECTIVE := $(if $(filter undefined,$(origin XNAT_AETITLE)),XNAT,$(XNAT_AETITLE)) PACS_UI_PORT_EFFECTIVE := $(or $(PACS_UI_PORT),8042) # Host path that backs this trust's XNAT bind mounts (parent of xnat-data/ @@ -255,6 +269,8 @@ endif $(MAKE) xnat-reset KIT=$(KIT) XNAT_PATH=${XNAT_PATH} \ XNAT_PORT=$(XNAT_PORT_EFFECTIVE) \ + XNAT_WEB_PORT=$(XNAT_WEB_PORT_EFFECTIVE) \ + XNAT_AETITLE=$(XNAT_AETITLE_EFFECTIVE) \ PACS_UI_PORT=$(PACS_UI_PORT_EFFECTIVE) \ DOCKER_NETWORK_NAME=$(XNAT_NETWORK) \ docker stack deploy --with-registry-auth --detach=false ${STACK_FILES} $(XNAT_STACK) @@ -348,6 +364,16 @@ xnat-reset: @if ! echo "$(XNAT_PORT_EFFECTIVE)" | grep -qE '^[0-9]+$$'; then \ echo "ERROR: XNAT_PORT must be numeric for KIT=$(KIT), got '$(XNAT_PORT_EFFECTIVE)'"; exit 1; \ fi + @if ! echo "$(XNAT_WEB_PORT_EFFECTIVE)" | grep -qE '^[0-9]+$$'; then \ + echo "ERROR: XNAT_WEB_PORT must be numeric for KIT=$(KIT), got '$(XNAT_WEB_PORT_EFFECTIVE)'"; exit 1; \ + fi + @if [ "$(XNAT_PORT_EFFECTIVE)" = "$(XNAT_WEB_PORT_EFFECTIVE)" ]; then \ + echo "ERROR: the web UI and the DICOM receiver are both published on the host, so"; \ + echo " XNAT_WEB_PORT ($(XNAT_WEB_PORT_EFFECTIVE)) and XNAT_PORT ($(XNAT_PORT_EFFECTIVE)) must differ."; \ + echo " They were one variable until FLIP#993 — set both in the kit file"; \ + echo " ($(KIT_FILE)), e.g. XNAT_PORT=8104 (DICOM) and XNAT_WEB_PORT=8105 (web UI)."; \ + exit 1; \ + fi @echo "Deleting previous XNAT data and creating new directories for $(KIT) ..."; \ if [ "$(PROD)" = "true" ] || [ "$(PROD)" = "stag" ]; then \ echo " 📍 Using prod data dir: $(XNAT_DATA_DIR)/"; \ diff --git a/trust/xnat/docker-compose-stack.yml b/trust/xnat/docker-compose-stack.yml index 8b7ebe310..5b2d803d5 100644 --- a/trust/xnat/docker-compose-stack.yml +++ b/trust/xnat/docker-compose-stack.yml @@ -17,7 +17,21 @@ services: placement: constraints: [node.role == manager] ports: - - ${XNAT_PORT}:8080 + # Host-published web UI, and the DICOM SCP receiver next to it. The receiver has to be + # published for a real trust PACS: FLIP retrieves by DQR, so after XNAT issues C-FIND/C-MOVE + # the PACS opens a *new* association back to XNAT to C-STORE the studies — that return leg is + # inbound, and without a published port it never arrives (queries succeed, retrievals silently + # time out; FLIP#993). The mocked Orthanc reaches xnat-web:${XNAT_PORT} over the container + # network and doesn't need the publication, but every deployment gets it so development + # exercises the same wiring a real-PACS trust runs. + # + # XNAT_PORT is used on both sides of the mapping deliberately: DQR matches the C-MOVE + # destination against a registered SCP receiver by exact AE title and port, so the port the + # PACS connects to must be the same number XNAT binds — no translation is possible on this + # leg. Both ports are host-published, so XNAT_WEB_PORT and XNAT_PORT must differ; the Makefile + # refuses to deploy if they collide. + - ${XNAT_WEB_PORT}:8080 + - ${XNAT_PORT}:${XNAT_PORT} security_opt: - no-new-privileges:true cap_drop: @@ -35,7 +49,28 @@ services: - XNAT_SERVICE_USER=${XNAT_SERVICE_USER} - XNAT_SERVICE_PASSWORD=${XNAT_SERVICE_PASSWORD} - XNAT_PORT=${XNAT_PORT} - - PACS_DICOM_PORT=${PACS_DICOM_PORT:-4242} + - XNAT_AETITLE=${XNAT_AETITLE-XNAT} + # Upstream PACS, read by configure-xnat.sh. Defaults are the mocked Orthanc, so an + # unconfigured stack behaves exactly as before; a real trust sets these in its kit file. + # ${VAR-default}, not ${VAR:-default}: compose substitutes the default for a set-but-empty + # variable, which would hand the script a value it considers configured and cancel its + # fail-loud guard. An operator who writes PACS_HOST= in a kit file must get an error, not + # a silent fallback to the mocked PACS (FLIP#993). + - PACS_HOST=${PACS_HOST-orthanc} + - PACS_AETITLE=${PACS_AETITLE-ORTHANC} + - PACS_QR_PORT=${PACS_QR_PORT-4242} + - PACS_LABEL=${PACS_LABEL-Test PACS instance} + # A per-PACS capability, not a preference: a PACS without relational-query support + # rejects the association unless this is false. + - PACS_SUPPORTS_EXTENDED_NEGOTIATIONS=${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS-true} + # PACS throttle — a production PACS may refuse further associations after a certain volume. + - PACS_AVAILABILITY_DAYS=${PACS_AVAILABILITY_DAYS-MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY} + - PACS_AVAILABILITY_START=${PACS_AVAILABILITY_START-00:00} + - PACS_AVAILABILITY_END=${PACS_AVAILABILITY_END-24:00} + - PACS_THREADS=${PACS_THREADS-1} + - PACS_UTILIZATION_PERCENT=${PACS_UTILIZATION_PERCENT-100} + - DQR_MAX_PACS_REQUEST_ATTEMPTS=${DQR_MAX_PACS_REQUEST_ATTEMPTS-100} + - DQR_RETRY_WAIT_SECONDS=${DQR_RETRY_WAIT_SECONDS-300} - XNAT_DATASOURCE_DRIVER=${XNAT_DATASOURCE_DRIVER} - XNAT_DATASOURCE_URL=${XNAT_DATASOURCE_URL} - XNAT_DATASOURCE_NAME=${XNAT_DATASOURCE_NAME} diff --git a/trust/xnat/tests/test_configure_pacs.py b/trust/xnat/tests/test_configure_pacs.py new file mode 100644 index 000000000..6556b93c4 --- /dev/null +++ b/trust/xnat/tests/test_configure_pacs.py @@ -0,0 +1,625 @@ +# Copyright (c) 2026 Guy's and St Thomas' NHS Foundation Trust & King's College London +# 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. +# +"""Execution tests for the PACS/DQR configuration in configure-xnat.sh (FLIP#993). + +The script is run for real against a stub ``curl`` that records every payload it is asked to POST or +PUT, so these assert what XNAT would actually receive rather than matching strings in the source. +""" + +import json +import os +import re +import subprocess +from pathlib import Path + +import pytest + +CONFIG_DIR = Path(__file__).resolve().parents[1] / "xnat" / "config" +SCRIPT = CONFIG_DIR / "configure-xnat.sh" + +# A miniature XNAT: it keeps the /xapi/pacs and /xapi/dicomscp collections in files and mutates them +# on POST/PUT/DELETE, so a GET reflects what the script actually did rather than what the test said. +# The earlier stub echoed the *configured* AE title back, which made "the configured title reached +# XNAT" assertions circular — they held even when the script sent something else. +STUB_CURL = r"""#!/bin/bash +url=""; data=""; method="GET"; status_only=0; outfile="" +prev="" +for a in "$@"; do + case "$prev" in -d) data="$a";; -X) method="$a";; -o|--output) outfile="$a";; esac + case "$a" in http*) url="$a";; '%{http_code}') status_only=1;; esac + prev="$a" +done + +# Every request, not only those carrying a body: a PUT whose whole meaning is its URL +# (/xapi/users/guest/enabled/false) is otherwise invisible to the tests. +printf '%s\n' "=== $method $url" >> "$PAYLOADS" +[ -n "$data" ] && printf '%s\n' "$data" >> "$PAYLOADS" + +edit() { # edit [args...] + local f="$1"; shift + jq "$@" "$f" > "$f.tmp" && mv "$f.tmp" "$f" +} + +status=200 +body='{}' +case "$url" in + *"/xapi/pacs/"*"/availability") + status="${AVAIL_STATUS:-200}" + body="${AVAIL_BODY:-{\}}" + ;; + *"/xapi/pacs") + if [ "$method" = "POST" ] && [ -z "$SWALLOW_PACS_POST" ]; then + # XNAT assigns the id. Ours start at 7 so nothing can pass by assuming 1. + edit "$PACS_STATE" --argjson id "$(( $(jq 'length' "$PACS_STATE") + 7 ))" --argjson e "$data" \ + '. + [$e + {id: $id}]' + fi + body=$(cat "$PACS_STATE") + ;; + *"/xapi/pacs/"*) + id="${url##*/}" + case "$method" in + PUT) edit "$PACS_STATE" --argjson id "$id" --argjson e "$data" \ + 'map(if .id == $id then $e + {id: $id} else . end)' ;; + DELETE) edit "$PACS_STATE" --argjson id "$id" 'map(select(.id != $id))' ;; + esac + body=$(cat "$PACS_STATE") + ;; + *"/xapi/dicomscp") + if [ "$method" = "POST" ]; then + edit "$SCP_STATE" --argjson id "$(( $(jq 'length' "$SCP_STATE") + 5 ))" --argjson e "$data" \ + '. + [$e + {id: $id}]' + fi + body=$(cat "$SCP_STATE") + ;; + *"/xapi/dicomscp/"*) + id="${url##*/}" + [ "$method" = "DELETE" ] && edit "$SCP_STATE" --argjson id "$id" 'map(select(.id != $id))' + body=$(cat "$SCP_STATE") + ;; +esac + +# Lets a test make one specific call fail, to exercise the error paths. +case "${FAIL_ON_URL:-__none__}" in + __none__) ;; + *) case "$url" in *"$FAIL_ON_URL"*) status="${FAIL_STATUS:-500}"; body='{"error":"stub failure"}' ;; esac ;; +esac + +[ -n "$outfile" ] && [ "$outfile" != "/dev/null" ] && printf '%s' "$body" > "$outfile" +if [ "$status_only" = "1" ]; then printf '%s' "$status"; else printf '%s\n%s' "$body" "$status"; fi +case "$status" in 2*) exit 0 ;; esac +exit 0 +""" + +BASE_ENV = { + "XNAT_ADMIN_USER": "admin", + "XNAT_ADMIN_INITIAL_PASSWORD": "initial", + "XNAT_ADMIN_PASSWORD": "rotated", + "XNAT_SERVICE_USER": "flipServiceAccount", + "XNAT_SERVICE_PASSWORD": "service", + "XNAT_PORT": "8104", + # The plugin-readiness wait polls until a DQR route answers, bounded only by wall clock. With + # sleep stubbed out, a test that makes that route fail would spin at full speed for the default + # 900s budget rather than failing; cap it so the harness can never hang on one. + "XNAT_PLUGIN_READINESS_TIMEOUT_SECONDS": "5", + "XNAT_PLUGIN_READINESS_POLL_SECONDS": "0", +} + +# What the stub reports as already registered when a test does not say otherwise. Carries every +# kit-managed field the drift check compares (a live XNAT GET /xapi/pacs returns them all), with +# values matching the script's defaults so the entry reads as in-sync unless a test drifts one. +MOCK_PACS_REGISTRATION = ( + '[{"id":7,"aeTitle":"ORTHANC","host":"orthanc","queryRetrievePort":4242,' + '"label":"Test PACS instance","supportsExtendedNegotiations":true}]' +) + +# The SCP receivers a test can seed XNAT with, one JSON object each — reclamation is scoped by what +# created a receiver, so which of these survives is the whole point of those tests. +# +# FLIP-owned: created by an earlier run of this script, identified by the identifier it stamps on, +# whatever AE title and port it happens to carry. `GET /xapi/dicomscp` does return `identifier` per +# receiver (verified against a live XNAT), which is what makes ownership readable at all. +FLIP_OWNED_RECEIVER = '{"id":3,"aeTitle":"FLIPXNAT","port":8104,"identifier":"dqrObjectIdentifier"}' +# XNAT's own, created by the webapp on first boot: always titled "XNAT", never carrying FLIP's +# identifier. +STOCK_RECEIVER = '{"id":1,"aeTitle":"XNAT","port":8104,"identifier":"dicomObjectIdentifier"}' +# An operator's, registered by hand for some other local DICOM source: neither marker, so nothing +# here owns it. +OPERATOR_RECEIVER = '{"id":9,"aeTitle":"WARDCT","port":11113,"identifier":"dicomObjectIdentifier"}' + + +def run_configure(tmp_path, env_overrides=None, pacs_state=None, scp_state=None): + """Runs configure-xnat.sh against the stub and returns (exit code, payloads, combined output). + + Args: + tmp_path: pytest tmp_path for the stub PATH and state files. + env_overrides (dict | None): Environment for the run, layered over BASE_ENV. + pacs_state (str | None): JSON array the stub starts with as XNAT's PACS registrations. + scp_state (str | None): JSON array the stub starts with as XNAT's SCP receivers. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir(parents=True) # parents: tests that run twice pass a nested tmp_path + stub = bin_dir / "curl" + stub.write_text(STUB_CURL) + stub.chmod(0o755) + # The script's fixed "wait for XNAT to settle" sleep is 10s of dead time per run, and the + # readiness loops it guards are already satisfied instantly by the stub. Stubbing sleep keeps + # the suite at seconds rather than minutes; nothing here is testing the waits. + no_sleep = bin_dir / "sleep" + no_sleep.write_text("#!/bin/sh\nexit 0\n") + no_sleep.chmod(0o755) + + payloads = tmp_path / "payloads.txt" + pacs_file = tmp_path / "pacs.json" + pacs_file.write_text(pacs_state if pacs_state is not None else "[]") + scp_file = tmp_path / "scp.json" + # Default: XNAT's stock receiver, as the webapp creates it on first boot — title "XNAT" and the + # plain DICOM identifier, not FLIP's. + scp_file.write_text(scp_state if scp_state is not None else f"[{STOCK_RECEIVER}]") + + env = { + **os.environ, + **BASE_ENV, + "PATH": f"{bin_dir}:{os.environ['PATH']}", + "PAYLOADS": str(payloads), + "PACS_STATE": str(pacs_file), + "SCP_STATE": str(scp_file), + **(env_overrides or {}), + } + + result = subprocess.run( + ["bash", str(SCRIPT)], cwd=CONFIG_DIR, env=env, capture_output=True, text=True, timeout=120 + ) + body = payloads.read_text() if payloads.exists() else "" + return result.returncode, body, result.stdout + result.stderr + + +def requests_made(payloads: str) -> list[tuple[str, str]]: + """Every (method, url) the script issued, in order.""" + made = [] + for block in payloads.split("=== "): + header = block.partition("\n")[0].split() + if len(header) == 2: + made.append((header[0], header[1])) + return made + + +def body_for(payloads: str, endpoint: str) -> str: + """The last raw request body sent to ``endpoint`` — for the payloads that are not objects.""" + found = None + for block in payloads.split("=== "): + header, _, rest = block.partition("\n") + parts = header.split() + if len(parts) == 2 and parts[1].endswith(endpoint) and rest.strip(): + found = rest.strip() + assert found is not None, f"no body sent to {endpoint}" + return found + + +def payload_for(payloads: str, endpoint: str) -> dict: + """Returns the last JSON payload sent to ``endpoint``. + + Matched on the URL's path suffix rather than a substring: ``/xapi/pacs`` would otherwise also + match ``/xapi/pacs/7/availability`` and return the wrong payload. + """ + found = None + for block in payloads.split("=== "): + header, _, rest = block.partition("\n") + parts = header.split() + url = parts[1] if len(parts) == 2 else "" + matches = url.endswith(endpoint) or ( + endpoint == "/xapi/pacs" and re.search(r"/xapi/pacs/\d+$", url) is not None + ) + if matches and rest.strip().startswith("{"): + found = json.loads(rest.strip()) + assert found is not None, f"no payload sent to {endpoint}" + return found + + +def test_defaults_configure_the_mocked_orthanc(tmp_path): + """An unconfigured deployment must still describe the mock exactly as before.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + + pacs = payload_for(payloads, "/xapi/pacs") + assert pacs["aeTitle"] == "ORTHANC" + assert pacs["host"] == "orthanc" + assert pacs["queryRetrievePort"] == 4242 + + receiver = payload_for(payloads, "/xapi/dicomscp") + assert receiver["aeTitle"] == "XNAT" + assert receiver["port"] == 8104 + + assert payload_for(payloads, "/xapi/dqr/settings")["dqrCallingAe"] == "XNAT" + + +def test_configured_pacs_and_ae_title_reach_xnat(tmp_path): + """Every configured value must appear in what XNAT is actually sent.""" + code, payloads, output = run_configure( + tmp_path, + { + "XNAT_AETITLE": "FLIPXNAT", + "PACS_HOST": "10.0.0.10", + "PACS_AETITLE": "SECTRA_QR", + "PACS_QR_PORT": "8059", + "PACS_LABEL": "GSTT Sectra PACS", + }, + ) + assert code == 0, output + + pacs = payload_for(payloads, "/xapi/pacs") + assert (pacs["aeTitle"], pacs["host"], pacs["queryRetrievePort"]) == ("SECTRA_QR", "10.0.0.10", 8059) + assert pacs["label"] == "GSTT Sectra PACS" + + # The AE title has to reach all three places that must agree, or the C-STORE association the + # PACS opens is addressed to a receiver that does not exist. + assert payload_for(payloads, "/xapi/dicomscp")["aeTitle"] == "FLIPXNAT" + assert payload_for(payloads, "/xapi/dqr/settings")["dqrCallingAe"] == "FLIPXNAT" + + +def test_throttle_settings_are_configurable(tmp_path): + """The availability window and retry behaviour are the throttle for a production PACS.""" + code, payloads, output = run_configure( + tmp_path, + { + "PACS_AVAILABILITY_DAYS": "SATURDAY,SUNDAY", + "PACS_AVAILABILITY_START": "19:00", + "PACS_AVAILABILITY_END": "07:00", + "PACS_THREADS": "2", + "PACS_UTILIZATION_PERCENT": "40", + "DQR_MAX_PACS_REQUEST_ATTEMPTS": "25", + "DQR_RETRY_WAIT_SECONDS": "120", + }, + ) + assert code == 0, output + + dqr = payload_for(payloads, "/xapi/dqr/settings") + assert dqr["dqrMaxPacsRequestAttempts"] == "25" + assert dqr["dqrWaitToRetryRequestInSeconds"] == "120" + + availability = payload_for(payloads, "/availability") + assert availability["availabilityStart"] == "19:00" + assert availability["availabilityEnd"] == "07:00" + assert availability["threads"] == 2 + assert availability["utilizationPercent"] == 40 + + assert output.count("Setting PACS availability for") == 2, "only the configured days should be scheduled" + + +def test_registration_updates_in_place_when_host_or_port_drift(tmp_path): + """A kit change must not be silently ignored on redeploy, leaving DQR on the old PACS.""" + code, payloads, output = run_configure( + tmp_path, + {"PACS_HOST": "10.0.0.10", "PACS_QR_PORT": "8059"}, + pacs_state=MOCK_PACS_REGISTRATION, + ) + assert code == 0, output + assert "updating to 10.0.0.10:8059" in output + + pacs = payload_for(payloads, "/xapi/pacs") + assert pacs["host"] == "10.0.0.10" + assert pacs["queryRetrievePort"] == 8059 + + +def test_registration_updates_in_place_when_flags_drift(tmp_path): + """Drift in a kit-managed field other than host/port must also land, not log "leaving as-is". + + supportsExtendedNegotiations is the documented lever for a real PACS that rejects extended + negotiation, and the k8s init job re-runs this script on every helm upgrade against persistent + XNAT data — a host/port-only comparison keeps the old flag forever with no signal. + """ + code, payloads, output = run_configure( + tmp_path, + {"PACS_SUPPORTS_EXTENDED_NEGOTIATIONS": "false"}, + pacs_state=MOCK_PACS_REGISTRATION, + ) + assert code == 0, output + assert "leaving as-is" not in output + assert "supportsExtendedNegotiations" in output + + pacs = payload_for(payloads, "/xapi/pacs") + assert pacs["supportsExtendedNegotiations"] is False + assert pacs["host"] == "orthanc" + assert pacs["queryRetrievePort"] == 4242 + + +def test_matching_registration_is_left_alone(tmp_path): + """An unchanged registration must not be rewritten on every redeploy.""" + code, payloads, output = run_configure(tmp_path, pacs_state=MOCK_PACS_REGISTRATION) + assert code == 0, output + assert "already registered at orthanc:4242 — leaving as-is" in output + assert not [m for m, u in requests_made(payloads) if m in ("POST", "PUT") and u.endswith("/xapi/pacs")] + + +def test_availability_uses_the_resolved_pacs_id(tmp_path): + """The schedule must be written against the real registration, not a hardcoded id of 1.""" + code, payloads, output = run_configure(tmp_path, pacs_state=MOCK_PACS_REGISTRATION) + assert code == 0, output + assert payload_for(payloads, "/availability")["pacsId"] == 7 + + +@pytest.mark.parametrize("var", ["XNAT_AETITLE", "PACS_HOST", "PACS_AETITLE", "PACS_QR_PORT"]) +def test_empty_values_fail_loudly(tmp_path, var): + """An empty value would produce malformed JSON that XNAT rejects silently (FLIP#822/#862).""" + code, _, output = run_configure(tmp_path, {var: ""}) + assert code != 0, f"empty {var} should abort the run" + assert var in output + + +def deletes(payloads: str) -> list[str]: + """URLs the script issued a DELETE against.""" + return [b.partition("\n")[0].split()[-1] for b in payloads.split("=== ") if b.startswith("DELETE ")] + + +def test_receiver_on_our_port_is_reclaimed_whatever_it_is_called(tmp_path): + """Renaming the AE title must not strand the old receiver fighting for the same port.""" + code, payloads, output = run_configure( + tmp_path, + scp_state=f"[{FLIP_OWNED_RECEIVER}]", + ) + assert code == 0, output + assert "(id 3)" in output + assert "FLIPXNAT" in output + assert any(u.endswith("/xapi/dicomscp/3") for u in deletes(payloads)) + + +def test_a_receiver_this_script_does_not_own_survives_the_reconcile(tmp_path): + """An operator's second receiver must not be collateral damage of every redeploy. + + Reclamation is scoped to what FLIP created (identifier ``dqrObjectIdentifier``) plus XNAT's + stock receiver. A receiver registered by hand for another local DICOM source carries neither + marker, and deleting it would be silent: on Kubernetes the only record is a Job pod log the + hook-delete-policy discards on success. + """ + code, payloads, output = run_configure( + tmp_path, + scp_state=f"[{OPERATOR_RECEIVER}, {FLIP_OWNED_RECEIVER}, {STOCK_RECEIVER}]", + ) + assert code == 0, output + + deleted = deletes(payloads) + assert not any(u.endswith("/xapi/dicomscp/9") for u in deleted), "deleted a receiver FLIP does not own" + assert "Removing SCP receiver 'WARDCT" not in output + # The two it does own still go, or the re-created receiver fights the old one for the port. + assert any(u.endswith("/xapi/dicomscp/3") for u in deleted), "left FLIP's own stale receiver behind" + assert any(u.endswith("/xapi/dicomscp/1") for u in deleted), "left XNAT's stock receiver behind" + + +def test_foreign_pacs_registrations_are_removed(tmp_path): + """A trust XNAT retrieves from one PACS; a stale entry would leave DQR's choice ambiguous.""" + code, payloads, output = run_configure( + tmp_path, + { + "PACS_AETITLE": "SECTRA_QR", + "PACS_HOST": "10.0.0.10", + "PACS_QR_PORT": "8059", + }, + pacs_state='[{"id":1,"aeTitle":"ORTHANC","host":"orthanc","queryRetrievePort":4242}]', + ) + assert code == 0, output + assert "Removing PACS ORTHANC at orthanc:4242 (id 1)" in output + assert any(u.endswith("/xapi/pacs/1") for u in deletes(payloads)) + + +def test_receiver_is_reclaimed_when_port_and_title_both_change(tmp_path): + """Matching on our port *or* our AE title left an orphan when both moved in one change.""" + code, payloads, output = run_configure( + tmp_path, + {"XNAT_PORT": "11112", "XNAT_AETITLE": "FLIPXNAT2"}, + scp_state=f"[{FLIP_OWNED_RECEIVER}]", + ) + assert code == 0, output + assert any(u.endswith("/xapi/dicomscp/3") for u in deletes(payloads)), ( + "the old receiver was left enabled and bound to its port" + ) + + +def test_refuses_to_delete_a_foreign_pacs_while_still_on_mock_defaults(tmp_path): + """An unrelated redeploy must not delete a PACS the operator registered by hand.""" + code, payloads, output = run_configure( + tmp_path, + pacs_state='[{"id":1,"aeTitle":"SECTRA_QR","host":"10.0.0.10","queryRetrievePort":8059}]', + ) + assert code != 0, "should refuse rather than delete a registration it may not own" + assert "SECTRA_QR at 10.0.0.10:8059" in output + assert not any("/xapi/pacs/1" in u for u in deletes(payloads)), "deleted it anyway" + + +def test_injected_json_in_a_pacs_value_cannot_change_other_fields(tmp_path): + """Values are data, not JSON fragments: a crafted host must not flip defaultQueryRetrievePacs.""" + code, payloads, output = run_configure( + tmp_path, + {"PACS_HOST": 'x", "defaultQueryRetrievePacs": false, "z": "y'}, + ) + assert code == 0, output + pacs = payload_for(payloads, "/xapi/pacs") + assert pacs["defaultQueryRetrievePacs"] is True, "injected key won" + assert pacs["host"] == 'x", "defaultQueryRetrievePacs": false, "z": "y' + + +def test_non_numeric_port_fails_naming_the_variable(tmp_path): + """A bad port must fail here, not reach XNAT as an opaque 400.""" + code, _, output = run_configure(tmp_path, {"PACS_QR_PORT": "8059abc"}) + assert code != 0, "a non-numeric port was accepted" + + +def test_scp_receiver_binds_the_configured_port(tmp_path): + """XNAT_PORT is the C-MOVE destination port; a receiver on any other port never gets the study.""" + code, payloads, output = run_configure(tmp_path, {"XNAT_PORT": "11112"}) + assert code == 0, output + assert payload_for(payloads, "/xapi/dicomscp")["port"] == 11112 + + +def test_scp_receiver_keeps_the_settings_the_dqr_import_path_depends_on(tmp_path): + """These four are why the receiver is re-created rather than left at XNAT's defaults. + + ``dqrObjectIdentifier`` is what routes an arriving study to the project DQR requested it for; + without ``directArchive`` + ``customProcessing`` the study lands in the prearchive and is never + archived; ``anonymizationEnabled`` is what applies the site-wide anonymization script, so + turning it off sends identifiable DICOM into the archive. + """ + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + + receiver = payload_for(payloads, "/xapi/dicomscp") + assert receiver["identifier"] == "dqrObjectIdentifier" + assert receiver["directArchive"] is True + assert receiver["customProcessing"] is True + assert receiver["anonymizationEnabled"] is True + assert receiver["enabled"] is True + # Routing/whitelisting are off deliberately: FLIP routes by DQR's identifier, and a whitelist + # here would silently drop studies the platform asked for. + assert receiver["whitelistEnabled"] is False + assert receiver["routingExpressionsEnabled"] is False + + +def test_site_wide_anonymization_is_uploaded_and_enabled(tmp_path): + """The receiver's anonymizationEnabled only matters if the site script is on.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + assert body_for(payloads, "/xapi/anonymize/site/enabled") == "true" + assert ("PUT", "http://xnat-web:8080/xapi/anonymize/site") in requests_made(payloads) + + +def test_dqr_stays_restricted_to_authorised_accounts(tmp_path): + """allowAllUsersToUseDqr would let any XNAT account pull arbitrary studies from the trust PACS.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + assert payload_for(payloads, "/xapi/dqr/settings")["allowAllUsersToUseDqr"] is False + + +def test_guest_account_is_disabled(tmp_path): + """An enabled guest is an unauthenticated reader of an archive holding patient imaging.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + assert ("PUT", "http://xnat-web:8080/xapi/users/guest/enabled/false") in requests_made(payloads) + + +def test_pacs_registration_carries_the_flags_dqr_selects_on(tmp_path): + """defaultQueryRetrievePacs is how DQR picks this PACS, and imaging-api how it resolves the id.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + + pacs = payload_for(payloads, "/xapi/pacs") + assert pacs["defaultQueryRetrievePacs"] is True + assert pacs["defaultStoragePacs"] is True + assert pacs["queryable"] is True + assert pacs["storable"] is True + + +def test_extended_negotiation_defaults_on_and_is_configurable(tmp_path): + """A PACS without relational-query support rejects the association unless this is off.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + assert payload_for(payloads, "/xapi/pacs")["supportsExtendedNegotiations"] is True + + code, payloads, output = run_configure(tmp_path / "off", {"PACS_SUPPORTS_EXTENDED_NEGOTIATIONS": "false"}) + assert code == 0, output + assert payload_for(payloads, "/xapi/pacs")["supportsExtendedNegotiations"] is False + + +@pytest.mark.parametrize("value", ["yes", "True", "1", ""]) +def test_non_boolean_extended_negotiation_is_refused(tmp_path, value): + """jq would take `1` as the number 1 and reject `yes` with a message naming neither the + variable nor the accepted values.""" + code, _, output = run_configure(tmp_path, {"PACS_SUPPORTS_EXTENDED_NEGOTIATIONS": value}) + assert code != 0, f"{value!r} was accepted as a boolean" + assert "PACS_SUPPORTS_EXTENDED_NEGOTIATIONS" in output + + +def test_availability_window_is_enabled_and_live(tmp_path): + """A window written disabled is a schedule that silently never applies.""" + code, payloads, output = run_configure(tmp_path) + assert code == 0, output + + availability = payload_for(payloads, "/availability") + assert availability["enabled"] is True + assert availability["availableNow"] is True + + +def test_dqr_retry_and_poll_settings_reach_xnat(tmp_path): + """The retry count and wait are the throttle a PACS team agrees to; a swap inverts it.""" + code, payloads, output = run_configure( + tmp_path, + {"DQR_MAX_PACS_REQUEST_ATTEMPTS": "25", "DQR_RETRY_WAIT_SECONDS": "120"}, + ) + assert code == 0, output + + dqr = payload_for(payloads, "/xapi/dqr/settings") + assert dqr["dqrMaxPacsRequestAttempts"] == "25" + assert dqr["dqrWaitToRetryRequestInSeconds"] == "120" + assert dqr["pacsAvailabilityCheckFrequency"] == "1 minute" + assert dqr["allowAllProjectsToUseDqr"] is True + + +def test_a_rejected_availability_window_fails_the_run(tmp_path): + """A 400 that is not an overlap means the schedule was refused. + + Reporting it as applied is the worst outcome available: a trust that negotiated an + out-of-hours window would be told it was in force while DQR retrieved around the clock. + """ + code, _, output = run_configure( + tmp_path, + {"AVAIL_STATUS": "400", "AVAIL_BODY": '{"error":"Unknown day of week: FUNDAY"}'}, + ) + assert code != 0, "a refused availability window was reported as applied" + assert "availability" in output.lower() + + +def test_a_server_error_on_availability_fails_the_run(tmp_path): + code, _, output = run_configure(tmp_path, {"AVAIL_STATUS": "500", "AVAIL_BODY": '{"error":"boom"}'}) + assert code != 0, "a failed availability write was swallowed" + + +def test_an_overlapping_availability_interval_is_tolerated(tmp_path): + """DQR pre-creates intervals at registration, so this specific 400 is not a failure.""" + code, _, output = run_configure( + tmp_path, + { + "PACS_AVAILABILITY_DAYS": "MONDAY", + "AVAIL_STATUS": "400", + "AVAIL_BODY": '{"error":"probable overlap with existing interval"}', + }, + ) + assert code == 0, output + assert "already exists" in output + + +def test_a_registration_that_did_not_take_fails_rather_than_configuring_nothing(tmp_path): + """XNAT answering 200 without persisting is exactly how FLIP#822 stayed hidden.""" + code, _, output = run_configure(tmp_path, {"SWALLOW_PACS_POST": "1"}) + assert code != 0, "an unregistered PACS was treated as configured" + assert "not registered" in output + + +def test_drift_is_corrected_in_place_rather_than_re_created(tmp_path): + """A delete-and-recreate would change the PACS id under imaging-api's cache mid-flight.""" + code, payloads, output = run_configure( + tmp_path, + {"PACS_HOST": "10.0.0.10", "PACS_QR_PORT": "8059"}, + pacs_state=MOCK_PACS_REGISTRATION, + ) + assert code == 0, output + assert ("PUT", "http://xnat-web:8080/xapi/pacs/7") in requests_made(payloads) + assert not [u for m, u in requests_made(payloads) if m == "DELETE" and "/xapi/pacs/" in u] + + +def test_credentials_are_not_echoed_when_a_call_fails(tmp_path): + """The configure output is tee'd to a log file on the XNAT host.""" + # The password-rotation PUT: the one call whose -d body is itself a credential. + code, _, output = run_configure( + tmp_path, + {"FAIL_ON_URL": "/xapi/users/admin", "FAIL_STATUS": "500"}, + ) + assert code != 0, "a 500 from XNAT did not abort the run" + assert "" in output, "the failing request was echoed without redaction" + for secret in ("rotated", "initial", "service"): + assert secret not in output, f"the {secret!r} password reached the configure log" diff --git a/trust/xnat/tests/test_deploy_wiring.py b/trust/xnat/tests/test_deploy_wiring.py new file mode 100644 index 000000000..6f397f580 --- /dev/null +++ b/trust/xnat/tests/test_deploy_wiring.py @@ -0,0 +1,252 @@ +# Copyright (c) 2026 Guy's and St Thomas' NHS Foundation Trust & King's College London +# 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. +# +"""Tests for the layers that carry configuration *to* configure-xnat.sh (FLIP#993). + +test_configure_pacs.py proves the script does the right thing with the environment it is given. +Nothing proved it is given that environment, and every bug found while deploying this change lived +in the gap: the Makefile defaulted the web port to a literal that collided with a second trust, it +exported an empty AE title, and the compose file's ``${VAR:-default}`` cancelled the script's own +fail-loud guard. None of those are visible from inside the script. + +These run make and read the deployment files; they never invoke docker. +""" + +import os +import re +import subprocess +from pathlib import Path + +import pytest + +XNAT_DIR = Path(__file__).resolve().parents[1] +REPO_ROOT = XNAT_DIR.parents[1] +SCRIPT = XNAT_DIR / "xnat" / "config" / "configure-xnat.sh" +COMPOSE = XNAT_DIR / "docker-compose-stack.yml" +INIT_JOB = REPO_ROOT / "deploy" / "providers" / "kubernetes" / "templates" / "xnat-init-job.yaml" + + +def make_vars(*names: str, **overrides: str) -> dict[str, str]: + """Resolves Make variables by evaluating them in a throwaway target. + + ``--eval`` appends the target after the makefiles are read, so the values are the ones a real + deploy would use. Only variables are expanded — no recipe from the Makefile itself runs, so + this needs neither docker nor a swarm. + + Args: + *names: Make variable names to resolve. + **overrides: Command-line variable assignments (``KIT=GSTT``), as an operator would pass. + + Returns: + dict[str, str]: Resolved value per requested name. + """ + # One recipe line per variable: make splits a recipe on newlines, so a single multi-line echo + # reaches the shell as several unterminated commands. + probe = "__probe:\n" + "".join(f"\t@echo '{n}=$({n})'\n" for n in names) + result = subprocess.run( + ["make", "--eval", probe, "__probe", *(f"{k}={v}" for k, v in overrides.items())], + cwd=XNAT_DIR, + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 0, result.stdout + result.stderr + resolved = {} + for line in result.stdout.splitlines(): + key, _, value = line.partition("=") + if key in names: + resolved[key] = value + assert set(resolved) == set(names), f"unresolved: {set(names) - set(resolved)}" + return resolved + + +def configurable_variables() -> set[str]: + """Names the script reads with a bare-dash default — its operator-configurable knobs. + + ``${VAR-default}`` rather than ``${VAR:-default}`` is the script's marker for a value an + operator may set and which must never silently fall back when set empty. + """ + return set(re.findall(r'^([A-Z0-9_]+)="\$\{\1-', SCRIPT.read_text(), flags=re.MULTILINE)) + + +def compose_environment() -> dict[str, str]: + """The xnat-web service's ``environment:`` list, as {name: raw value}.""" + entries = {} + for line in COMPOSE.read_text().splitlines(): + match = re.match(r"\s*-\s+([A-Z0-9_]+)=(.*)$", line) + if match: + entries[match.group(1)] = match.group(2) + return entries + + +def test_every_script_knob_is_wired_through_compose(): + """A knob the compose file does not pass is one a kit file can set with no effect.""" + missing = configurable_variables() - set(compose_environment()) + assert not missing, f"configure-xnat.sh reads {sorted(missing)}, but docker-compose-stack.yml never passes them" + + +def test_every_script_knob_is_wired_through_the_helm_init_job(): + """Same contract on the Kubernetes path, which runs the same script from the same image. + + The two deployments drifted apart once already: imaging-api was given the DICOM port but not + the AE title, so k8s trusts ran with a receiver XNAT would not answer to. + """ + declared = set(re.findall(r"- name: ([A-Z0-9_]+)", INIT_JOB.read_text())) + missing = configurable_variables() - declared + assert not missing, f"configure-xnat.sh reads {sorted(missing)}, but xnat-init-job.yaml never sets them" + + +def test_compose_defaults_do_not_cancel_the_scripts_empty_check(): + """``${VAR:-default}`` substitutes for a set-but-empty variable; ``${VAR-default}`` does not. + + With the colon form an operator who writes ``PACS_HOST=`` in a kit file gets the mocked Orthanc + silently, which is precisely the fail-loud behaviour the script's guards exist to provide. + """ + offenders = { + name: value + for name, value in compose_environment().items() + if name in configurable_variables() and ":-" in value + } + assert not offenders, f"these cancel the script's guard by defaulting an empty value: {sorted(offenders)}" + + +def published_ports() -> list[str]: + """The compose file's ``ports:`` entries, comments and blanks skipped.""" + entries = [] + in_ports = False + for line in COMPOSE.read_text().splitlines(): + if re.match(r"\s*ports:\s*$", line): + in_ports = True + continue + if not in_ports: + continue + # Comments and blank lines sit between `ports:` and its entries; only a key at the same + # or lower indent ends the block. + if not line.strip() or line.lstrip().startswith("#"): + continue + if not re.match(r"\s*-\s", line): + in_ports = False + continue + entries.append(line.strip().lstrip("- ").strip()) + return entries + + +def test_published_ports_are_flat_references(): + """``docker stack deploy`` rejects a nested default in a ports entry. + + ``${XNAT_WEB_PORT:-${XNAT_PORT}}:8080`` fails the whole deploy with "Does not match format + 'ports'" — the fallback has to be resolved by the Makefile, not by compose. + """ + for entry in published_ports(): + assert not re.search(r"\$\{[^}]*\$\{", entry), f"nested substitution in a ports entry: {entry}" + + +def test_dicom_receiver_is_published_on_the_port_xnat_binds(): + """The receiver is published unconditionally, and on the same number both sides of the mapping. + + FLIP retrieves by DQR: after the C-MOVE the PACS opens a new association back to XNAT to + C-STORE the studies, and DQR matches that destination against a registered receiver by exact + AE title and port — so no host:container translation is possible on this leg. It used to be an + opt-in overlay; now every deployment publishes it so development runs the same wiring a + real-PACS trust relies on. + """ + entries = published_ports() + assert "${XNAT_PORT}:${XNAT_PORT}" in entries, f"DICOM receiver not published: {entries}" + assert "${XNAT_WEB_PORT}:8080" in entries, f"web UI not published: {entries}" + + +def test_web_port_defaults_to_the_dicom_port(): + """They were one variable until this change, so a kit that predates it sets only XNAT_PORT. + + Deriving the web port from it routes such a kit into the collision guard — a loud instruction + to allocate a second port. Defaulting to a literal instead would silently move that kit's web + UI to a number nothing else expects (and broke the second trust on a host: KCH publishing on + the literal rather than its own port collides with a running GSTT). + """ + resolved = make_vars("XNAT_PORT_EFFECTIVE", "XNAT_WEB_PORT_EFFECTIVE", XNAT_PORT="8106") + assert resolved["XNAT_WEB_PORT_EFFECTIVE"] == "8106" + + +def test_web_port_can_be_separated_from_the_dicom_port(): + """Publishing the receiver on the host needs the two on different ports.""" + resolved = make_vars("XNAT_PORT_EFFECTIVE", "XNAT_WEB_PORT_EFFECTIVE", XNAT_PORT="8104", XNAT_WEB_PORT="8080") + assert (resolved["XNAT_PORT_EFFECTIVE"], resolved["XNAT_WEB_PORT_EFFECTIVE"]) == ("8104", "8080") + + +def test_ae_title_defaults_when_unset(): + """An unset AE title must reach the script as XNAT, not as an empty string.""" + assert make_vars("XNAT_AETITLE_EFFECTIVE")["XNAT_AETITLE_EFFECTIVE"] == "XNAT" + + +def test_ae_title_set_empty_stays_empty(): + """The empty value has to survive make so the script's guard is the thing that reports it. + + Substituting the default here would hand the script a configured-looking value and move the + failure to whenever the PACS first tries to C-STORE back — a wrong AE title is not detectable + by any earlier step. + """ + assert make_vars("XNAT_AETITLE_EFFECTIVE", XNAT_AETITLE="")["XNAT_AETITLE_EFFECTIVE"] == "" + + +def run_xnat_reset(tmp_path, **overrides: str) -> subprocess.CompletedProcess: + """Runs the real xnat-reset recipe with its destructive half neutered. + + ``make -n`` is no use here: the port guards are shell ``if``s inside the recipe, so a dry run + prints them without ever deciding anything. So the recipe runs for real, but against a data + directory under tmp_path and with ``sudo`` stubbed to a no-op — a guard that stops working + then creates a directory in a temp dir instead of deleting a trust's XNAT archive. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + fake_sudo = bin_dir / "sudo" + fake_sudo.write_text("#!/bin/sh\nexit 0\n") + fake_sudo.chmod(0o755) + env = {**os.environ, "PATH": f"{bin_dir}:{os.environ['PATH']}"} + return subprocess.run( + [ + "make", + "xnat-reset", + "KIT=GSTT", + f"XNAT_DATA_DIR={tmp_path / 'data'}", + *(f"{k}={v}" for k, v in overrides.items()), + ], + cwd=XNAT_DIR, + env=env, + capture_output=True, + text=True, + timeout=60, + ) + + +@pytest.mark.parametrize("port_var", ["XNAT_PORT", "XNAT_WEB_PORT"]) +def test_non_numeric_ports_are_refused_before_deploy(tmp_path, port_var): + """A non-numeric port reaches docker as an unparseable ports entry, after the data wipe.""" + result = run_xnat_reset(tmp_path, **{port_var: "80a4"}) + assert result.returncode != 0, f"a non-numeric {port_var} was accepted" + assert port_var in result.stdout + result.stderr + + +def test_valid_ports_are_accepted(tmp_path): + """The negative cases above are only meaningful if the guard lets a good configuration through.""" + result = run_xnat_reset(tmp_path, XNAT_PORT="8104", XNAT_WEB_PORT="8080") + assert result.returncode == 0, result.stdout + result.stderr + + +def test_refuses_to_publish_both_services_on_one_port(tmp_path): + """The DICOM receiver is published alongside the web UI; one host port cannot serve both. + + This is also the fate of a kit that predates the FLIP#993 split and sets only XNAT_PORT: the + web port derives from it (see test_web_port_defaults_to_the_dicom_port), so the deploy stops + here with instructions instead of silently moving one of the services. + """ + result = run_xnat_reset(tmp_path, XNAT_PORT="8104", XNAT_WEB_PORT="8104") + assert result.returncode != 0, "the collision was accepted" + assert "must differ" in result.stdout + result.stderr diff --git a/trust/xnat/xnat/config/configure-xnat.sh b/trust/xnat/xnat/config/configure-xnat.sh index ec7699b72..bd630e343 100644 --- a/trust/xnat/xnat/config/configure-xnat.sh +++ b/trust/xnat/xnat/config/configure-xnat.sh @@ -28,10 +28,61 @@ set -euo pipefail : "${XNAT_ADMIN_USER:?}" "${XNAT_ADMIN_INITIAL_PASSWORD:?}" "${XNAT_ADMIN_PASSWORD:?}" : "${XNAT_SERVICE_USER:?}" "${XNAT_SERVICE_PASSWORD:?}" "${XNAT_PORT:?}" -# The below are fixed values for now -XNAT_URL="http://xnat-web:8080" # internal to Docker network -ORTHANC_HOST="orthanc" # name of the service (container) in docker-compose -ORTHANC_AETITLE="ORTHANC" +# ${VAR-default} rather than ${VAR:-default} throughout: an *unset* variable takes the default, +# but one set to the empty string stays empty and trips the guard below. An operator who writes +# PACS_HOST= in a kit file must get a loud failure, not a silent fallback to the mocked PACS. +# The bare-dash form is also the marker test_deploy_wiring.py derives the operator-knob roster +# from, so XNAT_URL keeps the colon form deliberately: it is container-network wiring that +# neither compose nor the Helm init job exposes to kit files, not a knob. +# +# XNAT's own identity and the upstream PACS. Defaults reproduce the mocked Orthanc that ships for +# development, so an unconfigured deployment behaves exactly as before; a real trust overrides them +# from its kit file (Compose) or Helm values (Kubernetes). +# +# XNAT_AETITLE is XNAT's AE title in three places that must agree: the DICOM SCP receiver, the DQR +# calling AE, and the C-MOVE destination that imaging-api hands to the PACS. The PACS opens the +# C-STORE association addressed to the AE title it has registered, so a receiver configured under a +# different title rejects it. +XNAT_URL="${XNAT_URL:-http://xnat-web:8080}" # internal to the container network +XNAT_AETITLE="${XNAT_AETITLE-XNAT}" +PACS_HOST="${PACS_HOST-orthanc}" # service name in compose / k8s, or a real PACS host +PACS_AETITLE="${PACS_AETITLE-ORTHANC}" +PACS_QR_PORT="${PACS_QR_PORT-4242}" +PACS_LABEL="${PACS_LABEL-Test PACS instance}" +# Relational queries / extended negotiation. On by default because both the mocked Orthanc and +# every PACS we have integrated with support it, but it is a genuine per-PACS capability: a PACS +# that does not support it must have this off or it rejects the association (FLIP#993). +PACS_SUPPORTS_EXTENDED_NEGOTIATIONS="${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS-true}" + +# DQR retry behaviour and the PACS availability schedule — the throttle for a production PACS, which +# may refuse further associations after a certain volume (FLIP#993). Defaults are today's values. +DQR_MAX_PACS_REQUEST_ATTEMPTS="${DQR_MAX_PACS_REQUEST_ATTEMPTS-100}" +DQR_RETRY_WAIT_SECONDS="${DQR_RETRY_WAIT_SECONDS-300}" +PACS_AVAILABILITY_DAYS="${PACS_AVAILABILITY_DAYS-MONDAY,TUESDAY,WEDNESDAY,THURSDAY,FRIDAY,SATURDAY,SUNDAY}" +PACS_AVAILABILITY_START="${PACS_AVAILABILITY_START-00:00}" +PACS_AVAILABILITY_END="${PACS_AVAILABILITY_END-24:00}" +PACS_THREADS="${PACS_THREADS-1}" +PACS_UTILIZATION_PERCENT="${PACS_UTILIZATION_PERCENT-100}" + +# Same fail-loud contract as the credentials above: a default must never resolve to empty, or the +# interpolated JSON is malformed and XNAT rejects it silently (FLIP#822 / FLIP#862). +: "${XNAT_URL:?}" "${XNAT_AETITLE:?}" "${PACS_HOST:?}" "${PACS_AETITLE:?}" "${PACS_QR_PORT:?}" +: "${PACS_LABEL:?}" "${DQR_MAX_PACS_REQUEST_ATTEMPTS:?}" "${DQR_RETRY_WAIT_SECONDS:?}" +: "${PACS_AVAILABILITY_DAYS:?}" "${PACS_AVAILABILITY_START:?}" "${PACS_AVAILABILITY_END:?}" +: "${PACS_THREADS:?}" "${PACS_UTILIZATION_PERCENT:?}" "${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS:?}" + +# Validated here rather than left to jq: --argjson accepts any JSON, so a "yes" or "True" would +# fail inside the filter with a parse error that names neither the variable nor the accepted +# values, and a bare `1` would silently register as the number 1 rather than a boolean. +case "${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS}" in + true|false) ;; + *) echo "ERROR: PACS_SUPPORTS_EXTENDED_NEGOTIATIONS must be true or false, got '${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS}'" >&2; exit 1 ;; +esac + +# jq parses the /xapi/dicomscp and /xapi/pacs listings below. It ships in the xnat-web image +# (trust/xnat/xnat/Dockerfile), but fail loudly here rather than let a missing binary degrade into a +# silently-empty lookup that would re-register a PACS that already exists. +command -v jq >/dev/null || { echo "ERROR: jq is required by configure-xnat.sh" >&2; exit 1; } # Wait for XNAT to be available (wall-clock bounded, and each probe carries # its own timeout, so a dead or wedged XNAT fails the deploy loudly instead @@ -254,17 +305,18 @@ echo "Configuring DQR plugin..." xnat_curl -X POST "$XNAT_URL/xapi/dqr/settings" \ -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ -H "Content-Type: application/json" \ - -d '{ - "pacsAvailabilityCheckFrequency": "1 minute", - "dqrWaitToRetryRequestInSeconds": "300", - "assumeSameSessionIfArrivedWithin": "30 minutes", - "allowAllUsersToUseDqr": false, - "dqrCallingAe": "XNAT", - "notifyAdminOnImport": false, - "allowAllProjectsToUseDqr": true, - "leavePacsAuditTrail": false, - "dqrMaxPacsRequestAttempts": "100" - }' + -d "$(jq -n --arg ae "${XNAT_AETITLE}" \ + --argjson retry "${DQR_RETRY_WAIT_SECONDS}" --argjson attempts "${DQR_MAX_PACS_REQUEST_ATTEMPTS}" '{ + pacsAvailabilityCheckFrequency: "1 minute", + dqrWaitToRetryRequestInSeconds: ($retry | tostring), + assumeSameSessionIfArrivedWithin: "30 minutes", + allowAllUsersToUseDqr: false, + dqrCallingAe: $ae, + notifyAdminOnImport: false, + allowAllProjectsToUseDqr: true, + leavePacsAuditTrail: false, + dqrMaxPacsRequestAttempts: ($attempts | tostring) + }')" # Configure site-wide anonymization script echo "Configuring site-wide anonymization script..." @@ -280,60 +332,67 @@ xnat_curl -X PUT "$XNAT_URL/xapi/anonymize/site/enabled" \ -H "Content-Type: application/json" \ -d 'true' -# Get SCP receivers +# Remove the pre-existing SCP receivers this script owns, so the POST below can re-create exactly +# one. Ownership is a property of the receiver itself, not of the port it binds or the title it +# carries: every receiver FLIP creates is stamped with identifier "dqrObjectIdentifier" by the POST +# below, and XNAT's stock receiver — created by the webapp on first boot, always called "XNAT" — is +# the other one that has to go. `GET /xapi/dicomscp` returns `identifier` per receiver, so both +# markers are readable from the listing. +# +# Matching instead on "our port or our AE title" left an orphan whenever both moved in one change: +# an existing FLIPXNAT:8104 under new config XNAT_AETITLE=FLIPXNAT2 XNAT_PORT=11112 matched neither, +# so the old receiver stayed enabled and bound (FLIP#993). +# +# A receiver an operator registered for some other local DICOM source carries neither marker and is +# left alone — it would otherwise be deleted on every redeploy and `helm upgrade`, with the deletion +# visible only in a Job pod log that is discarded on success. If such a receiver is squatting the +# port we are about to bind, it survives to the POST below, which then fails loud through xnat_curl: +# deliberately the same choice the PACS-side guard makes further down — refuse rather than silently +# delete configuration this deployment may not own. +# +# Both markers are literals in the filter, so nothing operator-supplied is spliced into jq. response=$(xnat_curl -u "$XNAT_ADMIN_USER:$XNAT_ADMIN_PASSWORD" "$XNAT_URL/xapi/dicomscp") -# Debug: Print Raw Response -echo "Raw API Response: $response" - -# Check if response is empty or invalid if [[ -z "$response" || "$response" == "[]" ]]; then echo "No SCP receivers found." else - # Extract the SCP receiver ID with "aeTitle": "XNAT" using grep and sed - scp_receiver_data=$(echo "$response" | grep -o '{[^}]*"aeTitle"[^}]*}' | grep '"aeTitle":"XNAT"') - echo "SCP Receiver Data: $scp_receiver_data" - - if [[ -n "$scp_receiver_data" ]]; then - # Extract the "id" field of the SCP receiver - scp_receiver_id=$(echo "$scp_receiver_data" | sed -n 's/.*"id":\([0-9]\+\).*/\1/p') + stale_ids=$(printf '%s' "$response" \ + | jq -r '.[] | select(.identifier == "dqrObjectIdentifier" or .aeTitle == "XNAT") + | "\(.id):\(.aeTitle):\(.port)"') - if [[ -n "$scp_receiver_id" ]]; then - echo "Removing SCP Receiver with ID: $scp_receiver_id..." - - # Send DELETE request to remove the SCP receiver - delete_response=$(xnat_curl -u "$XNAT_ADMIN_USER:$XNAT_ADMIN_PASSWORD" -X DELETE "$XNAT_URL/xapi/dicomscp/$scp_receiver_id") - - echo "Delete Response: $delete_response" - echo "SCP Receiver removed successfully." - else - echo "Failed to extract SCP receiver ID." - fi + if [[ -z "$stale_ids" ]]; then + echo "No existing SCP receiver to replace." else - echo "No SCP receiver with aeTitle='XNAT' found." + while IFS=: read -r scp_receiver_id scp_receiver_ae scp_receiver_port; do + [[ -n "$scp_receiver_id" ]] || continue + echo "Removing SCP receiver '${scp_receiver_ae}:${scp_receiver_port}' (id ${scp_receiver_id})..." + xnat_curl -u "$XNAT_ADMIN_USER:$XNAT_ADMIN_PASSWORD" \ + -X DELETE "$XNAT_URL/xapi/dicomscp/$scp_receiver_id" >/dev/null + done <<< "$stale_ids" fi fi -# Configure SCP receiver to have dqrObjectIdentifier as the identifier (the default is not) -echo "Configuring SCP receiver..." +# Configure SCP receiver to have dqrObjectIdentifier as the identifier (the default is not). That +# identifier is also what marks the receiver as FLIP-owned for the reclamation above. +echo "Configuring SCP receiver '${XNAT_AETITLE}' on port ${XNAT_PORT}..." xnat_curl -X POST "$XNAT_URL/xapi/dicomscp" \ -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ -H "Content-Type: application/json" \ - -d "{ - \"aeTitle\": \"XNAT\", - \"port\": ${XNAT_PORT}, - \"enabled\": true, - \"customProcessing\": true, - \"directArchive\": true, - \"identifier\": \"dqrObjectIdentifier\", - \"anonymizationEnabled\": true, - \"whitelistEnabled\": false, - \"whitelistText\": \"\", - \"routingExpressionsEnabled\": false, - \"projectRoutingExpression\": \"\", - \"subjectRoutingExpression\": \"\", - \"sessionRoutingExpression\": \"\" - }" + -d "$(jq -n --arg ae "${XNAT_AETITLE}" --argjson port "${XNAT_PORT}" '{ + aeTitle: $ae, + port: $port, + enabled: true, + customProcessing: true, + directArchive: true, + identifier: "dqrObjectIdentifier", + anonymizationEnabled: true, + whitelistEnabled: false, + whitelistText: "", + routingExpressionsEnabled: false, + projectRoutingExpression: "", + subjectRoutingExpression: "", + sessionRoutingExpression: "" + }')" # Configure OHIF viewer echo "Configuring OHIF viewer..." @@ -342,63 +401,163 @@ xnat_curl -X POST "$XNAT_URL/xapi/siteConfig" \ -H "Content-Type: application/json" \ -d '{"addOhifViewLinkToProjectListingDefaults": true }' -# Register PACS -# queryRetrievePort is the port XNAT dials Orthanc on over the Docker network -# (the "host" below is the orthanc service name), so Orthanc's fixed container -# port 4242 is the only correct value. The old ${PACS_DICOM_PORT} indirection -# was nominally the *host-published* DICOM port — its "${PACS_DICOM_PORT}:4242" -# mappings are commented out in every compose file — so a kit setting it to -# anything but 4242 silently broke registration (FLIP#822 / FLIP#862). -# Check-then-create: a duplicate registration surfaces as an unspecific 500 -# (DB unique-constraint violation), so re-run idempotency is a lookup by -# aeTitle rather than a tolerated status code. +# Register the upstream PACS. +# +# queryRetrievePort is the port XNAT dials the PACS on and must be the port that is actually +# reachable from the XNAT container: the mock Orthanc's fixed container port 4242 over the container +# network, or the trust PACS's real query/retrieve port. The retired ${PACS_DICOM_PORT} variable +# meant the *host-published* port, which is not the same thing, so a kit setting it silently broke +# registration (FLIP#822 / FLIP#862) — hence PACS_QR_PORT is guarded non-empty above and documented +# as the reachable port. +# +# A trust XNAT talks to exactly one PACS, so this script owns the registration list: the configured +# PACS is created or updated in place, and any *other* registration is removed. Two things make that +# the right behaviour rather than merely tidy. A duplicate surfaces as an unspecific 500 (DB +# unique-constraint violation) if we re-POST, and — more importantly — we set +# defaultQueryRetrievePacs on ours, so a leftover entry claiming the same default leaves DQR's +# choice of PACS ambiguous. Changing PACS_AETITLE would otherwise strand the old entry exactly as a +# changed XNAT_AETITLE used to strand the old SCP receiver. +# +# Updating in place rather than delete-and-recreate keeps the PACS id stable, and means a kit change +# actually takes effect: the previous check-then-create silently ignored a drifted host or port on +# redeploy, leaving DQR pointing at the old PACS with no signal to the operator. +# Built with jq rather than string interpolation. Splicing operator-supplied values straight into +# JSON is an injection point: a PACS_HOST of `x", "defaultQueryRetrievePacs": false, "z": "y` +# produces *valid* JSON whose duplicate key wins, silently disabling the flag on the registration +# this script exists to make authoritative; a label containing a quote produces malformed JSON that +# XNAT rejects with an opaque 400 mid-run. --arg/--argjson keep them data. --argjson also parses the +# numeric fields, so a non-numeric port fails here naming the variable, rather than as an +# unexplained 400 (FLIP#993). +pacs_payload=$(jq -n \ + --arg ae "${PACS_AETITLE}" \ + --arg host "${PACS_HOST}" \ + --arg label "${PACS_LABEL}" \ + --argjson port "${PACS_QR_PORT}" \ + --argjson ext "${PACS_SUPPORTS_EXTENDED_NEGOTIATIONS}" \ + '{ + aeTitle: $ae, + defaultQueryRetrievePacs: true, + defaultStoragePacs: true, + host: $host, + label: $label, + ormStrategySpringBeanId: "dicomOrmStrategy", + queryRetrievePort: $port, + queryable: true, + storable: true, + supportsExtendedNegotiations: $ext + }') + existing_pacs=$(xnat_curl -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" "$XNAT_URL/xapi/pacs") -if printf '%s' "$existing_pacs" | grep -q "\"aeTitle\":\"${ORTHANC_AETITLE}\""; then - echo "PACS '${ORTHANC_AETITLE}' already registered — leaving as-is." -else - echo "Registering PACS..." +pacs_entry=$(printf '%s' "$existing_pacs" | jq -c --arg ae "${PACS_AETITLE}" \ + 'map(select(.aeTitle == $ae)) | .[0] // empty') + +# Remove every registration that is not the configured one, so exactly one survives. Logged per +# entry: this deletes configuration an administrator may have added through the XNAT UI, and that +# should be visible in the deploy output rather than silent. +# +# Refuse rather than delete when the configuration is still the shipped default. An operator whose +# XNAT already carries a hand-registered real PACS, running an unrelated upgrade without having set +# PACS_AETITLE, would otherwise have that registration deleted and replaced with the mock — a +# retrieval outage caused by a deploy that changed nothing else. Deleting is right only once the +# operator has said which PACS is theirs (FLIP#993). +foreign_pacs=$(printf '%s' "$existing_pacs" \ + | jq -r --arg ae "${PACS_AETITLE}" 'map(select(.aeTitle != $ae)) | .[] | @base64') +if [[ -n "$foreign_pacs" ]]; then + if [[ "${PACS_AETITLE}" == "ORTHANC" && "${PACS_HOST}" == "orthanc" ]]; then + echo "ERROR: XNAT has PACS registrations other than the configured one, but PACS_AETITLE and" >&2 + echo " PACS_HOST are still the mocked-Orthanc defaults. Refusing to delete a registration" >&2 + echo " this deployment may not own. Set PACS_AETITLE/PACS_HOST to the trust's PACS, or" >&2 + echo " remove the unwanted registration in XNAT's admin UI. Found:" >&2 + printf '%s\n' "$foreign_pacs" | while read -r entry; do + [[ -n "$entry" ]] || continue + printf ' %s\n' "$(printf '%s' "$entry" | base64 -d \ + | jq -r '"\(.aeTitle) at \(.host):\(.queryRetrievePort) (id \(.id))"')" >&2 + done + exit 1 + fi + while read -r entry; do + [[ -n "$entry" ]] || continue + # base64 per record: an IPv6 host contains colons, which a ':'-delimited read would mis-split. + stale=$(printf '%s' "$entry" | base64 -d) + stale_id=$(printf '%s' "$stale" | jq -r '.id') + echo "Removing PACS $(printf '%s' "$stale" | jq -r '"\(.aeTitle) at \(.host):\(.queryRetrievePort) (id \(.id))"'):" \ + "a trust XNAT retrieves from one PACS, and '${PACS_AETITLE}' is the configured one." + xnat_curl -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ + -X DELETE "$XNAT_URL/xapi/pacs/${stale_id}" >/dev/null + done <<< "$foreign_pacs" +fi + +if [[ -z "$pacs_entry" ]]; then + echo "Registering PACS '${PACS_AETITLE}' at ${PACS_HOST}:${PACS_QR_PORT}..." xnat_curl -X POST "$XNAT_URL/xapi/pacs" \ -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ -H "Content-Type: application/json" \ - -d "{ - \"aeTitle\": \"${ORTHANC_AETITLE}\", - \"defaultQueryRetrievePacs\": true, - \"defaultStoragePacs\": true, - \"host\": \"${ORTHANC_HOST}\", - \"label\": \"Test PACS instance\", - \"ormStrategySpringBeanId\": \"dicomOrmStrategy\", - \"queryRetrievePort\": 4242, - \"queryable\": true, - \"storable\": true, - \"supportsExtendedNegotiations\": true - }" + -d "$pacs_payload" +else + PACS_ID=$(printf '%s' "$pacs_entry" | jq -r '.id') + + # Drift is judged over every kit-managed field, not just host+port: label and + # supportsExtendedNegotiations come from the kit too, and the k8s init job re-runs this script on + # every helm upgrade against persistent XNAT data — a narrower comparison logs "leaving as-is" + # and keeps the old flag forever, exactly the silently-ignored kit change the in-place update + # exists to prevent. Comparison against the desired payload rather than the env vars keeps type + # handling in jq (port and the flag are JSON number/boolean in both documents, not strings). + pacs_drift=$(jq -cn --argjson entry "$pacs_entry" --argjson desired "$pacs_payload" \ + '[("host","queryRetrievePort","label","supportsExtendedNegotiations") + | select($entry[.] != $desired[.])]') + + if [[ "$pacs_drift" == "[]" ]]; then + echo "PACS '${PACS_AETITLE}' already registered at ${PACS_HOST}:${PACS_QR_PORT} — leaving as-is." + else + echo "PACS '${PACS_AETITLE}' drifted on $(printf '%s' "$pacs_drift" | jq -r 'join(", ")')," \ + "updating to ${PACS_HOST}:${PACS_QR_PORT}..." + xnat_curl -X PUT "$XNAT_URL/xapi/pacs/${PACS_ID}" \ + -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ + -H "Content-Type: application/json" \ + -d "$pacs_payload" + fi fi -# Configure PACS availability schedule (all days). DQR appears to pre-create -# availability intervals when the PACS is registered: on XNAT 1.10 + DQR 3.0.0 -# this POST returns 400 "probable overlap with existing interval" for an -# already-scheduled day, so 400 is treated as "already configured" rather than -# a failure. Anything else non-2xx is a real error and fails the deploy. -for DAY in MONDAY TUESDAY WEDNESDAY THURSDAY FRIDAY SATURDAY SUNDAY; do +# The availability schedule below is written against the registered PACS, so resolve its id whether +# it was just created or already existed. +PACS_ID=$(xnat_curl -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" "$XNAT_URL/xapi/pacs" \ + | jq -r --arg ae "${PACS_AETITLE}" 'map(select(.aeTitle == $ae)) | .[0].id // empty') +: "${PACS_ID:?PACS '${PACS_AETITLE}' is not registered after configuration}" + +# Configure the PACS availability schedule — the throttle for a production PACS, which may refuse +# further associations after a certain volume, and which a trust may want restricted to out-of-hours +# (FLIP#993). Defaults are all week, all day, one thread. +# +# DQR appears to pre-create availability intervals when the PACS is registered: on XNAT 1.10 + +# DQR 3.0.0 this POST returns 400 "probable overlap with existing interval" for an already-scheduled +# day, so that specific 400 is treated as "already configured" rather than a failure. +# +# The overlap text is matched, not the bare status: the payload is now built from operator-supplied +# values, so a bad day name or window also returns 400. Treating every 400 as an overlap reported a +# rejected schedule as applied, and a trust that had negotiated an out-of-hours window would have +# been told it was in force while DQR retrieved around the clock (FLIP#993). +for DAY in ${PACS_AVAILABILITY_DAYS//,/ }; do echo "Setting PACS availability for $DAY..." avail_body=/tmp/pacs-availability-response.json avail_status=$(curl -s -o "$avail_body" --connect-timeout 10 --max-time 120 -w '%{http_code}' \ - -X POST "$XNAT_URL/xapi/pacs/1/availability" \ + -X POST "$XNAT_URL/xapi/pacs/${PACS_ID}/availability" \ -u "${XNAT_ADMIN_USER}:${XNAT_ADMIN_PASSWORD}" \ -H "Content-Type: application/json" \ - -d "{ - \"availabilityEnd\": \"24:00\", - \"availabilityStart\": \"00:00\", - \"availableNow\": true, - \"dayOfWeek\": \"$DAY\", - \"enabled\": true, - \"pacsId\": 1, - \"threads\": 1, - \"utilizationPercent\": 100 - }") || avail_status="000" + -d "$(jq -n --arg start "${PACS_AVAILABILITY_START}" --arg end "${PACS_AVAILABILITY_END}" \ + --arg day "$DAY" --argjson pacs "${PACS_ID}" --argjson threads "${PACS_THREADS}" \ + --argjson util "${PACS_UTILIZATION_PERCENT}" '{ + availabilityEnd: $end, + availabilityStart: $start, + availableNow: true, + dayOfWeek: $day, + enabled: true, + pacsId: $pacs, + threads: $threads, + utilizationPercent: $util + }')") || avail_status="000" if [[ "$avail_status" == 2* ]]; then continue - elif [[ "$avail_status" == "400" ]]; then + elif [[ "$avail_status" == "400" ]] && grep -qi 'overlap' "$avail_body"; then echo " Availability interval for $DAY already exists (HTTP 400 overlap) — leaving as-is." else echo "ERROR: setting PACS availability for $DAY failed (HTTP $avail_status)" >&2