feat(config): lenient etcd parsing with tri-state compatibility reporting - #872
Merged
Conversation
An api_key document carrying a field this DP version does not know is currently whole-row rejected at schema validation. Issue #871 requires the row to load with the unknown field ignored and be reported as partially compatible, aggregated per kind and field path. The test pins the target contract and fails against current behavior; the PartialCompatEntry reporting surface is added as data only, with no behavior change yet.
Split resource schema validation into strict-write / lenient-read: - The strict validator set (validate_*, SCHEMAS) keeps rejecting unknown fields on every write path (Admin API, file source). Strictness now comes from a mechanical close_unknown_fields pass over the shared producers instead of #[serde(deny_unknown_fields)]; the published schemas/resources/*.json are byte-identical, proving the write contract unchanged. - The lenient set (validate_*_lenient, LENIENT_SCHEMAS) tolerates unknown fields and is used only by the etcd snapshot loader, so a document written by a newer control plane loads instead of being whole-row rejected under the CP-before-DP upgrade order. The loader classifies every row: - RED (incompatible): lenient schema or serde fails - type conflicts, missing required fields, range violations, unknown enum values, and unknown fields inside closed tagged-enum shapes where serde silently drops what it cannot see. Skipped as before, now logged at ERROR. - YELLOW (partially compatible): the row loads with unknown fields ignored; serde_ignored collects the exact paths, aggregated per (kind, field) with row counts in a buffer separate from the rejection list. WARN deduped per (kind, field-set). - GREEN: exact match, unchanged. deny_unknown_fields is dropped from the 25 plain resource structs; the guardrail and observability-exporter tagged payload structs keep it, and the hand-injected additionalProperties:false on the exporter branches and guardrail sub-enums stays closed in both sets (secret-smuggling guard; also the only non-silent option inside Content-buffered shapes). The three provider_key *_currently_rejected characterization tests flip to partially-compatible loads: the Bedrock and Vertex bridges read region/project from the credential JSON inside api_key and the Azure bridge derives its resource from api_base, so the adapter_map top-level fields are not consumed by this DP and a dead model field would be worse than an ignored-and-reported one. Serde-level rejects_unknown_* unit tests flip to tolerates_unknown_*_for_forward_compat; closed enum values keep rejecting.
…871) The supervisor retains the loader's per-row unknown-field observations in a buffer of its own, keyed by etcd key so watch events merge incrementally: a Put replaces or clears the key's entry (a rejected update keeps it - the previously loaded value is still what serves), a Delete removes it, a resync replaces the map wholesale. Capped separately from the rejection buffer so YELLOW volume can never evict RED entries, with a WARN when the cap truncates the report. Reporting surfaces, all landed with the leniency flip so there is no silent-tolerance window: - GET /status/config gains partially_compatible[] next to rejected[], aggregated per (kind, field) with row counts - the companion that keeps a matching config_hash from hiding that enforcement differs from the stored documents. - The managed-mode heartbeat gains partially_compatible_resources, omitted when empty so older control planes keep seeing the historical body shape (their handler tolerates unknown fields). - Prometheus gains aisix_config_partially_compatible_resources{kind}, a row-count gauge mirroring aisix_config_rejected_resources including the stale-label zeroing discipline; per-field detail stays off the labels (field paths are not a bounded set). File mode passes an empty aggregate: the standalone file source keeps validating strictly, since a hand-edited file has no version-skew writer and strictness there is typo protection.
E2E against the real binary and etcd: - an api_key document carrying an unknown field authenticates real traffic and is reported in partially_compatible[] with the exact field and row count, plus the per-kind gauge on the metrics listener; - an unknown enum value (routing strategy) stays rejected - no lenient fallback exists for a value the gateway cannot interpret; - deleting the forward-compat row clears the report and zeroes the gauge, and a converged same-version config reports zero partially compatible rows, preserving the typo-catching strictness; - the Admin API write path still rejects unknown fields with 400. CONTRIBUTING gains the rollout policy for resource-model changes: added fields are the safe, reported-as-partial change; new enum values are not forward compatible by design and each needs an explicit decision - version-gate at the control plane, a #[serde(other)] fallback only where semantically safe, or an accepted loud rejection.
The published resource schemas describe the Admin API write contract; the etcd read path is deliberately more lenient since the tri-state compatibility change, with unknown fields loaded-and-reported instead of whole-row rejected. Also records why the observability-exporter branches stay closed on both paths.
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughChangesThe change adds lenient etcd reads for unknown fields while retaining strict Admin API writes. It records partial compatibility in configuration status, Prometheus metrics, heartbeat payloads, and end-to-end tests. Known enum and schema violations remain rejected. Configuration compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Etcd
participant Loader
participant Supervisor
participant Status
participant Metrics
participant Heartbeat
Etcd->>Loader: Read configuration rows
Loader->>Loader: Validate and collect ignored fields
Loader->>Supervisor: Serve rows and compatibility data
Supervisor->>Status: Record partial compatibility
Supervisor->>Metrics: Synchronize per-kind counts
Supervisor->>Heartbeat: Provide recent compatibility aggregate
Heartbeat->>Heartbeat: Serialize non-empty compatibility entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
crates/aisix-core/src/models/model.rs (1)
515-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every newly lenient nested model type.
The test checks only an unknown top-level field on
Model. It does not verify unknown fields forModelCost,BackgroundModelCheck,CooldownConfig, orAutoPromptCaching. Add one nested compatibility case for each changed type.🤖 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-core/src/models/model.rs` around lines 515 - 527, Extend tolerates_unknown_top_level_fields_for_forward_compat with separate deserialization cases covering ModelCost, BackgroundModelCheck, CooldownConfig, and AutoPromptCaching. Add an unknown field to each type’s JSON fixture, assert deserialization succeeds, and verify a representative known field to confirm the nested model remains correctly parsed.Source: Coding guidelines
crates/aisix-server/src/heartbeat.rs (1)
482-487: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider bounding the aggregate size on the wire.
config_hashcarries a defensive clamp atCONFIG_HASH_MAX_CHARSeven though the value is a fixed-length hash.partially_compatible_resourceshas no equivalent bound.The DP side is already well bounded:
MAX_RETAINED_PARTIAL_ROWScaps retained rows at 1024, and array-index normalization keeps entry count tied to document shape rather than data volume. So the practical risk is small. A control plane that introduces many distinct unknown field names per kind would still grow this array without limit.A
take(N)here would match the defensive posture applied toconfig_hashand keep the heartbeat body size predictable.🤖 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/heartbeat.rs` around lines 482 - 487, The partially compatible resource aggregate is unbounded on the wire. In the heartbeat construction around partially_compatible_resources, limit the iterator to a suitable explicit maximum with take(N) before collecting, matching the defensive bounding used for config_hash; preserve the existing sorting and PartialCompatWire conversion behavior.crates/aisix-etcd/src/loader.rs (2)
482-508: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEmit the WARN outside the dedup lock.
warn_partial_compat_dedupedholds theWARNEDmutex guard acrosstracing::warn!. Two consequences follow:
- The critical section covers subscriber work — formatting and writer I/O.
Supervisor::apply_putruns concurrently (the supervisor has a 200-way concurrent-put test), so every YELLOW row on every thread serializes on this one global mutex while a log line is written.- A panic inside the subscriber while the guard is held poisons the mutex. Every later call then hits the
.expect(...)and panics, which turns a logging fault into a loader fault.Decide under the lock, then release it before emitting.
♻️ Proposed fix: release the lock before logging
let fields_joined = fields.join(","); let entry = (kind.to_string(), fields_joined); - let mut warned = WARNED - .get_or_init(|| Mutex::new(HashSet::new())) - .lock() - .expect("partial-compat warn dedup set is never poisoned"); - if warned.contains(&entry) { - return; - } + // Decide and record under the lock; log after releasing it so a + // subscriber's formatting/IO never runs inside the critical section + // (and a panic there cannot poison the set). + let set = WARNED.get_or_init(|| Mutex::new(HashSet::new())); + { + let mut warned = match set.lock() { + Ok(g) => g, + // A poisoned set only costs dedup accuracy, never correctness. + Err(poisoned) => poisoned.into_inner(), + }; + if warned.contains(&entry) { + return; + } + if warned.len() < MAX_REMEMBERED { + warned.insert(entry.clone()); + } + } tracing::warn!( key = %key, kind = %kind, ignored_fields = %entry.1, "row loaded with unknown fields ignored (partially compatible; \ likely written by a newer control plane)" ); - if warned.len() < MAX_REMEMBERED { - warned.insert(entry); - } }🤖 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-etcd/src/loader.rs` around lines 482 - 508, Update warn_partial_compat_deduped so the WARNED mutex is held only while checking and recording the entry, then explicitly release the guard before calling tracing::warn!. Preserve the existing deduplication and MAX_REMEMBERED behavior, including returning without logging for previously seen entries.
404-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRED rows re-log at ERROR on every resync.
Schema failures now log at ERROR. The YELLOW path is deduplicated by
warn_partial_compat_deduped, but this path is not. A resync rebuilds the whole snapshot on a cadence, so one permanently invalid row emits one ERROR per row per cycle for the life of the process.The severity bump is intentional and correct. Consider applying the same dedup discipline here so a single unfixable row does not dominate the error stream. The supervisor already retains the rejection for reporting, so log repetition adds no signal.
🤖 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-etcd/src/loader.rs` around lines 404 - 410, The schema-validation failure in the validate path logs repeatedly on every snapshot resync. Apply the existing deduplication approach used by warn_partial_compat_deduped to this rejection path, while preserving ERROR severity and the supervisor’s retained rejection reporting; ensure each permanently invalid key is logged only once.crates/aisix-server/src/main.rs (1)
780-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the new field to the heartbeat comment list.
The comment block above enumerates what each tick reports:
rejected_resources,applied_revision,config_hash,supported_guardrail_kinds, andexporter_health. The wiring now also reportspartially_compatible_resources. Extend the list so the enumeration stays complete.The fetcher wiring itself matches the three sibling fetchers exactly.
📝 Proposed comment update
// - config_hash: the hash of the applied (served) config set, so // cp-api can diff the hash a node reports against the hash it // expects that node to be serving (`#774`) + // - partially_compatible_resources: served rows carrying fields + // this build does not know, aggregated per (kind, field), so + // cp-api can show "loaded but not enforced" during a rollout + // where the CP writes ahead of the DP fleet (`#871`) // - supported_guardrail_kinds + exporter_health (`#519` B.6 / D.2)🤖 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/main.rs` around lines 780 - 810, Update the heartbeat comment block above heartbeat_task to include partially_compatible_resources in the enumerated fields reported on each tick, alongside the existing fields. Leave the fetcher wiring and surrounding implementation unchanged.crates/aisix-etcd/src/supervisor.rs (1)
1646-1731: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the retention cap.
The four new tests cover put, clear-on-exact-match, delete, resync replacement, and retention across a rejected update. They do not exercise
MAX_RETAINED_PARTIAL_ROWS.Two cap behaviors are worth pinning, because both are silent from the caller's view:
set_partial_rowsstops inserting after the cap and warns once.update_partial_rowskips a new key at the cap but still replaces an existing key.The second rule is the subtle one. A test would prevent a future refactor from dropping updates for rows already reported.
🤖 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-etcd/src/supervisor.rs` around lines 1646 - 1731, Add tests covering MAX_RETAINED_PARTIAL_ROWS through set_partial_rows and update_partial_row. Verify set_partial_rows stops adding rows at the cap and emits the warning only once, while update_partial_row ignores a new key when full but still replaces an existing retained key. Preserve assertions for retained contents and replacement behavior.
🤖 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 `@CONTRIBUTING.md`:
- Around line 56-59: Update the “Adding a field” guidance in CONTRIBUTING.md to
limit the compatibility rule to open schema branches. Explicitly state that
intentionally closed branches, including observability-exporter, remain strict
and may be rejected by older gateways rather than reported as partially
compatible; preserve the existing observability details for open branches.
In `@crates/aisix-core/src/bin/dump-schema.rs`:
- Around line 78-108: Regenerate all committed resource schemas using the
updated dump function and commit the resulting changes under schemas/resources.
Ensure cache policy, guardrail, guardrail attachment, observability exporter,
and any other struct-shaped resource roots include additionalProperties: false,
and verify the generated diff contains no remaining schema drift.
In `@tests/e2e/src/cases/config-forward-compat-e2e.test.ts`:
- Around line 173-179: Make the test named “deleting the forward-compat row
clears the report; converged config has zero partially-compatible rows”
self-contained instead of relying on the first test’s yellowKeyId assignment.
Create its own partially compatible API-key row and retain the existing delete
and assertions using that locally created identifier, so it passes when run
independently.
---
Nitpick comments:
In `@crates/aisix-core/src/models/model.rs`:
- Around line 515-527: Extend
tolerates_unknown_top_level_fields_for_forward_compat with separate
deserialization cases covering ModelCost, BackgroundModelCheck, CooldownConfig,
and AutoPromptCaching. Add an unknown field to each type’s JSON fixture, assert
deserialization succeeds, and verify a representative known field to confirm the
nested model remains correctly parsed.
In `@crates/aisix-etcd/src/loader.rs`:
- Around line 482-508: Update warn_partial_compat_deduped so the WARNED mutex is
held only while checking and recording the entry, then explicitly release the
guard before calling tracing::warn!. Preserve the existing deduplication and
MAX_REMEMBERED behavior, including returning without logging for previously seen
entries.
- Around line 404-410: The schema-validation failure in the validate path logs
repeatedly on every snapshot resync. Apply the existing deduplication approach
used by warn_partial_compat_deduped to this rejection path, while preserving
ERROR severity and the supervisor’s retained rejection reporting; ensure each
permanently invalid key is logged only once.
In `@crates/aisix-etcd/src/supervisor.rs`:
- Around line 1646-1731: Add tests covering MAX_RETAINED_PARTIAL_ROWS through
set_partial_rows and update_partial_row. Verify set_partial_rows stops adding
rows at the cap and emits the warning only once, while update_partial_row
ignores a new key when full but still replaces an existing retained key.
Preserve assertions for retained contents and replacement behavior.
In `@crates/aisix-server/src/heartbeat.rs`:
- Around line 482-487: The partially compatible resource aggregate is unbounded
on the wire. In the heartbeat construction around
partially_compatible_resources, limit the iterator to a suitable explicit
maximum with take(N) before collecting, matching the defensive bounding used for
config_hash; preserve the existing sorting and PartialCompatWire conversion
behavior.
In `@crates/aisix-server/src/main.rs`:
- Around line 780-810: Update the heartbeat comment block above heartbeat_task
to include partially_compatible_resources in the enumerated fields reported on
each tick, alongside the existing fields. Leave the fetcher wiring and
surrounding implementation unchanged.
🪄 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 Plus
Run ID: c0864c1b-0fa8-454d-bed6-631659eefe2b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
CONTRIBUTING.mdCargo.tomlcrates/aisix-admin/src/lib.rscrates/aisix-core/src/bin/dump-schema.rscrates/aisix-core/src/config_status.rscrates/aisix-core/src/filesource/status.rscrates/aisix-core/src/models/a2a_agent.rscrates/aisix-core/src/models/apikey.rscrates/aisix-core/src/models/embedding.rscrates/aisix-core/src/models/ensemble.rscrates/aisix-core/src/models/mcp_policy.rscrates/aisix-core/src/models/mcp_server.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/model.rscrates/aisix-core/src/models/oidc_provider.rscrates/aisix-core/src/models/provider_key.rscrates/aisix-core/src/models/rate_limit.rscrates/aisix-core/src/models/rate_limit_policy.rscrates/aisix-core/src/models/routing.rscrates/aisix-core/src/models/schema.rscrates/aisix-core/src/models/semantic.rscrates/aisix-etcd/Cargo.tomlcrates/aisix-etcd/src/loader.rscrates/aisix-etcd/src/supervisor.rscrates/aisix-obs/src/metrics.rscrates/aisix-server/src/heartbeat.rscrates/aisix-server/src/main.rsschemas/README.mdtests/e2e/src/cases/config-forward-compat-e2e.test.ts
Comment on lines
78
to
+108
| fn dump<T: JsonSchema>(out_dir: &Path, name: &str) { | ||
| // Serialize the `RootSchema` directly to preserve schemars' native key | ||
| // ordering. (Routing through `serde_json::Value` would re-sort keys.) | ||
| let mut json = | ||
| serde_json::to_string_pretty(&schemars::schema_for!(T)).expect("serialize schema"); | ||
| // These nested types belong to closed resources, so re-close the root | ||
| // and every struct-shaped definition on the typed schema — the same | ||
| // strictness `schema::close_unknown_fields` applies to the resource | ||
| // documents, kept typed here so the key order stays schemars-native. | ||
| let mut root = schemars::schema_for!(T); | ||
| close_object_schema(&mut root.schema); | ||
| for def in root.definitions.values_mut() { | ||
| if let schemars::schema::Schema::Object(obj) = def { | ||
| close_object_schema(obj); | ||
| } | ||
| } | ||
| let mut json = serde_json::to_string_pretty(&root).expect("serialize schema"); | ||
| json.push('\n'); | ||
| let path = out_dir.join(format!("{name}.schema.json")); | ||
| fs::write(&path, json).unwrap_or_else(|e| panic!("write {}: {e}", path.display())); | ||
| println!("wrote {}", path.display()); | ||
| } | ||
|
|
||
| /// Insert `additionalProperties: false` on a struct-shaped schema object | ||
| /// (one that lists `properties`), unless it already pins a value. | ||
| fn close_object_schema(schema: &mut schemars::schema::SchemaObject) { | ||
| let Some(object) = schema.object.as_deref_mut() else { | ||
| return; | ||
| }; | ||
| if !object.properties.is_empty() && object.additional_properties.is_none() { | ||
| object.additional_properties = Some(Box::new(schemars::schema::Schema::Bool(false))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show the committed nested-type schemas and check that every
# struct-shaped object carries additionalProperties: false.
set -euo pipefail
fd -t f '.*\.schema\.json$' schemas/resources | sort
for name in ensemble rate_limit routing semantic embedding; do
f="schemas/resources/${name}.schema.json"
[ -f "$f" ] || { echo "MISSING $f"; continue; }
echo "== $f =="
jq '{root_ap: .additionalProperties,
defs_missing_ap: (.definitions // {} | to_entries
| map(select(.value.properties != null and (.value | has("additionalProperties") | not)))
| map(.key))}' "$f"
done
# The strict resource schemas come from `resource_root_schema(_, true)`.
for f in $(fd -t f '.*\.schema\.json$' schemas/resources); do
jq -e 'if (.properties != null) then (.additionalProperties != null) else true end' "$f" >/dev/null \
|| echo "root not closed: $f"
doneRepository: api7/aisix
Length of output: 1585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dump-schema.rs relevant content =="
sed -n '1,160p' crates/aisix-core/src/bin/dump-schema.rs | cat -n
echo "== schema file summary =="
for f in $(fd -t f '.*\.schema\.json$' schemas/resources | sort); do
status="OK"
jq -e 'if (.properties != null or (is_object and any(. != null))) then (.additionalProperties != null) else true end' "$f" >/dev/null || status="CLOSED_FALSE"
jq -e 'if (.properties != null or (is_object and any(. != null))) then .definitions as $d | to_entries | map(select(.value.properties != null and (has("additionalProperties") | not or .value.additionalProperties == null))) | length == 0; else true end' "$f" >/dev/null || status="DEF_MISSING_AP"
echo "$status:$f"
doneRepository: api7/aisix
Length of output: 31347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
for path in sorted(pathlib.Path('schemas/resources').glob('*.schema.json')):
data = {}
with open(path, 'r') as f:
data = json.load(f)
root = data.get('additionalProperties')
missing_defs = [
defname for defname, def_schema in (data.get('definitions') or {}).items()
if isinstance(def_schema, dict) and def_schema.get('properties')
and def_schema.get('additionalProperties') is None
]
root_status = 'present' if root is not None else 'missing'
print(f"{path}: root_additionalProperties={root_status}, missing_struct_defs={missing_defs}")
PY
echo "== schema resource producer search =="
rg -n "resource_root_schema|close_unknown_fields|deny_unknown_fields" crates/aisix-core/src/models crates/aisix-core/src -g '*.rs' | head -200Repository: api7/aisix
Length of output: 11290
Regenerate the schema files before merging.
The generator now closes struct-shaped schemas with additionalProperties: false, but the committed outputs do not all match that shape. Some resource roots still miss additionalProperties, including cache policy, guardrail, guardrail attachment, and observability exporter; update schemas/resources/*.schema.json and run the generator diff so schema drift cannot land silently.
🤖 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-core/src/bin/dump-schema.rs` around lines 78 - 108, Regenerate
all committed resource schemas using the updated dump function and commit the
resulting changes under schemas/resources. Ensure cache policy, guardrail,
guardrail attachment, observability exporter, and any other struct-shaped
resource roots include additionalProperties: false, and verify the generated
diff contains no remaining schema drift.
MEDIUM-1: the managed control plane writes env_id on every guardrail_attachment document (its own tenancy scoping; the gateway never reads it). Declare the field on the model as known-and-ignored so a same-version managed fleet reports zero partially-compatible rows - a standing false version-skew alarm would train operators to ignore the YELLOW signal. A control-plane source sweep confirmed this is the only field any CP marshal site writes that the DP models do not know. MEDIUM-2: OnEmbeddingFailure is untagged with an object variant; serde buffers untagged content and silently swallows unknown fields inside it, invisible to serde_ignored on read and to the serde step on write. Close the object branch in the producer (both validator sets), same rule as the tagged-enum closures; the dump for the standalone nested schemas applies the same closure through anyOf branches. Published schema diff is this deliberate tightening plus the documented env_id. MEDIUM-3: pin the deliberate closures on the LENIENT set with tests - an exporter-branch smuggled field and a guardrail sub-enum unknown field must stay rejected on the read path, or a refactor moving branch closing into the strict pass would silently open them. MEDIUM-4: cap ignored-field paths at 64 per row with a visible truncation sentinel; one document could otherwise flood logs, the retained map, the status JSON and the heartbeat body. LOW: WARN-dedup lock is poison-tolerant (a panicking subscriber must not wedge snapshot builds); path-normalization aliasing documented; CONTRIBUTING names the two kinds (guardrail, observability_exporter) where any new field still whole-row rejects; e2e drives the YELLOW journey through a second traffic-bearing kind (a model with an unknown field serves chat).
Contributor
Author
Independent audit — outcomeA fresh, zero-context audit agent reviewed this PR per the repository merge gate. Verdict: no HIGH findings; all core claims verified (lenient parse mechanics, strict-write preservation via schema regeneration, separate YELLOW retention, all three reporting surfaces, CP-side verification that the flipped provider_key fields are genuinely unconsumed and that the CP heartbeat handler tolerates the new field). 4 MEDIUM and 4 LOW findings — all fixed in code in 7f3bc88:
Note on the "byte-identical schemas" claim in the description: it held exactly for the leniency flip itself (proving the closing transform reproduces Verification after fixes: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #871 (PR 1 of 2 — the lenient parse + tri-state + full reporting half; PR 2 covers RED last-known-good retention across resync/restart).
Problem
Every resource struct carried
#[serde(deny_unknown_fields)], which also generatedadditionalProperties: falsein the runtime schemas — so a document containing any field this DP version does not know was rejected whole-row at the etcd loader. Under the supported rolling-upgrade order (control plane first, data planes behind), every additive schema change became a breaking one: the admin edits a resource, an older DP rejects the watch event and keeps serving the stale value, and the next resync or restart silently drops the row entirely. For anapi_keythat means the credential stops authenticating, byte-identical to "no such key".Design
Uniform tri-state compatibility, derived mechanically at the existing
validate_and_parsechokepoint — no per-resource rules:rejected[](unchanged surface).serde_ignoredcollects the exact paths (array indices normalized to[],Optionlayers stripped). Logged WARN, deduplicated per (kind, field-set).Strict write / lenient read
Only the etcd loader becomes lenient. Strictness moved out of the structs into a mechanical
close_unknown_fieldspass over the shared schema producers: the write paths (Admin API, standalone file source) compile the closed set and keep returning 400 on unknown fields, while the loader compiles the open set. Both sets build from the same producers, so they cannot drift field-wise.The published
schemas/resources/*.jsonare byte-identical after regeneration — the closing transform provably reproduces whatdeny_unknown_fieldsused to generate, and the CI schema-drift gate doubles as a write-contract-unchanged check. The published files deliberately keep the strict shape: they feed the Admin API reference, which documents the write contract.Deliberate closures stay closed
The hand-injected
additionalProperties: falseon the observability-exporter branches and the guardrail tagged sub-enums survives in both validator sets. Two reasons, verified empirically: those branches guard thecredential_refindirection against plaintext-secret smuggling, and serde silently swallows unknown fields inside inline-tagged enum content (serde_ignoredcannot see through the Content buffering) — an open schema there would be an unreportable, silent tolerance, violating the no-silent-window constraint. Unknown fields in those subtrees therefore stay RED, same stance as unknown enum values.Reporting (landed atomically with the flip — no silent window)
GET /status/configgainspartially_compatible[]next torejected[], aggregated per (kind, field) with row counts. This is theconfig_hashcompanion the issue requires: YELLOW rows are served (so the hash covers them and the state stayssynced), and the explicit list is what distinguishes "fully synced" from "synced with fields this version does not enforce".partially_compatible_resources(omitted when empty; the CP heartbeat handler was verified to tolerate unknown fields, so no paired CP change is needed to ship — surfacing is api7/AISIX-Cloud#1227).aisix_config_partially_compatible_resources{kind}, mirroring the rejected-resources gauge including stale-label zeroing. Field paths stay off the labels (not a bounded set); per-field detail lives in/status/config.The three pinned
*_currently_rejectedpayloadsFlipped to partially-compatible loads, per-case: the Bedrock and Vertex bridges read region/project from the credential JSON inside
api_key, and the Azure bridge derives its resource fromapi_basewith a pinned API version — none of the top-levelaws_region/gcp_project/gcp_region/azure_resource_name/api_versionfields is consumed by this DP, so a dead model field would be strictly worse than an ignored-and-reported one.Enum-addition policy (rider)
CONTRIBUTING.md now documents the rollout rule: added fields are the safe, reported-as-partial change; a new enum value is not forward compatible by design (no old behavior exists for a value the DP cannot interpret) and each addition needs an explicit decision — version-gate at the CP, a
#[serde(other)]fallback only where semantically safe, or an accepted loud rejection.Ecosystem alignment
Surveyed mainstream implementations (primary sources linked from #871): service proxies consuming dynamically-delivered config default to ignoring unknown fields with a deduplicated warning and a counter, keeping strictness opt-in; the major container orchestrator makes readers lenient by several minor versions and puts strictness on the write side only; hybrid-mode API gateways either report a per-DP compatibility status enum or strip unknown fields CP-side per DP version; the dominant serialization framework restored unknown-field preservation as its default specifically for uncoordinated rolling upgrades. The previous behavior here (whole-row reject, no reporting) was stricter than every surveyed system; this PR lands on the common ground — lenient read, loud reporting, write-side strictness — plus the per-DP compatibility status idea for the heartbeat.
Scope notes
unknown_kind(an entirely new resource kind from a newer CP) keeps its existing behavior (skipped + reported), unchanged by this PR.Testing
api_keydocument with an unknown field must be accepted and reported partially compatible.config-forward-compat-e2e.test.ts): the unknown-fieldapi_keyauthenticates a real chat;partially_compatible[]+ gauge report it; an unknown routing strategy stays rejected; deleting the row clears the report and zeroes the gauge; a converged same-version config reports zero YELLOW (typo-catching preserved); the Admin API still 400s on unknown fields.cargo test --workspacegreen; full e2e suite green (175 files, 461 tests);cargo fmt --check,clippy --workspace,dump-schema(zero diff), anddump-openapiall clean.Summary by CodeRabbit