Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,44 @@ still build and pass its structural checks
implementation details.
4. Make sure the checks above pass locally before pushing.

### Changing resource models: unknown fields vs. new enum values

The gateway reads its resources leniently from etcd and strictly on write
(issue #871). That split makes two kinds of schema change behave very
differently on a data plane that has not been upgraded yet, and each needs a
different discipline:

- **Adding a field** is the safe, expected change. An older gateway loads the
document with the new field ignored and reports it as partially compatible
(`GET /status/config` `partially_compatible[]`, the heartbeat, and the
`aisix_config_partially_compatible_resources` metric). Never assume a new
Comment thread
coderabbitai[bot] marked this conversation as resolved.
field is enforced fleet-wide until every data plane runs a version that
knows it — this matters most for restriction-type fields (an old gateway
keeps allowing what the new field would forbid). Two kinds are exempt from
this tolerance: `guardrail` and `observability_exporter` documents flatten
their fields into closed tagged shapes, so ANY new field there still
whole-row rejects on older gateways — treat additions to those two like
enum values below.
- **Adding an enum value** (a routing strategy, an adapter, a guardrail
`kind`, …) is NOT forward compatible, by design: a value the gateway cannot
interpret has no old behavior to fall back to, so the whole document stays
rejected on older versions. Do not "fix" that by opening the enum. Every
new enum value needs an explicit rollout decision, made in the PR that adds
it:
1. **Version-gate at the control plane** (preferred for values that change
serving behavior): the control plane only offers the value once the
environment's data planes are on a version that knows it, using the
version the heartbeat already reports.
2. **Ship a degradable fallback** via `#[serde(other)]` — only when a
fallback is semantically safe (e.g. an advisory label where "unknown"
is a reasonable interpretation). Never for values that select serving
behavior: silently running a different routing strategy than configured
is worse than rejecting the document.
3. **Accept the rejection** for values that are new capabilities: an old
gateway that cannot serve the capability rejecting the row loudly (the
rejection reaches `rejected[]` and the heartbeat) can be the correct
outcome — state which of the three you chose and why in the PR.

### Commit and PR style

Commit subjects follow Conventional Commits, matching the existing history:
Expand Down
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ webpki-roots = "0.26"
# Serialization / validation
serde = { version = "1", features = ["derive"] }
serde_json = "1"
# Collects the paths of fields serde ignored during deserialization —
# how the etcd loader reports unknown fields from a newer control plane
# as "partially compatible" instead of dropping them silently (#871).
serde_ignored = "0.1"
jsonschema = "0.28"
schemars = "0.8"

Expand Down
16 changes: 16 additions & 0 deletions crates/aisix-admin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,8 @@ mod tests {
resource_counts: Default::default(),
}),
rejected: vec![],
partially_compatible: Vec::new(),
partially_compatible_rows_by_kind: Default::default(),
is_reload: true,
wholly_rejected: false,
});
Expand Down Expand Up @@ -836,6 +838,12 @@ mod tests {
last_error: "schema validation failed at `/display_name`".into(),
seen_at: chrono::Utc::now(),
}],
partially_compatible: vec![aisix_core::config_status::PartialCompatResource {
resource_kind: "api_keys".into(),
field: "quota_profile".into(),
count: 2,
}],
partially_compatible_rows_by_kind: [("api_keys".to_string(), 2)].into_iter().collect(),
is_reload: true,
wholly_rejected: false,
});
Expand Down Expand Up @@ -866,6 +874,12 @@ mod tests {
assert_eq!(v["applied"]["resource_counts"]["models"], 1);
assert_eq!(v["rejected"][0]["resource_kind"], "models");
assert_eq!(v["rejected"][0]["last_error_kind"], "schema_failed");
// The partially-compatible companion list (#871) rides next to
// rejected[] so a matching config_hash can't hide that some
// served rows carry fields this DP does not enforce.
assert_eq!(v["partially_compatible"][0]["resource_kind"], "api_keys");
assert_eq!(v["partially_compatible"][0]["field"], "quota_profile");
assert_eq!(v["partially_compatible"][0]["count"], 2);
}

