feat(rolling-update): forward EXTRA_ENV as docker -e flags#591
Conversation
Lets deploy.env expose tunable flags without editing this script. The immediate trigger is ELASTICKV_RAFT_DISPATCHER_LANES=1, a feature flag added in elastickv PR #577 that routes raft messages through 4 per-peer lanes (heartbeat/replication/snapshot/other) to keep heartbeats from starving under MsgApp/MsgSnap bursts. We want to flip it on without rebuilding or shipping a new image. EXTRA_ENV is a whitespace-separated KEY=VALUE list; each pair becomes a single docker run -e flag. Values may contain characters bash would otherwise interpret, but pairs themselves must not contain whitespace.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughNormalizes and shell-escapes forwarded runtime variables (including Changes
Sequence Diagram(s)sequenceDiagram
participant Local
participant SSH
participant RemoteShell
participant Docker
Local->>Local: Read EXTRA_ENV and runtime vars
Note right of Local: Normalize & shell-escape into *_Q
Local->>SSH: ssh ... env (sends escaped *_Q and EXTRA_ENV_Q)
SSH->>RemoteShell: Start remote shell with quoted env
RemoteShell->>RemoteShell: Parse EXTRA_ENV into KEY=VALUE pairs\nValidate keys and entries
RemoteShell->>Docker: docker run ... -e KEY=VALUE (for each pair)
Docker-->>RemoteShell: Container starts
RemoteShell-->>Local: Exit/status via SSH
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to pass additional environment variables to the Docker container using the EXTRA_ENV variable. The review feedback highlights a critical issue where the variable is not correctly passed to the remote shell during the SSH call, which would prevent the feature from working. Additionally, it is recommended to document the new variable in the script's usage section and to use read -a for safer word splitting to prevent unintended filename expansion.
| # characters that bash would otherwise interpret; pairs themselves must not | ||
| # contain whitespace. | ||
| local extra_env_flags=() | ||
| if [[ -n "${EXTRA_ENV:-}" ]]; then |
There was a problem hiding this comment.
The EXTRA_ENV variable is defined in the local shell (sourced from the environment file) but is not passed to the remote shell. Since the REMOTE heredoc is quoted (<<'REMOTE'), local variables are not expanded within it. You must add EXTRA_ENV="$EXTRA_ENV" to the env command in the ssh call (around line 421) to make it available to the remote run_container function. Without this, the feature will not work as intended.
| # Pass through additional container environment variables from EXTRA_ENV. | ||
| # Accepts a whitespace-separated list of KEY=VALUE pairs, e.g.: | ||
| # EXTRA_ENV="ELASTICKV_RAFT_DISPATCHER_LANES=1 ELASTICKV_PEBBLE_CACHE_MB=512" | ||
| # Each pair is forwarded as a single `-e KEY=VALUE` flag so VALUE may contain | ||
| # characters that bash would otherwise interpret; pairs themselves must not | ||
| # contain whitespace. |
There was a problem hiding this comment.
The EXTRA_ENV variable should be documented in the usage() function (lines 7-65) so that users can discover it and understand its format when running the script with --help.
| # shellcheck disable=SC2206 | ||
| local -a extra_env_pairs=( ${EXTRA_ENV} ) |
There was a problem hiding this comment.
Using ( ${EXTRA_ENV} ) for word splitting also triggers filename expansion (globbing). If EXTRA_ENV contains characters like * or ?, it may be expanded to local filenames on the remote host. It is safer to use read -a which performs word splitting without globbing.
| # shellcheck disable=SC2206 | |
| local -a extra_env_pairs=( ${EXTRA_ENV} ) | |
| local -a extra_env_pairs | |
| read -r -a extra_env_pairs <<< "${EXTRA_ENV}" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/rolling-update.sh (1)
395-422:⚠️ Potential issue | 🔴 CriticalForward
EXTRA_ENVinto the remote script.
run_containerreadsEXTRA_ENVon the remote host, butupdate_one_nodenever passes the locally sourced value through thessh ... env ... bash -sinvocation. As written,EXTRA_ENVfromdeploy.envwill usually be empty remotely, so no Docker-eflags are added.🐛 Proposed fix
RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP" \ RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP" \ + EXTRA_ENV="${EXTRA_ENV:-}" \ bash -s <<'REMOTE'Also applies to: 675-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/rolling-update.sh` around lines 395 - 422, The remote ssh env block in update_one_node fails to forward the locally-sourced EXTRA_ENV so run_container on the remote host reads an empty value; update the ssh invocation that constructs the env list (the long ssh "... env IMAGE=... bash -s <<'REMOTE'") to include EXTRA_ENV="$EXTRA_ENV" so the remote script's run_container can consume it, and apply the same change to the other ssh/env block used around lines 675-682; ensure the referenced variable name EXTRA_ENV is passed exactly as shown so remote run_container and any Docker -e flags are populated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/rolling-update.sh`:
- Around line 675-681: Validate each token from EXTRA_ENV before adding to
extra_env_flags: in the loop iterating over extra_env_pairs (derived from
EXTRA_ENV) ensure each pair contains an '=' and a non-empty key (e.g. test with
[[ "$pair" == *=* ]] and then check key portion is not empty after splitting),
and if validation fails emit a clear error and exit non-zero to prevent passing
bare names to Docker; keep using the same variables (EXTRA_ENV, extra_env_pairs,
pair, extra_env_flags) and perform trimming/quoting as needed so only
well-formed KEY=VALUE entries are appended with extra_env_flags+=(-e "$pair").
---
Outside diff comments:
In `@scripts/rolling-update.sh`:
- Around line 395-422: The remote ssh env block in update_one_node fails to
forward the locally-sourced EXTRA_ENV so run_container on the remote host reads
an empty value; update the ssh invocation that constructs the env list (the long
ssh "... env IMAGE=... bash -s <<'REMOTE'") to include EXTRA_ENV="$EXTRA_ENV" so
the remote script's run_container can consume it, and apply the same change to
the other ssh/env block used around lines 675-682; ensure the referenced
variable name EXTRA_ENV is passed exactly as shown so remote run_container and
any Docker -e flags are populated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9beaf6dfc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # characters that bash would otherwise interpret; pairs themselves must not | ||
| # contain whitespace. | ||
| local extra_env_flags=() | ||
| if [[ -n "${EXTRA_ENV:-}" ]]; then |
There was a problem hiding this comment.
This new block reads EXTRA_ENV on the remote host, but update_one_node's SSH invocation only forwards a fixed set of variables and does not include EXTRA_ENV (see the env ... bash -s assignments around scripts/rolling-update.sh:395-422). In the normal case where EXTRA_ENV is set locally via ROLLING_UPDATE_ENV_FILE, it is not present remotely, so this condition is false and no docker run -e flags are added; the feature effectively does nothing unless the remote environment already defines EXTRA_ENV independently.
Useful? React with 👍 / 👎.
The SSH invocation forwards a fixed list of variables via `env KEY=VAL bash -s` with a QUOTED heredoc, so `EXTRA_ENV` referenced by `run_container` inside the remote block was never populated. Add it to the env pre-command with a default empty value so run_container's existing null-check works when deploy.env doesn't set it.
Switch from an unquoted array expansion to `read -r -a` when splitting EXTRA_ENV so values containing glob metacharacters (*, ?) are not expanded against the control host's cwd. Also reject entries that do not match KEY=VALUE with a non-empty KEY, since docker treats `-e NAME` (copy from environment) differently from `-e NAME=value` and a malformed pair would silently behave unexpectedly.
Add an EXTRA_ENV entry to the Optional environment section of the script's help text, with the expected whitespace-separated KEY=VALUE format and an example, so operators can discover the feature via `./scripts/rolling-update.sh --help`.
Review fixesAddresses the 5 review items (CRITICAL + 2 medium + 1 minor; codex P1 and gemini CRITICAL are the same bug).
|
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to scripts/rolling-update.sh, allowing users to pass additional environment variables to the remote Docker container. A critical issue was identified regarding the transmission of this variable over SSH; without proper shell escaping, whitespace-separated KEY=VALUE pairs are incorrectly parsed by the remote shell, leading to truncated environment variables. A suggestion was provided to use printf '%q' to ensure the entire string is preserved.
| ALL_NODE_HOSTS_CSV="$all_node_hosts_csv" \ | ||
| RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP" \ | ||
| RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP" \ | ||
| EXTRA_ENV="${EXTRA_ENV:-}" \ |
There was a problem hiding this comment.
The EXTRA_ENV variable is intended to contain whitespace-separated pairs, but when passed to the remote host via SSH in this manner, the remote shell will split the string before it reaches the env command. For example, if EXTRA_ENV="A=1 B=2", the remote shell executes env ... EXTRA_ENV=A=1 B=2 ..., resulting in EXTRA_ENV being set to only A=1 (and B=2 becoming a separate, likely ignored, environment variable). To ensure the entire string is preserved as a single value for the remote EXTRA_ENV variable, it should be shell-escaped using printf %q.
| EXTRA_ENV="${EXTRA_ENV:-}" \ | |
| EXTRA_ENV="$(printf '%q' "${EXTRA_ENV:-}")" \ |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ssh(1) joins remaining argv into a single command string which the remote shell re-parses, so a whitespace-containing EXTRA_ENV (its documented format) was split on the remote side: the remote saw `EXTRA_ENV=FOO=1 BAR=2 bash -s`, which set EXTRA_ENV to only the first pair and turned the rest into additional `env` assignments. Escape EXTRA_ENV with `printf %q` before handing it to ssh so the remote shell reassembles the original string verbatim. Also escape S3_CREDENTIALS_FILE defensively — it is a user-supplied path that could plausibly contain spaces even if no caller does so today.
|
Addressed gemini HIGH ( Root cause. Fix. Escape with Smoke (local simulation of the remote re-parse via
Pre-fix reproduction for the record: same
/gemini review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to scripts/rolling-update.sh, allowing users to pass additional environment variables to remote Docker containers. The changes include logic to escape these variables for safe transport over SSH and a parsing mechanism to convert the whitespace-separated list into Docker -e flags. Feedback suggests strengthening the validation of EXTRA_ENV keys by implementing a regex check to ensure they are valid environment variable names, rather than only checking for the presence of an equals sign.
| if [[ "$pair" != *=* || "$pair" == =* ]]; then | ||
| echo "invalid EXTRA_ENV entry '$pair'; expected KEY=VALUE" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The validation logic for EXTRA_ENV entries correctly ensures that each pair contains an = and does not start with one. However, it does not validate that the key is a valid environment variable name (e.g., it could start with a digit or contain invalid characters). While docker run -e will likely catch these, adding a basic regex check for the key would improve robustness.
References
- Centralize common complex logic, such as validation or retry mechanisms, into shared helper functions to improve maintainability and robustness.
|
Addressed Gemini medium review: Smoke tests (local extraction of the validator):
/gemini review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to the rolling-update.sh script, allowing users to pass additional environment variables to remote Docker containers. It also implements shell escaping for EXTRA_ENV and S3_CREDENTIALS_FILE to ensure safe transport over SSH. The review feedback suggests extending this escaping to other variables like DATA_DIR and IMAGE for consistency and defense in depth, and recommends moving the escaping logic outside the node iteration loop to improve efficiency.
| # ssh joins remaining arguments into a single command string which the remote | ||
| # shell re-parses, so values containing whitespace or shell metacharacters | ||
| # must be escaped before transport. EXTRA_ENV is documented as a | ||
| # whitespace-separated list of KEY=VALUE pairs and therefore always needs | ||
| # quoting; other forwarded variables are structurally whitespace-free today | ||
| # but we still escape the ones most likely to evolve (path-like values) for | ||
| # defense in depth. |
There was a problem hiding this comment.
The comment correctly identifies the need to escape values containing whitespace or shell metacharacters before transport via SSH. However, it mentions escaping "path-like values" for defense in depth but only applies this to S3_CREDENTIALS_FILE. Other path-like variables such as DATA_DIR and SERVER_ENTRYPOINT, as well as IMAGE, are still passed without escaping in the ssh call below. For full defense in depth and consistency, these should also be escaped using printf '%q'.
| local extra_env_escaped s3_credentials_file_escaped | ||
| extra_env_escaped="$(printf '%q' "${EXTRA_ENV:-}")" | ||
| s3_credentials_file_escaped="$(printf '%q' "${S3_CREDENTIALS_FILE:-}")" |
There was a problem hiding this comment.
Since EXTRA_ENV and S3_CREDENTIALS_FILE do not change per node, they could be escaped once outside the for node_id in "${ROLLING_NODE_IDS[@]}" loop (around line 857) to avoid redundant printf calls for every node in the cluster. While the performance impact is negligible for small clusters, it improves the script's efficiency.
…ath-vars Addresses gemini medium review on 2fd1bf0: - EXTRA_ENV / S3_CREDENTIALS_FILE escaping did not change per node; moved from inside update_one_node (ran once per host) to file scope, computed once after ensure_remote_raftadmin_binaries. - For defense in depth, also escape IMAGE, DATA_DIR, SERVER_ENTRYPOINT -- path-like values forwarded over the SSH re-parse path. Structurally whitespace-free today, but cheap to harden against future deploy.env values containing metacharacters. - Non-path-like vars (CSV / numeric / boolean) left unquoted; adding printf %q there is inert but adds noise. Naming: *_Q suffix for the escaped forms to keep the env block readable. Smoke test: IMAGE=ghcr.io/bootjp/elastickv:latest -> unchanged DATA_DIR="/var/lib/my data/kv" -> remote sees [/var/lib/my data/kv] EXTRA_ENV="FOO=1 BAR=2" -> remote sees [FOO=1 BAR=2]
|
Addressed gemini medium review on 2fd1bf0 in 5415d8f. Moved escaping outside the per-node loop. Defense-in-depth escape for path-like vars. Extended Smoke (simulating the remote re-parse with
/gemini review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to the rolling-update.sh script, allowing users to pass additional environment variables to Docker containers. It also implements shell escaping for several variables transmitted via SSH to prevent parsing errors or security issues with special characters. The review feedback suggests improving the parsing of EXTRA_ENV to handle multi-line strings and extending the escaping logic to other variables like RAFTADMIN_REMOTE_BIN and CONTAINER_NAME for better consistency and robustness.
| if [[ -n "${EXTRA_ENV:-}" ]]; then | ||
| # Split on whitespace without triggering filename globbing. | ||
| local -a extra_env_pairs=() | ||
| read -r -a extra_env_pairs <<< "${EXTRA_ENV}" |
There was a problem hiding this comment.
The read -r -a command with a here-string (<<<) only reads the first line of the input. If EXTRA_ENV is defined as a multi-line string (which is common for long lists of environment variables in .env files), only the first line of variables will be processed, and subsequent lines will be silently ignored. To handle all whitespace including newlines while avoiding globbing, you should replace newlines with spaces before reading.
| read -r -a extra_env_pairs <<< "${EXTRA_ENV}" | |
| read -r -a extra_env_pairs <<< "${EXTRA_ENV//$'/\n'/ }" |
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV:-}")" | ||
| S3_CREDENTIALS_FILE_Q="$(printf '%q' "${S3_CREDENTIALS_FILE:-}")" | ||
| IMAGE_Q="$(printf '%q' "$IMAGE")" | ||
| DATA_DIR_Q="$(printf '%q' "$DATA_DIR")" | ||
| SERVER_ENTRYPOINT_Q="$(printf '%q' "$SERVER_ENTRYPOINT")" |
There was a problem hiding this comment.
There is an inconsistency in the escaping of variables passed to the remote shell. While you have correctly escaped IMAGE, DATA_DIR, and others for "defense in depth," you missed RAFTADMIN_REMOTE_BIN (which is a path) and CONTAINER_NAME (which is user-configurable). If these contain spaces or shell metacharacters, the env command in update_one_node will fail or behave unexpectedly. You should escape these as well.
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV:-}")" | |
| S3_CREDENTIALS_FILE_Q="$(printf '%q' "${S3_CREDENTIALS_FILE:-}")" | |
| IMAGE_Q="$(printf '%q' "$IMAGE")" | |
| DATA_DIR_Q="$(printf '%q' "$DATA_DIR")" | |
| SERVER_ENTRYPOINT_Q="$(printf '%q' "$SERVER_ENTRYPOINT")" | |
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV:-}")" | |
| S3_CREDENTIALS_FILE_Q="$(printf '%q' "${S3_CREDENTIALS_FILE:-}")" | |
| IMAGE_Q="$(printf '%q' "$IMAGE")" | |
| DATA_DIR_Q="$(printf '%q' "$DATA_DIR")" | |
| SERVER_ENTRYPOINT_Q="$(printf '%q' "$SERVER_ENTRYPOINT")" | |
| RAFTADMIN_REMOTE_BIN_Q="$(printf '%q' "$RAFTADMIN_REMOTE_BIN")" | |
| CONTAINER_NAME_Q="$(printf '%q' "$CONTAINER_NAME")" |
…IN/CONTAINER_NAME Addresses two gemini medium review items on 5415d8f: 1. Multi-line EXTRA_ENV values were silently truncated to the first line. `read -r -a <<< "$EXTRA_ENV"` consumes only up to the first newline of a here-string, so a deploy.env with EXTRA_ENV="FOO=1 BAR=2" would set FOO=1 and drop BAR=2 without error. Fix: normalise newlines to spaces before the read: extra_env_normalised="${EXTRA_ENV//$'\n'/ }" read -r -a extra_env_pairs <<< "${extra_env_normalised}" Smoke: three pairs spread across 2 lines now parse as 3 entries. 2. Extended `printf %q` defense-in-depth escaping to RAFTADMIN_REMOTE_BIN (path-like, may grow spaces) and CONTAINER_NAME (user-configurable). CSV / numeric / boolean vars remain unescaped. `bash -n scripts/rolling-update.sh` clean.
|
Addressed both gemini medium items on 5415d8f in 2014758. 1. Multi-line EXTRA_ENV silently truncated. would set local extra_env_normalised="${EXTRA_ENV//$'\n'/ }"
read -r -a extra_env_pairs <<< "${extra_env_normalised}"Smoke (input 2. Extended
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2014758cec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # quoting; other forwarded variables are structurally whitespace-free today | ||
| # but we still escape the path-like ones most likely to evolve for defense | ||
| # in depth. Escape once here since these don't change per node. | ||
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV:-}")" |
There was a problem hiding this comment.
printf '%q' can emit ANSI-C quotes (for example $'FOO=1\nBAR=2'), but the SSH command string is parsed by the remote account’s login shell before bash -s runs. On hosts where that shell is /bin/sh/dash, $'...' is treated literally, so multiline EXTRA_ENV arrives as a literal $...\n... string and the new KEY=VALUE validator rejects it, aborting the rollout. This means the documented multiline deploy.env format is not portable across common non-bash remote shells.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to scripts/rolling-update.sh, allowing users to pass additional environment variables to the remote Docker container. It also improves the robustness of the script by escaping several variables before they are transmitted via SSH. A potential issue was identified regarding the use of printf '%q' for escaping; this method can produce Bash-specific syntax that may not be correctly interpreted by the remote shell if it is not Bash-compatible, potentially breaking the parsing of multi-line environment variables.
| # quoting; other forwarded variables are structurally whitespace-free today | ||
| # but we still escape the path-like ones most likely to evolve for defense | ||
| # in depth. Escape once here since these don't change per node. | ||
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV:-}")" |
There was a problem hiding this comment.
Using printf '%q' in Bash 4+ to escape variables containing newlines (like a multi-line EXTRA_ENV) produces the $'...' syntax. While this is valid Bash, the remote shell that initially parses the ssh command line (often /bin/sh, which is dash on Ubuntu/Debian) does not support this syntax. This can result in the literal string $'...' being passed into the remote environment instead of the intended value, which would then break the subsequent parsing logic in run_container.
If multi-line EXTRA_ENV values are expected to be common, consider ensuring the local EXTRA_ENV is normalized (newlines replaced with spaces) before escaping, or use a more portable escaping method.
Addresses gemini medium + codex P2 on 2014758 (same root cause). `printf '%q'` emits ANSI-C $'...' quoting for strings containing non- printable characters like newlines. The remote *login* shell re-parses the SSH command string before exec'ing `bash -s`. On Debian/Ubuntu where /bin/sh is dash, $'...' is not a special token -- the literal string is forwarded through, EXTRA_ENV arrives on the remote as `$'FOO=1\nBAR=2'`, and the KEY=VALUE validator in run_container rejects it, aborting the rollout. Fix: normalise newlines to spaces locally *before* printf %q, so the escape only emits plain backslash quoting which every POSIX shell understands. Confirms via smoke test: pre-fix: printf %q "FOO=1\nBAR=2" -> $'FOO=1\nBAR=2' (bash-only) post-fix: printf %q "FOO=1 BAR=2" -> FOO=1\ BAR=2 (portable) The existing in-run_container normalisation (also on newlines) is kept as defense in depth but is now unreachable via the rolling-update path. `bash -n scripts/rolling-update.sh` clean.
|
Addressed the combined gemini medium + codex P2 on 2014758 (same bug, portability-of- Root cause. Fix. Normalise newlines to spaces locally, before EXTRA_ENV_NORMALISED="${EXTRA_ENV//$'\n'/ }"
EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV_NORMALISED:-}")"Smoke (EXTRA_ENV="FOO=1\nBAR=2"):
The existing newline-normalisation inside
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ded4d1ec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| EXTRA_ENV_NORMALISED="${EXTRA_ENV//$'\n'/ }" | ||
| EXTRA_ENV_Q="$(printf '%q' "${EXTRA_ENV_NORMALISED:-}")" |
There was a problem hiding this comment.
%q escaping EXTRA_ENV
EXTRA_ENV_NORMALISED currently replaces only newlines, but printf '%q' still emits ANSI-C $'...' quoting when EXTRA_ENV contains other non-printable whitespace such as tabs (or \r from CRLF deploy files). On hosts where the SSH login shell is dash, that quoting is treated literally, so EXTRA_ENV reaches run_container as a malformed first token and fails the KEY validator, aborting the rollout. Since the script documents EXTRA_ENV as whitespace-separated, tabs should work as separators too; normalize all relevant whitespace before %q.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to the rolling-update.sh script, allowing users to pass additional environment variables to remote Docker containers. It also enhances the script's robustness by implementing proper shell quoting for variables transmitted via SSH. A review comment correctly identified a potential crash when EXTRA_ENV is unset due to the set -u flag and suggested including carriage return normalization for better cross-platform compatibility.
| # shells (/bin/sh -> dash on Debian/Ubuntu) don't grok $'...' and would | ||
| # pass it through as literal characters, breaking the `KEY=VALUE` | ||
| # validator in run_container. | ||
| EXTRA_ENV_NORMALISED="${EXTRA_ENV//$'\n'/ }" |
There was a problem hiding this comment.
The script uses set -u (via set -euo pipefail on line 2), which causes it to exit immediately if an unset variable is referenced. Since EXTRA_ENV is not initialized with a default value earlier in the script, this line will trigger a fatal error if EXTRA_ENV is not provided in the environment or the env file.
Additionally, while you are normalizing newlines, it is good practice to also normalize carriage returns (\r) to handle deploy.env files that might have been edited on Windows.
| EXTRA_ENV_NORMALISED="${EXTRA_ENV//$'\n'/ }" | |
| EXTRA_ENV_NORMALISED="${EXTRA_ENV:-}" | |
| EXTRA_ENV_NORMALISED="${EXTRA_ENV_NORMALISED//[$'\r\n']/ }" |
Addresses gemini HIGH + codex P2 on 8ded4d1. 1. Gemini HIGH -- `set -u` crash when EXTRA_ENV is unset. `${EXTRA_ENV//$'\n'/ }` dereferences $EXTRA_ENV, which under `set -u` aborts the script when the variable is unset (EXTRA_ENV is documented as optional in deploy.env). Fixed by going through `${EXTRA_ENV:-}` first. 2. Codex P2 -- `printf %q` still emits ANSI-C quoting for tabs and CR. Newline was only one of several non-printable characters that trigger the $'...' escape form. Tab and \r both do the same thing. Tabs can appear as legitimate separators; \r appears when deploy.env is edited on Windows. Normalise all three to spaces before %q. Smoke (all passing; see /tmp/pr591_smoke.sh): unset: [''] -- no crash CRLF+tab: [FOO=1\ \ BAR=2\ BAZ=3] -- dash-portable, no $'...' simple: [ELASTICKV_RAFT_DISPATCHER_LANES=1] glob-prone: [WILD=\*] -- not expanded multi-line: [FOO=1\ BAR=2\ BAZ=3]
|
Addressed both items on 8ded4d1 in acc3ab1. 1. Gemini HIGH — Correct. EXTRA_ENV_NORMALISED="${EXTRA_ENV:-}"
EXTRA_ENV_NORMALISED="${EXTRA_ENV_NORMALISED//[$'\t\r\n']/ }"2. Codex P2 — normalise tab and CR, not just newline. Also correct. Smoke tests (5 scenarios, all pass):
Explicitly verified no
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to scripts/rolling-update.sh, enabling the passage of additional environment variables to remote Docker containers. It also enhances the script's robustness by applying shell escaping to several variables before they are transmitted over SSH. The review feedback suggests extending this escaping to RAFT_TO_REDIS_MAP and RAFT_TO_S3_MAP for consistency and improved defense in depth.
| RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP" \ | ||
| RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP" \ |
There was a problem hiding this comment.
For consistency with the 'defense in depth' approach taken for other variables in this PR (like IMAGE_Q and DATA_DIR_Q), RAFT_TO_REDIS_MAP and RAFT_TO_S3_MAP should also be escaped using printf %q before being passed to the remote host. These variables contain complex mapping strings that could benefit from the same level of protection against shell re-parsing issues.
| RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP" \ | |
| RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP" \ | |
| RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP_Q" \ | |
| RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP_Q" \ |
| DATA_DIR_Q="$(printf '%q' "$DATA_DIR")" | ||
| SERVER_ENTRYPOINT_Q="$(printf '%q' "$SERVER_ENTRYPOINT")" | ||
| RAFTADMIN_REMOTE_BIN_Q="$(printf '%q' "$RAFTADMIN_REMOTE_BIN")" | ||
| CONTAINER_NAME_Q="$(printf '%q' "$CONTAINER_NAME")" |
There was a problem hiding this comment.
Define the escaped versions of the map variables here to support their use in the update_one_node function.
| CONTAINER_NAME_Q="$(printf '%q' "$CONTAINER_NAME")" | |
| CONTAINER_NAME_Q="$(printf '%q' "$CONTAINER_NAME")" | |
| RAFT_TO_REDIS_MAP_Q="$(printf '%q' "$RAFT_TO_REDIS_MAP")" | |
| RAFT_TO_S3_MAP_Q="$(printf '%q' "$RAFT_TO_S3_MAP")" |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Addresses gemini medium on acc3ab1. Both CSV maps are derived (not directly user-supplied) and structurally contain only `=`, `:`, `,`, digits and identifier chars today -- none shell-special. `printf %q` is therefore a no-op under the current NODES format, but adding the escape aligns them with the other forwarded vars (IMAGE_Q, DATA_DIR_Q, etc.) so future evolution of the NODES schema can't silently corrupt the remote command line.
|
Addressed gemini medium on acc3ab1 in 3a4e2be. Extended Caveat worth noting: both maps are derived (not user-supplied) — constructed by
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV variable to scripts/rolling-update.sh, enabling the pass-through of additional environment variables to remote Docker containers. It also enhances the script's reliability by properly escaping shell variables before transport over SSH using printf %q. A review comment suggests explicitly defining IFS when parsing EXTRA_ENV to ensure consistent splitting behavior.
| # would be silently dropped. | ||
| local -a extra_env_pairs=() | ||
| local extra_env_normalised="${EXTRA_ENV//$'\n'/ }" | ||
| read -r -a extra_env_pairs <<< "${extra_env_normalised}" |
There was a problem hiding this comment.
Explicitly setting IFS for the read command is a best practice to ensure the splitting logic remains consistent even if the global IFS variable is modified elsewhere in the script. Since EXTRA_ENV is intended to be whitespace-separated, using the default whitespace IFS (space, tab, newline) is appropriate and robust.
| read -r -a extra_env_pairs <<< "${extra_env_normalised}" | |
| IFS=$' \t\n' read -r -a extra_env_pairs <<< "${extra_env_normalised}" |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Addresses gemini medium on 3a4e2be. `read -r -a` inherits the ambient IFS. Default is already space/tab/ newline, but making the splitting deterministic against any future IFS mutation elsewhere in the script (or sourced file) is cheap insurance. Smoke-tested: with IFS=":" in the outer scope, the pinned IFS=$' \t\n' still splits "FOO=1 BAR=2 BAZ=3" into three entries.
|
Addressed gemini medium on 3a4e2be in e871564. Pinned IFS explicitly on the IFS=$' \t\n' read -r -a extra_env_pairs <<< "${extra_env_normalised}"Smoke: under a perturbed outer
/gemini review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTRA_ENV environment variable to the rolling-update.sh script, allowing users to pass additional configuration to the remote Docker container as a whitespace-separated list of KEY=VALUE pairs. The update also improves the script's robustness by implementing proper shell escaping (using printf %q) for variables transmitted via SSH, ensuring that whitespace and shell metacharacters are correctly handled by the remote login shell. I have no feedback to provide.
1 participant
Summary
Adds an
EXTRA_ENVpass-through toscripts/rolling-update.shso operators can set container environment variables fromdeploy.envwithout editing this script each time.Motivation
Immediate trigger: enabling
ELASTICKV_RAFT_DISPATCHER_LANES=1(feature flag added in PR #577). We want to flip it on via the deploy envelope, not by rebuilding the image or hand-editing this script.Post-#575 raft metrics are already healthy, so the 4-lane dispatcher is being enabled as a defensive measure — it keeps heartbeats from being starved by MsgApp / MsgSnap bursts under extreme write load, rather than as a fix for anything actively broken.
Change
run_container: ifEXTRA_ENVis set, split on whitespace and forward each pair as a singledocker run -e KEY=VALUEflag.Usage
Test plan
bash -n scripts/rolling-update.shpassesshellcheck/make lint— 0 issuesdocker inspect elastickv --format '{{.Config.Env}}'on each nodeSummary by CodeRabbit