Skip to content

Conversation

aayush-sophos
Copy link

@aayush-sophos aayush-sophos commented Jun 13, 2025

User description

Description

What -

  • Added support for multi-account for AWS integration without requiring access to the root account.

Why -

  • This enhancement allows for a multi-account setup where the integration can function across multiple AWS accounts without involving the root account. This is essential for organizations that prefer to limit root account usage for security and governance reasons.

Type of change

Please leave one option from the following and delete the rest:

  • New feature (non-breaking change which adds functionality)

All tests should be run against the port production environment(using a testing org).

Core testing checklist

  • Integration able to create all default resources from scratch
  • Resync finishes successfully
  • Resync able to create entities
  • Resync able to update entities
  • Resync able to detect and delete entities
  • Scheduled resync able to abort existing resync and start a new one
  • Tested with at least 2 integrations from scratch
  • Tested with Kafka and Polling event listeners
  • Tested deletion of entities that don't pass the selector

Integration testing checklist

  • Integration able to create all default resources from scratch
  • Resync able to create entities
  • Resync able to update entities
  • Resync able to detect and delete entities
  • Resync finishes successfully
  • If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the examples folder in the integration directory.
  • If resource kind is updated, run the integration with the example data and check if the expected result is achieved
  • If new resource kind is added or updated, validate that live-events for that resource are working as expected
  • Docs PR link here

Preflight checklist

  • Handled rate limiting
  • Handled pagination
  • Implemented the code in async
  • Support Multi account

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.


PR Type

Enhancement


Description

• Added multi-account AWS support without requiring root account access
• Introduced member_accounts configuration parameter for direct account specification
• Enhanced session manager to handle member accounts independently from organization access


Changes walkthrough 📝

Relevant files
Enhancement
session_manager.py
Enhanced session manager for rootless multi-account support

integrations/aws/aws/session_manager.py

• Added _get_member_accounts() method to retrieve configured member
accounts
• Implemented
_update_available_access_credentials_without_root() for non-root
multi-account access
• Enhanced reset() method to call new credential
update function
• Added concurrent processing for member account
credential updates

+44/-0   
Configuration changes
spec.yaml
Added member accounts configuration parameter                       

integrations/aws/.port/spec.yaml

• Added memberAccounts configuration parameter as optional array
• Set
default value as empty array for member accounts list
• Added
description explaining usage without organizationRoleArn

+6/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @aayush-sophos aayush-sophos requested a review from a team as a code owner June 13, 2025 12:16
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The method catches all exceptions broadly and re-raises them, which may mask specific AWS credential or permission errors that could be handled more gracefully. Consider catching specific AWS exceptions for better error handling.

    except Exception as e:
        logger.error(f"Error fetching member accounts: {str(e)}")
        raise
    Logic Issue

    The method modifies the first element of _aws_accessible_accounts list when the current account matches, but this assumes the list always has at least one element. This could cause an IndexError if the list is empty.

    self._aws_accessible_accounts[0] = account
    continue

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add early return for empty accounts

    Add early return when member_accounts is empty to avoid unnecessary processing.
    This prevents the method from executing when no member accounts are configured,
    improving performance and clarity.

    integrations/aws/aws/session_manager.py [140-170]

     async def _update_available_access_credentials_without_root(self) -> None:
         logger.info("Updating AWS credentials")
    +    member_accounts = self._get_member_accounts()
    +    if not member_accounts:
    +        logger.info("No member accounts configured, skipping credential update")
    +        return
    +        
         async with (
             typing.cast(aioboto3.Session, self._application_session).client(
                 "sts"
             ) as sts_client
         ):
             try:
    -            member_accounts = self._get_member_accounts()
                 tasks = []
                 for account in member_accounts:
                     if account["Id"] == self._application_account_id:
                         # Skip the current account as it is already added
                         # Replace the Temp account details with the current account details
                         self._aws_accessible_accounts[0] = account
                         continue
                     tasks.append(
                         self._assume_role_and_update_credentials(
                             sts_client, account
                         )
                     )
                     if len(tasks) >= CONCURRENT_ACCOUNTS:
                         await asyncio.gather(*tasks)
                         tasks.clear()
                 if tasks:
                     await asyncio.gather(*tasks)
                     tasks.clear()
             except Exception as e:
                 logger.error(f"Error fetching member accounts: {str(e)}")
                 raise
         logger.info(f"Found {len(self._aws_credentials)} AWS accounts")
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly proposes an early return if member_accounts is empty. This improves code clarity and avoids unnecessarily entering the async with block to create an sts_client. It's a good practice for readability and minor performance optimization.

    Low
    • Update

    Copy link

    This pull request is automatically being deployed by Amplify Hosting (learn more).

    Access this pull request here: https://pr-1787.d1ftd8v2gowp8w.amplifyapp.com

    Copy link
    Member

    @mk-armah mk-armah left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Lets bump integration version to 0.3.0-dev in the pyproject.toml

    Comment on lines +150 to +155
    for account in member_accounts:
    if account["Id"] == self._application_account_id:
    # Skip the current account as it is already added
    # Replace the Temp account details with the current account details
    self._aws_accessible_accounts[0] = account
    continue
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    in this case all accessible accounts, including the application account will not have complete data, how are we compensating for this ?

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This case is handled using a function named "_get_member_accounts", which converts the list of member account IDs into a list of dictionaries. Each dictionary contains both the Id and Name of the account.

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I'm actually curios about what resources ingested into Accounts kind look like, it will all be dummy accounts, No ?

    e.g [{"Id": f"{account_id}", "Name": "No name found"}, {"Id": f"{account_id}", "Name": "No name found"} ...]

    since self._aws_accessible_accounts isn't getting updated with actual properties, we aren't getting the full data that describes and Account.

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We will be passing list of accounts like this - OCEAN__INTEGRATION__CONFIG__MEMBER_ACCOUNTS: ["12333222232","232932392832"], which would be actual account ids. Just to support format that is being used, we are converting this list to dict which would have ID and Name as key.

    Got the idea to convert the list to a dictionary from here. - https://github.com/port-labs/ocean/blob/main/integrations/aws/aws/session_manager.py#L56

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I've raised a PR for the Helm chart to add support for arrays in the integration config - port-labs/helm-charts#188

    @aayush-sophos
    Copy link
    Author

    Lets bump integration version to 0.3.0-dev in the pyproject.toml

    Done

    @aayush-sophos aayush-sophos requested a review from mk-armah June 16, 2025 05:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants