Skip to content

feat: add cross-account IAM role support for S3 operations and update docs/tests#519

Merged
skxeve merged 6 commits intoBrainPad:masterfrom
nasudadada:feature/s3-cross-account-role
Sep 25, 2025
Merged

feat: add cross-account IAM role support for S3 operations and update docs/tests#519
skxeve merged 6 commits intoBrainPad:masterfrom
nasudadada:feature/s3-cross-account-role

Conversation

@nasudadada
Copy link
Copy Markdown
Contributor

@nasudadada nasudadada commented Sep 24, 2025

Brief

  • Add cross-account IAM role support to S3 modules (role_arn/external_id)
  • Update tests and docs accordingly

Points to Check

  • role_arn must not be combined with keys or profile

Test

  • Local E2E (A→B with external_id):
    • Upload → Exists → Download → DownloadFileDelete → Upload → Delete

Review Limit

  • None

Fixes: #462

@skxeve skxeve self-requested a review September 25, 2025 05:30
@skxeve skxeve added feat A new feature enhancement Improvement to an existing feature and removed feat A new feature labels Sep 25, 2025
@skxeve
Copy link
Copy Markdown
Collaborator

skxeve commented Sep 25, 2025

Hi, it seems that our CI build is failing after the dependency packages were updated.
Would you mind syncing your branch with the latest master and ensuring that all CI checks pass? I appreciate your help.

@nasudadada nasudadada force-pushed the feature/s3-cross-account-role branch from a065ddc to 6528595 Compare September 25, 2025 05:50
@nasudadada
Copy link
Copy Markdown
Contributor Author

Hi, it seems that our CI build is failing after the dependency packages were updated. Would you mind syncing your branch with the latest master and ensuring that all CI checks pass? I appreciate your help.

@skxeve
Thank you for the heads-up. I’ve synced the branch with the latest master and fixed the failing tests caused by the dependency updates.

Copy link
Copy Markdown
Collaborator

@skxeve skxeve left a comment

Choose a reason for hiding this comment

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

Overall, the logic looks good, but I've left a few comments with some refactoring suggestions.
Please take a look and either make the changes or let me know your thoughts on them. I look forward to your response.

Comment thread cliboa/adapter/aws.py Outdated
"Either access_key and secret_key or profile path can be specified."
)

if role_arn and (access_key or secret_key or profile):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understand you'd like to check for a state where only one of three is specified. Is that correct?
In Python, since a bool is also an int, it would be smarter to check the three boolean conditions at once, like A + B + C == 1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Your interpretation is correct.
I updated the validation to allow “at most one” auth method (zero or one). access_key+secret_key count as one, and profile and role_arn each count as one. Does this match your intent?

Comment thread cliboa/adapter/aws.py
aws_session_token=credentials["SessionToken"],
)

def _get_cross_account_resource(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This function seems to share a lot of logic with the preceding _get_cross_account_client function.
Wouldn't it be better to refactor them into a single function and use a parameter to handle the different cases? If there's a reason for keeping them separate, I'd appreciate it if you could let me know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for calling this out — you’re right, these two functions share most of the logic.
I’ve refactored to extract the common part into a helper that builds the cross-account session, and then use a single function that switches by a parameter.

@nasudadada nasudadada requested a review from skxeve September 25, 2025 08:53
@skxeve
Copy link
Copy Markdown
Collaborator

skxeve commented Sep 25, 2025

LGTM! Thank you for your contribution.

@skxeve skxeve merged commit 1c9c801 into BrainPad:master Sep 25, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for cross-account IAM role access to S3.

2 participants