Skip to content

Comments

Fix assume role if user explicit set credentials#26946

Merged
eladkal merged 3 commits intoapache:mainfrom
Taragolis:aws-assume-role-with-initial-credentials
Oct 21, 2022
Merged

Fix assume role if user explicit set credentials#26946
eladkal merged 3 commits intoapache:mainfrom
Taragolis:aws-assume-role-with-initial-credentials

Conversation

@Taragolis
Copy link
Contributor

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.

@Taragolis Taragolis requested a review from eladkal as a code owner October 8, 2022 08:07
@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Oct 8, 2022
@Taragolis Taragolis force-pushed the aws-assume-role-with-initial-credentials branch from 52d72e6 to e8a134d Compare October 10, 2022 19:41
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +132 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@Taragolis Taragolis Oct 11, 2022

Choose a reason for hiding this comment

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

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)

session = botocore.session.get_session()
session._credentials = credentials
region_name = self.basic_session.region_name
session.set_config_variable("region", region_name)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

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.

@Taragolis Taragolis force-pushed the aws-assume-role-with-initial-credentials branch from e8a134d to 39909f5 Compare October 19, 2022 15:05
Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Thanks for updating with some documentation!

@eladkal eladkal merged commit 737e50a into apache:main Oct 21, 2022
@pierrejeambrun
Copy link
Member

pierrejeambrun commented Oct 21, 2022

@Taragolis Taragolis deleted the aws-assume-role-with-initial-credentials branch October 21, 2022 20:08
@Taragolis
Copy link
Contributor Author

@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

@crua-godaddy
Copy link

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...

@Taragolis
Copy link
Contributor Author

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

@crua-godaddy
Copy link

crua-godaddy commented Feb 22, 2023

@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.

@Taragolis
Copy link
Contributor Author

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 - #troubleshooting, #newbie-questions, rather than write comment to the closed and merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants