Add multi-instance Docker manager for isolated auth profiles#312
Add multi-instance Docker manager for isolated auth profiles#312ShutovKS wants to merge 2 commits intoCJackHwang:mainfrom
Conversation
Add a reusable multi-instance Docker manager that launches one container per saved auth profile with isolated runtime directories and documented operational guidance.\n\nCloses CJackHwang#300
There was a problem hiding this comment.
Pull request overview
Adds an external multi-instance Docker launcher to run one container per saved auth profile, plus documentation references for operating it.
Changes:
- Introduces
scripts/multi-instance-manager/run-multi-instance.shto discoverauth_profiles/saved/*.jsonand launch one Docker container per profile with port allocation and safety checks. - Adds usage/operational documentation for the multi-instance manager and links it from existing docs/README.
- Ignores the runtime state directory (
.multi-instance-runtime/) created by the script.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/multi-instance-manager/run-multi-instance.sh | New multi-container launcher with per-profile container creation, port checks, and runtime profile mounting logic |
| scripts/multi-instance-manager/README.md | New guide describing behavior, options, and operational workflows for the launcher |
| docs/deployment-and-operations.md | Adds a deployment/ops section pointing to the multi-instance manager |
| README.md | Adds a top-level link to the multi-instance manager docs |
| .gitignore | Adds .multi-instance-runtime/ to prevent committing runtime artifacts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cp "$profile_path" "$runtime_active_host_path" | ||
|
|
There was a problem hiding this comment.
--dry-run still copies the profile into .multi-instance-runtime/active/ because cp runs unconditionally before the dry-run check. This makes dry-run mode have side effects and can overwrite files. Gate the copy (and any other mutations) behind DRY_RUN != true, or simulate the copy when dry-running without touching the filesystem.
| -v "$PROJECT_ROOT/auth_profiles:/app/auth_profiles" | ||
| -v "$runtime_active_host_path:${container_active_profile_path}:ro" |
There was a problem hiding this comment.
This mounts the entire host auth_profiles/ directory into every container (and writable), so each instance can read/modify other profiles on disk. That conflicts with the PR goal of “dedicated auth profile mounting / isolated auth profiles”. If strict isolation is intended, mount only the selected saved profile file (and the per-container active file) and keep the rest read-only or container-local.
| This dual-path startup is intentional. Although [`launcher/runner.py::_resolve_auth_file_path()`](../../launcher/runner.py:270) and [`browser_utils/initialization/core.py::initialize_page_logic()`](../../browser_utils/initialization/core.py:48) can consume [`ACTIVE_AUTH_JSON_PATH`](../../launcher/runner.py:413), real Docker headless startup may still traverse logic that expects at least one file under [`auth_profiles/active/`](../../auth_profiles/active/). The runtime copy avoids polluting the shared host [`auth_profiles/active/`](../../auth_profiles/active/) pool while still satisfying that container-local requirement. | ||
|
|
There was a problem hiding this comment.
These Markdown links use runner.py:270 / core.py:48 style suffixes, which GitHub won’t interpret as line anchors (it expects #L270, etc.). Update the links to proper GitHub line anchors or remove the line-specific suffix so the references don’t 404.
|
|
||
| 1. It mounts the shared [`auth_profiles/`](../../auth_profiles/) tree into the container. | ||
| 2. It sets `ACTIVE_AUTH_JSON_PATH=/app/auth_profiles/saved/<profile>.json` for that specific container. | ||
| 3. It copies the selected host profile into [` .multi-instance-runtime/active/`](../../.multi-instance-runtime/) and bind-mounts that copy as `/app/auth_profiles/active/<profile>.json` inside the container. |
There was a problem hiding this comment.
There’s an extra leading space inside the inline-code link text ([ .multi-instance-runtime/active/]), which renders as a different path and looks like a typo. Remove the leading space so the path displays consistently as .multi-instance-runtime/active/.
docs/deployment-and-operations.md
Outdated
|
|
||
| 如需为多个已保存认证同时启动隔离容器，可使用外部管理脚本 [`scripts/multi-instance-manager/README.md`](../scripts/multi-instance-manager/README.md)。 | ||
|
|
||
| 当前 multi-instance 管理器会为每个容器显式设置独立的 `ACTIVE_AUTH_JSON_PATH`，并把选中的 profile 复制到 [` .multi-instance-runtime/active/`](../.multi-instance-runtime/) 后再挂载为容器内的 [`auth_profiles/active/`](../auth_profiles/active/) 单文件。默认同时关闭启动时自动轮转与运行时自动轮转，这样既能维持一容器一认证隔离，也能满足 Docker headless 启动阶段对 active-dir 的运行时要求。 |
There was a problem hiding this comment.
There’s an extra leading space inside the inline-code link text ([ .multi-instance-runtime/active/]), which renders as a different path and looks like a typo. Remove the leading space so the path displays consistently as .multi-instance-runtime/active/.
| print_summary() { | ||
| log "" | ||
| log "✅ Done. Processed profiles: ${#PROFILE_FILES[@]}" | ||
| log "🔑 Shared API key: ${API_KEY}" |
There was a problem hiding this comment.
print_summary logs the full value of API_KEY to stdout via log "🔑 Shared API key: ${API_KEY}", which can expose a real production API key in terminal history or aggregated CI logs. Anyone with access to those logs could reuse the key to impersonate this service or access upstream resources. Avoid printing sensitive secrets; if you need to indicate that an API key is configured, log a redacted or boolean status instead of the raw value.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CONTAINER_PREFIX="${CONTAINER_PREFIX:-ai-proxy}" | ||
| API_KEY="${API_KEY:-sk-dummy}" | ||
| SERVER_LOG_LEVEL="${SERVER_LOG_LEVEL:-INFO}" |
There was a problem hiding this comment.
The script treats --api-key/API_KEY as the “shared API key”, but the server’s API-key enforcement is based on auth_profiles/key.txt (see api_utils/auth_utils.py), and I can’t find any runtime code that reads an API_KEY env var. As-is, this option is likely a no-op and may give a false sense of auth being enabled; consider either wiring this flag to write/mount a per-container key.txt, or remove the flag/docs and instruct users to manage auth_profiles/key.txt instead.
| local container_active_profile_path="/app/auth_profiles/active/${profile_name}.json" | ||
|
|
||
| cp "$profile_path" "$runtime_active_host_path" | ||
|
|
||
| local -a docker_args=( |
There was a problem hiding this comment.
--dry-run still executes cp "$profile_path" "$runtime_active_host_path", which writes a copy of potentially sensitive auth JSONs into .multi-instance-runtime/ even though the user asked for a preview. Consider skipping the copy when DRY_RUN=true (or at least making it explicit) and ensuring the runtime copy is created with restrictive permissions (e.g., chmod 600 / umask 077) since it contains credentials.
| -p "127.0.0.1:${api_port}:2048" | ||
| -p "127.0.0.1:${stream_port}:3120" | ||
| -v "$PROJECT_ROOT/auth_profiles:/app/auth_profiles" | ||
| -v "$runtime_active_host_path:${container_active_profile_path}:ro" | ||
| -e "ACTIVE_AUTH_JSON_PATH=${container_profile_path}" |
There was a problem hiding this comment.
-v "$PROJECT_ROOT/auth_profiles:/app/auth_profiles" mounts the entire host auth profile tree into every container (including other saved/active profiles and key.txt), which weakens the “one container per auth profile” isolation goal. If isolation is a requirement, consider mounting only the selected saved profile + a container-specific active/runtime location (or mount the directory read-only and ensure only the intended files are accessible).
| local container_name="$1" | ||
| local port="$2" | ||
|
|
||
| docker ps --filter "name=^${container_name}$" --format '{{.Ports}}' | grep -E "(^|[ ,])127\.0\.0\.1:${port}->|(^|[ ,])0\.0\.0\.0:${port}->|(^|[ ,])\[::\]:${port}->" >/dev/null 2>&1 | ||
| } |
There was a problem hiding this comment.
docker ps --filter "name=^${container_name}$" uses a regex name filter; since sanitize_name allows . and other regex metacharacters, this can mis-match containers and make the “port already in use by same container” check unreliable. Consider avoiding regex filters here (e.g., inspect by exact name via docker inspect, or list names/ports and match with grep -Fx on the name column).
| #!/usr/bin/env bash | ||
|
|
||
| set -euo pipefail | ||
|
|
There was a problem hiding this comment.
PR description says “workflow/docs changes only”, but this PR adds an operational launcher script (run-multi-instance.sh) that affects how users run the service. Please update the PR description/testing note to reflect that executable logic was added (and ideally include at least a basic smoke test plan, e.g., --dry-run + one-profile run).
| | `--docker-env-file PATH` | Path to mounted Docker env file | `docker/.env` | | ||
| | `--image NAME` | Docker image name | `ai-studio-proxy:latest` with automatic fallback to `docker-ai-studio-proxy:latest` after a standard `docker compose build` | | ||
| | `--container-prefix PREFIX` | Prefix for launched containers | `ai-proxy` | | ||
| | `--api-key KEY` | Shared API key for all containers | `sk-dummy` | | ||
| | `--log-level LEVEL` | `SERVER_LOG_LEVEL` value | `INFO` | | ||
| | `--base-api-port PORT` | Starting host API port | `2048` | | ||
| | `--base-stream-port PORT` | Starting host stream port | `3120` | |
There was a problem hiding this comment.
This doc describes --api-key / API_KEY as the shared API key for launched containers, but the current codebase enforces API keys via auth_profiles/key.txt (and doesn’t appear to read an API_KEY env var). Please either adjust the manager to actually provision key.txt (and document that), or update the README to direct users to configure auth_profiles/key.txt for API-key auth.
Summary
Testing
Related issue
Closes #300