#[tokio::test]
Expand All @@ -883,6 +897,8 @@ mod tests {
resource_counts: [("models".to_string(), 2)].into_iter().collect(),
}),
rejected: vec![],
partially_compatible: Vec::new(),
partially_compatible_rows_by_kind: Default::default(),
is_reload: true,
wholly_rejected: false,
});
Expand Down
91 changes: 61 additions & 30 deletions crates/aisix-core/src/bin/dump-schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,38 +38,35 @@ fn main() {
fs::create_dir_all(&out_dir).expect("create schemas/resources dir");

// Every resource with a runtime validator goes through the SAME
// `*_root_schema()` producer the validator uses, so the published schema ==
// the enforced schema by construction. `ensemble`/`rate_limit`/`routing`
// have no standalone validator (they are nested struct types) so they dump
// straight from the struct via `schema_for!`.
dump_value(&out_dir, "api_key", schema::apikey_root_schema());
dump_value(&out_dir, "cache_policy", schema::cache_policy_root_schema());
dump_value(&out_dir, "model", schema::model_root_schema());
dump_value(
&out_dir,
// `resource_root_schema(name, strict: true)` producer the write-path
// validators compile, so the published schema == the enforced write
// contract by construction. The published files deliberately carry the
// STRICT shape: they document the Admin API write contract (unknown
// fields are a 400) and the etcd loader's lenient read tolerance is a
// runtime behavior, not a contract callers may write against.
// `ensemble`/`rate_limit`/`routing` have no standalone validator (they
// are nested struct types) so they dump straight from the struct via
// `schema_for!`, closed the same way.
for resource in [
"api_key",
"cache_policy",
"model",
"rate_limit_policy",
schema::rate_limit_policy_root_schema(),
);
dump_value(&out_dir, "provider_key", schema::provider_key_root_schema());
dump_value(
&out_dir,
"provider_key",
"observability_exporter",
schema::observability_exporter_root_schema(),
);
dump_value(&out_dir, "guardrail", schema::guardrail_root_schema());
dump_value(
&out_dir,
"guardrail",
"guardrail_attachment",
schema::guardrail_attachment_root_schema(),
);
dump_value(&out_dir, "mcp_server", schema::mcp_server_root_schema());
dump_value(&out_dir, "mcp_policy", schema::mcp_policy_root_schema());
dump_value(&out_dir, "a2a_agent", schema::a2a_agent_root_schema());
dump_value(
&out_dir,
"mcp_server",
"mcp_policy",
"a2a_agent",
"oidc_provider",
schema::oidc_provider_root_schema(),
);
] {
dump_value(
&out_dir,
resource,
schema::resource_root_schema(resource, true),
);
}

dump::<EnsembleConfig>(&out_dir, "ensemble");
dump::<RateLimit>(&out_dir, "rate_limit");
Expand All @@ -81,14 +78,48 @@ fn main() {
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. Recurses
/// into `anyOf` branches so an untagged enum's object variant closes too —
/// serde silently swallows unknown fields inside untagged content, so the
/// schema closure is the only non-silent guard there (the resource
/// producers apply the same rule, e.g. `OnEmbeddingFailure` in `model`).
fn close_object_schema(schema: &mut schemars::schema::SchemaObject) {
if let Some(sub) = schema.subschemas.as_deref_mut() {
if let Some(any_of) = sub.any_of.as_mut() {
for branch in any_of.iter_mut() {
if let schemars::schema::Schema::Object(b) = branch {
close_object_schema(b);
}
}
}
}
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)));
}
}
Comment on lines 78 to +121

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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"
done

Repository: 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"
done

Repository: 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 -200

Repository: 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.


/// Write a pre-assembled schema `Value`. Used for resources whose canonical
/// schema is built by a dedicated producer rather than a bare `schema_for!`
/// (e.g. `model`, which injects the cross-field `oneOf`).
Expand Down
Loading
Loading