[HOTFIX] Add IAM Role / Instance Profile auth mode to AWS Bedrock adapter#1944
Conversation
Match the S3/MinIO connector pattern: when AWS access keys are left blank on the Bedrock LLM and embedding adapter forms, drop them from the kwargs dict so boto3's default credential chain handles authentication. This unlocks IAM role / instance profile / IRSA / AWS Profile scenarios on hosts that already have ambient AWS credentials (e.g. EKS workers with IRSA, EC2 with an instance profile). - llm1/static/bedrock.json: clarify access-key descriptions to mention IRSA and instance profile (already non-required at v0.163.2 base). - embedding1/static/bedrock.json: drop aws_access_key_id and aws_secret_access_key from top-level required; same description fix; expose aws_profile_name for parity with the LLM form. - base1.py: AWSBedrockLLMParameters and AWSBedrockEmbeddingParameters now strip empty access-key values from the validated kwargs before returning, so empty strings don't override boto3's default chain. AWSBedrockEmbeddingParameters fields gain explicit None defaults and an aws_profile_name field. Backward-compatible: existing adapters with access keys filled in continue to work unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an explicit `auth_type` selector with two options, making the auth choice clear to users: - "Access Keys" (default): existing flow, keys required - "IAM Role / Instance Profile (on-prem AWS only)": no fields; relies on boto3's default credential chain (IRSA on EKS, task role on ECS, instance profile on EC2). Description on the selector explicitly notes this option is only for AWS-hosted Unstract deployments. The form-only auth_type field is stripped before LiteLLM validation in both AWSBedrockLLMParameters.validate() and AWSBedrockEmbeddingParameters. validate(). Empty access keys continue to be stripped so boto3 falls through to the default chain even when the access_keys arm is selected without values (matches the S3/MinIO connector pattern). Backward-compatible: legacy adapters without auth_type behave as "Access Keys" mode (the default), and existing keys are forwarded unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Adds shared _resolve_bedrock_aws_credentials helper; correctly unconditionally drops keys in iam_role mode, enforces non-blank in access_keys mode, and preserves legacy leniency — fixes the previously flagged stale-key bug. |
| unstract/sdk1/src/unstract/sdk1/adapters/embedding1/static/bedrock.json | Removes access key fields from top-level required, adds auth_type enum with dependencies/oneOf block that conditionally renders credentials; structure mirrors the LLM schema correctly. |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/bedrock.json | Moves access key fields into dependencies/oneOf under auth_type; auth_type defaults to access_keys preserving existing form behaviour for all existing adapter configurations. |
| unstract/sdk1/tests/test_bedrock_adapter.py | Comprehensive new test file covering all auth matrix permutations (iam_role, access_keys, legacy) for both LLM and embedding adapters, including whitespace-key rejection and stale-key regression. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[adapter_metadata submitted] --> B{auth_type present?}
B -- No / null --> C[Legacy mode: strip blank/None keys only]
B -- access_keys --> D{aws_access_key_id & aws_secret_access_key non-blank?}
B -- iam_role --> E[Unconditionally drop aws_access_key_id & aws_secret_access_key]
B -- other --> F[Raise ValueError: Unknown auth_type]
D -- Yes --> G[Forward keys to LiteLLM]
D -- No --> H[Raise ValueError: key is required]
C --> I[boto3 default credential chain takes over for missing keys]
E --> I
G --> J[LiteLLM / boto3 Authenticate with explicit keys]
I --> K[boto3: IRSA / task role / instance profile / env vars / profile]
Reviews (2): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile
jaseemjaskp
left a comment
There was a problem hiding this comment.
Comprehensive PR review via PR Review Toolkit (Comment Analyzer, PR Test Analyzer, Silent Failure Hunter, Type Design Analyzer, Code Reviewer, Code Simplifier). The greptile P1 on base1.py:548 is acknowledged and not duplicated; the findings below extend it (embedding-side parity) and surface separate concerns.
Priority distribution: 4 P0 (silent-failure / correctness), 3 P1 (structural / coverage), 4 P2 (style / docs).
Fixes the P0/P1 issues raised by greptile-apps and jaseemjaskp on PR #1944. Behaviour fixes: - Stale-key leak in IAM Role mode: switching an existing adapter from Access Keys to IAM Role would carry truthy stored access keys through the strip-empty-only loop, so boto3 silently authenticated with the old long-lived credentials instead of falling through to the host's IRSA / instance-profile identity. Both LLM and embedding paths were affected. - Silent acceptance of unknown auth_type: a typo (e.g. "access_key") or a malformed payload from a non-UI client passed through the dict comprehension untouched, with no enum guard. - Cross-field validation gap: explicit Access Keys mode with blank or whitespace-only values silently fell through to the default credential chain instead of surfacing the misconfiguration. Implementation: - Add a module-level _resolve_bedrock_aws_credentials helper used by both AWSBedrockLLMParameters.validate() and AWSBedrock EmbeddingParameters.validate(), so the auth-type contract is expressed once. - Validates auth_type against an allowlist (None | "access_keys" | "iam_role"); raises ValueError on anything else. - iam_role: unconditionally drops aws_access_key_id and aws_secret_access_key. - access_keys (explicit): requires non-blank values; raises ValueError if either is empty or whitespace-only. - Legacy (auth_type absent): retains the lenient strip behaviour so pre-PR adapter configurations continue to deserialise unchanged. - Restore aws_region_name as required (no `= None` default) on AWSBedrockEmbeddingParameters; only credentials may legitimately be absent. - Drop the orphan aws_profile_name field from embedding1/static/bedrock.json: it was added for parity with the LLM form but lives outside the auth_type oneOf and contradicts the selector's "no further input" semantics. The LLM form already had aws_profile_name pre-PR and is left alone for backwards compatibility. Tests: - New tests/test_bedrock_adapter.py covers 15 cases across LLM and embedding adapters: legacy-no-auth-type, explicit access_keys with valid/blank/whitespace keys, iam_role with stale/no keys, unknown auth_type rejection, cross-field validation, and preservation of unrelated params (model_id, aws_profile_name, region, thinking). Skipped (P2 nice-to-have): - Comment-scope clarification, MinIO reference rewording, validate-mutates-caller'\''s-dict, and the LLM form description nit about aws_profile_name visibility. These don'\''t change behaviour and can be addressed in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
|



