feat(cli): aisix export — emit resources.yaml from a running etcd store - #761
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds ChangesResource export workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as aisix export
participant Etcd as etcd
participant Loader as Snapshot loader
participant Builder as build_export_document
participant Emitter as emit_yaml
participant FileGateway as file-mode gateway
CLI->>Etcd: load resources under prefix
Etcd-->>Loader: return stored documents
Loader-->>Builder: provide AisixSnapshot
Builder-->>Emitter: provide redacted ExportDocument
Emitter-->>CLI: return resources YAML
CLI->>FileGateway: start with YAML and environment values
FileGateway-->>CLI: serve equivalent gateway behavior
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ 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.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/aisix-server/src/export/secrets.rs (1)
76-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared redaction-context struct instead of repeating the 5-arg tail across
redact_top_level,redact_by_key, andredact_headers.All three functions carry the same
(kind_token, kind, identity, reveal, out)tail. Bundling them into a smallstruct RedactionCtx<'a> { kind_token: &'a str, kind: &'static str, identity: &'a str, reveal: bool, out: &'a mut Vec<SecretPlaceholder> }would cut duplication and remove the risk of transposing two same-typed positional&strargs (kind_tokenvskind) at a call site.♻️ Sketch
pub struct RedactionCtx<'a> { pub kind_token: &'a str, pub kind: &'static str, pub identity: &'a str, pub reveal: bool, pub out: &'a mut Vec<SecretPlaceholder>, } pub fn redact_top_level(doc: &mut Value, field: &str, ctx: &mut RedactionCtx) { ... }🤖 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 `@crates/aisix-server/src/export/secrets.rs` around lines 76 - 183, Introduce a shared RedactionCtx<'a> containing kind_token, kind, identity, reveal, and out, then update redact_top_level, redact_by_key, and redact_headers to accept the context instead of their repeated five-argument tail. Adjust recursive redact_by_key calls and all callers to pass the context while preserving existing redaction behavior and placeholder output.tests/e2e/src/cases/export-roundtrip-e2e.test.ts (1)
110-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
afterAllcleanup isn't isolated — an early failure skips the remaining teardown.If
fileApp?.exit()rejects,etcdApp?.exit()andupstream?.close()never run, potentially leaking the spawned processes for subsequent test files.♻️ Proposed fix
afterAll(async () => { - await fileApp?.exit(); - await etcdApp?.exit(); - await upstream?.close(); + await Promise.allSettled([fileApp?.exit(), etcdApp?.exit(), upstream?.close()]); });🤖 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 `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts` around lines 110 - 114, Update the afterAll teardown to isolate cleanup failures so a rejection from fileApp?.exit() does not prevent etcdApp?.exit() or upstream?.close() from running. Ensure each resource cleanup is attempted independently while preserving the existing optional-resource behavior.crates/aisix-server/src/export/yaml_emit.rs (1)
20-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCentralize the resources-file format version
yaml_emit.rsandfilesource/mod.rsboth pin"1"separately. Keep a single shared constant so a future version bump updates the emitter and loader together.🤖 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 `@crates/aisix-server/src/export/yaml_emit.rs` around lines 20 - 29, Centralize the resources-file format version currently duplicated in yaml_emit.rs and filesource/mod.rs into one shared public constant. Update emit_yaml and the loader’s version validation to reference that constant, removing both local "1" definitions so future version changes require a single update.
🤖 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 `@crates/aisix-server/src/export/mod.rs`:
- Around line 84-96: Update the output-file handling in the export command
around args.output and Export::reveal_secrets so files written with
--reveal-secrets are created with owner-only permissions (0600) before any
secret-containing data is written. Preserve the existing write error reporting
and normal output behavior for exports without revealed secrets.
In `@crates/aisix-server/src/export/secrets.rs`:
- Around line 46-66: Add a post-pass in document.rs::build_export_document over
ExportDocument::secret_placeholders to detect repeated env_var values. Emit a
warning, matching the existing duplicate file-identity check, whenever two
distinct placeholders resolve to the same environment variable, including enough
identity context to identify both secrets. Preserve the existing placeholder
generation and other validation behavior.
In `@crates/aisix-server/src/main.rs`:
- Around line 87-89: Update the prefix argument definition in the main CLI
configuration to reuse the canonical default from aisix_core::EtcdConfig instead
of hardcoding "/aisix". Preserve the existing long option and String type while
ensuring aisix export stays aligned with future EtcdConfig prefix changes.
In `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts`:
- Around line 56-66: Update the beforeAll setup around EtcdClient.ping so
unavailable etcd always follows the existing skip flow, including when
process.env.CI is set. Remove the hard Error path and ensure the test’s
established ctx.skip() handling remains responsible for skipping without
failing.
---
Nitpick comments:
In `@crates/aisix-server/src/export/secrets.rs`:
- Around line 76-183: Introduce a shared RedactionCtx<'a> containing kind_token,
kind, identity, reveal, and out, then update redact_top_level, redact_by_key,
and redact_headers to accept the context instead of their repeated five-argument
tail. Adjust recursive redact_by_key calls and all callers to pass the context
while preserving existing redaction behavior and placeholder output.
In `@crates/aisix-server/src/export/yaml_emit.rs`:
- Around line 20-29: Centralize the resources-file format version currently
duplicated in yaml_emit.rs and filesource/mod.rs into one shared public
constant. Update emit_yaml and the loader’s version validation to reference that
constant, removing both local "1" definitions so future version changes require
a single update.
In `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts`:
- Around line 110-114: Update the afterAll teardown to isolate cleanup failures
so a rejection from fileApp?.exit() does not prevent etcdApp?.exit() or
upstream?.close() from running. Ensure each resource cleanup is attempted
independently while preserving the existing optional-resource behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4ad96d1-be9d-4d5c-9125-90254ff5a78a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/aisix-server/Cargo.tomlcrates/aisix-server/src/export/document.rscrates/aisix-server/src/export/mod.rscrates/aisix-server/src/export/secrets.rscrates/aisix-server/src/export/yaml_emit.rscrates/aisix-server/src/main.rstests/e2e/src/cases/export-roundtrip-e2e.test.ts
83acbca to
bf7bf94
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/aisix-server/src/export/document.rs`:
- Around line 362-371: Make duplicate identity detection in the export flow
fatal instead of continuing to append the colliding document: when
seen.insert(id.clone()) fails, return an error or otherwise abort ordinary
export before resugar, redact, and out.push. If the exporter already exposes an
explicit lossy-mode option, only allow continuation when that mode is enabled;
otherwise preserve the existing warning context and add coverage proving
ordinary export cannot succeed with duplicate identities.
- Around line 126-148: Update guardrail export handling in the guardrails
emit_entries flow and its related import/serialization paths so attachment scope
is preserved; do not export guardrails when attachments cannot be represented
equivalently, unless the existing explicit lossy-export opt-in is enabled.
Ensure scoped guardrails cannot become gateway-wide policies after import.
In `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts`:
- Around line 131-141: Extend the no-live-credential test around placeholderVars
to assert that stderr does not contain PROVIDER_SECRET, while preserving the
existing checks for the placeholder variable diagnostics and redacted YAML
output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b0816a8-57fa-4ba7-91cc-b76f7e266626
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/aisix-server/Cargo.tomlcrates/aisix-server/src/export/document.rscrates/aisix-server/src/export/mod.rscrates/aisix-server/src/export/secrets.rscrates/aisix-server/src/export/yaml_emit.rscrates/aisix-server/src/main.rstests/e2e/src/cases/export-roundtrip-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/aisix-server/src/main.rs
- crates/aisix-server/src/export/yaml_emit.rs
- crates/aisix-server/src/export/mod.rs
- crates/aisix-server/src/export/secrets.rs
bf7bf94 to
7a24b99
Compare
Review triage (pushed in 7a24b99)CodeRabbit findings — all 4 addressed:
Independent audit (fresh cold-context agent, per our merge-gate) also surfaced and I fixed:
Gates: |
c3d18b0 to
66a40ee
Compare
Second review round triaged (head
|
66a40ee to
e6c9cef
Compare
Add an `aisix export` subcommand that reads the resources currently stored
in etcd and emits a resources.yaml the file source (`resources_file`) can
load back — the inverse of the file loader and the migration/backup path
for a standalone deployment moving from Admin-API-plus-etcd to the
declarative file.
Export decodes each kind through the same typed models the gateway loads
(via the shared snapshot builder), then rewrites the set into idiomatic
file form:
- resugar references: model.provider_key_id -> `provider_key: <name>`;
rate_limit_policy.scope_ref (api_key/model) -> the referenced entry's
name; cache_policy.applies_to `api_key:<id>` -> the id the file loader
will derive for that key. A dangling model/rate-limit reference (or an
identity collision) leaves the file non-loadable: it is surfaced and
the command exits non-zero (the file is still written for inspection)
so a scripted migration can't mistake a broken export for a finished one.
- ids are dropped (the file derives them from names); api_keys get a
deterministic `apikey-<hash…>` display_name synthesized from their
already-hashed key_hash, which is emitted verbatim.
- guardrails: the file has no attachment collection, so a file guardrail
applies gateway-wide. Only guardrails already gateway-wide in etcd (an
env-scoped attachment) are exported; an attachment-scoped or unattached
guardrail would silently widen to all traffic on import, so it is
omitted with a warning.
Secrets are never emitted in cleartext by default: provider_key api_key
and its request.default_headers / default_body_fields, mcp/a2a secret,
guardrail moderation credentials, and OTLP exporter auth headers are
replaced with derived `${VAR}` placeholders, and a companion "set these
before loading" list is printed to stderr (never into the file).
`--reveal-secrets` opts into inline values for air-gapped same-host
migration; the `-o` file is written owner-only (0600 on unix).
Emission reuses the file source's own YAML library so the output is
guaranteed to re-parse, and every literal string value is `$`-escaped so
a stored value containing `$` or a literal `${...}` (e.g. a guardrail
blocklist rule for `${jndi:...}`) round-trips through the loader's
interpolation instead of being re-interpreted.
Round-trip proven end to end: a unit test builds -> emits -> re-loads
through the real file source, and e2e seeds etcd -> `aisix export` ->
loads the file into a file-mode gateway -> asserts identical /v1/models,
chat completion, and allowed_models 403 against the etcd-mode gateway,
plus a per-kind no-live-credential assertion (file and stderr) at
default settings.
e6c9cef to
bc9fd43
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/src/cases/export-roundtrip-e2e.test.ts (1)
203-273: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover
a2a_agents.secretin the no-leak E2E test.The test claims every secret-bearing kind but only seeds provider keys, MCP, guardrails, and OTLP. Seed a valid A2A agent with a distinct marker, assert it is exported, and check both YAML and stderr for leakage.
As per coding guidelines, prioritize source-blind E2E coverage and never serialize authentication secrets without redaction.
🤖 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 `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts` around lines 203 - 273, Extend the test default export redaction case around the existing marker and seed setup to include a valid A2A agent with a distinct a2a_agents.secret marker. Include the agent name in the exported-entry presence assertions, and add its marker to the YAML and stderr leak checks so the test verifies redaction while preserving the existing placeholder assertion.Source: Coding guidelines
🤖 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 `@crates/aisix-server/src/export/document.rs`:
- Around line 314-327: Update the placeholder collision tracking in the loop
using var_owner so ownership includes both the placeholder identity and its raw
field path, rather than identity alone. Compare the full (identity, field) owner
when detecting collisions, while preserving the existing warning for distinct
owners that derive the same environment variable.
In `@crates/aisix-server/src/export/secrets.rs`:
- Around line 175-187: Update redact_string_map to recursively traverse nested
objects and arrays under the selected field, redacting every non-empty string
while preserving non-string scalars. Extend field-key generation and placeholder
labels to identify nested values consistently, and add coverage for nested
objects and array elements such as authentication tokens.
---
Nitpick comments:
In `@tests/e2e/src/cases/export-roundtrip-e2e.test.ts`:
- Around line 203-273: Extend the test default export redaction case around the
existing marker and seed setup to include a valid A2A agent with a distinct
a2a_agents.secret marker. Include the agent name in the exported-entry presence
assertions, and add its marker to the YAML and stderr leak checks so the test
verifies redaction while preserving the existing placeholder assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d3cfe75-3232-4cbb-91dd-f3fb37a97926
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/aisix-server/Cargo.tomlcrates/aisix-server/src/export/document.rscrates/aisix-server/src/export/document_tests.rscrates/aisix-server/src/export/mod.rscrates/aisix-server/src/export/secrets.rscrates/aisix-server/src/export/yaml_emit.rscrates/aisix-server/src/main.rstests/e2e/src/cases/export-roundtrip-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/aisix-server/src/main.rs
- crates/aisix-server/src/export/yaml_emit.rs
| let mut var_owner: BTreeMap<&str, &str> = BTreeMap::new(); | ||
| for placeholder in &placeholders { | ||
| match var_owner.get(placeholder.env_var.as_str()) { | ||
| Some(&owner) if owner != placeholder.identity => diag.warnings.push(format!( | ||
| "secret placeholder ${{{}}} is derived for both {:?} and {:?}; their names \ | ||
| collapse to the same environment variable, so one real credential cannot be \ | ||
| supplied to each — rename one entry to disambiguate", | ||
| placeholder.env_var, owner, placeholder.identity | ||
| )), | ||
| Some(_) => {} | ||
| None => { | ||
| var_owner.insert(&placeholder.env_var, &placeholder.identity); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Include the field when detecting placeholder collisions.
var_owner treats every placeholder from the same identity as equivalent. For example, headers x-auth-token and x.auth.token on one provider key derive the same variable but receive no warning, causing one credential to feed both fields. Track (identity, field)—preferably the raw field path—as the owner.
This is the same collision class as the earlier cross-identity finding, but the same-identity variant remains unresolved.
🤖 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 `@crates/aisix-server/src/export/document.rs` around lines 314 - 327, Update
the placeholder collision tracking in the loop using var_owner so ownership
includes both the placeholder identity and its raw field path, rather than
identity alone. Compare the full (identity, field) owner when detecting
collisions, while preserving the existing warning for distinct owners that
derive the same environment variable.
| pub fn redact_string_map(doc: &mut Value, field: &str, label: &str, ctx: &mut RedactionCtx<'_>) { | ||
| if ctx.reveal { | ||
| return; | ||
| } | ||
| let Some(Value::Object(map)) = doc.get_mut(field) else { | ||
| return; | ||
| }; | ||
| for (name, value) in map.iter_mut() { | ||
| if is_nonempty_str(value) { | ||
| let field_key = format!("{}_{}", sanitize(field), sanitize(name)); | ||
| *value = ctx.placeholder(&field_key, format!("{label} {name}")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Recursively redact nested default_body_fields strings.
This loop only redacts direct string children. Values such as {"auth":{"token":"live-secret"}} or arrays retain their credentials verbatim. Traverse nested objects and arrays, preserving non-string scalars, and add nested-value coverage.
As per coding guidelines, “Do not log, serialize, or return API keys, tokens, OAuth client secrets, or authentication headers without redaction.”
🤖 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 `@crates/aisix-server/src/export/secrets.rs` around lines 175 - 187, Update
redact_string_map to recursively traverse nested objects and arrays under the
selected field, redacting every non-empty string while preserving non-string
scalars. Extend field-key generation and placeholder labels to identify nested
values consistently, and add coverage for nested objects and array elements such
as authentication tokens.
Source: Coding guidelines
What
Adds an
aisix exportsubcommand — the inverse of the file source (resources_file). It reads the resources currently stored in etcd and emits aresources.yamlthat the file loader can load back, so a standalone deployment can move from Admin-API-plus-etcd to the declarative file (or take a portable, declarative backup of its live config).Default output is stdout; the secret-placeholder list and any warnings go to stderr.
How it stays faithful to the loader
Export decodes every kind through the same typed models the gateway loads (the shared snapshot builder), so the exported set is exactly what the running gateway would serve. It then rewrites each entry into idiomatic file form — the exact sugar the file source desugars on the way in:
model.provider_key_id→provider_key: <that provider key's display_name>rate_limit_policy.scope_refforapi_key/modelscopes → the referenced entry's name;team/member/team_memberpass through verbatim.idis emitted. Canonical api-key documents carry no name, so a deterministicapikey-<hash…>display_nameis synthesized from the entry's already-hashedkey_hash(emitted verbatim — it round-trips exactly and is not plaintext).guardrail_attachments(which the file format has no collection for), are reported on stderr rather than dropped silently.Secret policy
No live credential is written in cleartext by default. Each secret-bearing field is replaced with a derived, stable, greppable
${VAR}placeholder, and a companion "set these before loading" list is printed to stderr — never into the file:provider_key.api_key,mcp_server.secret,a2a_agent.secretapi_key,access_key_secret, and the nested Bedrocksecret_access_key)headersapi_keysemitkey_hashdirectly (already a hash — round-trips exactly; no placeholder).--reveal-secretsopts into emitting the real stored values inline; it is documented as unsafe and intended only for air-gapped, same-host migration.The placeholder names use the
AISIXSECRET_…prefix rather thanAISIX_…: the gateway's config loader claims theAISIX_prefix and rejects unknown keys, so anAISIX_…secret variable set in the data plane's environment would be misread as a bad config override and fail boot (the same reason the existingSLS_CRED_…/OBJSTORE_CRED_…conventions keep secret variables off that prefix).Emission reuses the file source's own YAML library, so the output is guaranteed to re-parse (including the mandatory quoted
_format_version: "1").Tests
key_hashpassthrough, duplicate-name warn, id-stripping, secret placeholder derivation + no-cleartext-by-default +--reveal-secrets, recursive/nested guardrail + header redaction, and YAML round-trip (format-version stays a string, placeholders survive).tests/e2e/src/cases/export-roundtrip-e2e.test.ts): seed etcd → runaisix export→ load the exported file into a file-mode gateway (supplying the real secret through the emitted placeholder variable) → assert identical observable behavior (/v1/models, a chat completion, and anallowed_models403) against the etcd-mode gateway, and assert the exported file contains no live credential at default settings. This closes the loop: etcd → export → file → same behavior.Full workspace suite green (2299 passed) run twice;
cargo fmt --checkandcargo clippy --workspace --all-targets -D warningsclean; resource schemas untouched.Summary by CodeRabbit
aisix exportcommand to export gateway resources from etcd into a YAML “resources” document.${VAR}placeholders, with operator-facing warning/blocking reporting.${...}and improved handling of missing, dangling, duplicate, and omitted items.