refactor: move Bedrock config from meta to key level#218
Conversation
Summary by CodeRabbit
WalkthroughThis change removes all support for provider-level "meta config" and migrates AWS Bedrock provider configuration to a per-key Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant HTTP_API
participant Store
participant BedrockProvider
User->>UI: Enter Bedrock key config fields (access_key, secret_key, etc.)
UI->>HTTP_API: Submit provider config with bedrock_key_config
HTTP_API->>Store: Store provider and key config (no meta config)
Store-->>HTTP_API: Confirmation
HTTP_API-->>UI: Success
User->>UI: Make Bedrock model request
UI->>HTTP_API: Send request with selected key (includes bedrock_key_config)
HTTP_API->>BedrockProvider: Call Bedrock with key config fields
BedrockProvider-->>HTTP_API: Return result
HTTP_API-->>UI: Show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (6)
transports/bifrost-http/lib/models.go (2)
169-188: Missing Bedrock deployments serialization in BeforeSave hookThe
BeforeSavehook handles Azure deployments serialization but is missing the logic to serialize Bedrock deployments. This will cause Bedrock deployment mappings to be lost when saving to the database.Add the following serialization logic after the Azure config handling:
func (k *DBKey) BeforeSave(tx *gorm.DB) error { if k.Models != nil { data, err := json.Marshal(k.Models) if err != nil { return err } k.ModelsJSON = string(data) } if k.AzureKeyConfig != nil && k.AzureKeyConfig.Deployments != nil { data, err := json.Marshal(k.AzureKeyConfig.Deployments) if err != nil { return err } deployments := string(data) k.AzureDeploymentsJSON = &deployments } + if k.BedrockKeyConfig != nil && k.BedrockKeyConfig.Deployments != nil { + data, err := json.Marshal(k.BedrockKeyConfig.Deployments) + if err != nil { + return err + } + deployments := string(data) + k.BedrockDeploymentsJSON = &deployments + } + return nil }
264-306: Missing Bedrock config reconstruction in AfterFind hookThe
AfterFindhook reconstructs Azure and Vertex configs but is missing the logic to reconstruct Bedrock config from database fields. This will cause Bedrock configuration to be unavailable after loading from the database.Add the following deserialization logic after the Vertex config handling:
// Reconstruct Vertex config if fields are present if k.VertexProjectID != nil { config := &schemas.VertexKeyConfig{ ProjectID: *k.VertexProjectID, } if k.VertexRegion != nil { config.Region = *k.VertexRegion } if k.VertexAuthCredentials != nil { config.AuthCredentials = *k.VertexAuthCredentials } k.VertexKeyConfig = config } + // Reconstruct Bedrock config if fields are present + if k.BedrockAccessKey != nil { + config := &schemas.BedrockKeyConfig{ + AccessKey: *k.BedrockAccessKey, + } + + if k.BedrockSecretKey != nil { + config.SecretKey = *k.BedrockSecretKey + } + if k.BedrockSessionToken != nil { + config.SessionToken = k.BedrockSessionToken + } + if k.BedrockRegion != nil { + config.Region = k.BedrockRegion + } + if k.BedrockARN != nil { + config.ARN = k.BedrockARN + } + if k.BedrockDeploymentsJSON != nil { + var deployments map[string]string + if err := json.Unmarshal([]byte(*k.BedrockDeploymentsJSON), &deployments); err != nil { + return err + } + config.Deployments = deployments + } + + k.BedrockKeyConfig = config + } + return nil }transports/bifrost-http/lib/store.go (4)
834-940: Missing redaction logic for BedrockKeyConfig in GetProviderConfigRedactedThe
GetProviderConfigRedactedfunction currently handles redaction for Azure and Vertex key configs but is missing redaction logic forBedrockKeyConfig. This could expose sensitive AWS credentials (AccessKey, SecretKey, SessionToken) in API responses.Add redaction logic for BedrockKeyConfig after the Vertex config handling (after line 936):
redactedConfig.Keys[i].VertexKeyConfig = vertexConfig } + + // Redact Bedrock key config if present + if key.BedrockKeyConfig != nil { + bedrockConfig := &schemas.BedrockKeyConfig{ + Deployments: key.BedrockKeyConfig.Deployments, + } + + // Redact AccessKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.access_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.AccessKey = "env." + envVar + } else { + bedrockConfig.AccessKey = RedactKey(key.BedrockKeyConfig.AccessKey) + } + + // Redact SecretKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.secret_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.SecretKey = "env." + envVar + } else { + bedrockConfig.SecretKey = RedactKey(key.BedrockKeyConfig.SecretKey) + } + + // Redact SessionToken if present + if key.BedrockKeyConfig.SessionToken != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.session_token", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + sessionToken := "env." + envVar + bedrockConfig.SessionToken = &sessionToken + } else { + redactedToken := RedactKey(*key.BedrockKeyConfig.SessionToken) + bedrockConfig.SessionToken = &redactedToken + } + } + + // Region is not sensitive, handle env vars only + if key.BedrockKeyConfig.Region != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.region", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + region := "env." + envVar + bedrockConfig.Region = ®ion + } else { + bedrockConfig.Region = key.BedrockKeyConfig.Region + } + } + + // Redact ARN if present + if key.BedrockKeyConfig.ARN != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.arn", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + arn := "env." + envVar + bedrockConfig.ARN = &arn + } else { + redactedARN := RedactKey(*key.BedrockKeyConfig.ARN) + bedrockConfig.ARN = &redactedARN + } + } + + redactedConfig.Keys[i].BedrockKeyConfig = bedrockConfig + } }
441-574: Missing restoration logic for BedrockKeyConfig in writeConfigToFileThe
writeConfigToFilefunction currently restores environment variable references for Azure and Vertex key configs but is missing restoration logic forBedrockKeyConfig. This breaks round-trip fidelity when saving configuration back to file, potentially exposing AWS credentials as plain text instead of environment variable references.Add restoration logic for BedrockKeyConfig after the Vertex config handling (after line 542):
redactedKeys[i].VertexKeyConfig = vertexConfig } + + // Restore Bedrock key config if present + if key.BedrockKeyConfig != nil { + bedrockConfig := &schemas.BedrockKeyConfig{ + Deployments: key.BedrockKeyConfig.Deployments, + } + + // Restore AccessKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.access_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.AccessKey = "env." + envVar + } else { + bedrockConfig.AccessKey = key.BedrockKeyConfig.AccessKey + } + + // Restore SecretKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.secret_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.SecretKey = "env." + envVar + } else { + bedrockConfig.SecretKey = key.BedrockKeyConfig.SecretKey + } + + // Restore SessionToken if present + if key.BedrockKeyConfig.SessionToken != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.session_token", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + sessionToken := "env." + envVar + bedrockConfig.SessionToken = &sessionToken + } else { + bedrockConfig.SessionToken = key.BedrockKeyConfig.SessionToken + } + } + + // Restore Region if present + if key.BedrockKeyConfig.Region != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.region", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + region := "env." + envVar + bedrockConfig.Region = ®ion + } else { + bedrockConfig.Region = key.BedrockKeyConfig.Region + } + } + + // Restore ARN if present + if key.BedrockKeyConfig.ARN != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.arn", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + arn := "env." + envVar + bedrockConfig.ARN = &arn + } else { + bedrockConfig.ARN = key.BedrockKeyConfig.ARN + } + } + + redactedKeys[i].BedrockKeyConfig = bedrockConfig + } }
341-349: Missing BedrockKeyConfig when loading providers from databaseThe
loadProvidersFromDBfunction is not restoringBedrockKeyConfigwhen convertingDBKeytoschemas.Key. This will cause loss of Bedrock configurations on system restart.Add BedrockKeyConfig to the key conversion:
keys[i] = schemas.Key{ ID: dbKey.KeyID, Value: dbKey.Value, Models: dbKey.Models, Weight: dbKey.Weight, AzureKeyConfig: dbKey.AzureKeyConfig, VertexKeyConfig: dbKey.VertexKeyConfig, + BedrockKeyConfig: dbKey.BedrockKeyConfig, }
680-705: Missing BedrockKeyConfig assignment and field extraction in saveProvidersToDBThe
saveProvidersToDBfunction is not properly handlingBedrockKeyConfigwhen creatingDBKeyrecords. While Azure and Vertex configs are saved with both the full config objects and individual fields, Bedrock config is missing.Add BedrockKeyConfig handling similar to Azure and Vertex:
dbKey := DBKey{ ProviderID: dbProvider.ID, KeyID: key.ID, Value: key.Value, Models: key.Models, Weight: key.Weight, AzureKeyConfig: key.AzureKeyConfig, VertexKeyConfig: key.VertexKeyConfig, + BedrockKeyConfig: key.BedrockKeyConfig, } // Handle Azure config if key.AzureKeyConfig != nil { dbKey.AzureEndpoint = &key.AzureKeyConfig.Endpoint dbKey.AzureAPIVersion = key.AzureKeyConfig.APIVersion } // Handle Vertex config if key.VertexKeyConfig != nil { dbKey.VertexProjectID = &key.VertexKeyConfig.ProjectID dbKey.VertexRegion = &key.VertexKeyConfig.Region dbKey.VertexAuthCredentials = &key.VertexKeyConfig.AuthCredentials } + + // Handle Bedrock config + if key.BedrockKeyConfig != nil { + dbKey.BedrockAccessKey = &key.BedrockKeyConfig.AccessKey + dbKey.BedrockSecretKey = &key.BedrockKeyConfig.SecretKey + dbKey.BedrockSessionToken = key.BedrockKeyConfig.SessionToken + dbKey.BedrockRegion = key.BedrockKeyConfig.Region + dbKey.BedrockARN = key.BedrockKeyConfig.ARN + } dbKeys = append(dbKeys, dbKey)Note: This assumes the
DBKeystruct has been updated with the appropriate Bedrock fields. Please verify that the database model includes these fields.
Pratham-Mishra04
force-pushed
the
08-06-fix_bedrock_key_config_structure_fixes
branch
from
25b8a28 to
17b7ef8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (7)
transports/bifrost-http/lib/models.go (2)
169-188: Add serialization for Bedrock deployments in BeforeSave hookThe
BeforeSavehook is missing serialization logic forBedrockKeyConfig.Deployments. Without this, Bedrock deployments won't be persisted to the database.Add the following serialization logic after the Azure deployments serialization:
if k.AzureKeyConfig != nil && k.AzureKeyConfig.Deployments != nil { data, err := json.Marshal(k.AzureKeyConfig.Deployments) if err != nil { return err } deployments := string(data) k.AzureDeploymentsJSON = &deployments } + + if k.BedrockKeyConfig != nil && k.BedrockKeyConfig.Deployments != nil { + data, err := json.Marshal(k.BedrockKeyConfig.Deployments) + if err != nil { + return err + } + deployments := string(data) + k.BedrockDeploymentsJSON = &deployments + } return nil
264-305: Add reconstruction logic for BedrockKeyConfig in AfterFind hookThe
AfterFindhook is missing logic to reconstructBedrockKeyConfigfrom the database fields. This will cause the Bedrock configuration to be lost when keys are loaded from the database.Add the following reconstruction logic after the Vertex config reconstruction:
// Reconstruct Vertex config if fields are present if k.VertexProjectID != nil { config := &schemas.VertexKeyConfig{ ProjectID: *k.VertexProjectID, } if k.VertexRegion != nil { config.Region = *k.VertexRegion } if k.VertexAuthCredentials != nil { config.AuthCredentials = *k.VertexAuthCredentials } k.VertexKeyConfig = config } + + // Reconstruct Bedrock config if fields are present + if k.BedrockAccessKey != nil { + config := &schemas.BedrockKeyConfig{ + AccessKey: *k.BedrockAccessKey, + } + + if k.BedrockSecretKey != nil { + config.SecretKey = *k.BedrockSecretKey + } + if k.BedrockSessionToken != nil { + config.SessionToken = k.BedrockSessionToken + } + if k.BedrockRegion != nil { + config.Region = k.BedrockRegion + } + if k.BedrockARN != nil { + config.ARN = k.BedrockARN + } + if k.BedrockDeploymentsJSON != nil { + var deployments map[string]string + if err := json.Unmarshal([]byte(*k.BedrockDeploymentsJSON), &deployments); err != nil { + return err + } + config.Deployments = deployments + } + + k.BedrockKeyConfig = config + } return niltransports/bifrost-http/lib/store.go (5)
543-543: Missing Bedrock config restoration in writeConfigToFile.The
writeConfigToFilemethod restores environment variable references for Azure (lines 485-511) and Vertex (lines 513-542) configs, but Bedrock config restoration is missing. This means Bedrock environment variables won't be properly restored when writing config back to file.Add Bedrock config restoration after the Vertex config section:
redactedKeys[i].VertexKeyConfig = vertexConfig } + + // Restore Bedrock key config if present + if key.BedrockKeyConfig != nil { + bedrockConfig := &schemas.BedrockKeyConfig{ + Deployments: key.BedrockKeyConfig.Deployments, + } + + // Restore AccessKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.access_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.AccessKey = "env." + envVar + } else { + bedrockConfig.AccessKey = key.BedrockKeyConfig.AccessKey + } + + // Restore SecretKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.secret_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.SecretKey = "env." + envVar + } else { + bedrockConfig.SecretKey = key.BedrockKeyConfig.SecretKey + } + + // Restore SessionToken if present + if key.BedrockKeyConfig.SessionToken != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.session_token", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + sessionToken := "env." + envVar + bedrockConfig.SessionToken = &sessionToken + } else { + bedrockConfig.SessionToken = key.BedrockKeyConfig.SessionToken + } + } + + // Restore Region if present + if key.BedrockKeyConfig.Region != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.region", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + region := "env." + envVar + bedrockConfig.Region = ®ion + } else { + bedrockConfig.Region = key.BedrockKeyConfig.Region + } + } + + // Restore ARN if present + if key.BedrockKeyConfig.ARN != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.arn", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + arn := "env." + envVar + bedrockConfig.ARN = &arn + } else { + bedrockConfig.ARN = key.BedrockKeyConfig.ARN + } + } + + redactedKeys[i].BedrockKeyConfig = bedrockConfig + } }
937-937: Missing Bedrock config redaction in GetProviderConfigRedacted.The
GetProviderConfigRedactedmethod redacts sensitive values for Azure (lines 878-904) and Vertex (lines 907-936) configs, but Bedrock config redaction is missing. This could expose sensitive AWS credentials in API responses.Add Bedrock config redaction after the Vertex config section:
redactedConfig.Keys[i].VertexKeyConfig = vertexConfig } + + // Redact Bedrock key config if present + if key.BedrockKeyConfig != nil { + bedrockConfig := &schemas.BedrockKeyConfig{ + Deployments: key.BedrockKeyConfig.Deployments, + } + + // Redact AccessKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.access_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.AccessKey = "env." + envVar + } else { + bedrockConfig.AccessKey = RedactKey(key.BedrockKeyConfig.AccessKey) + } + + // Redact SecretKey + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.secret_key", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + bedrockConfig.SecretKey = "env." + envVar + } else { + bedrockConfig.SecretKey = RedactKey(key.BedrockKeyConfig.SecretKey) + } + + // Redact SessionToken if present + if key.BedrockKeyConfig.SessionToken != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.session_token", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + sessionToken := "env." + envVar + bedrockConfig.SessionToken = &sessionToken + } else { + redactedToken := RedactKey(*key.BedrockKeyConfig.SessionToken) + bedrockConfig.SessionToken = &redactedToken + } + } + + // Region is not sensitive, handle env vars only + if key.BedrockKeyConfig.Region != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.region", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + region := "env." + envVar + bedrockConfig.Region = ®ion + } else { + bedrockConfig.Region = key.BedrockKeyConfig.Region + } + } + + // Redact ARN if present + if key.BedrockKeyConfig.ARN != nil { + path = fmt.Sprintf("providers.%s.keys[%s].bedrock_key_config.arn", provider, key.ID) + if envVar, ok := envVarsByPath[path]; ok { + arn := "env." + envVar + bedrockConfig.ARN = &arn + } else { + redactedARN := RedactKey(*key.BedrockKeyConfig.ARN) + bedrockConfig.ARN = &redactedARN + } + } + + redactedConfig.Keys[i].BedrockKeyConfig = bedrockConfig + } }
341-349: Missing BedrockKeyConfig in loadProvidersFromDB.The function loads Azure and Vertex key configs from the database but doesn't include BedrockKeyConfig when reconstructing the keys.
Add BedrockKeyConfig to the key reconstruction:
keys[i] = schemas.Key{ ID: dbKey.KeyID, Value: dbKey.Value, Models: dbKey.Models, Weight: dbKey.Weight, AzureKeyConfig: dbKey.AzureKeyConfig, VertexKeyConfig: dbKey.VertexKeyConfig, + BedrockKeyConfig: dbKey.BedrockKeyConfig, }
682-690: Missing BedrockKeyConfig in saveProvidersToDB.The function saves Azure and Vertex key configs to the database but doesn't include BedrockKeyConfig.
Add BedrockKeyConfig to the DBKey struct initialization:
dbKey := DBKey{ ProviderID: dbProvider.ID, KeyID: key.ID, Value: key.Value, Models: key.Models, Weight: key.Weight, AzureKeyConfig: key.AzureKeyConfig, VertexKeyConfig: key.VertexKeyConfig, + BedrockKeyConfig: key.BedrockKeyConfig, }
704-704: Add Bedrock config persistence in saveProvidersToDBThe
DBKeystruct defines Bedrock fields butsaveProvidersToDBonly populates Azure and Vertex. After the Vertex block in
transports/bifrost-http/lib/store.go(around line 699), add:// Handle Vertex config if key.VertexKeyConfig != nil { dbKey.VertexProjectID = &key.VertexKeyConfig.ProjectID dbKey.VertexRegion = &key.VertexKeyConfig.Region dbKey.VertexAuthCredentials = &key.VertexKeyConfig.AuthCredentials } + // Handle Bedrock config + if key.BedrockKeyConfig != nil { + dbKey.BedrockAccessKey = &key.BedrockKeyConfig.AccessKey + dbKey.BedrockSecretKey = &key.BedrockKeyConfig.SecretKey + dbKey.BedrockSessionToken = &key.BedrockKeyConfig.SessionToken + dbKey.BedrockRegion = &key.BedrockKeyConfig.Region + dbKey.BedrockARN = &key.BedrockKeyConfig.ARN + + // Serialize Bedrock deployments map into JSON + depsJSON, err := json.Marshal(key.BedrockKeyConfig.Deployments) + if err != nil { + return fmt.Errorf("failed to marshal Bedrock deployments: %w", err) + } + depsStr := string(depsJSON) + dbKey.BedrockDeploymentsJSON = &depsStr + } dbKeys = append(dbKeys, dbKey)Also ensure
import ( "encoding/json"; "fmt" )is present at the top of the file.
♻️ Duplicate comments (6)
core/utils.go (1)
20-24: Fix the comment grammar as previously noted.The grammar issue in the comment was already identified in a previous review but hasn't been addressed yet.
docs/contributing/provider.md (2)
493-493: Fix grammatical and formatting issues.
578-578: Fix formatting and punctuation issues.docs/usage/http-transport/openapi.json (1)
2777-2813: Adddescriptiontobedrock_key_configfor schema parity with other provider configs
The newbedrock_key_configblock still lacks a top-level"description"field, whereas bothazure_key_configandvertex_key_configinclude one. This breaks consistency and downstream tooling (doc generators, UI forms) will render this object without context.Same feedback was provided earlier; please address.
ui/components/config/provider-form.tsx (2)
181-197: Fix validation logic for Bedrock deploymentsThe validation logic incorrectly treats Bedrock deployments as required, but according to the schema and UI label, deployments should be optional.
779-806: Add proper label associations for accessibilityThe label elements for Bedrock configuration fields are not properly associated with their input elements, which impacts accessibility for screen reader users.
Pratham-Mishra04
force-pushed
the
08-06-fix_bedrock_key_config_structure_fixes
branch
from
17b7ef8 to
38ae920
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
core/providers/bedrock.go (2)
295-306: Reconsider the credential validation approach to align with established patterns.The current implementation checks for an empty
SecretKeybefore signing, but based on the team's established pattern (from PR #54), AWS credential validation should be left to AWS itself. This preemptive check prevents the use of alternative AWS authentication methods like IAM roles or environment variables.Consider removing the preemptive validation to allow AWS SDK to handle authentication naturally:
- if config.SecretKey != "" { - if err := signAWSRequest(req, config.AccessKey, config.SecretKey, config.SessionToken, region, "bedrock"); err != nil { - return nil, err - } - } else { - return nil, &schemas.BifrostError{ - IsBifrostError: false, - Error: schemas.ErrorField{ - Message: "secret access key not set", - }, - } - } + if err := signAWSRequest(req, config.AccessKey, config.SecretKey, config.SessionToken, region, "bedrock"); err != nil { + return nil, err + }This allows AWS to provide its own authentication error messages which can be more informative than custom validation errors.
1332-1338: Duplicate credential validation should follow the same pattern ascompleteRequest.This code duplicates the SecretKey validation from
completeRequest. If the earlier suggestion to remove preemptive validation is accepted, this should be updated similarly to allow AWS SDK to handle authentication naturally.- if key.BedrockKeyConfig.SecretKey != "" { - if signErr := signAWSRequest(req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock"); signErr != nil { - return nil, signErr - } - } else { - return nil, newConfigurationError("secret access key not set", schemas.Bedrock) - } + if signErr := signAWSRequest(req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock"); signErr != nil { + return nil, signErr + }
♻️ Duplicate comments (6)
transports/bifrost-http/main.go (1)
20-20: Terminology correctly updated to reflect key config approach.The change from "meta config" to "key config" properly aligns with the broader refactor in this PR that removes provider-level meta configuration support in favor of key-level configuration.
core/utils.go (1)
20-24: Fix the comment grammar as noted in previous review.The function implementation is correct and properly centralizes the logic for providers that allow empty key values. However, the comment grammar needs to be fixed.
Fix the comment grammar:
-// Some provider like Vertex and Bedrock have their credentials in additional key configs.. +// Some providers like Vertex and Bedrock have their credentials in additional key configs.docs/contributing/provider.md (2)
493-493: Fix grammatical and formatting issues.The sentence has grammatical and spacing issues that affect readability.
Apply this diff to fix the issues:
-The HTTP transport layer requires specific changes to handle provider configuration, and model patterns. +The HTTP transport layer requires specific changes to handle provider configuration and model patterns.
578-578: Fix formatting and punctuation issues.The line has spacing and dash usage issues that affect readability.
Apply this diff to fix the formatting:
-Note: API key handling is automatic - you only need to implement the key config processing if your provider requires additional configuration beyond API keys. +Note: API key handling is automatic—you only need to implement the key config processing if your provider requires additional configuration beyond API keys.docs/usage/http-transport/openapi.json (1)
2777-2813: Add missingdescriptionforbedrock_key_configto stay consistent with other provider key-config objects
azure_key_configandvertex_key_configboth include a top-leveldescription. Omitting it here breaks schema uniformity and causes downstream docs tooling to render this block without context.ui/components/config/provider-form.tsx (1)
778-841: Add proper label associations for accessibilityThe label elements for Bedrock configuration fields are not properly associated with their input elements, which impacts accessibility for screen reader users.
Associate each label with its corresponding input using the
htmlForattribute:<div> - <label className="text-sm font-medium">Access Key (Required)</label> + <label htmlFor={`bedrock-access-key-${index}`} className="text-sm font-medium">Access Key (Required)</label> <Input + id={`bedrock-access-key-${index}`} placeholder="your-aws-access-key or env.AWS_ACCESS_KEY" value={key.bedrock_key_config?.access_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'access_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Secret Key (Required)</label> + <label htmlFor={`bedrock-secret-key-${index}`} className="text-sm font-medium">Secret Key (Required)</label> <Input + id={`bedrock-secret-key-${index}`} placeholder="your-aws-secret-key or env.AWS_SECRET_KEY" value={key.bedrock_key_config?.secret_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'secret_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Session Token (Optional)</label> + <label htmlFor={`bedrock-session-token-${index}`} className="text-sm font-medium">Session Token (Optional)</label> <Input + id={`bedrock-session-token-${index}`} placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={key.bedrock_key_config?.session_token || ''} onChange={(e) => updateKeyBedrockConfig(index, 'session_token', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Deployments (Optional)</label> + <label htmlFor={`bedrock-deployments-${index}`} className="text-sm font-medium">Deployments (Optional)</label> <div className="text-muted-foreground mb-2 text-xs"> JSON object mapping model names to inference profile names </div> <Textarea + id={`bedrock-deployments-${index}`}
Pratham-Mishra04
force-pushed
the
08-06-fix_bedrock_key_config_structure_fixes
branch
from
38ae920 to
de48a3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
core/utils.go (1)
20-24: Well-implemented utility function.The
canProviderKeyValueBeEmptyfunction properly centralizes the logic for identifying providers that allow empty key values, supporting more maintainable key selection logic throughout the codebase.The grammar issue in the comment was already identified in a previous review.
docs/contributing/provider.md (2)
493-493: Fix grammatical and formatting issues.The sentence has grammatical and spacing issues that affect readability.
Apply this diff to fix the issues:
-The HTTP transport layer requires specific changes to handle provider configuration, and model patterns. +The HTTP transport layer requires specific changes to handle provider configuration and model patterns.
578-578: Fix formatting and punctuation issues.The line has spacing and dash usage issues that affect readability.
Apply this diff to fix the formatting:
-Note: API key handling is automatic - you only need to implement the key config processing if your provider requires additional configuration beyond API keys. +Note: API key handling is automatic—you only need to implement the key config processing if your provider requires additional configuration beyond API keys.docs/usage/http-transport/configuration/providers.md (1)
485-487: Fix environment variable name inconsistency.There's an inconsistency between the environment variable name used here (
AWS_SECRET_KEY) and the one referenced in the configuration examples (AWS_SECRET_ACCESS_KEY).Apply this diff to match the configuration examples:
-export AWS_SECRET_KEY="your-secret-key" +export AWS_SECRET_ACCESS_KEY="your-secret-key"docs/usage/http-transport/openapi.json (1)
2777-2813: Add missingdescriptionforbedrock_key_configto align with other provider configs
azure_key_configandvertex_key_configobjects have a human-readabledescription; omitting it here breaks schema consistency and downstream doc generation.- "bedrock_key_config": { - "type": "object", + "bedrock_key_config": { + "type": "object", + "description": "Bedrock key configuration",ui/components/config/provider-form.tsx (1)
776-841: Fix missing label associations for accessibility.The label elements for Bedrock configuration fields lack proper association with their inputs, impacting screen reader users. Add
htmlForattributes and correspondingidattributes:- <label className="text-sm font-medium">Access Key (Required)</label> + <label htmlFor={`bedrock-access-key-${index}`} className="text-sm font-medium">Access Key (Required)</label> <Input + id={`bedrock-access-key-${index}`} placeholder="your-aws-access-key or env.AWS_ACCESS_KEY" value={key.bedrock_key_config?.access_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'access_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> - <label className="text-sm font-medium">Secret Key (Required)</label> + <label htmlFor={`bedrock-secret-key-${index}`} className="text-sm font-medium">Secret Key (Required)</label> <Input + id={`bedrock-secret-key-${index}`} placeholder="your-aws-secret-key or env.AWS_SECRET_KEY" value={key.bedrock_key_config?.secret_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'secret_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> - <label className="text-sm font-medium">Session Token (Optional)</label> + <label htmlFor={`bedrock-session-token-${index}`} className="text-sm font-medium">Session Token (Optional)</label> <Input + id={`bedrock-session-token-${index}`} placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={key.bedrock_key_config?.session_token || ''} onChange={(e) => updateKeyBedrockConfig(index, 'session_token', e.target.value)} className="transition-all duration-200 ease-in-out" /> - <label className="text-sm font-medium">Deployments (Optional)</label> + <label htmlFor={`bedrock-deployments-${index}`} className="text-sm font-medium">Deployments (Optional)</label> <div className="text-muted-foreground mb-2 text-xs"> JSON object mapping model names to inference profile names </div> <Textarea + id={`bedrock-deployments-${index}`}transports/bifrost-http/lib/store.go (2)
544-590: Critical: Missing assignment of restored Bedrock config to keyThe restored
bedrockConfigis never assigned back toredactedKeys[i].BedrockKeyConfig. This means the Bedrock configuration won't be written to the config file.Add the missing assignment after line 588:
bedrockConfig.ARN = key.BedrockKeyConfig.ARN } + redactedKeys[i].BedrockKeyConfig = bedrockConfig }
754-761: Missing ARN field in database save operationThe
BedrockKeyConfig.ARNfield is not being saved to the database. Based on the BedrockKeyConfig struct definition, the ARN field should be persisted.Add the missing ARN field assignment:
// Handle Bedrock config if key.BedrockKeyConfig != nil { dbKey.BedrockAccessKey = &key.BedrockKeyConfig.AccessKey dbKey.BedrockSecretKey = &key.BedrockKeyConfig.SecretKey dbKey.BedrockSessionToken = key.BedrockKeyConfig.SessionToken dbKey.BedrockRegion = key.BedrockKeyConfig.Region + dbKey.BedrockARN = key.BedrockKeyConfig.ARN }core/providers/bedrock.go (1)
1120-1122: Embedding handlers correctly updated to use BedrockKeyConfig.The method calls now pass the complete BedrockKeyConfig instead of just the access key, enabling full AWS credential and region access.
Pratham-Mishra04
force-pushed
the
08-06-fix_bedrock_key_config_structure_fixes
branch
from
de48a3d to
f59a0cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (20)
transports/bifrost-http/main.go (1)
20-20: Synchronize provider list across comment sections for clarityNice catch on switching to “key config support”, but the earlier overview sentence (Line 2) still omits Azure and Vertex. Keeping both lists in sync avoids reader confusion.
-// for text and chat completions using various AI model providers (OpenAI, Anthropic, Bedrock, Mistral, Ollama, etc.). +// for text and chat completions using various AI model providers (OpenAI, Anthropic, Azure, Bedrock, Mistral, Vertex, Ollama, etc.).tests/core-providers/go.mod (1)
28-30: Previous concerns about Sonic dependency chain still apply.The addition of bytedance/sonic and related dependencies (loader, base64x, cpuid/v2, golang-asm, x/arch) was already flagged in previous reviews regarding the cost/benefit analysis of these dependencies versus standard library or existing alternatives.
Also applies to: 34-34, 38-38, 42-42
core/utils.go (1)
20-24: Well-implemented utility function supporting the key-level config refactor.The
canProviderKeyValueBeEmptyfunction properly centralizes the logic for identifying providers (Vertex and Bedrock) that allow empty key values. This aligns well with the broader refactoring to move Bedrock configuration from meta to key level.tests/core-providers/config/account.go (1)
99-105: LGTM! Well-structured key-level configuration for Bedrock.The migration from meta config to
BedrockKeyConfigis well-implemented and aligns perfectly with the PR objectives. The structure matches the schema definition and uses standard AWS environment variable names.Note: There was a previous suggestion about handling empty
SessionTokenmore explicitly by checking if the environment variable is empty and setting it tonilrather than a pointer to an empty string. However, based on the established patterns in the codebase and the preference to let AWS handle authentication naturally, the current implementation is acceptable.docs/usage/go-package/account.md (1)
148-153: Fix environment variable inconsistency.There's an inconsistency in AWS access key environment variable names - Line 100 checks for
AWS_ACCESS_KEY_IDbut Line 149 usesAWS_ACCESS_KEY. This should be consistent throughout the file using the standard AWS environment variable naming.docs/contributing/provider.md (2)
493-493: Fix grammatical and formatting issues.The sentence has grammatical and spacing issues that affect readability.
Apply this diff to fix the issues:
-The HTTP transport layer requires specific changes to handle provider configuration, and model patterns. +The HTTP transport layer requires specific changes to handle provider configuration and model patterns.
578-578: Fix formatting and punctuation issues.The line has spacing and dash usage issues that affect readability.
Apply this diff to fix the formatting:
-Note: API key handling is automatic - you only need to implement the key config processing if your provider requires additional configuration beyond API keys. +Note: API key handling is automatic—you only need to implement the key config processing if your provider requires additional configuration beyond API keys.docs/usage/http-transport/configuration/providers.md (1)
484-485: Fix environment variable name inconsistency.There's an inconsistency between the environment variable name used here (
AWS_SECRET_KEY) and the one referenced in the configuration examples (AWS_SECRET_ACCESS_KEY).Apply this diff to match the configuration examples:
-export AWS_SECRET_KEY="your-secret-key" +export AWS_SECRET_ACCESS_KEY="your-secret-key"docs/usage/http-transport/openapi.json (1)
2777-2813: Add missingdescriptionforbedrock_key_configto keep schema uniform
azure_key_configandvertex_key_configeach include adescriptionfield.bedrock_key_configomits it, which breaks consistency and causes autogenerated docs to render this object without context."bedrock_key_config": { "type": "object", + "description": "Bedrock key configuration", "properties": {ui/components/config/provider-form.tsx (3)
435-441: Initialize all optional fields for consistency.Consider initializing all optional fields when creating a new
bedrock_key_configto match the pattern used increateDefaultKey:if (!keyToUpdate.bedrock_key_config) { keyToUpdate.bedrock_key_config = { access_key: '', secret_key: '', session_token: '', + region: '', + arn: '', + deployments: {}, } }
780-807: Fix missing label associations for accessibility.The label elements are not properly associated with their input elements, impacting screen reader accessibility:
- <label className="text-sm font-medium">Access Key (Required)</label> + <label htmlFor={`bedrock-access-key-${index}`} className="text-sm font-medium">Access Key (Required)</label> <Input + id={`bedrock-access-key-${index}`} placeholder="your-aws-access-key or env.AWS_ACCESS_KEY" value={key.bedrock_key_config?.access_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'access_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Secret Key (Required)</label> + <label htmlFor={`bedrock-secret-key-${index}`} className="text-sm font-medium">Secret Key (Required)</label> <Input + id={`bedrock-secret-key-${index}`} placeholder="your-aws-secret-key or env.AWS_SECRET_KEY" value={key.bedrock_key_config?.secret_key || ''} onChange={(e) => updateKeyBedrockConfig(index, 'secret_key', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Session Token (Optional)</label> + <label htmlFor={`bedrock-session-token-${index}`} className="text-sm font-medium">Session Token (Optional)</label> <Input + id={`bedrock-session-token-${index}`} placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={key.bedrock_key_config?.session_token || ''} onChange={(e) => updateKeyBedrockConfig(index, 'session_token', e.target.value)} className="transition-all duration-200 ease-in-out" /> </div> <div> - <label className="text-sm font-medium">Deployments (Optional)</label> + <label htmlFor={`bedrock-deployments-${index}`} className="text-sm font-medium">Deployments (Optional)</label>
811-838: Add id attribute to deployments Textarea for accessibility.Complete the accessibility fix by adding the id attribute to the Textarea element:
<Textarea + id={`bedrock-deployments-${index}`} placeholder='{"gpt-4": "my-gpt4-deployment", "gpt-3.5-turbo": "my-gpt35-deployment"}'transports/bifrost-http/lib/store.go (2)
55-55: Fix misleading comment about KeyID for bedrock_configThe comment incorrectly states that KeyID is empty for
bedrock_config. Since Bedrock config is now a key-level configuration (likeazure_configandvertex_config), the KeyID field is populated when processing Bedrock key configs.
544-590: Critical: Missing assignment of restored Bedrock config to keyThe restored
bedrockConfigis created and populated but never assigned back toredactedKeys[i].BedrockKeyConfig. Add the missing assignment before the closing brace:bedrockConfig.ARN = key.BedrockKeyConfig.ARN } + redactedKeys[i].BedrockKeyConfig = bedrockConfig }core/providers/bedrock.go (6)
253-253: Consider passing BedrockKeyConfig by pointer for better performance.The
completeRequestmethod signature should accept*schemas.BedrockKeyConfiginstead ofschemas.BedrockKeyConfigto avoid copying the struct, which may contain large deployment mappings.
820-820: Update method call if pointer parameter suggestion is accepted.If
completeRequestis updated to accept*schemas.BedrockKeyConfig, this call should passkey.BedrockKeyConfigdirectly without dereferencing.
945-945: Update method call if pointer parameter suggestion is accepted.If
completeRequestis updated to accept*schemas.BedrockKeyConfig, this call should passkey.BedrockKeyConfigdirectly without dereferencing.
1120-1122: Consider updating embedding handler signatures to accept pointer parameters.Similar to the earlier suggestion, the embedding handler methods should accept
*schemas.BedrockKeyConfigto avoid struct copying, and these calls should pass the pointer directly.
1129-1129: Update method signature to accept pointer parameter.For consistency with the earlier suggestion, this method should accept
*schemas.BedrockKeyConfiginstead ofschemas.BedrockKeyConfigby value.
1200-1200: Update method signature to accept pointer parameter.For consistency with the earlier suggestion, this method should accept
*schemas.BedrockKeyConfiginstead ofschemas.BedrockKeyConfigby value.
# Move Bedrock configuration from provider meta config to key-level config This PR refactors the AWS Bedrock provider to use key-level configuration instead of provider-level meta configuration. This change aligns Bedrock with other providers like Azure and Vertex AI that already use key-level configuration. Key changes: - Added `BedrockKeyConfig` struct to store AWS credentials and configuration - Removed the provider-level `meta` field and `MetaConfig` interface - Updated the Bedrock provider to read configuration from key config - Added support for empty key values when provider uses alternative credentials - Updated UI to show Bedrock configuration fields at the key level - Updated documentation and example configuration This change improves consistency across providers and allows for more flexible configuration when using multiple AWS credentials with different regions or deployments.
2 participants
Move Bedrock configuration from provider meta config to key-level config
This PR refactors the AWS Bedrock provider to use key-level configuration instead of provider-level meta configuration. This change aligns Bedrock with other providers like Azure and Vertex AI that already use key-level configuration.
Key changes:
BedrockKeyConfigstruct to store AWS credentials and configurationmetafield andMetaConfiginterfaceThis change improves consistency across providers and allows for more flexible configuration when using multiple AWS credentials with different regions or deployments.