feat: bedrock batch role arn in key config - #5474
Conversation
📝 WalkthroughWalkthroughChangesBedrock batch role configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant ConfigStore
participant BatchCreate
participant AWSBedrock
ProviderConfig->>ConfigStore: save batch_role_arn
ConfigStore->>BatchCreate: load configured BatchRoleARN
BatchCreate->>AWSBedrock: create batch job with selected roleArn
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/bedrock.go (2)
2977-2980: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the missing-role error message.
The new key-level
batch_role_arnsource is omitted from the remediation text, which incorrectly tells operators to configure onlyextra_params. Mention bothbedrock_key_config.batch_role_arnandextra_params.role_arn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/bedrock.go` around lines 2977 - 2980, Update the missing-role error message in the roleArn validation to direct operators to both bedrock_key_config.batch_role_arn and extra_params.role_arn, and use the same complete remediation text for provider.logger.Error and NewBifrostOperationError.
2959-2975: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the configured batch role cannot be resolved.
When
BatchRoleARNis configured as anenv./vault reference butGetValue()returns empty, this code falls through to the client-controlledrequest.ExtraParams["role_arn"]. That violates the contract that the configured batch role takes priority and allows callers to select a different IAM role. Treat a configuredBatchRoleARNas authoritative: if resolution is empty, return an error; consult requestrole_arnonly when the key field is genuinely unset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/bedrock.go` around lines 2959 - 2975, The batch-role resolution around BatchRoleARN must fail closed when a configured value resolves to empty. Distinguish BatchRoleARN being unset from being configured: if configured, use its GetValue result and return an error when empty; only consult request.ExtraParams["role_arn"] when BatchRoleARN is genuinely nil, preserving the existing fallback to RoleARN otherwise.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/provider_keys.go (1)
386-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd batch-role coverage to masked-update tests.
Extend
TestMergeUpdatedKey_ProviderConfigMaskedPreviewswith a maskedBatchRoleARNfixture/assertion and anenv.reference case. Without this, a regression could overwrite or drop the stored batch role during edits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/provider_keys.go` around lines 386 - 411, Extend TestMergeUpdatedKey_ProviderConfigMaskedPreviews with a masked BatchRoleARN fixture and assertion, covering preservation of the stored batch-role value during updates. Add an env. reference case for BatchRoleARN, matching the existing Bedrock provider configuration test patterns.framework/configstore/migrations.go (1)
456-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd migration regression coverage.
Please add or verify a test using the existing legacy
config_keysfixture that asserts the column is created, the migration is idempotent, and rollback removes it. This protects the registered migration step and its reversible schema path.Also applies to: 10891-10914
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/migrations.go` at line 456, Add regression coverage for the registered migrationAddBedrockBatchRoleARNColumn step using the existing legacy config_keys fixture. Assert the new column is created, rerunning the migration is idempotent, and rollback removes the column, covering both forward and reversible schema paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 2977-2980: Update the missing-role error message in the roleArn
validation to direct operators to both bedrock_key_config.batch_role_arn and
extra_params.role_arn, and use the same complete remediation text for
provider.logger.Error and NewBifrostOperationError.
- Around line 2959-2975: The batch-role resolution around BatchRoleARN must fail
closed when a configured value resolves to empty. Distinguish BatchRoleARN being
unset from being configured: if configured, use its GetValue result and return
an error when empty; only consult request.ExtraParams["role_arn"] when
BatchRoleARN is genuinely nil, preserving the existing fallback to RoleARN
otherwise.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Line 456: Add regression coverage for the registered
migrationAddBedrockBatchRoleARNColumn step using the existing legacy config_keys
fixture. Assert the new column is created, rerunning the migration is
idempotent, and rollback removes the column, covering both forward and
reversible schema paths.
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 386-411: Extend TestMergeUpdatedKey_ProviderConfigMaskedPreviews
with a masked BatchRoleARN fixture and assertion, covering preservation of the
stored batch-role value during updates. Add an env. reference case for
BatchRoleARN, matching the existing Bedrock provider configuration test
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85bed944-002d-4b27-a9b8-9ab79bdeb099
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/schemas/account.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/config.schema.jsonui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/lib/schemas/providerForm.tsui/lib/types/schemas.ts
Merge activity
|
## Summary Adds a server-configured `batch_role_arn` field to the Bedrock key configuration, allowing operators to pin the IAM service role used for Bedrock batch jobs at the server level rather than relying on clients to supply it via `role_arn` in request extra params. When set, the server-side value takes priority over any client-provided `role_arn`. ## Changes - Added `BatchRoleARN *SecretVar` to `BedrockKeyConfig` in `schemas/account.go`, stored under the JSON key `batch_role_arn` and kept separate from the STS AssumeRole identity (`bedrock_role_arn`). - Updated `BatchCreate` in `bedrock.go` so that `key.BedrockKeyConfig.BatchRoleARN` is resolved first; the client-supplied `role_arn` in `ExtraParams` is only used as a fallback when the server value is absent. - Added a database migration (`add_bedrock_batch_role_arn_column`) that adds the `bedrock_batch_role_arn` column to `config_keys`, with rollback support. - Wired `BedrockBatchRoleARN` through all RDB read/write paths (`tableKeyFromSchemaKey`, `UpdateProvidersConfig`, `UpdateProvider`, `AddProvider`) and through `BeforeSave`/`AfterFind` hooks including encryption and decryption. - Updated the `AfterFind` Bedrock config reconstruction condition to include `BedrockBatchRoleARN`. - Added `BatchRoleARN` to the `mergeUpdatedKey` preserve logic in the HTTP handler so partial updates do not accidentally clear the field. - Added `batch_role_arn` to the config JSON schema with a description noting its priority semantics and `env.` prefix support. - Added `batch_role_arn` to the Zod schemas (`providerForm.ts`, `schemas.ts`) and rendered a **Batch Role ARN** input field in the UI form, visible only when the provider supports the batch API. - Added redaction support for `BatchRoleARN` in `clientconfig.go`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` **Manual validation:** 1. Configure a Bedrock provider key with `batch_role_arn` set (either as a literal ARN or via `env.AWS_BATCH_ROLE_ARN`). 2. Submit a batch create request that also includes `role_arn` in `extra_params`. 3. Confirm that the server-configured `batch_role_arn` is used and the client-supplied value is ignored. 4. Remove `batch_role_arn` from the key config and resubmit; confirm the client-supplied `role_arn` is now used. 5. Verify the value is stored encrypted in the database and appears redacted in API responses. **New config field:** | Field | JSON key | Description | |---|---|---| | `BatchRoleARN` | `batch_role_arn` | Service role ARN Bedrock assumes for batch S3 access. Supports `env.` prefix. Takes priority over client-supplied `role_arn`. | ## Breaking changes - [ ] Yes - [x] No ## Security considerations `BatchRoleARN` is treated as a secret: it is encrypted at rest via the existing `encryptSecretVarPtr`/`decryptSecretVarPtr` pipeline and redacted in API responses, consistent with other credential fields such as `RoleARN` and `ExternalID`. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a server-configured `batch_role_arn` field to the Bedrock key configuration, allowing operators to pin the IAM service role used for Bedrock batch jobs at the server level rather than relying on clients to supply it via `role_arn` in request extra params. When set, the server-side value takes priority over any client-provided `role_arn`. ## Changes - Added `BatchRoleARN *SecretVar` to `BedrockKeyConfig` in `schemas/account.go`, stored under the JSON key `batch_role_arn` and kept separate from the STS AssumeRole identity (`bedrock_role_arn`). - Updated `BatchCreate` in `bedrock.go` so that `key.BedrockKeyConfig.BatchRoleARN` is resolved first; the client-supplied `role_arn` in `ExtraParams` is only used as a fallback when the server value is absent. - Added a database migration (`add_bedrock_batch_role_arn_column`) that adds the `bedrock_batch_role_arn` column to `config_keys`, with rollback support. - Wired `BedrockBatchRoleARN` through all RDB read/write paths (`tableKeyFromSchemaKey`, `UpdateProvidersConfig`, `UpdateProvider`, `AddProvider`) and through `BeforeSave`/`AfterFind` hooks including encryption and decryption. - Updated the `AfterFind` Bedrock config reconstruction condition to include `BedrockBatchRoleARN`. - Added `BatchRoleARN` to the `mergeUpdatedKey` preserve logic in the HTTP handler so partial updates do not accidentally clear the field. - Added `batch_role_arn` to the config JSON schema with a description noting its priority semantics and `env.` prefix support. - Added `batch_role_arn` to the Zod schemas (`providerForm.ts`, `schemas.ts`) and rendered a **Batch Role ARN** input field in the UI form, visible only when the provider supports the batch API. - Added redaction support for `BatchRoleARN` in `clientconfig.go`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` **Manual validation:** 1. Configure a Bedrock provider key with `batch_role_arn` set (either as a literal ARN or via `env.AWS_BATCH_ROLE_ARN`). 2. Submit a batch create request that also includes `role_arn` in `extra_params`. 3. Confirm that the server-configured `batch_role_arn` is used and the client-supplied value is ignored. 4. Remove `batch_role_arn` from the key config and resubmit; confirm the client-supplied `role_arn` is now used. 5. Verify the value is stored encrypted in the database and appears redacted in API responses. **New config field:** | Field | JSON key | Description | |---|---|---| | `BatchRoleARN` | `batch_role_arn` | Service role ARN Bedrock assumes for batch S3 access. Supports `env.` prefix. Takes priority over client-supplied `role_arn`. | ## Breaking changes - [ ] Yes - [x] No ## Security considerations `BatchRoleARN` is treated as a secret: it is encrypted at rest via the existing `encryptSecretVarPtr`/`decryptSecretVarPtr` pipeline and redacted in API responses, consistent with other credential fields such as `RoleARN` and `ExternalID`. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a server-configured `batch_role_arn` field to the Bedrock key configuration, allowing operators to pin the IAM service role used for Bedrock batch jobs at the server level rather than relying on clients to supply it via `role_arn` in request extra params. When set, the server-side value takes priority over any client-provided `role_arn`. ## Changes - Added `BatchRoleARN *SecretVar` to `BedrockKeyConfig` in `schemas/account.go`, stored under the JSON key `batch_role_arn` and kept separate from the STS AssumeRole identity (`bedrock_role_arn`). - Updated `BatchCreate` in `bedrock.go` so that `key.BedrockKeyConfig.BatchRoleARN` is resolved first; the client-supplied `role_arn` in `ExtraParams` is only used as a fallback when the server value is absent. - Added a database migration (`add_bedrock_batch_role_arn_column`) that adds the `bedrock_batch_role_arn` column to `config_keys`, with rollback support. - Wired `BedrockBatchRoleARN` through all RDB read/write paths (`tableKeyFromSchemaKey`, `UpdateProvidersConfig`, `UpdateProvider`, `AddProvider`) and through `BeforeSave`/`AfterFind` hooks including encryption and decryption. - Updated the `AfterFind` Bedrock config reconstruction condition to include `BedrockBatchRoleARN`. - Added `BatchRoleARN` to the `mergeUpdatedKey` preserve logic in the HTTP handler so partial updates do not accidentally clear the field. - Added `batch_role_arn` to the config JSON schema with a description noting its priority semantics and `env.` prefix support. - Added `batch_role_arn` to the Zod schemas (`providerForm.ts`, `schemas.ts`) and rendered a **Batch Role ARN** input field in the UI form, visible only when the provider supports the batch API. - Added redaction support for `BatchRoleARN` in `clientconfig.go`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` **Manual validation:** 1. Configure a Bedrock provider key with `batch_role_arn` set (either as a literal ARN or via `env.AWS_BATCH_ROLE_ARN`). 2. Submit a batch create request that also includes `role_arn` in `extra_params`. 3. Confirm that the server-configured `batch_role_arn` is used and the client-supplied value is ignored. 4. Remove `batch_role_arn` from the key config and resubmit; confirm the client-supplied `role_arn` is now used. 5. Verify the value is stored encrypted in the database and appears redacted in API responses. **New config field:** | Field | JSON key | Description | |---|---|---| | `BatchRoleARN` | `batch_role_arn` | Service role ARN Bedrock assumes for batch S3 access. Supports `env.` prefix. Takes priority over client-supplied `role_arn`. | ## Breaking changes - [ ] Yes - [x] No ## Security considerations `BatchRoleARN` is treated as a secret: it is encrypted at rest via the existing `encryptSecretVarPtr`/`decryptSecretVarPtr` pipeline and redacted in API responses, consistent with other credential fields such as `RoleARN` and `ExternalID`. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds a server-configured
batch_role_arnfield to the Bedrock key configuration, allowing operators to pin the IAM service role used for Bedrock batch jobs at the server level rather than relying on clients to supply it viarole_arnin request extra params. When set, the server-side value takes priority over any client-providedrole_arn.Changes
BatchRoleARN *SecretVartoBedrockKeyConfiginschemas/account.go, stored under the JSON keybatch_role_arnand kept separate from the STS AssumeRole identity (bedrock_role_arn).BatchCreateinbedrock.goso thatkey.BedrockKeyConfig.BatchRoleARNis resolved first; the client-suppliedrole_arninExtraParamsis only used as a fallback when the server value is absent.add_bedrock_batch_role_arn_column) that adds thebedrock_batch_role_arncolumn toconfig_keys, with rollback support.BedrockBatchRoleARNthrough all RDB read/write paths (tableKeyFromSchemaKey,UpdateProvidersConfig,UpdateProvider,AddProvider) and throughBeforeSave/AfterFindhooks including encryption and decryption.AfterFindBedrock config reconstruction condition to includeBedrockBatchRoleARN.BatchRoleARNto themergeUpdatedKeypreserve logic in the HTTP handler so partial updates do not accidentally clear the field.batch_role_arnto the config JSON schema with a description noting its priority semantics andenv.prefix support.batch_role_arnto the Zod schemas (providerForm.ts,schemas.ts) and rendered a Batch Role ARN input field in the UI form, visible only when the provider supports the batch API.BatchRoleARNinclientconfig.go.Type of change
Affected areas
How to test
Manual validation:
batch_role_arnset (either as a literal ARN or viaenv.AWS_BATCH_ROLE_ARN).role_arninextra_params.batch_role_arnis used and the client-supplied value is ignored.batch_role_arnfrom the key config and resubmit; confirm the client-suppliedrole_arnis now used.New config field:
BatchRoleARNbatch_role_arnenv.prefix. Takes priority over client-suppliedrole_arn.Breaking changes
Security considerations
BatchRoleARNis treated as a secret: it is encrypted at rest via the existingencryptSecretVarPtr/decryptSecretVarPtrpipeline and redacted in API responses, consistent with other credential fields such asRoleARNandExternalID.Checklist
docs/contributing/README.mdand followed the guidelines