Fix assume role if user explicit set credentials#26946
Conversation
52d72e6 to
e8a134d
Compare
There was a problem hiding this comment.
Do you have a doc link covers this (the fact that only region name is required and adding other kwargs can cause issues) that you can place here in a comment?
There was a problem hiding this comment.
Unfortunately not, this something which not well covered in boto3 documentation.
Initially boto3.session.Session create low-level botocore session or use provided botocore.session and after that it applied explicit credentials if it provided.
botocore has only short info about itself in documentation which not cover how to work with their session. Let me refer to code boto3 and botocore
boto3.session.Session init constructor
if aws_access_key_id or aws_secret_access_key or aws_session_token:
self._session.set_credentials(
aws_access_key_id, aws_secret_access_key, aws_session_token
)botocore.session.set_credentials
When we create botocore session for assume_role we use a bit hacky approach (access to private methods/properties)
airflow/airflow/providers/amazon/aws/hooks/base_aws.py
Lines 150 to 153 in 8e2e80a
So if we provide:
aws_access_key_idoraws_secret_access_keyoraws_session_tokenit will replace assumed credentials in botocore session. We already use this credentials when assume roleprofile_name- might be nothing bad happen but better do not provide it, since we already use it during session creationregion_name- Nothing bad happen, we use exactly the same region_name as user provide for initial and assume_role sessions. For initial sessionregion_nameonly affect to endpoint for STS
There was a problem hiding this comment.
Hmm, okay, I don't really like codifying assumptions against a non-public API, but this seems like a valuable fix if you're seeing issues with the current implementation.
There was a problem hiding this comment.
There is a lot of grey zones in boto3, botocore and BaseSessionFactory. I can't find any valuable information about boto3.session.Session and best practices except this:
- https://boto3.amazonaws.com/v1/documentation/api/latest/guide/session.html
- https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html
Also when i tried to found best practices how to assign RefreshableCredentials to boto3.session.Session I usual found on the Internet something like: SO link 1, SO link 2, some link from top 10 in Google search
So I wasn't surprised when I initially sow how it resolved in provider
session = botocore.session.get_session()
session._credentials = credentials
region_name = self.basic_session.region_name
session.set_config_variable("region", region_name)
Let's me try to add some additional information why we set some parameters and not set another. It might help next time when someone add changes and broke something.
e8a134d to
39909f5
Compare
o-nikolas
left a comment
There was a problem hiding this comment.
Thanks for updating with some documentation!
|
@Taragolis I just rebased my branch and the added test is failing for me is this related ? |
yeah it might be the issue that two PR work fine independent (this and #27177) but when it merged it have some side effects. I will have a look tomorrow morning. Most probably it related to new version of moto or something like that |
|
Hi, currently I'm using airflow 2.4.3 with apache-airflow-providers-amazon 6.0.0, the issue that I'm facing is that I add the keys of some aws user and also add the role_arn in the extra field, but the connection is not assuming the role, and instead is using the aws user identity, so looks like the problem is there... |
Upgrade to |
|
@Taragolis by chance do you know you which constrains file I can use for that version?, is just that I'm using this https://raw.githubusercontent.com/apache/airflow/constraints-2.4.3/constraints-3.10.txt, and that file has the apache-airflow-providers-amazon fixed to 6.0.0. thanks so much. |
|
Please follow this instruction: Installing/upgrading/downgrading providers separately from Airflow core, also could be useful read Fixing Constraint files at release time for understand purpose of constraints which released in the same time when new version of Airflow released. One suggestion if you have additional question better always open the discussion or as ad-hock questions in the Apache Airflow slack - |
This PR fix bug when user provide initial connection credentials which will use for assume role.
Right now instead of actual assumed role would use initial
boto3.session.Session.It not affected right now if user use EC2 Instance Profile, ECS Task Execution role and EKS IRSA
cc @vincbeck @ferruzzi @o-nikolas
A believe one day we could switch from current implementation to botocore Credential Providers instead of SessionFactory.