fix(sluice): use virtual key for deploy smoke#135
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe dashboard Terraform module can resolve an Entra gateway proxy key from Key Vault, production configures ChangesGateway proxy key authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeployWorkflow
participant AzureKeyVault
participant GatewaySmokeTest
DeployWorkflow->>AzureKeyVault: Read vkey-dashboard-playground
AzureKeyVault-->>DeployWorkflow: Return virtual key
DeployWorkflow->>GatewaySmokeTest: Pass validated smoke_key output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Updates Sluice’s deploy smoke tests and dashboard Entra “BFF” configuration to use a LiteLLM virtual key (user-facing traffic) rather than the master gateway key (Terraform/runtime configuration), and documents the distinction to reduce operational confusion.
Changes:
- Switch GitHub Actions deploy smoke tests to fetch
vkey-dashboard-playgroundfrom the environment Key Vault and use it for gateway smoke calls. - Extend the
dashboard_acaTerraform module to optionally source the BFF’s gateway proxy key by Key Vault secret name (virtual key), falling back to the existing versionless secret ID. - Document the separation between
AIGATEWAY_KEY(master key) and smoke traffic virtual keys indocs/CI_CD.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
infra/modules/dashboard_aca/variables.tf |
Adds gateway_proxy_key_secret_name and repositions gateway_key_versionless_id as a fallback, clarifying Entra/Key Vault requirements. |
infra/modules/dashboard_aca/main.tf |
Reads the configured virtual-key secret (by name) and injects its versionless ID into the dashboard Container App secret when in Entra mode. |
infra/env/prod/main.tf |
Wires prod dashboard Entra config to use vkey-dashboard-playground as the gateway proxy key. |
docs/CI_CD.md |
Documents that deploy smoke tests use the Key Vault virtual key while AIGATEWAY_KEY remains the master key for Terraform/runtime config. |
.github/workflows/deploy-environment.yaml |
Fetches vkey-dashboard-playground from Key Vault and passes it to the smoke-test composite action instead of secrets.AIGATEWAY_KEY. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set -euo pipefail | ||
| KV_NAME=$(printf 'pvc-%s-%s-kv' "${TF_VAR_env}" "${TF_VAR_projname}" | tr '[:upper:]_' '[:lower:]-' | cut -c1-24) | ||
| KEY=$(az keyvault secret show --vault-name "${KV_NAME}" --name vkey-dashboard-playground --query value -o tsv) |
![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 `@infra/modules/dashboard_aca/main.tf`:
- Around line 19-20: Update the gateway_proxy_key_secret_id expression to
determine whether the conditional data source exists by checking the length of
data.azurerm_key_vault_secret.gateway_proxy_key, rather than
gateway_proxy_key_secret_name. Preserve the existing fallback to
var.gateway_key_versionless_id when the data source array is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c53d63be-1ee5-4213-aa1f-cb0e5285a28c
📒 Files selected for processing (5)
.github/workflows/deploy-environment.yamldocs/CI_CD.mdinfra/env/prod/main.tfinfra/modules/dashboard_aca/main.tfinfra/modules/dashboard_aca/variables.tf
| gateway_proxy_key_secret_name = trimspace(var.gateway_proxy_key_secret_name) | ||
| gateway_proxy_key_secret_id = local.gateway_proxy_key_secret_name != "" ? data.azurerm_key_vault_secret.gateway_proxy_key[0].versionless_id : var.gateway_key_versionless_id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix "Index out of bounds" evaluation error.
When a secret name is provided but auth_mode is not "entra", the ternary condition local.gateway_proxy_key_secret_name != "" will evaluate to true. This causes Terraform to evaluate data.azurerm_key_vault_secret.gateway_proxy_key[0]. Since local.use_entra is false, the data source count is 0, causing an immediate "Index out of bounds" failure during plan/apply.
Check the length of the conditionally created data source array instead.
🐛 Proposed fix
gateway_proxy_key_secret_name = trimspace(var.gateway_proxy_key_secret_name)
- gateway_proxy_key_secret_id = local.gateway_proxy_key_secret_name != "" ? data.azurerm_key_vault_secret.gateway_proxy_key[0].versionless_id : var.gateway_key_versionless_id
+ gateway_proxy_key_secret_id = length(data.azurerm_key_vault_secret.gateway_proxy_key) > 0 ? data.azurerm_key_vault_secret.gateway_proxy_key[0].versionless_id : var.gateway_key_versionless_id📝 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.
| gateway_proxy_key_secret_name = trimspace(var.gateway_proxy_key_secret_name) | |
| gateway_proxy_key_secret_id = local.gateway_proxy_key_secret_name != "" ? data.azurerm_key_vault_secret.gateway_proxy_key[0].versionless_id : var.gateway_key_versionless_id | |
| gateway_proxy_key_secret_name = trimspace(var.gateway_proxy_key_secret_name) | |
| gateway_proxy_key_secret_id = length(data.azurerm_key_vault_secret.gateway_proxy_key) > 0 ? data.azurerm_key_vault_secret.gateway_proxy_key[0].versionless_id : var.gateway_key_versionless_id |
🤖 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 `@infra/modules/dashboard_aca/main.tf` around lines 19 - 20, Update the
gateway_proxy_key_secret_id expression to determine whether the conditional data
source exists by checking the length of
data.azurerm_key_vault_secret.gateway_proxy_key, rather than
gateway_proxy_key_secret_name. Preserve the existing fallback to
var.gateway_key_versionless_id when the data source array is empty.
![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: a0f4abe868
ℹ️ 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".
| entra_client_id = var.entra_client_id | ||
| key_vault_id = module.sluice.key_vault_id | ||
| gateway_key_versionless_id = module.sluice.gateway_key_versionless_id | ||
| gateway_proxy_key_secret_name = "vkey-dashboard-playground" |
There was a problem hiding this comment.
When prod runs in the default Entra mode, this wires the BFF's LITELLM_GATEWAY_KEY to the dashboard-playground virtual key instead of the master key. The dashboard admin page is restricted to dashboard.admin but its key explorer calls /key/list through dashboard/hooks/use-keys.ts; LiteLLM documents the master key as the Proxy Admin key (https://docs.litellm.ai/docs/proxy/virtual_keys), while the generated dashboard-playground key in scripts/keys.yaml only has model/budget limits. As a result, signed-in admins will no longer be able to list/manage gateway keys through the Entra dashboard and will fall back to seeing only that virtual key or a permission error. Keep the admin BFF path on the master/admin credential, or split playground/smoke traffic onto the virtual key separately.
Useful? React with 👍 / 👎.
Summary
Verification
Note: local dashboard pnpm install is blocked by Windows ACL errors under dashboard/node_modules, so dashboard vitest/next build were not rerun for this workflow/Terraform-only follow-up.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation