-
Notifications
You must be signed in to change notification settings - Fork 194
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
Glue: Allow for assuming role for Glue #1299
base: main
Are you sure you want to change the base?
Conversation
) | ||
refreshable_credentials = DeferredRefreshableCredentials( | ||
method="assume-role", | ||
refresh_using=fetcher.fetch_credentials, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm relying on calling an in-house webservice that will provide new credentials. As I see it, this implementation will not allow a custom implementation of refresh_using
that would enable me to call the webservice.
) | ||
from botocore.session import Session as BotoSession | ||
|
||
botocore_session = BotoSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about accepting a boto3.Session as input? Then the client can configure it as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thanks @cshenrik for the quick reply!
I think we could do both, so you can also configure the Glue client similar to the S3 one (it uses similar properties).
@HonahX @kevinjqliu How strong are we on only passing str
's? Otherwise, I think we might start jumping through hoops to get it working (eg. do something similar as when loading a custom FileIO).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im leaning towards loading custom clients instead of passing everything through properties. Every time we add another property, we need to map a name, a possible default, and pass to the underlying object.
The logic above is already pretty complicated, might as well split it out and call it something like RefreshableGlueClient
credentials = Credentials( | ||
access_key=get_first_property_value(properties, GLUE_ACCESS_KEY_ID, AWS_ACCESS_KEY_ID), | ||
secret_key=get_first_property_value(properties, GLUE_SECRET_ACCESS_KEY, AWS_SECRET_ACCESS_KEY), | ||
token=get_first_property_value(properties, GLUE_SESSION_TOKEN, AWS_SESSION_TOKEN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took me a long time to find this, but it looks like token
is correct here
https://github.com/boto/botocore/blob/179e8b5361cd83d4c4acdf0c1bc1708b4a6966e9/botocore/session.py#L944-L948
) | ||
from botocore.session import Session as BotoSession | ||
|
||
botocore_session = BotoSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im leaning towards loading custom clients instead of passing everything through properties. Every time we add another property, we need to map a name, a possible default, and pass to the underlying object.
The logic above is already pretty complicated, might as well split it out and call it something like RefreshableGlueClient
@@ -61,7 +61,7 @@ | |||
AWS_SECRET_ACCESS_KEY = "client.secret-access-key" | |||
AWS_SESSION_TOKEN = "client.session-token" | |||
AWS_ROLE_ARN = "aws.role-arn" | |||
AWS_SESSION_NAME = "aws.session-name" | |||
AWS_ROLE_SESSION_NAME = "aws.role-session-name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is originally from #1296, which has not been released yet. So we don't need to worry about backwards compatibility
Fixes #1104
PTAL @BTheunissen @cshenrik