Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The demo script assumes the
model-clibinary exists at a hard-coded path (${REPO_ROOT}/model-cli/target/release/model-cli); consider either checking for its existence with a helpful error message or allowing an override via an environment variable (e.g.,MODEL_CLI_BIN). - Several steps depend on
python3and theopenaipackage being available; you already skip the SDK step whenopenaiis missing, but it may be more robust to add an early prerequisite check (forpython3and core Python usage) and exit with a clear message if they are not installed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The demo script assumes the `model-cli` binary exists at a hard-coded path (`${REPO_ROOT}/model-cli/target/release/model-cli`); consider either checking for its existence with a helpful error message or allowing an override via an environment variable (e.g., `MODEL_CLI_BIN`).
- Several steps depend on `python3` and the `openai` package being available; you already skip the SDK step when `openai` is missing, but it may be more robust to add an early prerequisite check (for `python3` and core Python usage) and exit with a clear message if they are not installed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a new interactive demo for the model-cli gateway command, providing configuration examples and a bash script to walk users through features like load balancing, retries, and fallbacks. The review identified a critical security issue regarding the use of eval in the demo script's run_step function, which should be refactored to avoid potential command injection.
| run_step() { | ||
| local description="$1" | ||
| local command="$2" | ||
|
|
||
| echo | ||
| printf "${dim}# %s${reset}\n" "$description" | ||
| printf "${bold}${green}\$${reset} " | ||
| typewrite "$command" 0.035 | ||
| printf "${dim} ▌${reset}" # blinking-cursor illusion | ||
|
|
||
| # Wait for Enter | ||
| read -r -s _ | ||
| printf "\r${bold}${green}\$${reset} ${white}%s${reset}\n" "$command" | ||
|
|
||
| # Actually run it | ||
| eval "$command" | ||
| } |
There was a problem hiding this comment.
The run_step function uses eval on a command string that has had variables expanded into it. This is a critical security and correctness issue. If a variable (like $API_KEY) contains shell metacharacters (e.g., a single quote), it can break the command syntax or lead to arbitrary code execution.
According to the repository style guide, which prioritizes security, eval should be avoided.
I recommend removing this function and inlining its logic at its two call sites (in Step 3 and Step 4). This will make variable handling explicit and safe for each command, which is consistent with how more complex commands are handled in other steps of this script.
Here is an example of how the health check in Step 3 could be rewritten safely:
section "Step 3 — Health check"
local description="The gateway exposes /health — no auth required"
local command="curl -s http://localhost:${GATEWAY_PORT}/health | python3 -m json.tool"
echo
printf "${dim}# %s\n" "$description"
printf "${bold}${green}\\$${reset} "
typewrite "$command" 0.035
printf "${dim} ▌${reset}" # blinking-cursor illusion
# Wait for Enter
read -r -s _
printf "\r${bold}${green}\\$${reset} ${white}%s${reset}\n" "$command"
# Actually run it
curl -s "http://localhost:${GATEWAY_PORT}/health" | python3 -m json.toolReferences
- The use of
evalpresents a significant security risk (potential for command injection), which goes against the security principles outlined in the style guide. The guide states a priority on security, asking "What are the threat surfaces? Are trust boundaries respected? ... Think about the OWASP top 10..." (link)
33c2c7b to
1c7f10c
Compare
Introduces demos/gateway/ with a step-by-step shell demo for the model-cli gateway command. Each step pauses for Enter, types commands character-by-character as 'docker model gateway', and covers health, auth, chat completions, streaming, embeddings, load balancing, fallbacks, and OpenAI SDK compatibility. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
vllm-metal uses ZMQ IPC sockets at temporary paths under /private/var/folders (the macOS TMPDIR) for internal inter-process communication between API server workers. The Python sandbox profile only allowed network-bind for Unix sockets matching the inference.*-[0-9]+\.sock$ pattern and TCP loopback, which caused a ZMQError: Operation not permitted when vllm-metal tried to bind those sockets. Allow network-bind on paths under /private/var/folders so vllm-metal can create its internal ZMQ IPC sockets in the system temp directory.
Introduces demos/gateway/ with a step-by-step shell demo for the model-cli gateway command. Each step pauses for Enter, types commands character-by-character as 'docker model gateway', and covers health, auth, chat completions, streaming, embeddings, load balancing, fallbacks, and OpenAI SDK compatibility.