Add KEYVIZ_KEY_BUCKETS_PER_ROUTE validation#844
Conversation
📝 WalkthroughWalkthrough
ChangesKEYVIZ_KEY_BUCKETS_PER_ROUTE rollout support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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 KEYVIZ_KEY_BUCKETS_PER_ROUTE configuration to scripts/rolling-update.sh to support sub-range buckets per route in KeyViz. Feedback suggests improving the validation logic for this variable by restricting it to a range of 1 to 256 via a precise regular expression, preventing potential integer overflow or silent clamping issues.
| if [[ -n "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" && ! "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" =~ ^[1-9][0-9]*$ ]]; then | ||
| echo "rolling-update: KEYVIZ_KEY_BUCKETS_PER_ROUTE must be empty or a positive integer, got '$KEYVIZ_KEY_BUCKETS_PER_ROUTE'" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The current validation checks if KEYVIZ_KEY_BUCKETS_PER_ROUTE is a positive integer, but it does not enforce the maximum limit of 256 (defined as MaxKeyBucketsPerRoute in keyviz/sampler.go).
If an operator configures a value larger than 256, it will be silently clamped by the Go binary. More importantly, if they enter an extremely large number that overflows a standard integer, the Go flag parser will fail to parse it, leading to a container crash-loop. This defeats the purpose of pre-validating the flag in this script.
We can use a precise regular expression to restrict the value to the valid range of 1 to 256 inclusive, which is also completely immune to integer overflow.
| if [[ -n "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" && ! "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" =~ ^[1-9][0-9]*$ ]]; then | |
| echo "rolling-update: KEYVIZ_KEY_BUCKETS_PER_ROUTE must be empty or a positive integer, got '$KEYVIZ_KEY_BUCKETS_PER_ROUTE'" >&2 | |
| exit 1 | |
| fi | |
| if [[ -n "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" && ! "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" =~ ^([1-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-6])$ ]]; then | |
| echo "rolling-update: KEYVIZ_KEY_BUCKETS_PER_ROUTE must be empty or an integer between 1 and 256, got '$KEYVIZ_KEY_BUCKETS_PER_ROUTE'" >&2 | |
| exit 1 | |
| fi |
|
@codex review |
![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: 4db14f086a
ℹ️ 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".
| # Validate before it reaches the SSH heredoc / the binary's flag parser | ||
| # so an operator typo fails here with a clear message rather than as a | ||
| # container crash-loop on an unparseable --keyvizKeyBucketsPerRoute. | ||
| if [[ -n "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" && ! "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" =~ ^([1-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-6])$ ]]; then |
There was a problem hiding this comment.
This rejects KEYVIZ_KEY_BUCKETS_PER_ROUTE even when KEYVIZ_ENABLED=false, although the surrounding KeyViz configuration and build_keyviz_flags make the other KeyViz knobs no-ops unless the sampler is enabled. In a deploy env that carries a stale or placeholder bucket value while KeyViz is disabled, rolling-update.sh now exits before doing an otherwise unrelated rollout instead of ignoring the disabled knob. Please only validate this value when KEYVIZ_ENABLED=true or when the flag is actually emitted.
Useful? React with 👍 / 👎.
|
@codex review Addressed the current P2 finding in commit adc2db3:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adc2db3708
ℹ️ 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".
| "$ADMIN_TLS_CERT_FILE" "$ADMIN_TLS_KEY_FILE" \ | ||
| "$ADMIN_ALLOW_PLAINTEXT_NON_LOOPBACK" "$ADMIN_ALLOW_INSECURE_DEV_COOKIE" \ | ||
| "$KEYVIZ_ENABLED" "$KEYVIZ_FANOUT_NODES" \ | ||
| "$KEYVIZ_ENABLED" "$KEYVIZ_FANOUT_NODES" "$KEYVIZ_KEY_BUCKETS_PER_ROUTE" \ |
There was a problem hiding this comment.
When KEYVIZ_ENABLED=false, build_keyviz_flags returns without emitting any KeyViz flags, so KEYVIZ_KEY_BUCKETS_PER_ROUTE does not affect the remote docker run configuration. Including it unconditionally in the config fingerprint means changing or inheriting a stale placeholder bucket value while KeyViz is disabled still makes every node report a config change and recreate the container, even though the effective deployment is unchanged. Gate this fingerprint input the same way as the flag emission (or normalize it to empty when disabled).
Useful? React with 👍 / 👎.
|
@codex review Addressed current-head P2 inline finding in 3dde408:
Validation:
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4543fcfba4
ℹ️ 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".
| if [[ -n "$key_buckets" ]]; then | ||
| _flags+=(--keyvizKeyBucketsPerRoute "$key_buckets") |
There was a problem hiding this comment.
This new if is never closed before the function body ends, so the remote heredoc sent to bash -s is syntactically invalid for every rollout, not just KeyViz-enabled ones. I extracted the remote block from lines 691-1482 and bash -n fails with syntax error near unexpected token '}', so the first node aborts before it can recreate the container; add the missing fi after appending --keyvizKeyBucketsPerRoute.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/rolling-update.sh`:
- Around line 1426-1428: The config fingerprint assembly in rolling-update.sh is
still including raw KEYVIZ_FANOUT_NODES even when KeyViz is disabled, which can
cause unnecessary churn from stale fanout changes. Update the config_fp inputs
so the fingerprint uses the gated KeyViz fingerprint variables from the
KeyViz-related section and does not hash KEYVIZ_FANOUT_NODES directly when
KEYVIZ_ENABLED is false; check the fingerprint-building block around
keyviz_fanout_nodes_fp and keyviz_key_buckets_per_route_fp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| "$KEYVIZ_ENABLED" "$keyviz_fanout_nodes_fp" "$keyviz_key_buckets_per_route_fp" \ | ||
| "$KEYVIZ_ENABLED" "$KEYVIZ_FANOUT_NODES" \ | ||
| "$KEYVIZ_HOT_KEYS_ENABLED" "$KEYVIZ_HOT_KEYS_PER_ROUTE" \ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
config_fp still hashes raw fanout while KeyViz is disabled.
Line 1427 reintroduces KEYVIZ_FANOUT_NODES directly, so disabled KeyViz can still churn fingerprints on stale fanout changes.
Suggested fix
- "$KEYVIZ_ENABLED" "$keyviz_fanout_nodes_fp" "$keyviz_key_buckets_per_route_fp" \
- "$KEYVIZ_ENABLED" "$KEYVIZ_FANOUT_NODES" \
+ "$KEYVIZ_ENABLED" "$keyviz_fanout_nodes_fp" "$keyviz_key_buckets_per_route_fp" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "$KEYVIZ_ENABLED" "$keyviz_fanout_nodes_fp" "$keyviz_key_buckets_per_route_fp" \ | |
| "$KEYVIZ_ENABLED" "$KEYVIZ_FANOUT_NODES" \ | |
| "$KEYVIZ_HOT_KEYS_ENABLED" "$KEYVIZ_HOT_KEYS_PER_ROUTE" \ | |
| "$KEYVIZ_ENABLED" "$keyviz_fanout_nodes_fp" "$keyviz_key_buckets_per_route_fp" \ | |
| "$KEYVIZ_HOT_KEYS_ENABLED" "$KEYVIZ_HOT_KEYS_PER_ROUTE" \ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/rolling-update.sh` around lines 1426 - 1428, The config fingerprint
assembly in rolling-update.sh is still including raw KEYVIZ_FANOUT_NODES even
when KeyViz is disabled, which can cause unnecessary churn from stale fanout
changes. Update the config_fp inputs so the fingerprint uses the gated KeyViz
fingerprint variables from the KeyViz-related section and does not hash
KEYVIZ_FANOUT_NODES directly when KEYVIZ_ENABLED is false; check the
fingerprint-building block around keyviz_fanout_nodes_fp and
keyviz_key_buckets_per_route_fp.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4543fcfba4
ℹ️ 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".
| if [[ -n "$key_buckets" ]]; then | ||
| _flags+=(--keyvizKeyBucketsPerRoute "$key_buckets") |
There was a problem hiding this comment.
When this script reaches update_one_node, the remote heredoc is passed to bash -s, and the new if [[ -n "$key_buckets" ]]; then has no matching fi before the function's closing brace. I extracted the REMOTE heredoc and bash -n reports syntax error near unexpected token '}', so every rolling update attempt will fail while parsing the remote script before it can start or skip a container.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes