fix(routing): apply each target's client-IP allowlist in group dispatch - #786
Merged
Conversation
A model's `allowed_cidrs` was only checked on the alias the caller named. Reaching that model as a Model Group target skipped it entirely, so adding an IP-restricted model to a group silently published it to every caller — a direct request got 403 while the same model behind a group served 200. Filter the candidate set by the caller's source IP in `resolve_attempt_models`, before the strategy picks a target: an out-of-range target is not a candidate, the group serves from whatever remains, and a group whose every target excludes the caller returns the same 403 the direct path returns. Running before selection means `max_fallbacks` budgets attempts across reachable targets and a metric-based strategy ranks only those. Deliberately not folded into `filter_attempt_models`: that filter's `when_all_unavailable: try_anyway` policy hands back the unfiltered candidate list, which would send the request to a target the operator just declared off-limits. An allowlist has no "try anyway". The group's own `allowed_cidrs` stays enforced pre-dispatch — the two tiers are additive — and the rejection keeps the generic #557 envelope, so a group can't be used to probe which members exist or what ranges they allow. Behavior change: a group with an IP-restricted member now excludes that member for out-of-range callers instead of dispatching to it. Fixes api7/AISIX-Cloud#1089
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughModel-group routing now receives the client source IP, filters members against their ChangesModel group IP allowlist enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant messages_dispatch
participant resolve_attempt_models
participant targets_allowed_for_ip
participant UpstreamModel
Client->>messages_dispatch: request with source_ip
messages_dispatch->>resolve_attempt_models: RoutingRequest
resolve_attempt_models->>targets_allowed_for_ip: group targets and source_ip
targets_allowed_for_ip-->>resolve_attempt_models: allowed targets
resolve_attempt_models->>UpstreamModel: selected target request
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
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.
Problem
A model's
allowed_cidrswas only checked on the alias the caller named (dispatch::check_ip_access, pre-dispatch). Reaching that model as a Model Group target skipped the check entirely — so a direct request tom-internalcorrectly returned403, while the same model behind a group served200to any caller. Adding an IP-restricted model to a group silently published it.Same class as api7/AISIX-Cloud#1087 (member rate limits ignored in group dispatch, fixed in #783), on the other per-model gate.
Fix
Filter the candidate set by the caller's source IP in
resolve_attempt_models, before the strategy picks:403 ip_restrictedthe direct path returns;max_fallbacksbudgets attempts across the targets this caller can actually reach, and a metric-based strategy ranks only those.RoutingRequestgainssource_ip; it defaults to"", whichModel::ip_allowedtreats as out-of-range, so a call site that forgets to thread it fails closed rather than silently disabling the allowlist.Deliberately not folded into
filter_attempt_models: that filter'swhen_all_unavailable: try_anywaypolicy hands back the unfiltered candidate list, which would send the request to a target the operator just declared off-limits. An allowlist has no "try anyway".The group's own
allowed_cidrsstays enforced pre-dispatch — the two tiers are additive. The rejection keeps the generic #557 envelope (no model name, no CIDR), so a group can't be used to probe which members exist or what ranges they allow.Behavior change
A group with an IP-restricted member now excludes that member for out-of-range callers, instead of dispatching to it. A group whose members are all restricted returns
403where it previously returned200. This is the point of the fix, but it is a live-traffic change for anyone who had (knowingly or not) been relying on the bypass. Theapi7/docsline stating thatallowed_cidrs"applies to the model alias the caller names" is corrected in the paired docs PR.Tests
Extends
model-ip-restriction-e2e.test.ts(both new cases fail pre-fix, pass post-fix):[restricted, open]group is served by the open member — asserted on a marker string from the open member's own upstream, because both members answer200and an earlier request-count assertion passed pre-fix too;403, upstream untouched, and the message leaks neither member name nor CIDR.Plus five
routing::unit tests for the filter itself: drops only the out-of-range target, empty when all excluded, fails closed on an unattributable IP, leaves unresolvable names for the existing config-error path, and is a no-op when nothing restricts.Fixes api7/AISIX-Cloud#1089
Summary by CodeRabbit
New Features
Bug Fixes