You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/08 08:07:44 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #26946: Fix assume role if user explicit set credentials
Taragolis opened a new pull request, #26946:
URL: https://github.com/apache/airflow/pull/26946
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](https://github.com/boto/botocore/blob/8cdfb7567b1f680e684186e50eb0b8c781a935b4/botocore/credentials.py#L972-L982) instead of SessionFactory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1287396180
> @Taragolis I just rebased my branch and the added test is failing for me is this related ? https://github.com/apache/airflow/actions/runs/3299613133/jobs/5443399166#step:11:10343
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992780811
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review Comment:
Unfortunately not, this something which not well covered in boto3 documentation.
Initially [boto3.session.Session](https://boto3.amazonaws.com/v1/documentation/api/latest/_modules/boto3/session.html#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](https://botocore.amazonaws.com/v1/documentation/api/latest/tutorial/index.html) witch not cover their session well. So let me refer to code
_boto3.session.Session init constructor_
```python
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](https://github.com/boto/botocore/blob/fec0e5bd5e4a9d7dcadb36198423e61437294fe6/botocore/session.py#L476-L495)
When we create botocore session for assume_role we use a bit hacky approach (access to private methods/properties)
https://github.com/apache/airflow/blob/8e2e80a0ce0e1819874e183fb1662e879cdd8a08/airflow/providers/amazon/aws/hooks/base_aws.py#L150-L153
So if we provide:
1. `aws_access_key_id` or `aws_secret_access_key` or `aws_session_token` it will replace assumed credentials in botocore session. We already use this credentials when assume role
2. `profile_name` - might be nothing bad happen but better do not provide it, since we already use it during session creation
3. `region_name` - Nothing bad happen, we use exactly the same region_name as user provide for initial and assume_role. For initial session `region_name` only affect to endpoint for STS endpoint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r993076634
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review 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](https://stackoverflow.com/a/65962450), [SO link 2](https://stackoverflow.com/a/69226170), [some link from top 10 in Google search](https://feeney.mba/refreshable-aws-boto-credentials.html)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992932136
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992732045
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
Review Comment:
```suggestion
# Values stored in AwsConnectionWrapper.session_kwargs are intended to be used only to create the initial boto3 session.
# If the user wants to use the 'assume_role' mechanism then only the 'region_name' needs to be
# provided, otherwise other parameters might conflict with the base botocore session.
```
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992780811
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review Comment:
Unfortunately not, this something which not well covered in boto3 documentation.
Initially [boto3.session.Session](https://boto3.amazonaws.com/v1/documentation/api/latest/_modules/boto3/session.html#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](https://botocore.amazonaws.com/v1/documentation/api/latest/tutorial/index.html) which not cover how to work with their session. Let me refer to code `boto3` and `botocore`
_boto3.session.Session init constructor_
```python
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](https://github.com/boto/botocore/blob/fec0e5bd5e4a9d7dcadb36198423e61437294fe6/botocore/session.py#L476-L495)
When we create botocore session for assume_role we use a bit hacky approach (access to private methods/properties)
https://github.com/apache/airflow/blob/8e2e80a0ce0e1819874e183fb1662e879cdd8a08/airflow/providers/amazon/aws/hooks/base_aws.py#L150-L153
So if we provide:
1. `aws_access_key_id` or `aws_secret_access_key` or `aws_session_token` it will replace assumed credentials in botocore session. We already use this credentials when assume role
2. `profile_name` - might be nothing bad happen but better do not provide it, since we already use it during session creation
3. `region_name` - Nothing bad happen, we use exactly the same region_name as user provide for initial and assume_role sessions. For initial session `region_name` only affect to endpoint for STS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
eladkal merged PR #26946:
URL: https://github.com/apache/airflow/pull/26946
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] crua-godaddy commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by "crua-godaddy (via GitHub)" <gi...@apache.org>.
crua-godaddy commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1440077400
@Taragolis by chance do know you which constrains 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992780811
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review Comment:
Unfortunately not, this something which not well covered in boto3 documentation.
Initially [boto3.session.Session](https://boto3.amazonaws.com/v1/documentation/api/latest/_modules/boto3/session.html#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](https://botocore.amazonaws.com/v1/documentation/api/latest/tutorial/index.html) which not cover how to work with their session. Let me refer to code `boto3` and `botocore`
_boto3.session.Session init constructor_
```python
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](https://github.com/boto/botocore/blob/fec0e5bd5e4a9d7dcadb36198423e61437294fe6/botocore/session.py#L476-L495)
When we create botocore session for assume_role we use a bit hacky approach (access to private methods/properties)
https://github.com/apache/airflow/blob/8e2e80a0ce0e1819874e183fb1662e879cdd8a08/airflow/providers/amazon/aws/hooks/base_aws.py#L150-L153
So if we provide:
1. `aws_access_key_id` or `aws_secret_access_key` or `aws_session_token` it will replace assumed credentials in botocore session. We already use this credentials when assume role
2. `profile_name` - might be nothing bad happen but better do not provide it, since we already use it during session creation
3. `region_name` - Nothing bad happen, we use exactly the same region_name as user provide for initial and assume_role. For initial session `region_name` only affect to endpoint for STS endpoint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1440222224
Please follow this instruction: [Installing/upgrading/downgrading providers separately from Airflow core](https://airflow.apache.org/docs/apache-airflow/2.5.1/installation/installing-from-pypi.html#installing-upgrading-downgrading-providers-separately-from-airflow-core), also could be useful read [Fixing Constraint files at release time](https://airflow.apache.org/docs/apache-airflow/2.5.1/installation/installing-from-pypi.html#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](https://github.com/apache/airflow/discussions) or as ad-hock questions [in the Apache Airflow slack](https://apache-airflow-slack.herokuapp.com/) - `#troubleshooting`, `#newbie-questions`, rather than write comment to the closed and merged PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] crua-godaddy commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by "crua-godaddy (via GitHub)" <gi...@apache.org>.
crua-godaddy commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1439178281
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1439190448
> 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 `apache-airflow-providers-amazon>=6.1.0`, for more details see [Changelog for Amazon Provider](https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/index.html#id16)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #26946:
URL: https://github.com/apache/airflow/pull/26946#issuecomment-1287351001
@Taragolis I just rebased and the added test is failing for me:
https://github.com/apache/airflow/actions/runs/3299613133/jobs/5443399166#step:11:10343 is it related ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #26946: Fix assume role if user explicit set credentials
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26946:
URL: https://github.com/apache/airflow/pull/26946#discussion_r992780811
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -125,7 +125,13 @@ def create_session(self) -> boto3.session.Session:
return boto3.session.Session(region_name=self.region_name)
elif not self.role_arn:
return self.basic_session
- return self._create_session_with_assume_role(session_kwargs=self.conn.session_kwargs)
+ # Values stored in AwsConnectionWrapper.session_kwargs intend to use only create initial boto3 session
+ # If user want to use 'assume_role' mechanism we need provide only 'region_name'
+ # otherwise other parameters might conflict with base botocore session.
+ assume_session_kwargs = {}
+ if self.conn.region_name:
+ assume_session_kwargs["region_name"] = self.conn.region_name
Review Comment:
Unfortunately not, this something which not well covered in boto3 documentation.
Initially [boto3.session.Session](https://boto3.amazonaws.com/v1/documentation/api/latest/_modules/boto3/session.html#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](https://botocore.amazonaws.com/v1/documentation/api/latest/tutorial/index.html) which not cover their session well. So let me refer to code
_boto3.session.Session init constructor_
```python
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](https://github.com/boto/botocore/blob/fec0e5bd5e4a9d7dcadb36198423e61437294fe6/botocore/session.py#L476-L495)
When we create botocore session for assume_role we use a bit hacky approach (access to private methods/properties)
https://github.com/apache/airflow/blob/8e2e80a0ce0e1819874e183fb1662e879cdd8a08/airflow/providers/amazon/aws/hooks/base_aws.py#L150-L153
So if we provide:
1. `aws_access_key_id` or `aws_secret_access_key` or `aws_session_token` it will replace assumed credentials in botocore session. We already use this credentials when assume role
2. `profile_name` - might be nothing bad happen but better do not provide it, since we already use it during session creation
3. `region_name` - Nothing bad happen, we use exactly the same region_name as user provide for initial and assume_role. For initial session `region_name` only affect to endpoint for STS endpoint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org