Conversation
Summary by CodeRabbit
WalkthroughThis update refactors provider key and configuration handling for Azure and Vertex AI by moving provider-specific metadata from global meta config objects to per-key configuration structs within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Server
participant Provider
User->>UI: Submit provider config with keys (Azure/Vertex)
UI->>Server: POST /providers (keys include AzureKeyConfig/VertexKeyConfig)
Server->>Server: Merge/validate keys, store per-key configs
Server->>Provider: Call method with schemas.Key (per-key config)
Provider->>Provider: Use AzureKeyConfig/VertexKeyConfig from key
Provider-->>Server: Response
Server-->>UI: Success/failure
UI-->>User: Show result
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (4)
core/providers/azure.go (2)
199-209: Fix typo in error message.- Message: fmt.Sprintf("deployment if not found for model %s", model), + Message: fmt.Sprintf("deployment is not found for model %s", model),
531-540: Fix typo in error message (same as earlier).- Message: fmt.Sprintf("deployment not found for model %s", model), + Message: fmt.Sprintf("deployment is not found for model %s", model),Note: Consider extracting the deployment lookup logic to avoid duplication between
completeRequestandChatCompletionStream.core/providers/vertex.go (2)
188-196: Minor inconsistency in error message.- Message: "region is not set in meta config", + Message: "region is not set",The error message references "meta config" but meta configs have been removed in favor of key-level configs.
407-414: Same inconsistency in error message.- Message: "region is not set in meta config", + Message: "region is not set",
♻️ Duplicate comments (3)
transports/bifrost-http/ui/plugins/index.txt (1)
19-20: Same hashed‐CSS concern as main index pageThe plugins page now relies on the same
e2d791eafd0cc299.css. Please ensure the file is present and shipped
with the release artefacts (see earlier verification script).transports/bifrost-http/ui/404.html (1)
1-1: Verify 404 page picks up new stylesheetThe 404 page also switched to
e2d791eafd0cc299.css. A missing file here will render the error page
without styles, which is user-visible.transports/bifrost-http/ui/mcp-clients/index.txt (1)
19-20: CSS fingerprint same remarkAgain, just make sure the CSS file is committed and deployed.
Pratham-Mishra04
force-pushed
the
07-18-feat_azure_and_vertex_config_to_keys_and_optimised_key_management_in_store
branch
from
a86dfba to
03c452f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
core/providers/azure.go (2)
188-223: Fix typo in error message.There's a typo in the error message that should be corrected.
- Message: fmt.Sprintf("deployment if not found for model %s", model), + Message: fmt.Sprintf("deployment not found for model %s", model),
499-555: Fix typo and consider refactoring duplicate validation logic.The same typo exists here as in the
completeRequestmethod. Additionally, there's significant duplication of validation logic betweencompleteRequestandChatCompletionStream.- Message: fmt.Sprintf("deployment not found for model %s", model), + Message: fmt.Sprintf("deployment not found for model %s", model),Consider extracting the Azure configuration validation into a helper method to reduce duplication:
func (provider *AzureProvider) validateAzureConfig(key schemas.Key, model string) (endpoint string, deployment string, apiVersion string, err *schemas.BifrostError) { if key.AzureKeyConfig == nil { return "", "", "", &schemas.BifrostError{ IsBifrostError: false, Error: schemas.ErrorField{ Message: "azure key config not set", }, } } if key.AzureKeyConfig.Endpoint == "" { return "", "", "", &schemas.BifrostError{ IsBifrostError: false, Error: schemas.ErrorField{ Message: "endpoint not set", }, } } if key.AzureKeyConfig.Deployments == nil { return "", "", "", &schemas.BifrostError{ IsBifrostError: false, Error: schemas.ErrorField{ Message: "deployments not set", }, } } deployment = key.AzureKeyConfig.Deployments[model] if deployment == "" { return "", "", "", &schemas.BifrostError{ IsBifrostError: false, Error: schemas.ErrorField{ Message: fmt.Sprintf("deployment not found for model %s", model), }, } } apiVersionPtr := key.AzureKeyConfig.APIVersion if apiVersionPtr == nil { apiVersionPtr = StrPtr("2024-02-01") } return key.AzureKeyConfig.Endpoint, deployment, *apiVersionPtr, nil }core/providers/vertex.go (2)
188-196: Fix error message to reflect key config instead of meta config.The error message references "meta config" but should reference the key configuration since meta configs have been removed.
- Message: "region is not set in meta config", + Message: "region is not set in key config",
406-414: Fix error message consistency in streaming method.Same issue as in ChatCompletion - the error message should reference key config instead of meta config.
- Message: "region is not set in meta config", + Message: "region is not set in key config",
♻️ Duplicate comments (7)
transports/bifrost-http/lib/config.go (1)
21-21: Avoid over-specifying the identifier typeThe comment still states “UUIDs”, but
schemas.Key.IDis only required to be unique, not specifically a UUID. The earlier review raised this; please update the wording.- Keys []schemas.Key `json:"keys"` // API keys for the provider with UUIDs + Keys []schemas.Key `json:"keys"` // API keys for the provider; each key has its own unique IDtransports/bifrost-http/ui/index.html (1)
1-1: (Optional) add SRI for static assetsAll fingerprinted assets (CSS/JS) are still shipped without
integrityattributes, leaving clients unprotected against CDN tampering. Same comment as before.docs/usage/http-transport/configuration/providers.md (1)
190-203: Sameregionvslocationmismatch for Vertex blockSee previous comment – update to whichever name the schema finally settles on.
docs/usage/http-transport/openapi.json (1)
2337-2381: Acknowledge the previous review comment about extracting inline schemas.The previous review comment about promoting
azure_key_configandvertex_key_configto reusable component schemas is still valid and applicable to these changes. Extracting these to top-level component schemas would improve maintainability and provide better type safety.docs/usage/providers.md (1)
371-386: Fix field name inconsistency in Vertex configuration.The Vertex configuration example uses
Locationbut should useRegionto match the actual schema field name.VertexKeyConfig: &schemas.VertexKeyConfig{ ProjectID: "your-project-id", - Location: "us-central1", + Region: "us-central1", AuthCredentials: os.Getenv("VERTEX_AUTH_CREDENTIALS"), // Or read from file },transports/bifrost-http/handlers/providers.go (1)
407-493: Add validation for required fields in Azure/Vertex configurations.After merging keys, validate that required fields (e.g., Azure endpoint, Vertex project ID) are not empty to prevent runtime errors.
core/providers/vertex.go (1)
31-52: Consider implementing pool size limits or TTL-based eviction.The client pooling implementation is well-designed with SHA-256 cache keys and error-based eviction. However, as previously noted, consider adding pool size limits or TTL-based eviction to prevent unbounded memory growth in long-running services.
Pratham-Mishra04
force-pushed
the
07-18-feat_azure_and_vertex_config_to_keys_and_optimised_key_management_in_store
branch
from
03c452f to
0f8d855
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
transports/bifrost-http/handlers/completions.go (1)
92-92: Consider usingstrings.SplitNfor consistency.The fallback parsing still uses
strings.Splitwhich could face the same issue as the main model parsing if fallback model names contain forward slashes.- fallbackModel := strings.Split(fallback, "/") + fallbackModel := strings.SplitN(fallback, "/", 2)docs/usage/http-transport/openapi.json (1)
2320-2337: Consider markingidasformat: uuid(optional).If the new
Key.IDis meant to be a UUID, adding"format": "uuid", "example": "8e7b91b9-765d-4ef5-a1f5-8d3b0d5b6b4c"helps client validation and docs.
♻️ Duplicate comments (11)
transports/bifrost-http/lib/config.go (1)
21-21: Comment still over-specifies “UUIDs”
Theschemas.Key.IDfield is only documented as a unique identifier; callers are free to use any scheme. Replace “UUIDs” with “unique IDs” to avoid misleading integrators.- Keys []schemas.Key `json:"keys"` // API keys for the provider with UUIDs + Keys []schemas.Key `json:"keys"` // API keys for the provider; each key carries its own unique IDtransports/bifrost-http/ui/index.html (1)
1-1: Still missing SRI integrity attribute on stylesheet linkSame nitpick as previously raised – please consider adding an
integrityattribute for stronger supply-chain safety.transports/bifrost-http/ui/404.html (1)
1-1: Repeat: add SRI on external stylesheetThe stylesheet reference continues to ship without an
integrityhash; see earlier feedback.transports/config.example.json (1)
140-150: Missing requiredvaluefield in Vertex key configuration.The Vertex key configuration is missing the mandatory
valuefield that theschemas.Keystructure requires for identification and hashing purposes.Please add the
valuefield to the Vertex key configuration:{ + "value": "", "models": ["gemini-2.0-flash-001"], "weight": 1.0, "vertex_key_config": { "project_id": "env.VERTEX_PROJECT_ID", "region": "us-central1", "auth_credentials": "env.VERTEX_CREDENTIALS" } }docs/usage/http-transport/integrations/genai-compatible.md (2)
547-556: Sameregionvslocationmismatch as previously flagged
See earlier comment – update to"location"for consistency with the actual config schema.
618-626: Ditto – please renameregiontolocationhere as well
Keeping both terms in docs will confuse users.transports/bifrost-http/ui/_next/static/css/e2d791eafd0cc299.css (1)
1-1: Non-standard “in oklab” gradient still lacks a fallback
The.bg-gradient-to-rdeclaration keepsbackground-image: linear-gradient(var(--tw-gradient-stops))with--tw-gradient-position: to right in oklab;. Browsers that haven’t shippedin oklab(e.g. Firefox ≤ 127, Safari ≤ 17) will drop the whole rule. Please prepend a standard direction fallback before the custom-property version in your source CSS/Tailwind pipeline:.bg-gradient-to-r{ + /* Fallback for browsers without “in oklab” support */ + background-image: linear-gradient(to right, var(--tw-gradient-stops)); --tw-gradient-position: to right in oklab; background-image: linear-gradient(var(--tw-gradient-stops)); }That ensures graceful degradation while keeping the advanced color-space for supporting engines.
docs/usage/http-transport/openapi.json (1)
2337-2381: Inline provider-specific configs duplicate schema & lackadditionalProperties: false
azure_key_configandvertex_key_configare defined inline again, repeating ~40 lines and allowing silent typos. A top-level component schema keeps theKeyobject concise and gives generators reusable types.- "azure_key_config": { - "type": "object", - "properties": { - ... - }, - "description": "Azure key configuration" - }, + "azure_key_config": { "$ref": "#/components/schemas/AzureKeyConfig" }, - "vertex_key_config": { - "type": "object", - "properties": { - ... - }, - "description": "Vertex key configuration" - } + "vertex_key_config": { "$ref": "#/components/schemas/VertexKeyConfig" }Add corresponding component schemas alongside others:
"AzureKeyConfig": { "type": "object", "additionalProperties": false, "properties": { "endpoint": { "type": "string" }, "deployments": { "type": "object", "additionalProperties": { "type": "string" } }, "api_version": { "type": "string" } } }, "VertexKeyConfig": { "type": "object", "additionalProperties": false, "properties": { "project_id": { "type": "string" }, "region": { "type": "string" }, "auth_credentials": { "type": "string" } } }This mirrors the UI/Go structs, eliminates duplication, and prevents stray fields.
docs/usage/providers.md (1)
371-405: LGTM: Vertex configuration updated correctly and past issues addressed.The Vertex configuration has been properly updated to use key-level configuration with
VertexKeyConfig. The structure correctly embeds Vertex-specific settings (project_id, region, auth_credentials) within each key. The previous field name inconsistency betweenLocationandRegionhas been resolved.transports/bifrost-http/handlers/providers.go (1)
407-493: Add nil checks to prevent potential panics in nested config handling.The code assumes
oldRedactedKeys[i]has the same structure asupdateKey, but if the old key doesn't haveAzureKeyConfigorVertexKeyConfigwhile the update does, accessing fields will cause a nil pointer dereference.Apply this diff to add nil checks:
// Handle Azure config redacted values if updateKey.AzureKeyConfig != nil && oldRedactedKeys[i].AzureKeyConfig != nil { if lib.IsRedacted(updateKey.AzureKeyConfig.Endpoint) && (!strings.HasPrefix(updateKey.AzureKeyConfig.Endpoint, "env.") || !strings.EqualFold(updateKey.AzureKeyConfig.Endpoint, oldRedactedKeys[i].AzureKeyConfig.Endpoint)) { mergedKey.AzureKeyConfig.Endpoint = oldRawKey.AzureKeyConfig.Endpoint } if updateKey.AzureKeyConfig.APIVersion != nil { + if oldRedactedKeys[i].AzureKeyConfig.APIVersion != nil { if lib.IsRedacted(*updateKey.AzureKeyConfig.APIVersion) && (!strings.HasPrefix(*updateKey.AzureKeyConfig.APIVersion, "env.") || !strings.EqualFold(*updateKey.AzureKeyConfig.APIVersion, *oldRedactedKeys[i].AzureKeyConfig.APIVersion)) { mergedKey.AzureKeyConfig.APIVersion = oldRawKey.AzureKeyConfig.APIVersion } + } } } // Handle Vertex config redacted values if updateKey.VertexKeyConfig != nil && oldRedactedKeys[i].VertexKeyConfig != nil { if lib.IsRedacted(updateKey.VertexKeyConfig.ProjectID) && (!strings.HasPrefix(updateKey.VertexKeyConfig.ProjectID, "env.") || !strings.EqualFold(updateKey.VertexKeyConfig.ProjectID, oldRedactedKeys[i].VertexKeyConfig.ProjectID)) { mergedKey.VertexKeyConfig.ProjectID = oldRawKey.VertexKeyConfig.ProjectID } if lib.IsRedacted(updateKey.VertexKeyConfig.Region) && (!strings.HasPrefix(updateKey.VertexKeyConfig.Region, "env.") || !strings.EqualFold(updateKey.VertexKeyConfig.Region, oldRedactedKeys[i].VertexKeyConfig.Region)) { mergedKey.VertexKeyConfig.Region = oldRawKey.VertexKeyConfig.Region } if lib.IsRedacted(updateKey.VertexKeyConfig.AuthCredentials) && (!strings.HasPrefix(updateKey.VertexKeyConfig.AuthCredentials, "env.") || !strings.EqualFold(updateKey.VertexKeyConfig.AuthCredentials, oldRedactedKeys[i].VertexKeyConfig.AuthCredentials)) { mergedKey.VertexKeyConfig.AuthCredentials = oldRawKey.VertexKeyConfig.AuthCredentials } }core/providers/vertex.go (1)
31-52: Client pool implementation looks good.The use of SHA-256 for secure cache keys and sync.Map for thread-safe storage is well-designed. The eviction strategy on auth/network errors helps maintain pool health by preventing reuse of invalid clients.
akshaydeo
deleted the
07-18-feat_azure_and_vertex_config_to_keys_and_optimised_key_management_in_store
branch
2 participants
No description provided.