Conversation
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughBedrock provider signatures now thread a common schemas.Key; completeRequest uses Bearer auth when key.Value is set, otherwise performs AWS SigV4 signing using credentials/region from key.BedrockKeyConfig. Model path resolution unified; UI, docs, and configstore JSON helpers adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Caller
participant Provider as BedrockProvider
participant Auth as completeRequest
participant AWS as AWS Bedrock
UI->>Provider: Text/Chat/Embedding/Stream request (includes schemas.Key)
Provider->>Auth: Build request + pass key
alt key.Value present
note right of Auth #D6EAF8: API Key (Bearer)
Auth->>AWS: HTTP request with Authorization: Bearer <key.Value>
else SigV4 path
note right of Auth #F9E79F: SigV4 (AWS creds from key.BedrockKeyConfig)
Auth->>AWS: SigV4-signed request (region from key.BedrockKeyConfig)
end
AWS-->>Auth: Response / Event Stream
Auth-->>Provider: Response / parsed chunks
Provider-->>UI: Result / stream updates
sequenceDiagram
autonumber
participant Caller as UI
participant Stream as ChatCompletionStream
participant Auth as completeRequest (stream)
participant Bedrock as AWS Bedrock (event-stream)
Caller->>Stream: Start stream (with schemas.Key)
Stream->>Auth: Initiate streaming request + pass key
alt key.Value present
Auth->>Bedrock: Open event-stream with Bearer auth
else SigV4 path
Auth->>Bedrock: Open event-stream with SigV4 signing
end
loop stream frames
Bedrock-->>Auth: Event frames
Auth-->>Stream: Parsed deltas (capture stream/message ID)
Stream-->>Caller: onChunk/onDone callbacks (final chunk includes ID)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
TejasGhatte
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
52219b3 to
4b396fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/lib/types/config.ts (1)
45-64: Fix unsafe defaults: avoid “undefined as unknown as string” and incorrect Required<> castCasting undefined to string defeats type-safety and can break consumers at runtime. Since access_key/secret_key became optional, DefaultBedrockKeyConfig should not be “Required<...>”.
Apply:
export const DefaultBedrockKeyConfig: BedrockKeyConfig = { access_key: "", secret_key: "", - session_token: undefined as unknown as string, + session_token: undefined, region: "us-east-1", - arn: undefined as unknown as string, + arn: undefined, deployments: {}, -} as const satisfies Required<BedrockKeyConfig>; +} as const satisfies BedrockKeyConfig;core/providers/bedrock.go (1)
241-243: Compile-time bug: invalid range over int
for range <int>doesn’t compile. Use a counted loop.- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - bedrockChatResponsePool.Put(&BedrockChatResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&BedrockChatResponse{}) + }
🧹 Nitpick comments (14)
framework/configstore/migrations.go (1)
12-16: Nit: inline return for simpler control flowYou can shorten this to a single return.
- if err := migrationInit(db); err != nil { - return err - } - return nil + return migrationInit(db)core/schemas/account.go (1)
34-35: Clarify docs to match optional fields in UIMention that omitting the fields (JSON empty/omitted) is equivalent to empty string.
-// NOTE: To use Vertex IAM role authentication, set AuthCredentials to empty string. +// NOTE: To use Vertex IAM role authentication, set AuthCredentials to empty string (or omit it in JSON). -// NOTE: To use Bedrock IAM role authentication, set both AccessKey and SecretKey to empty strings. +// NOTE: To use Bedrock IAM role authentication, set both AccessKey and SecretKey to empty strings (or omit them). // To use Bedrock API Key authentication, set Value in Key struct instead.Also applies to: 47-49
core/providers/bedrock.go (3)
1030-1036: Use PathEscape for path segments (consistency + correctness)We escape this for a path segment; PathEscape is more appropriate and consistent with embedding code.
- encodedModelIdentifier := url.QueryEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) + encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) path = fmt.Sprintf("%s/converse", encodedModelIdentifier)
1465-1473: Set streaming Accept header (and set before signing)Streaming should advertise the event stream. Also ensure JSON Content-Type for body.
// Create HTTP request for streaming req, reqErr := http.NewRequestWithContext(ctx, "POST", fmt.Sprintf("https://bedrock-runtime.%s.amazonaws.com/model/%s", region, path), bytes.NewReader(jsonBody)) if reqErr != nil { return nil, newBifrostOperationError("error creating request", reqErr, providerName) } // Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) + // Streaming headers + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/vnd.amazon.eventstream")
1151-1153: Don’t override caller-provided Accept; set only if absentLets streaming set eventstream Accept before signing.
- req.Header.Set("Accept", "application/json") + if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/json") + }ui/app/providers/views/modelProviderKeysTableView.tsx (1)
41-46: Replace KnownProvidersNames[index] checks with explicit provider namesKnownProvidersNames[3] === "bedrock" and KnownProvidersNames[5] === "vertex"; matching by index is brittle—use literal names.
File: ui/app/providers/views/modelProviderKeysTableView.tsx (lines 41-46)switch (provider.name) { - case KnownProvidersNames[5]: + case "vertex": return key.vertex_key_config?.auth_credentials || "unknown"; - case KnownProvidersNames[3]: + case "bedrock": return key.value || key.bedrock_key_config?.access_key || "system IAM";ui/app/providers/fragments/apiKeysFormFragment.tsx (8)
24-24: Bedrock models placeholder: include current Claude 3 namesSuggest reflecting commonly used Bedrock model ids to reduce user guesswork.
- bedrock: "e.g. claude-v2, titan-text-express-v1", + bedrock: "e.g. claude-3-sonnet, claude-v2, titan-text-express-v1",
45-49: Clarify auth precedence and required fields in the help copyMake it explicit that fields should not be mixed and that Region still applies.
- <AlertTitle>Authentication Methods</AlertTitle> - <AlertDescription> - You can either use IAM role authentication or API key authentication. Please leave API Key empty when using IAM role - authentication. - </AlertDescription> + <AlertTitle>Authentication Methods</AlertTitle> + <AlertDescription> + Two options: Bedrock API Key or AWS IAM (role/keys). If using IAM roles, leave “API Key” empty. If using an API Key, + leave Access Key / Secret Key / Session Token empty. Region (and optionally ARN) still apply for both. + </AlertDescription>
53-69: Bedrock API key: label + mask input; avoid mixed-auth confusionSince this block now shows for Bedrock, make the label explicit and mask the secret to align with other providers. Also consider preventing mixed configs.
- <FormLabel>API Key</FormLabel> + <FormLabel>{isBedrock ? "Bedrock API Key" : "API Key"}</FormLabel> + {isBedrock && ( + <FormDescription>If set, Bedrock requests use API Key auth instead of SigV4.</FormDescription> + )} <FormControl> - <Input placeholder="API Key or env.MY_KEY" type="text" {...field} /> + <Input + placeholder={isBedrock ? "Bedrock API Key or env.BEDROCK_API_KEY" : "API Key or env.MY_KEY"} + type={isBedrock ? "password" : "text"} + autoComplete="off" + data-lpignore="true" + {...field} + /> </FormControl>Optional (future): add a radio/toggle to pick “API Key” vs “IAM (role/keys)” and disable the other inputs accordingly to avoid ambiguity.
277-285: Remove negative z-index; mark icon decorativeNegative z-index can make the Alert non-interactive under overlapping parents (container has overflow-hidden). Also hide the icon from SRs.
- <Separator className="my-6" /> - <Alert variant="default" className="-z-10"> - <Info className="mt-0.5 h-4 w-4 flex-shrink-0 text-blue-600" /> + <Separator className="my-6" /> + <Alert variant="default"> + <Info aria-hidden="true" className="mt-0.5 h-4 w-4 flex-shrink-0 text-blue-600" />
287-303: Access Key ID: clearer label; keep controlled; optional redaction hintMinor copy tweak; keep value handling. Consider showing a “stored securely” hint when value is redacted.
- <FormLabel>Access Key</FormLabel> + <FormLabel>Access Key ID</FormLabel> <FormControl> <Input placeholder="your-aws-access-key or env.AWS_ACCESS_KEY_ID" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + autoComplete="off" /> </FormControl>Optionally, add (mirroring Vertex) right below FormControl:
{isRedacted(typeof field.value === "string" ? field.value : "") && ( <div className="text-muted-foreground mt-1 flex items-center gap-1 text-xs"> <Info className="h-3 w-3" aria-hidden="true" /> <span>Access Key ID is stored securely. Edit to update.</span> </div> )}
327-343: Session token should be maskedTemporary credentials are sensitive too; mask and disable autofill.
<FormControl> <Input placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" /> </FormControl>Optional: add the redaction hint here as well.
348-365: Region input: enforce required at the control levelSince label says Required, also mark the control required to catch accidental omissions.
<FormControl> <Input placeholder="us-east-1 or env.AWS_REGION" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + required /> </FormControl>
368-385: Clarify ARN type and provide realistic exampleUse “Inference Profile ARN” to avoid confusion with other ARNs and provide a concrete example format.
- <FormLabel>ARN</FormLabel> + <FormLabel>Inference Profile ARN (Optional)</FormLabel> <FormControl> <Input - placeholder="arn:aws:bedrock:us-east-1:123:inference-profile or env.AWS_ARN" + placeholder="arn:aws:bedrock:us-east-1::inference-profile/us.anthropic.claude-3-sonnet-20240229-v1:0 or env.BEDROCK_INFERENCE_PROFILE_ARN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} /> </FormControl>Please confirm the exact ARN shape your backend expects. If it also accepts model ids or old-style resource ARNs, we can broaden the placeholder/copy accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/providers/bedrock.go(9 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
🧰 Additional context used
🧬 Code graph analysis (3)
framework/configstore/sqlite.go (1)
framework/vectorstore/store.go (1)
Config(92-96)
core/providers/bedrock.go (1)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
ui/components/ui/separator.tsx (1)
Separator(43-43)ui/components/ui/input.tsx (1)
Input(7-21)
🔇 Additional comments (6)
framework/configstore/sqlite.go (2)
605-617: LGTM: internal JSON marshal helper usageSwitch to local marshalToStringPtr looks correct and preserves nil semantics.
644-655: LGTM: logs store marshallingGood move to local helper; matches vector store handling.
framework/configstore/utils.go (1)
11-33: LGTM: lightweight JSON marshal helpersClear behavior (nil→nil pointer/empty string) and minimal surface.
ui/lib/types/schemas.ts (1)
31-33: LGTM: make Bedrock creds optionalOptional access/secret aligns with IAM and API key paths; min(1) validators still enforce when provided.
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
6-6: LGTM: separator importGood call adding visual separation for Bedrock sections.
395-395: LGTM: Bedrock deployments placeholderThe example aligns with inference profile names and common Claude ids.
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
4b396fd to
58b40a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/providers/bedrock.go (5)
240-244: Compile-time bug: ranging over an int
for range config.ConcurrencyAndBufferSize.Concurrencywill not compile; you cannot range over an int.- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - bedrockChatResponsePool.Put(&BedrockChatResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&BedrockChatResponse{}) + }
262-314: API Key path missing JSON headersWhen
key.Valueis used, we never setContent-Type/Accept. Some Bedrock endpoints requireapplication/json. Add safe defaults without clobbering user-supplied headers.// Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) + // Ensure JSON headers for Bedrock JSON endpoints (do not override if user set them) + if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") + } + if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/json") + } + // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value))
1028-1036: Path escaping: use PathEscape and avoid QueryEscape for path segmentsUse
url.PathEscapefor path segments.QueryEscapeis for query strings and encodes spaces as+.- path := fmt.Sprintf("%s/converse", model) + path := url.PathEscape(model) + "/converse" if key.BedrockKeyConfig.Deployments != nil { if inferenceProfileId, ok := key.BedrockKeyConfig.Deployments[model]; ok { if key.BedrockKeyConfig.ARN != nil { - encodedModelIdentifier := url.QueryEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) - path = fmt.Sprintf("%s/converse", encodedModelIdentifier) + encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) + path = fmt.Sprintf("%s/converse", encodedModelIdentifier) } } }
1443-1452: Streaming path: escape and build correctlyMirror the non-streaming fix for streaming.
- path := fmt.Sprintf("%s/converse-stream", model) + path := url.PathEscape(model) + "/converse-stream" if key.BedrockKeyConfig.Deployments != nil { if inferenceProfileId, ok := key.BedrockKeyConfig.Deployments[model]; ok { if key.BedrockKeyConfig.ARN != nil { - encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) - path = fmt.Sprintf("%s/converse-stream", encodedModelIdentifier) + encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) + path = fmt.Sprintf("%s/converse-stream", encodedModelIdentifier) } } }
1149-1153: Don’t force Accept in signer (breaks streaming)
signAWSRequestsetsAccept: application/jsonunconditionally, which is incompatible with streaming whereAcceptmust beapplication/vnd.amazon.eventstream. Let callers setAccept; only setContent-Typeif missing.- // Set required headers before signing - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/json") + // Ensure Content-Type before signing; do not force Accept here + if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") + }
♻️ Duplicate comments (3)
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
312-319: Mask Secret Key input and disable autofillSecrets should not render in plain text and should opt out of autofill.
<Input placeholder="your-aws-secret-key or env.AWS_SECRET_ACCESS_KEY" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" />Optionally render a redaction hint below using your existing pattern (as done for Vertex).
333-339: Treat Session Token as a secret (mask and disable autofill)Session tokens are sensitive. Mirror the Secret Key treatment.
<Input placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" />core/providers/bedrock.go (1)
1471-1482: Streaming headers: set JSON Content-Type and event-stream Accept; ensure signer doesn’t overwriteFor streaming, Bedrock expects event stream responses; set
Accept: application/vnd.amazon.eventstreamandContent-Type: application/jsonbefore auth.// Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) + // Ensure headers for streaming + if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") + } + // Required for converse-stream responses + req.Header.Set("Accept", "application/vnd.amazon.eventstream") + // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication - if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { + if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { return nil, err } }
🧹 Nitpick comments (6)
ui/lib/types/config.ts (1)
47-53: Optional AWS creds: good, but fix DefaultBedrockKeyConfig typing hackMaking
access_key/secret_keyoptional aligns with dual-auth. However,DefaultBedrockKeyConfigfakes optional fields as strings (session_token: undefined as unknown as string, same forarn) to satisfyRequired<...>. This is brittle and leaksundefinedat runtime.
- Drop
Required<BedrockKeyConfig>on the default to avoid coercing optionals into required.- Keep true
undefinedfor optional fields.Example adjustment outside the changed lines:
// Replace the assertion on DefaultBedrockKeyConfig export const DefaultBedrockKeyConfig: BedrockKeyConfig = { access_key: "", secret_key: "", session_token: undefined, region: "us-east-1", arn: undefined, deployments: {}, } as const;docs/quickstart/gateway/provider-configuration.mdx (2)
767-774: Clarify dual-auth steps with explicit API exampleThe Web UI steps look good. Consider adding an API/config.json example that uses API Key auth (i.e., include
"value": "env.AWS_BEDROCK_API_KEY"in the key object) so readers see both paths concretely, not just IAM. This reduces ambiguity for users scanning only the examples.Proposed addition (API tab):
# Bedrock via API Key auth curl --location 'http://localhost:8080/api/providers' \ --header 'Content-Type: application/json' \ --data '{ "provider": "bedrock", "keys": [ { "value": "env.AWS_BEDROCK_API_KEY", "models": ["anthropic.claude-3-sonnet-20240229-v1:0"], "weight": 1.0, "bedrock_key_config": { "region": "us-east-1" } } ] }'
837-841: Add “Region is required” to Notes for both auth methodsRegion is mandatory for Bedrock regardless of auth mode. Add a bullet to the Notes to avoid misconfigurations.
- Region is required in both API Key and IAM role authentication modes.
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
96-101: Numeric field typed as string
type="string"is invalid; use a numeric input to avoid invalid values and to improve UX.- type="string" + type="number" + inputMode="decimal" + step="any"
53-69: Also mask the top-level API KeyThe main “API Key” field (Line 62) should be masked and opt-out of autofill to match how we treat other providers’ secrets. Consider adding the redaction hint when the value is redacted.
- <Input placeholder="API Key or env.MY_KEY" type="text" {...field} /> + <Input + placeholder="API Key or env.MY_KEY" + type="password" + autoComplete="off" + data-lpignore="true" + {...field} + />core/providers/bedrock.go (1)
262-268: Name shadowing: avoidconfiglocal var
config := key.BedrockKeyConfigshadows the importedconfigpackage, hurting readability. Rename tobrCfgorbedrockCfg.- config := key.BedrockKeyConfig + brCfg := key.BedrockKeyConfig - if config.Region != nil { - region = *config.Region + if brCfg.Region != nil { + region = *brCfg.Region }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/providers/bedrock.go(9 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)docs/quickstart/gateway/provider-configuration.mdx(2 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
✅ Files skipped from review due to trivial changes (1)
- docs/quickstart/go-sdk/provider-configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/configstore/migrations.go
- framework/configstore/sqlite.go
- ui/lib/types/schemas.ts
- framework/configstore/utils.go
- core/schemas/account.go
- ui/app/providers/views/modelProviderKeysTableView.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
core/providers/bedrock.go (1)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)
ui/app/providers/fragments/apiKeysFormFragment.tsx (4)
ui/components/ui/alert.tsx (3)
AlertTitle(41-41)AlertDescription(41-41)Alert(41-41)ui/components/ui/separator.tsx (1)
Separator(43-43)ui/components/ui/form.tsx (4)
FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(7-21)
🔇 Additional comments (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
277-284: Good UX: IAM guidance alertNice addition. Clear separation of API Key vs IAM helps.
TejasGhatte
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
58b40a9 to
47c6332
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
53-69: Mask API keys and disable autofillAPI keys should not render as plain text and should avoid autofill/password managers.
<FormItem> <FormLabel>API Key</FormLabel> <FormControl> - <Input placeholder="API Key or env.MY_KEY" type="text" {...field} /> + <Input + placeholder="API Key or env.MY_KEY" + type="password" + autoComplete="off" + data-lpignore="true" + spellCheck={false} + {...field} + /> </FormControl> + {isBedrock && isRedacted(typeof field.value === "string" ? field.value : "") && ( + <div className="text-muted-foreground mt-1 flex items-center gap-1 text-xs"> + <Info className="h-3 w-3" aria-hidden="true" /> + <span>API Key is stored securely. Edit to update.</span> + </div> + )} <FormMessage /> </FormItem>
310-319: Secret Key should be masked and non‑autofillRender as password and disable autofill/PM capture; add redaction hint like Vertex credentials.
<Input placeholder="your-aws-secret-key or env.AWS_SECRET_ACCESS_KEY" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + spellCheck={false} + data-lpignore="true" /> + {isRedacted(typeof field.value === "string" ? field.value : "") && ( + <div className="text-muted-foreground mt-1 flex items-center gap-1 text-xs"> + <Info className="h-3 w-3" aria-hidden="true" /> + <span>Secret Key is stored securely. Edit to update.</span> + </div> + )}core/providers/bedrock.go (2)
291-304: Missing JSON headers on API‑key pathWhen key.Value is used, Content-Type/Accept aren’t set. Some Bedrock routes expect JSON headers.
// Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) +// Ensure JSON defaults if not already provided +if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") +} +if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/json") +} // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication if err := signAWSRequest(ctx, req, config.AccessKey, config.SecretKey, config.SessionToken, region, "bedrock", provider.GetProviderKey()); err != nil { return nil, err } }Also applies to: 306-314
844-845: URL‑escape model in pathUnsafe for models containing reserved chars (e.g., inference profile ids). Use url.PathEscape.
-body, err := provider.completeRequest(ctx, requestBody, fmt.Sprintf("%s/invoke", model), key) +path := url.PathEscape(model) + "/invoke" +body, err := provider.completeRequest(ctx, requestBody, path, key)
🧹 Nitpick comments (8)
ui/app/providers/fragments/apiKeysFormFragment.tsx (4)
43-45: Avoid negative z-index on AlertsUsing -z-10 can push the Alert behind other content and harm a11y/interaction in some layouts. Drop the negative z-index unless there’s a concrete stacking need.
- <Alert variant="default" className="-z-10"> + <Alert variant="default">Also applies to: 279-281
92-102: Weight input should be numeric and robustly parsedtype="string" is invalid. Use number input, guard NaN, and set sensible constraints.
<FormControl> <Input placeholder="1.0" className="w-20" {...field} onChange={(e) => { const v = e.target.value.trim(); - field.onChange(v === "" ? undefined : Number(v)); + if (v === "") return field.onChange(undefined); + const n = Number(v); + field.onChange(Number.isFinite(n) ? n : field.value); }} - type="string" + type="number" + inputMode="decimal" + step="any" + min={0} /> </FormControl>
292-301: Access Key: consider disabling autofill and typo‑preventionWhile not secret alone, it’s sensitive. Reduce autofill and accidental mutations.
<Input placeholder="your-aws-access-key or env.AWS_ACCESS_KEY_ID" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + autoComplete="off" + spellCheck={false} + data-lpignore="true" />
332-339: Session Token should also be maskedSTS session tokens are credentials; mask and disable autofill.
<Input placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + spellCheck={false} + data-lpignore="true" />core/providers/bedrock.go (4)
1150-1153: Don’t overwrite Accept/Content-Type if caller set themLet callers (e.g., streaming) set specialized headers; only apply JSON defaults when absent.
-req.Header.Set("Content-Type", "application/json") -req.Header.Set("Accept", "application/json") +if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") +} +if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/json") +}
1028-1037: Consistently escape path segments (chat)Use PathEscape for the plain model path, and for ARN/profile compound identifiers.
-// Format the path with proper model identifier -path := fmt.Sprintf("%s/converse", model) +// Format the path with proper model identifier +path := url.PathEscape(model) + "/converse" ... - encodedModelIdentifier := url.QueryEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) - path = fmt.Sprintf("%s/converse", encodedModelIdentifier) + encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) + path = encodedModelIdentifier + "/converse" ... -responseBody, err := provider.completeRequest(ctx, requestBody, path, key) +responseBody, err := provider.completeRequest(ctx, requestBody, path, key)Also applies to: 1039-1041
1466-1482: Set streaming Accept to AWS eventstream (and keep it in signature)converse-stream responds with application/vnd.amazon.eventstream. Set Accept before signing and avoid overwrite (see suggested change in signAWSRequest).
req, reqErr := http.NewRequestWithContext(ctx, "POST", fmt.Sprintf("https://bedrock-runtime.%s.amazonaws.com/model/%s", region, path), bytes.NewReader(jsonBody)) ... // Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) +// Expect AWS event stream for streaming endpoints +if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/vnd.amazon.eventstream") +} + // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { return nil, err } }
262-268: Nil‑safety for BedrockKeyConfig (optional)All current Bedrock call sites already guard key.BedrockKeyConfig (core/providers/bedrock.go:834, 975, 1227, 1392). Adding an early nil check inside completeRequest is optional but recommended for future-proofing.
func (provider *BedrockProvider) completeRequest(ctx context.Context, requestBody map[string]interface{}, path string, key schemas.Key) ([]byte, *schemas.BifrostError) { - config := key.BedrockKeyConfig + config := key.BedrockKeyConfig + if config == nil { + return nil, newConfigurationError("bedrock key config is not provided", provider.GetProviderKey()) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/providers/bedrock.go(9 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)docs/quickstart/gateway/provider-configuration.mdx(2 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (9)
- framework/configstore/migrations.go
- core/schemas/account.go
- docs/quickstart/gateway/provider-configuration.mdx
- framework/configstore/sqlite.go
- framework/configstore/utils.go
- ui/lib/types/config.ts
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/schemas.ts
- docs/quickstart/go-sdk/provider-configuration.mdx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/providers/fragments/apiKeysFormFragment.tsx (4)
ui/components/ui/alert.tsx (3)
AlertTitle(41-41)AlertDescription(41-41)Alert(41-41)ui/components/ui/separator.tsx (1)
Separator(43-43)ui/components/ui/form.tsx (4)
FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(7-21)
core/providers/bedrock.go (1)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)
🔇 Additional comments (4)
ui/app/providers/fragments/apiKeysFormFragment.tsx (3)
45-49: Clear dual‑auth guidance — good additionThe Bedrock‑specific alert messaging is concise and correctly sets expectations for IAM vs API key flows.
277-284: IAM role explainer — good UXNicely clarifies when to leave keys empty for role‑based auth.
353-360: LGTM on Bedrock region/ARN/deployments UIRequired Region, ARN, and deployments mapping look correct and align with backend changes.
Also applies to: 373-380, 395-401
core/providers/bedrock.go (1)
1268-1270: Good: Path escaping added for embedding endpointsUsing url.PathEscape for Titan and Cohere model paths prevents SIGv4 mismatches on reserved characters.
Also applies to: 1330-1332
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
47c6332 to
83da938
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/providers/bedrock.go (3)
1028-1042: URL‑escape model and use PathEscape for ARN/profile identifiersUnescaped
modelandurl.QueryEscapeon a path segment can cause signing/route issues. Useurl.PathEscapeconsistently.- // Format the path with proper model identifier - path := fmt.Sprintf("%s/converse", model) + // Format the path with proper model identifier + path := url.PathEscape(model) + "/converse" if key.BedrockKeyConfig.Deployments != nil { if inferenceProfileId, ok := key.BedrockKeyConfig.Deployments[model]; ok { if key.BedrockKeyConfig.ARN != nil { - encodedModelIdentifier := url.QueryEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) - path = fmt.Sprintf("%s/converse", encodedModelIdentifier) + encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) + path = encodedModelIdentifier + "/converse" } } }
1443-1451: Streaming: set event‑stream Accept and avoid overwriting it during signingSet
Accept: application/vnd.amazon.eventstreamandContent-Type: application/jsonfor streaming; ensure signing doesn’t clobberAccept.- // Format the path with proper model identifier for streaming - path := fmt.Sprintf("%s/converse-stream", model) + // Format the path with proper model identifier for streaming + path := url.PathEscape(model) + "/converse-stream" @@ - // Set any extra headers from network config + // Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) - // If Value is set, use API Key authentication - else use IAM role authentication + // Streaming headers + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/vnd.amazon.eventstream") + + // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication - if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { + if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { return nil, err } }Also applies to: 1475-1482
1151-1154: Don’t hard‑set Accept; set defaults only if absentPrevent clobbering caller‑specified
Accept(needed for streaming).- // Set required headers before signing - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/json") + // Set defaults before signing (do not override caller intent) + if req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json") + } + if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "application/json") + }
♻️ Duplicate comments (3)
ui/app/providers/fragments/apiKeysFormFragment.tsx (2)
307-323: Secret Key should be masked and non‑autofillMask the AWS Secret Key and disable autofill; also show the redaction hint.
<FormControl> - <Input + <Input placeholder="your-aws-secret-key or env.AWS_SECRET_ACCESS_KEY" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" + data-1p-ignore="true" /> </FormControl> + {isRedacted(typeof field.value === "string" ? field.value : "") && ( + <div className="text-muted-foreground mt-1 flex items-center gap-1 text-xs"> + <Info className="h-3 w-3" aria-hidden="true" /> + <span>Secret Key is stored securely. Edit to update.</span> + </div> + )}
327-344: Session Token is a credential; mask and disable autofillTreat the session token like a secret.
<FormControl> - <Input + <Input placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" + data-1p-ignore="true" /> </FormControl>core/providers/bedrock.go (1)
306-314: API key path misses JSON headersWhen
key.Value != "",Content-Type/Acceptaren’t set. Some Bedrock routes enforce JSON headers.- // If Value is set, use API Key authentication - else use IAM role authentication - if key.Value != "" { - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) + // If Value is set, use API Key authentication - else use IAM role authentication + if key.Value != "" { + // Default JSON headers for API key path + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else {
🧹 Nitpick comments (2)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
45-45: Mask Bedrock API key in table; avoid rendering full secretDon’t render
key.valueverbatim. Truncate/mask to last 4 and unify label to “IAM role”.- return key.value || key.bedrock_key_config?.access_key || "system IAM"; + return (key.value ? redactKey(key.value) : "") || key.bedrock_key_config?.access_key || "IAM role";Add helper (anywhere in this file/module scope):
function redactKey(s: string, tail: number = 4) { if (!s) return ""; return "•••• " + s.slice(-tail); }ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
53-69: Consider masking provider API keys during entryOptional but recommended: make the generic “API Key” field a password input to reduce shoulder‑surf risk across providers (non‑Vertex).
- <Input placeholder="API Key or env.MY_KEY" type="text" {...field} /> + <Input placeholder="API Key or env.MY_KEY" + type="password" + autoComplete="off" + data-lpignore="true" + data-1p-ignore="true" + {...field} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/providers/bedrock.go(9 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)docs/quickstart/gateway/provider-configuration.mdx(2 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/lib/types/config.ts
- framework/configstore/migrations.go
- framework/configstore/sqlite.go
- ui/lib/types/schemas.ts
- framework/configstore/utils.go
- core/schemas/account.go
- docs/quickstart/go-sdk/provider-configuration.mdx
- docs/quickstart/gateway/provider-configuration.mdx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/providers/fragments/apiKeysFormFragment.tsx (4)
ui/components/ui/alert.tsx (3)
AlertTitle(41-41)AlertDescription(41-41)Alert(41-41)ui/components/ui/separator.tsx (1)
Separator(43-43)ui/components/ui/form.tsx (4)
FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(7-21)
core/providers/bedrock.go (2)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)core/schemas/bifrost.go (2)
EmbeddingInput(126-128)ModelParameters(225-242)
🔇 Additional comments (3)
core/providers/bedrock.go (3)
844-846: Nice: path is now safely escapedGood fix with
url.PathEscape(model) + "/invoke".
1268-1271: Embeddings: consistent PathEscape usageConsistent escaping in Titan/Cohere embedding paths looks good.
Also applies to: 1330-1333
23-28: OK: repo targets Go ≥1.21 — stdlib maps.Copy is safeNearest module (core/go.mod) sets
go 1.24and all repository modules are >=go 1.24(or 1.24.1), so usingmaps.Copyfrom the standard library is supported.
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
83da938 to
9853050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/providers/bedrock.go (3)
306-314: Add JSON headers for API‑key path; guard nil for IAM path; update var name.Without Content‑Type/Accept on the API‑key branch, some endpoints may reject the body. Also ensure
bedrockCfgis non‑nil before signing.// If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { + // Default JSON headers for API-key auth + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { - // Sign the request using either explicit credentials or IAM role authentication - if err := signAWSRequest(ctx, req, config.AccessKey, config.SecretKey, config.SessionToken, region, "bedrock", provider.GetProviderKey()); err != nil { + // Sign the request using either explicit credentials or IAM role authentication + if bedrockCfg == nil { + return nil, newConfigurationError("bedrock key config is not provided", provider.GetProviderKey()) + } + if err := signAWSRequest(ctx, req, bedrockCfg.AccessKey, bedrockCfg.SecretKey, bedrockCfg.SessionToken, region, "bedrock", provider.GetProviderKey()); err != nil { return nil, err } }
1454-1465: Set Bedrock streaming response header; avoid changing Accept after signing.For robust streaming, set
x-amzn-bedrock-accept: application/vnd.amazon.eventstreambefore signing. Optionally setAcceptfor the API‑key branch only; the IAM branch’s signer setsAccept: application/json, so rely onx-amzn-bedrock-acceptthere.// Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) + // Request event-stream response; must be set before signing + req.Header.Set("Content-Type", "application/json") + req.Header.Set("x-amzn-bedrock-accept", "application/vnd.amazon.eventstream") + // If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { + // For API-key auth, also set Accept explicitly + req.Header.Set("Accept", "application/vnd.amazon.eventstream") req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication - if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { + if err := signAWSRequest(ctx, req, key.BedrockKeyConfig.AccessKey, key.BedrockKeyConfig.SecretKey, key.BedrockKeyConfig.SessionToken, region, "bedrock", providerName); err != nil { return nil, err } }Note: do not mutate
Acceptafter callingsignAWSRequest; it alters signed headers. Please verify end‑to‑end that streaming still works when IAM auth is used with onlyx-amzn-bedrock-acceptset.
1826-1840: URL‑escape model identifiers in all cases (fixes signatures for ids containing colons).Currently only the ARN/profile path is escaped; plain
modelisn’t. Bedrock model ids often include:, and leaving them unescaped can cause SigV4 mismatches or routing issues. Escapemodelunconditionally and add nil‑safety for the config access.-func (provider *BedrockProvider) getModelPath(basePath string, model string, key schemas.Key) string { - // Format the path with proper model identifier for streaming - path := fmt.Sprintf("%s/%s", model, basePath) - - if key.BedrockKeyConfig.Deployments != nil { - if inferenceProfileId, ok := key.BedrockKeyConfig.Deployments[model]; ok { - if key.BedrockKeyConfig.ARN != nil { - encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) - path = fmt.Sprintf("%s/%s", encodedModelIdentifier, basePath) - } - } - } - - return path -} +func (provider *BedrockProvider) getModelPath(basePath string, model string, key schemas.Key) string { + // Default: /model/{modelId}/... + path := fmt.Sprintf("%s/%s", url.PathEscape(model), basePath) + + if cfg := key.BedrockKeyConfig; cfg != nil && cfg.Deployments != nil { + if inferenceProfileID, ok := cfg.Deployments[model]; ok && cfg.ARN != nil { + encoded := url.PathEscape(fmt.Sprintf("%s/%s", *cfg.ARN, inferenceProfileID)) + path = fmt.Sprintf("%s/%s", encoded, basePath) + } + } + return path +}
🧹 Nitpick comments (1)
core/providers/bedrock.go (1)
262-269: Avoid nil deref and shadowed import; prefer a clearer name thanconfig.Use a nil‑safe read of region and avoid shadowing the imported
configpackage by renaming the local tobedrockCfg.- config := key.BedrockKeyConfig + bedrockCfg := key.BedrockKeyConfig region := "us-east-1" - if config.Region != nil { - region = *config.Region + if bedrockCfg != nil && bedrockCfg.Region != nil { + region = *bedrockCfg.Region }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/providers/bedrock.go(11 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)docs/quickstart/gateway/provider-configuration.mdx(2 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (10)
- framework/configstore/migrations.go
- ui/lib/types/schemas.ts
- framework/configstore/sqlite.go
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/config.ts
- docs/quickstart/go-sdk/provider-configuration.mdx
- framework/configstore/utils.go
- core/schemas/account.go
- docs/quickstart/gateway/provider-configuration.mdx
- ui/app/providers/fragments/apiKeysFormFragment.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
core/providers/bedrock.go (2)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)core/schemas/bifrost.go (5)
BifrostError(768-777)EmbeddingInput(126-128)ModelParameters(225-242)ModelProvider(38-38)BifrostResponse(445-458)
🔇 Additional comments (8)
core/providers/bedrock.go (8)
844-846: LGTM: unified path + key threading.Using
getModelPath(..., key)and funneling throughcompleteRequestis consistent.
1029-1033: LGTM: chat path uses the same resolver.
1225-1228: LGTM: model‑specific handlers are clearly dispatched.
1234-1234: LGTM: handler now acceptskeyfor auth parity.
1260-1262: LGTM, assuminggetModelPathsafely escapes model ids.See my comment on
getModelPathfor escaping details.
1308-1308: LGTM: cohere embedding handler signature change mirrors titan’s.
1322-1324: LGTM: shared path construction for embeddings.
1435-1435: LGTM: streaming usesconverse-streamvia the same resolver.
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
9853050 to
3292a13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/utils.go (1)
101-124: Bug: invalid range over integer in prepareParams.
for i := range val.NumField()won’t compile; iterate with an index.Apply:
- // Iterate through all fields - for i := range val.NumField() { + // Iterate through all fields + for i := 0; i < val.NumField(); i++ {
♻️ Duplicate comments (3)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
312-319: Mask Secret Key; add non‑autofill (repeat from earlier review)Render as password and disable autofill. Optionally show a redaction hint (same pattern as Vertex).
<Input placeholder="your-aws-secret-key or env.AWS_SECRET_ACCESS_KEY" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" />Optional redaction hint (place just below FormControl):
{isRedacted(typeof field.value === "string" ? field.value : "") && ( <div className="text-muted-foreground mt-1 flex items-center gap-1 text-xs"> <Info className="h-3 w-3" aria-hidden="true" /> <span>Secret Key is stored securely. Edit to update.</span> </div> )}core/providers/bedrock.go (2)
1826-1840: URL‑escape model id when not using ARN/deploymentsEncode the plain model segment too for consistency and safer canonicalization.
func (provider *BedrockProvider) getModelPath(basePath string, model string, key schemas.Key) string { - // Format the path with proper model identifier for streaming - path := fmt.Sprintf("%s/%s", model, basePath) + // Format the path with proper model identifier + encodedModel := url.PathEscape(model) + path := fmt.Sprintf("%s/%s", encodedModel, basePath) if key.BedrockKeyConfig.Deployments != nil { if inferenceProfileId, ok := key.BedrockKeyConfig.Deployments[model]; ok { if key.BedrockKeyConfig.ARN != nil { encodedModelIdentifier := url.PathEscape(fmt.Sprintf("%s/%s", *key.BedrockKeyConfig.ARN, inferenceProfileId)) path = fmt.Sprintf("%s/%s", encodedModelIdentifier, basePath) } } } return path }
306-314: Set JSON headers for API‑key pathWithout SigV4 signing, JSON headers aren’t set; some Bedrock endpoints expect them.
// Set any extra headers from network config setExtraHeadersHTTP(req, provider.networkConfig.ExtraHeaders, nil) -// If Value is set, use API Key authentication - else use IAM role authentication +// If Value is set, use API Key authentication - else use IAM role authentication if key.Value != "" { + // Default JSON headers for API key auth + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) } else { // Sign the request using either explicit credentials or IAM role authentication
🧹 Nitpick comments (11)
framework/configstore/migrations.go (3)
12-16: Inline the trivial error path.This block can be simplified without changing behavior.
- if err := migrationInit(db); err != nil { - return err - } - return nil + return migrationInit(db)
10-11: Fix the doc comment to match the function name.Golint-style comments should start with the identifier.
-// Migrate performs the necessary database migrations. +// triggerMigrations performs the necessary database migrations.
172-175: Preserve the wrapped error with %w instead of formatting the string.Avoid losing the original error type and stack by using
%w.- if err != nil { - return fmt.Errorf("error while running db migration: %s", err.Error()) - } + if err != nil { + return fmt.Errorf("db migration: %w", err) + }docs/quickstart/gateway/provider-configuration.mdx (2)
767-774: Clarify label: say “Bedrock API Key” to avoid confusion with AWS access/secret keysRename the step label so users don’t confuse the Bedrock bearer token with AWS Access/Secret keys.
-2. Set **API Key**: AWS API Key (or leave empty if using IAM role authentication) +2. Set **Bedrock API Key**: Bedrock API key (leave empty if using IAM role authentication)
836-841: Notes: show an API‑key example and align Region requirement with server default
- Add an explicit example referencing the
valuefield for API‑key auth.- Docs say Region is required; server defaults to us-east-1 if unset. Either enforce it in UI or mention the default here to avoid surprises.
-- If using API Key authentication, set `value` field to the API key, else leave it empty for IAM role authentication. +- If using API Key authentication, set `value` to your Bedrock API key (for example, "env.BEDROCK_API_KEY"); leave it empty to use IAM role authentication. - In IAM role authentication, if both `access_key` and `secret_key` are empty, Bifrost uses IAM role authentication from the environment. +- Region is required in the UI; the server currently defaults to "us-east-1" if omitted. Keep these consistent to avoid unexpected regions.Example (non-diff, add anywhere under “Using API” or “Using config.json”):
{ "provider": "bedrock", "keys": [ { "value": "env.BEDROCK_API_KEY", "weight": 1.0, "bedrock_key_config": { "region": "us-east-1" } } ] }ui/app/providers/fragments/apiKeysFormFragment.tsx (3)
43-51: Remove negative z-index on Alerts (content can be hidden/un-clickable)
className="-z-10"can push the alert behind siblings. Drop it for predictable layering.- <Alert variant="default" className="-z-10"> + <Alert variant="default">(Apply to both Alerts in the Bedrock sections.)
Also applies to: 277-284
60-63: Mask API Key and disable autofillAPI keys are secrets; render as password and prevent browser autofill.
- <Input placeholder="API Key or env.MY_KEY" type="text" {...field} /> + <Input + placeholder="API Key or env.MY_KEY" + type="password" + autoComplete="off" + data-lpignore="true" + {...field} + />
333-339: Mask Session Token as wellSession tokens are credentials; mask them and disable autofill.
<Input placeholder="your-aws-session-token or env.AWS_SESSION_TOKEN" value={field.value ?? ""} onChange={field.onChange} onBlur={field.onBlur} name={field.name} ref={field.ref} + type="password" + autoComplete="off" + data-lpignore="true" />core/providers/bedrock.go (3)
262-264: Defensive nil‑check for BedrockKeyConfigAll call sites check for non‑nil, but adding a guard here prevents accidental panics if reused elsewhere.
func (provider *BedrockProvider) completeRequest(ctx context.Context, requestBody map[string]interface{}, path string, key schemas.Key) ([]byte, *schemas.BifrostError) { - config := key.BedrockKeyConfig + config := key.BedrockKeyConfig + if config == nil { + return nil, newConfigurationError("bedrock key config is not provided", provider.GetProviderKey()) + }
1457-1465: Streaming: set Accept for API‑key pathFor converse‑stream, return is AWS event stream; be explicit on the Accept when using API key auth.
if key.Value != "" { - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", key.Value)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/vnd.amazon.eventstream") } else {Note: SigV4 path sets
Accept: application/jsoninsignAWSRequest. If Bedrock ignores this for streaming, fine; otherwise we may need to parameterize signing to support eventstream Accept safely.
1438-1441: Minor: avoid duplicating region resolutionRegion is derived multiple times across methods. Consider a tiny helper (e.g.,
provider.resolveRegion(key) string) for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/providers/anthropic.go(1 hunks)core/providers/bedrock.go(12 hunks)core/providers/openai.go(3 hunks)core/providers/utils.go(1 hunks)core/schemas/account.go(2 hunks)core/utils.go(0 hunks)docs/quickstart/gateway/provider-configuration.mdx(2 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(1 hunks)framework/configstore/sqlite.go(2 hunks)framework/configstore/utils.go(2 hunks)ui/app/providers/fragments/apiKeysFormFragment.tsx(5 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/configstore/sqlite.go
- core/schemas/account.go
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/config.ts
- ui/lib/types/schemas.ts
- framework/configstore/utils.go
- docs/quickstart/go-sdk/provider-configuration.mdx
🧰 Additional context used
🧬 Code graph analysis (3)
core/providers/utils.go (1)
core/schemas/bifrost.go (4)
LLMUsage(461-467)ModelParameters(225-242)ModelProvider(38-38)BifrostResponse(445-458)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ui/components/ui/input.tsx (1)
Input(7-21)
core/providers/bedrock.go (2)
core/schemas/account.go (2)
Key(8-16)BedrockKeyConfig(38-45)core/schemas/bifrost.go (2)
EmbeddingInput(126-128)BifrostResponse(445-458)
🔇 Additional comments (8)
core/providers/openai.go (3)
441-441: Good: capture stream-level ID early.This enables tagging the final chunk with a stable ID without affecting intermediate chunks.
517-520: Set ID once from the first non-empty chunk.Avoids ID churn; simple and correct.
537-539: Plumb ID into the final chunk.Matches the updated utility signature and keeps provider parity.
core/providers/anthropic.go (1)
1201-1203: Final chunk includes messageID.Consistent with OpenAI path; keeps stream termination chunk self-identifying.
core/providers/utils.go (1)
806-835: createBifrostChatCompletionChunkResponse call sites updated — resolved.
rg found usages in core/providers/openai.go:537, core/providers/anthropic.go:1201, and core/providers/bedrock.go:1529; each call passes an ID.ui/app/providers/fragments/apiKeysFormFragment.tsx (3)
24-26: LGTM: clearer Bedrock model placeholdersThe updated examples better match Bedrock model ids.
348-360: Region “Required” vs server defaultUI labels Region as required, but backend defaults to us-east-1 when missing. Decide one behavior and document/enforce consistently.
Would you like me to scan the codebase to confirm all UI validators treat Region as required for Bedrock?
374-381: Verify ARN placeholder formatEnsure the ARN example matches Bedrock inference profile ARN format expected by the backend/path builder.
If needed, I can add a quick validation hint to reduce misconfigurations.
Merge activity
|
akshaydeo
deleted the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
## Add API Key Authentication Support for AWS Bedrock This PR adds support for API Key authentication with AWS Bedrock, allowing users to authenticate using either IAM roles or API keys. Previously, only IAM role authentication was supported. Solves #479 ## Changes - Modified `completeRequest` function to accept the entire `schemas.Key` instead of just the Bedrock config - Added conditional logic to use API Key authentication when `key.Value` is set - Updated the UI to support both authentication methods with clear separation - Added explanatory notes in the schema documentation about authentication options - Moved utility functions `MarshalToString` and `MarshalToStringPtr` from core to configstore package - Updated UI form to include proper placeholders and descriptions for Bedrock authentication ## Type of change - [x] Feature - [x] Refactor - [x] Documentation ## Affected areas - [x] Core (Go) - [x] Providers/Integrations - [x] UI (Next.js) ## How to test 1. Configure a Bedrock provider with API Key authentication: - Add a new Bedrock key in the UI - Enter your API key in the "API Key" field - Leave the IAM credentials (Access Key, Secret Key) empty - Set the required Region 2. Configure a Bedrock provider with IAM role authentication: - Add a new Bedrock key in the UI - Leave the API Key field empty - Either provide Access Key and Secret Key or leave both empty to use the instance IAM role - Set the required Region ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i pnpm test pnpm build ``` ## Breaking changes - [x] No ## Security considerations This PR enhances authentication options for AWS Bedrock but doesn't change the security model. API keys are handled with the same security considerations as other provider keys.
2 participants
Add API Key Authentication Support for AWS Bedrock
This PR adds support for API Key authentication with AWS Bedrock, allowing users to authenticate using either IAM roles or API keys. Previously, only IAM role authentication was supported.
Solves #479
Changes
completeRequestfunction to accept the entireschemas.Keyinstead of just the Bedrock configkey.Valueis setMarshalToStringandMarshalToStringPtrfrom core to configstore packageType of change
Affected areas
How to test
Configure a Bedrock provider with API Key authentication:
Configure a Bedrock provider with IAM role authentication:
Breaking changes
Security considerations
This PR enhances authentication options for AWS Bedrock but doesn't change the security model. API keys are handled with the same security considerations as other provider keys.