refactor: extract publishRoutingAllowlist and filter by model when publishing VK provider allowlist - #4221
Conversation
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughThe PR moves when the governance plugin publishes the routing-allowed provider set from immediately after virtual-key selection to after routing decisions. A new ChangesModel-aware provider allowlist publication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 3/5The routing allowlist filtering logic is an improvement over the old unconditional publish, but the model-allow check used in The plugins/governance/main.go — specifically the Important Files Changed
Reviews (1): Last reviewed commit: "fix: routing governance allowlsit fixes" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/governance/main.go`:
- Around line 613-620: The current loop in main.go treats each ProviderConfig
independently and can include a provider if any config allows the model, but
loadBalanceProvider treats a provider as blocked if any config blacklists the
model. Fix by aggregating per provider: iterate virtualKey.ProviderConfigs and
build per-provider flags (hasAllowed, hasBlacklisted); then for each provider,
if modelStr=="" include it; otherwise exclude the provider if hasBlacklisted is
true; include the provider only if hasAllowed is true and hasBlacklisted is
false (so a single blacklist across any config blocks the provider, matching
loadBalanceProvider behavior). Use the same identifiers:
virtualKey.ProviderConfigs, pc.AllowedModels.IsAllowed,
pc.BlacklistedModels.IsBlocked, and produce the final allowed slice of
schemas.ModelProvider accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cead6a04-41ef-4dad-8dc6-0a702f6860f7
📒 Files selected for processing (1)
plugins/governance/main.go
Merge activity
|
…publishing VK provider allowlist (#4221) ## Summary The routing allowlist published to `BifrostContextKeyRoutingAllowedProviders` previously included all providers on a virtual key regardless of their `allowed_models` / `blocked_models` configuration. This meant a downstream routing layer (load balancing, model-catalog resolution) could select a provider that the VK explicitly forbids for the requested model. This PR fixes that by filtering the allowlist against the model being routed before publishing it. ## Changes - Extracted the allowlist-publishing logic into a dedicated `publishRoutingAllowlist` method that filters each provider config against the resolved model using `AllowedModels.IsAllowed` / `BlacklistedModels.IsBlocked` before adding it to the allowed set. - Removed the previous inline allowlist construction in `PreRequestHook`, which unconditionally included every provider on the VK without any model-level filtering. - `publishRoutingAllowlist` is now called after routing rules have been applied (so the model is in its final post-routing state) for both the large-payload path (using `ParseModelString` on `LargePayloadMetadata.Model`) and the standard path (using `req.GetRequestFields()`). - An empty allowed slice continues to mean "no provider is permitted," preserving the existing fail-closed behaviour enforced by the empty-provider validation in `handleRequest`. - A `nil` virtual key is a no-op, so unauthenticated or VK-less requests are unaffected. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test ```sh go test ./... ``` 1. Configure a virtual key with two providers where one provider has `allowed_models` that excludes the requested model. 2. Send a request for that model and confirm the excluded provider is never selected by the load balancer or model-catalog resolver. 3. Confirm that a request for a model permitted by both providers can still be routed to either. 4. Confirm that a request where no provider permits the model fails closed rather than falling through to a forbidden provider. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This tightens provider selection enforcement on virtual keys. Previously, a model-level restriction on a VK provider config could be bypassed by a downstream routing layer picking that provider after governance failed to select one. That bypass path is now closed. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved consistency between routing rule application and provider allowlist enforcement for virtual keys with model restrictions, ensuring correct provider filtering is applied after routing decisions are made. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…publishing VK provider allowlist (maximhq#4221) ## Summary The routing allowlist published to `BifrostContextKeyRoutingAllowedProviders` previously included all providers on a virtual key regardless of their `allowed_models` / `blocked_models` configuration. This meant a downstream routing layer (load balancing, model-catalog resolution) could select a provider that the VK explicitly forbids for the requested model. This PR fixes that by filtering the allowlist against the model being routed before publishing it. ## Changes - Extracted the allowlist-publishing logic into a dedicated `publishRoutingAllowlist` method that filters each provider config against the resolved model using `AllowedModels.IsAllowed` / `BlacklistedModels.IsBlocked` before adding it to the allowed set. - Removed the previous inline allowlist construction in `PreRequestHook`, which unconditionally included every provider on the VK without any model-level filtering. - `publishRoutingAllowlist` is now called after routing rules have been applied (so the model is in its final post-routing state) for both the large-payload path (using `ParseModelString` on `LargePayloadMetadata.Model`) and the standard path (using `req.GetRequestFields()`). - An empty allowed slice continues to mean "no provider is permitted," preserving the existing fail-closed behaviour enforced by the empty-provider validation in `handleRequest`. - A `nil` virtual key is a no-op, so unauthenticated or VK-less requests are unaffected. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test ```sh go test ./... ``` 1. Configure a virtual key with two providers where one provider has `allowed_models` that excludes the requested model. 2. Send a request for that model and confirm the excluded provider is never selected by the load balancer or model-catalog resolver. 3. Confirm that a request for a model permitted by both providers can still be routed to either. 4. Confirm that a request where no provider permits the model fails closed rather than falling through to a forbidden provider. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This tightens provider selection enforcement on virtual keys. Previously, a model-level restriction on a VK provider config could be bypassed by a downstream routing layer picking that provider after governance failed to select one. That bypass path is now closed. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved consistency between routing rule application and provider allowlist enforcement for virtual keys with model restrictions, ensuring correct provider filtering is applied after routing decisions are made. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…publishing VK provider allowlist (maximhq#4221) ## Summary The routing allowlist published to `BifrostContextKeyRoutingAllowedProviders` previously included all providers on a virtual key regardless of their `allowed_models` / `blocked_models` configuration. This meant a downstream routing layer (load balancing, model-catalog resolution) could select a provider that the VK explicitly forbids for the requested model. This PR fixes that by filtering the allowlist against the model being routed before publishing it. ## Changes - Extracted the allowlist-publishing logic into a dedicated `publishRoutingAllowlist` method that filters each provider config against the resolved model using `AllowedModels.IsAllowed` / `BlacklistedModels.IsBlocked` before adding it to the allowed set. - Removed the previous inline allowlist construction in `PreRequestHook`, which unconditionally included every provider on the VK without any model-level filtering. - `publishRoutingAllowlist` is now called after routing rules have been applied (so the model is in its final post-routing state) for both the large-payload path (using `ParseModelString` on `LargePayloadMetadata.Model`) and the standard path (using `req.GetRequestFields()`). - An empty allowed slice continues to mean "no provider is permitted," preserving the existing fail-closed behaviour enforced by the empty-provider validation in `handleRequest`. - A `nil` virtual key is a no-op, so unauthenticated or VK-less requests are unaffected. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test ```sh go test ./... ``` 1. Configure a virtual key with two providers where one provider has `allowed_models` that excludes the requested model. 2. Send a request for that model and confirm the excluded provider is never selected by the load balancer or model-catalog resolver. 3. Confirm that a request for a model permitted by both providers can still be routed to either. 4. Confirm that a request where no provider permits the model fails closed rather than falling through to a forbidden provider. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This tightens provider selection enforcement on virtual keys. Previously, a model-level restriction on a VK provider config could be bypassed by a downstream routing layer picking that provider after governance failed to select one. That bypass path is now closed. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved consistency between routing rule application and provider allowlist enforcement for virtual keys with model restrictions, ensuring correct provider filtering is applied after routing decisions are made. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
The routing allowlist published to
BifrostContextKeyRoutingAllowedProviderspreviously included all providers on a virtual key regardless of theirallowed_models/blocked_modelsconfiguration. This meant a downstream routing layer (load balancing, model-catalog resolution) could select a provider that the VK explicitly forbids for the requested model. This PR fixes that by filtering the allowlist against the model being routed before publishing it.Changes
publishRoutingAllowlistmethod that filters each provider config against the resolved model usingAllowedModels.IsAllowed/BlacklistedModels.IsBlockedbefore adding it to the allowed set.PreRequestHook, which unconditionally included every provider on the VK without any model-level filtering.publishRoutingAllowlistis now called after routing rules have been applied (so the model is in its final post-routing state) for both the large-payload path (usingParseModelStringonLargePayloadMetadata.Model) and the standard path (usingreq.GetRequestFields()).handleRequest.nilvirtual key is a no-op, so unauthenticated or VK-less requests are unaffected.Type of change
Affected areas
How to test
go test ./...allowed_modelsthat excludes the requested model.Breaking changes
Related issues
Security considerations
This tightens provider selection enforcement on virtual keys. Previously, a model-level restriction on a VK provider config could be bypassed by a downstream routing layer picking that provider after governance failed to select one. That bypass path is now closed.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit