Skip to content
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5d0b6fe
feat(trust): make the PACS connection configurable (#993)
atriaybagur Aug 18, 2026
e0e8129
feat(k8s): expose XNAT's DICOM receiver and unify the XNAT config (#993)
atriaybagur Aug 18, 2026
eddb411
fix(trust): correct the port split found by deploying it (#993)
atriaybagur Aug 18, 2026
06cf709
refactor(trust): own the PACS registration list, one PACS per XNAT (#…
atriaybagur Aug 18, 2026
d6c234c
docs: scope the inbound-connections claim to the internet and the hub…
atriaybagur Aug 18, 2026
89a4cf2
docs: give PACS its own component page (#993)
atriaybagur Aug 19, 2026
2ead070
docs: list PACS after XNAT in the components index
atriaybagur Aug 19, 2026
fd320cd
docs: stop the PACS defaults reading as instructions to the trust (#993)
atriaybagur Aug 19, 2026
4ef0de1
docs: complete the list of what to ask a trust's PACS team (#993)
atriaybagur Aug 19, 2026
010237d
docs: stop XNAT_WEB_PORT's default reading as a literal value (#993)
atriaybagur Aug 19, 2026
426180f
fix: close five silent-failure bugs found in review (#993)
atriaybagur Aug 19, 2026
fa87015
fix: address the important-tier review findings (#993)
atriaybagur Aug 19, 2026
6ea694f
test: close the mutation gaps, and cover the wiring around the script…
atriaybagur Aug 19, 2026
5d3adce
docs: scope the inbound-connection claim to on-premises trusts (#993)
atriaybagur Aug 19, 2026
0d81f17
fix: address the Copilot review findings (#993)
atriaybagur Aug 19, 2026
96fe3e4
fix: keep XNAT_URL on the colon-dash form — it is wiring, not a knob …
atriaybagur Aug 19, 2026
9c71d05
feat: always publish the DICOM SCP receiver, retiring the REAL_PACS o…
atriaybagur Aug 19, 2026
1acbe1e
docs: align the mocked-PACS paragraph with the always-published recei…
atriaybagur Aug 19, 2026
764f4c0
fix: scope SCP receiver reclamation and guard the DICOM return leg (#…
atriaybagur Aug 21, 2026
8469c5b
review: refuse a real-PACS render on a ClusterIP service; document th…
atriaybagur Aug 26, 2026
3eab64b
Merge remote-tracking branch 'origin/develop' into 993-real-pacs-support
atriaybagur Aug 27, 2026
277ec6b
fix(xnat): compare every kit-managed PACS field in the in-place drift…
atriaybagur Aug 27, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions .github/workflows/test_helm_chart.yml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,70 @@ 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

# 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
Expand Down
13 changes: 9 additions & 4 deletions .github/workflows/test_trust_xnat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
on:
push:
branches: [main, develop]
Expand All @@ -26,6 +27,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"
Expand All @@ -44,6 +47,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"
Expand Down
23 changes: 23 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,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/<backend>/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/<backend>/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=<N>` (flower). To run a backend standalone + submit without the full stack: `make -C fl-services/<backend> up` (or `up-secure`) then `make -C fl-services/<backend> submit APP=<job>`.
- `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 `<FL_APP_BASE_DIR>/<backend>/<job_type>/` to bundle an application (uploading those files into `FL_APP_DESTINATION_BUCKET/<model_id>`) and reads each backend's manifest from `<FL_APP_BASE_DIR>/<backend>/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://<AICENTRE_BUCKET_NAME>/fl-flare-participant-kits/<FLARE_KIT_DATE>/net-<N>/services/<slot>/` 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=<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)
- `AES_KEY_BASE64` — encryption key for trust communication
- A remote trust operator only needs their kit file (`trust/.env.<KIT>`) — no hub `.env.<env>` needed on trust hosts.
Expand Down
23 changes: 23 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,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/<backend>/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/<backend>/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=<N>` (flower). To run a backend standalone + submit without the full stack: `make -C fl-services/<backend> up` (or `up-secure`) then `make -C fl-services/<backend> submit APP=<job>`.
- `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 `<FL_APP_BASE_DIR>/<backend>/<job_type>/` to bundle an application (uploading those files into `FL_APP_DESTINATION_BUCKET/<model_id>`) and reads each backend's manifest from `<FL_APP_BASE_DIR>/<backend>/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://<AICENTRE_BUCKET_NAME>/fl-flare-participant-kits/<FLARE_KIT_DATE>/net-<N>/services/<slot>/` 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=<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)
- `AES_KEY_BASE64` — encryption key for trust communication
- A remote trust operator only needs their kit file (`trust/.env.<KIT>`) — no hub `.env.<env>` needed on trust hosts.
Expand Down
2 changes: 1 addition & 1 deletion deploy/providers/AWS/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,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 |
Expand Down
Loading
Loading