fix(mcp): keep loading api_key rows projected before the layered ACL - #993
Merged
Conversation
A document written under the previous shape carries mcp_access.mode and no allow list. With allow required at the type level that row failed to deserialize, and the loader skips a row it cannot represent — so the key stopped authenticating for every kind of traffic, not just MCP. The runtime loader now defaults a missing allow to empty, which resolves to a layer allowing nothing: fail-closed on MCP while the key keeps working elsewhere. The write path is unchanged — the strict schema adds allow to required on both layers, so neither a resources file nor the admin API can leave it out.
|
Important Review skippedThis review includes 5 billable files. This on-demand review is free during your promotion. Your included review limit has been reached. Run
⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
You can disable this status message by setting the ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ired The loader skips a row it cannot deserialize, and a skipped api_key row stops authenticating every kind of traffic — so requiredness belongs in the strict schema, with a fail-closed serde default on the struct.
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.
Follow-up to #992, which made
allowrequired on every MCP ACL layer.Required at the type level turned out to be too strong for the read path. Documents projected before the layered shape carry
mcp_access: {"mode": "inherit"}and noallow, and nothing re-emits an api_key whose stored shape did not otherwise change. Such a row failed to deserialize, and the loader skips a row it cannot represent — so the key stopped authenticating for every kind of traffic, not just MCP. That is a much worse failure than losing tool access, and it would not have healed on its own.The runtime loader now defaults a missing
allowto empty, so a stale row loads as a layer allowing nothing: fail-closed on MCP while the key keeps serving LLM and A2A traffic. The write path is unchanged — the strict schema addsallowtorequiredon bothMcpPolicyand the key'sMcpAccess, so neither a resources file nor the admin API can leave it out, and the publishedschemas/resources/*.jsonstill document it as required. That is the usual split in this crate: the strict schema forbids, the lenient loader tolerates.The control-plane side re-emits both collections once after its migration (api7/AISIX-Cloud#1335), so the stale documents are replaced rather than merely tolerated. This change is what keeps a key alive in the window before that runs.