[Integration][AWS] | Fix InvalidToken Exceptions Due to Improper Token Refresh Calls #1190
Conversation
…on error when querying resources in a region without permission
…nto aws/port-11555
|
Can you explain how will this solve the expired tokens issue? |
mk-armah
force-pushed
the
aws/port-11555
branch
from
17c9cdf to
7e5e81e
Compare
| logger.info( | ||
| f"Handling global resource {kind} for account {credentials.account_id}" | ||
| ) |
| ) | ||
| async with session.client("sts") as sts_client: | ||
| initial_credentials = await self._refresh_credentials(sts_client) | ||
| refresh_credentials = partial(self._refresh_credentials, sts_client) |
There was a problem hiding this comment.
is using the same sts_client here thread safe?
I mean since we are running in parallel, can sharing the sts_client result in unexpected errors?
| @@ -121,6 +123,10 @@ async def _get_organization_session(self) -> aioboto3.Session | None: | |||
| def _get_account_read_role_name(self) -> str: | |||
| return ocean.integration_config.get("account_read_role_name", "") | |||
|
|
|||
| @staticmethod | |||
| def _assume_role_duration_seconds() -> int: | |||
| return int(ocean.integration_config.get("assume_role_duration", 900)) | |||
There was a problem hiding this comment.
shouldn't this default to ASSUME_ROLE_DURATION_SECONDS?
| async with session.client("sts") as sts_client: | ||
| initial_credentials = await self._refresh_credentials(sts_client) | ||
| refresh_credentials = partial(self._refresh_credentials, sts_client) | ||
| refreshable_credentials = ( | ||
| AioRefreshableCredentials.create_from_metadata( | ||
| metadata=initial_credentials, | ||
| refresh_using=refresh_credentials, | ||
| method="sts-assume-role", | ||
| ) | ||
| ) | ||
| botocore_session = get_session() | ||
| setattr(botocore_session, "_credentials", refreshable_credentials) | ||
| if region: | ||
| botocore_session.set_config_variable("region", region) | ||
| autorefresh_session = aioboto3.Session( | ||
| botocore_session=botocore_session | ||
| ) | ||
| return autorefresh_session |
There was a problem hiding this comment.
this is a pretty complex part of the code what do you think about adding maybe some comments about why this code exists and what is happening baisically?
| ) | ||
|
|
||
| async def create_session_for_each_region( | ||
| async def create_refreshable_session_for_each_region( |
There was a problem hiding this comment.
I don't see why change the name pf this function, from an API standpoint the person using this function it will not matter if it's refreshable or not, WDYT?
Description
What:
We've encountered timeouts and InvalidToken exceptions despite implementing a time-based token refresh strategy designed to refresh the token when it's 80% close to expiry, this occurs arbitrarily.
Why:
The token refresh may not be occurring at the appropriate points in the application.
It appears that the refresh is triggered while an active session is still utilizing the old token.
When the token is replaced during an ongoing session, the active session becomes obsolete, leading to InvalidToken exceptions.
The session refresh call is placed incorrectly within the application flow.
Active sessions are not updated with the new token after a refresh, causing them to fail when attempting to access AWS resources.
How (Proposed Solution):
Review and adjust the placement of the token refresh logic to ensure it does not interfere with active sessions.
Implement a check to verify if a session's token is valid or has a significant amount of time remaining before each resource (kind) retrieval operation.
Ensure that new tokens are only used for new sessions, or gracefully handle the transition for active sessions.
Type of change
Please leave one option from the following and delete the rest:
All tests should be run against the port production environment(using a testing org).
Core testing checklist
Integration testing checklist
examplesfolder in the integration directory.Preflight checklist
Screenshots
Include screenshots from your environment showing how the resources of the integration will look.
API Documentation
Provide links to the API documentation used for this integration.