-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add AWS_USE_IMDS support for ambient IAM credential resolution on Bedrock #2307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ira-at-work
wants to merge
15
commits into
The-PR-Agent:main
Choose a base branch
from
Agrematch:feature/imds-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
616b648
feat: add AWS_USE_IMDS support for ambient IAM credential resolution
2253287
feat: add missing Bedrock model IDs for Sonnet 4.6 and Opus 4.5/4.6
8e906a1
fix: harden IMDS credential handling
1127d9a
test: add unit tests for AWS_USE_IMDS credential support
1b3b9be
fix: address PR review feedback on IMDS credential handling
4eb9027
fix: address self-review findings on IMDS credential handling
9420cdc
test: replace AWS-shaped fake credential values with unambiguous plac…
9e00f35
fix: two IMDS correctness bugs — stale refresh and missing region export
d4265cc
refactor: extract _write_frozen_creds_to_env; make refresh return boo…
5ddd51d
fix: drop unused 'as e' binding in IMDS credential exception handler
1a21ab2
refactor: rename IMDS helpers to include aws qualifier
0b921dc
fix: export AWS_SESSION_TOKEN in non-IMDS static credentials path
f42f1d6
fix: clear stale AWS_SESSION_TOKEN in IMDS-failed static fallback path
1ec24a7
fix: narrow except Exception to BotoCoreError in IMDS paths; fix unus…
0acbbc7
fix: also catch ClientError in IMDS exception handlers; add ClientErr…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,25 @@ | ||
| import json | ||
| import os | ||
|
|
||
| import litellm | ||
| import openai | ||
| import requests | ||
| from litellm import acompletion | ||
| from tenacity import retry, retry_if_exception_type, retry_if_not_exception_type, stop_after_attempt | ||
|
|
||
| from pr_agent.algo import CLAUDE_EXTENDED_THINKING_MODELS, NO_SUPPORT_TEMPERATURE_MODELS, SUPPORT_REASONING_EFFORT_MODELS, USER_MESSAGE_ONLY_MODELS, STREAMING_REQUIRED_MODELS | ||
| from tenacity import (retry, retry_if_exception_type, | ||
| retry_if_not_exception_type, stop_after_attempt) | ||
|
|
||
| from pr_agent.algo import (CLAUDE_EXTENDED_THINKING_MODELS, | ||
| NO_SUPPORT_TEMPERATURE_MODELS, | ||
| STREAMING_REQUIRED_MODELS, | ||
| SUPPORT_REASONING_EFFORT_MODELS, | ||
| USER_MESSAGE_ONLY_MODELS) | ||
| from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler | ||
| from pr_agent.algo.ai_handlers.litellm_helpers import _handle_streaming_response, MockResponse, _get_azure_ad_token, \ | ||
| _process_litellm_extra_body | ||
| from pr_agent.algo.ai_handlers.litellm_helpers import ( | ||
| MockResponse, _get_azure_ad_token, _handle_streaming_response, | ||
| _process_litellm_extra_body) | ||
| from pr_agent.algo.utils import ReasoningEffort, get_version | ||
| from pr_agent.config_loader import get_settings | ||
| from pr_agent.log import get_logger | ||
| import json | ||
|
|
||
| MODEL_RETRIES = 2 | ||
| DUMMY_LITELLM_API_KEY = "dummy_key" # placeholder set when no OpenAI key is configured | ||
|
|
@@ -33,6 +40,9 @@ def __init__(self): | |
| self.azure = False | ||
| self.api_base = None | ||
| self.repetition_penalty = None | ||
| self._aws_imds_mode = False | ||
| self._aws_static_creds = None | ||
| self._aws_imds_fell_back = False | ||
|
|
||
| if get_settings().get("LITELLM.DISABLE_AIOHTTP", False): | ||
| litellm.disable_aiohttp_transport = True | ||
|
|
@@ -41,7 +51,46 @@ def __init__(self): | |
| litellm.openai_key = get_settings().openai.key | ||
| elif 'OPENAI_API_KEY' not in os.environ: | ||
| litellm.api_key = DUMMY_LITELLM_API_KEY | ||
| if get_settings().get("aws.AWS_ACCESS_KEY_ID"): | ||
| if os.environ.get("AWS_USE_IMDS", "").strip().lower() in ("1", "true", "yes"): | ||
| import boto3 | ||
| session = boto3.Session() | ||
| try: | ||
| creds = session.get_credentials() | ||
| if creds: | ||
| frozen = creds.get_frozen_credentials() | ||
| os.environ["AWS_ACCESS_KEY_ID"] = frozen.access_key | ||
| os.environ["AWS_SECRET_ACCESS_KEY"] = frozen.secret_key | ||
| if frozen.token: | ||
| os.environ["AWS_SESSION_TOKEN"] = frozen.token | ||
| elif "AWS_SESSION_TOKEN" in os.environ: | ||
| del os.environ["AWS_SESSION_TOKEN"] | ||
| self._aws_imds_mode = True | ||
| get_logger().info("Using ambient AWS credentials from IMDS/task-role/IRSA") | ||
| else: | ||
| get_logger().warning("AWS_USE_IMDS is set but boto3 found no credentials; falling through to static keys") | ||
| except Exception as e: | ||
| get_logger().error(f"AWS_USE_IMDS: failed to resolve credentials via boto3: {e}; falling through to static keys") | ||
|
ira-at-work marked this conversation as resolved.
Outdated
|
||
| if not os.environ.get("AWS_REGION_NAME") and not get_settings().get("aws.AWS_REGION_NAME"): | ||
| try: | ||
| region = session.region_name | ||
| if region: | ||
| os.environ["AWS_REGION_NAME"] = region | ||
| get_logger().info(f"AWS region resolved from environment: {region}") | ||
| else: | ||
| get_logger().warning("AWS_USE_IMDS: could not determine AWS region; set AWS_REGION_NAME explicitly") | ||
| except Exception as e: | ||
| get_logger().warning(f"AWS_USE_IMDS: failed to resolve region via boto3: {e}") | ||
| if get_settings().get("aws.AWS_ACCESS_KEY_ID"): | ||
| if get_settings().aws.AWS_SECRET_ACCESS_KEY and get_settings().aws.AWS_REGION_NAME: | ||
| self._aws_static_creds = { | ||
| "AWS_ACCESS_KEY_ID": get_settings().aws.AWS_ACCESS_KEY_ID, | ||
| "AWS_SECRET_ACCESS_KEY": get_settings().aws.AWS_SECRET_ACCESS_KEY, | ||
| "AWS_REGION_NAME": get_settings().aws.AWS_REGION_NAME, | ||
| } | ||
| static_token = get_settings().get("aws.AWS_SESSION_TOKEN", None) | ||
| if static_token: | ||
| self._aws_static_creds["AWS_SESSION_TOKEN"] = static_token | ||
| elif get_settings().get("aws.AWS_ACCESS_KEY_ID"): | ||
| assert get_settings().aws.AWS_SECRET_ACCESS_KEY and get_settings().aws.AWS_REGION_NAME, "AWS credentials are incomplete" | ||
| os.environ["AWS_ACCESS_KEY_ID"] = get_settings().aws.AWS_ACCESS_KEY_ID | ||
| os.environ["AWS_SECRET_ACCESS_KEY"] = get_settings().aws.AWS_SECRET_ACCESS_KEY | ||
|
ira-at-work marked this conversation as resolved.
Comment on lines
+124
to
127
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Static token not exported When AWS_USE_IMDS is not set, the static-credentials path exports access key/secret/region but never exports aws.AWS_SESSION_TOKEN, so STS-derived static credentials (that require a session token) will fail. Agent Prompt
|
||
|
|
@@ -108,7 +157,7 @@ def __init__(self): | |
| # Support mistral models | ||
| if get_settings().get("MISTRAL.KEY", None): | ||
| os.environ["MISTRAL_API_KEY"] = get_settings().get("MISTRAL.KEY") | ||
|
|
||
| # Support codestral models | ||
| if get_settings().get("CODESTRAL.KEY", None): | ||
| os.environ["CODESTRAL_API_KEY"] = get_settings().get("CODESTRAL.KEY") | ||
|
|
@@ -120,7 +169,7 @@ def __init__(self): | |
| access_token = _get_azure_ad_token() | ||
| litellm.api_key = access_token | ||
| openai.api_key = access_token | ||
|
|
||
| # Set API base from settings | ||
| self.api_base = get_settings().azure_ad.api_base | ||
| litellm.api_base = self.api_base | ||
|
|
@@ -153,6 +202,37 @@ def __init__(self): | |
| # Models that require streaming | ||
| self.streaming_required_models = STREAMING_REQUIRED_MODELS | ||
|
|
||
| def _refresh_imds_credentials(self): | ||
| """Refresh ambient AWS credentials from boto3 provider chain. Called before each Bedrock call | ||
| to avoid serving stale credentials from long-lived processes (EC2 roles rotate every ~6h).""" | ||
| try: | ||
| import boto3 | ||
| creds = boto3.Session().get_credentials() | ||
| if creds: | ||
| frozen = creds.get_frozen_credentials() | ||
| os.environ["AWS_ACCESS_KEY_ID"] = frozen.access_key | ||
| os.environ["AWS_SECRET_ACCESS_KEY"] = frozen.secret_key | ||
| if frozen.token: | ||
| os.environ["AWS_SESSION_TOKEN"] = frozen.token | ||
| elif "AWS_SESSION_TOKEN" in os.environ: | ||
| del os.environ["AWS_SESSION_TOKEN"] | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
Outdated
|
||
| else: | ||
| get_logger().warning("IMDS credential refresh: boto3 returned no credentials") | ||
| except Exception as e: | ||
| get_logger().warning(f"IMDS credential refresh failed: {e}") | ||
|
|
||
| def _activate_static_aws_fallback(self): | ||
| """Swap process env to static credentials for Bedrock fallback after IMDS failure.""" | ||
| os.environ["AWS_ACCESS_KEY_ID"] = self._aws_static_creds["AWS_ACCESS_KEY_ID"] | ||
| os.environ["AWS_SECRET_ACCESS_KEY"] = self._aws_static_creds["AWS_SECRET_ACCESS_KEY"] | ||
| os.environ["AWS_REGION_NAME"] = self._aws_static_creds["AWS_REGION_NAME"] | ||
| if "AWS_SESSION_TOKEN" in self._aws_static_creds: | ||
| os.environ["AWS_SESSION_TOKEN"] = self._aws_static_creds["AWS_SESSION_TOKEN"] | ||
| elif "AWS_SESSION_TOKEN" in os.environ: | ||
| del os.environ["AWS_SESSION_TOKEN"] | ||
| self._aws_imds_fell_back = True | ||
| get_logger().warning("Bedrock call failed with ambient (IMDS) credentials; retrying with static credentials") | ||
|
|
||
| def prepare_logs(self, response, system, user, resp, finish_reason): | ||
| response_log = response.dict().copy() | ||
| response_log['system'] = system | ||
|
|
@@ -268,6 +348,8 @@ def deployment_id(self): | |
| stop=stop_after_attempt(MODEL_RETRIES), | ||
| ) | ||
| async def chat_completion(self, model: str, system: str, user: str, temperature: float = 0.2, img_path: str = None): | ||
| if self._aws_imds_mode and not self._aws_imds_fell_back and 'bedrock/' in model: | ||
| self._refresh_imds_credentials() | ||
| try: | ||
| resp, finish_reason = None, None | ||
| deployment_id = self.deployment_id | ||
|
|
@@ -419,6 +501,11 @@ async def chat_completion(self, model: str, system: str, user: str, temperature: | |
| get_logger().error(f"Rate limit error during LLM inference: {e}") | ||
| raise | ||
| except openai.APIError as e: | ||
| if (self._aws_imds_mode | ||
| and not self._aws_imds_fell_back | ||
| and self._aws_static_creds | ||
| and 'bedrock/' in model): | ||
| self._activate_static_aws_fallback() | ||
| get_logger().warning(f"Error during LLM inference: {e}") | ||
| raise | ||
| except Exception as e: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.