What
Adds an Authentication Type selector to the AWS Bedrock LLM and embedding adapter forms with two options:
aws_access_key_idandaws_secret_access_keyboto3's default credential chainWhen the IAM Role mode is selected, the form-only
auth_typefield is stripped before LiteLLM validation, and any empty access keys are dropped from the validated kwargs soboto3falls through to its default credential chain (IRSA on EKS, task role on ECS, instance profile on EC2, AWS Profile, env vars).Why
Customers running Unstract on AWS infrastructure (specifically EKS with IRSA) cannot — and don't want to — paste long-lived AWS access keys into the adapter form. The pod's service account already has a federated identity that
boto3can use to call Bedrock; the adapter form just needs to accept blank credentials so the ambient identity takes over.The S3/MinIO connector at
unstract/connectors/src/unstract/connectors/filesystems/minio/minio.pyalready implements this idiom for S3:This PR applies the same pattern to the Bedrock adapter, with the addition of an explicit
auth_typeselector to make the user's choice clear in the UI.How
unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/bedrock.jsonAdds an
auth_typeenum with two options (access_keysdefault,iam_role) and adependencies/oneOfblock that:aws_access_key_id+aws_secret_access_key(both required) whenauth_type == access_keysauth_type == iam_roleThe selector's description explicitly notes the IRSA / instance-profile mode is for AWS-hosted Unstract deployments only.
unstract/sdk1/src/unstract/sdk1/adapters/embedding1/static/bedrock.jsonSame restructure. Drops
aws_access_key_idandaws_secret_access_keyfrom top-levelrequired. Addsaws_profile_namefor parity with the LLM form.unstract/sdk1/src/unstract/sdk1/adapters/base1.pyIn both
AWSBedrockLLMParameters.validate()andAWSBedrockEmbeddingParameters.validate():auth_typefield from validation metadata before Pydantic dumpaws_access_key_id/aws_secret_access_keyfrom the validated kwargs soboto3's default chain takes over (mirrors the S3/MinIO connector idiom)AWSBedrockEmbeddingParametersfields gain explicitNonedefaults so the access-key fields are truly optional in either mode (parity withAWSBedrockLLMParameterswhich already had this from earlier work).Can this PR break any existing features?
No. Backward-compatible across every scenario:
auth_typefield stored)auth_typedefaults toaccess_keys; keys forwarded toboto3unchangedboto3boto3default credential chain takes overaws_profile_namesetauth_type)enable_thinking/model_id(Application Inference Profile)End-to-end verified against real AWS Bedrock during development:
aws_role_name=arn:...:role/BedrockInvokeRolewith bootstrap creds in env:STS:AssumeRolesucceeds,Bedrock InvokeModelsucceeds with temp creds (token usage returned).auth_type=iam_rolewith no keys + ambient creds in env:boto3default chain picks up the env vars; Bedrock call signs and routes correctly.Relevant Docs
unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py