Skip to content
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

Add support for Aio refreshable creds for assume role #1304

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

pankajkoti
Copy link
Collaborator

@pankajkoti pankajkoti commented Aug 14, 2023

Currently, for assume_role method, the hook gets a one time credentials
and when they expire the sensor fails. Thus, we leverage refreshable
credentials to support the sensor and not fail when using assume_role
method with the role_arn type connections.

closes: #1292

@pankajkoti
Copy link
Collaborator Author

Verified that credentials get refreshed by having added some temporary logs and tweaking around expiration.

Screenshot 2023-08-14 at 10 14 46 PM

@pankajkoti
Copy link
Collaborator Author

will work next on tests

@pankajkoti
Copy link
Collaborator Author

Cc: @abhishekbhakat

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (08a68cb) 98.57% compared to head (22b7f5d) 98.58%.

❗ Current head 22b7f5d differs from pull request most recent head ada2cbb. Consider uploading reports for the commit ada2cbb to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1304   +/-   ##
=======================================
  Coverage   98.57%   98.58%           
=======================================
  Files          91       91           
  Lines        5404     5429   +25     
=======================================
+ Hits         5327     5352   +25     
  Misses         77       77           
Files Changed Coverage Δ
astronomer/providers/amazon/aws/hooks/base_aws.py 100.00% <100.00%> (ø)
astronomer/providers/amazon/aws/triggers/s3.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pankajkoti pankajkoti force-pushed the 1292-aio-refreshable-creds branch 4 times, most recently from 68f1ba1 to cfe1852 Compare August 15, 2023 09:05
@pankajkoti pankajkoti marked this pull request as ready for review August 16, 2023 05:50
Copy link
Contributor

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

LGTM.

  1. we should test the existing aws integration test on Astro once
  2. what do you think about adding an integration test or using some existing one to use this method of authentication?

@pankajkoti
Copy link
Collaborator Author

pankajkoti commented Aug 16, 2023

LGTM.

  1. we should test the existing aws integration test on Astro once
  2. what do you think about adding an integration test or using some existing one to use this method of authentication?
  1. I tested existing AWS integration locally on runtime image without the role_arn i.e. existing AWS integration and it runs fine too.
  2. Yes perhaps, we can use the same example DAG with a different connection using role_arn. Will create a follow-up PR for this.

@pankajkoti
Copy link
Collaborator Author

@abhishekbhakat would you like to test this PR?

@pankajkoti pankajkoti merged commit 5c0066e into main Aug 16, 2023
@pankajkoti pankajkoti deleted the 1292-aio-refreshable-creds branch August 16, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AioRefreshableCredentials for AwsBaseHookAsync
3 participants