[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML #7619

baolsen · 2020-03-04T06:51:05Z

Note that using AssumeRoleWithSAML uses requests_gssapi to obtain a SAML assertion from your IDP.
You must then pip install gssapi rather than pip install python-gssapi to avoid a conflict with paramiko.

Also per code comment, this PR updates the required Paramiko version to be compatible.

requests_gssapi will need paramiko > 2.6 since you'll need

'gssapi' not 'python-gssapi' from PyPi.

paramiko/paramiko#131

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
[] Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

baolsen · 2020-03-04T06:51:49Z

I am unsure how to unit test this - suggestions welcome :).
I have tested it against my AWS environment and IDP and it is working as intended.

turbaszek · 2020-03-04T09:09:12Z

airflow/providers/amazon/aws/hooks/base_aws.py

+                        method = self._assume_role_with_saml
+                    else:
+                        raise NotImplementedError(
+                            'assume_role_method=%s' % assume_role_method)


When I get this exception I've got no idea what went wrong. The information should be more meaningful and probably NotImplementedError is not a best here. Also, we can us f-strings for formatting :)

Great point, thanks. What makes sense to the developer might make no sense to the user ;) I will update these cases accordingly.

turbaszek · 2020-03-04T09:09:43Z

airflow/providers/amazon/aws/hooks/base_aws.py

+            sts_client,
+            extra_config,
+            role_arn,
+            assume_role_kwargs):


Can you please add type annotation? Those really help :)

Thanks, done

turbaszek · 2020-03-04T09:10:42Z

airflow/providers/amazon/aws/hooks/base_aws.py

+                mutual_auth = saml_config['mutual_authentication']
+                if mutual_auth == 'REQUIRED':
+                    auth = requests_gssapi.HTTPSPNEGOAuth(
+                        requests_gssapi.REQUIRED)


Is there any reason why we cannot keep this in one line? 🤔

I asked VS Code and it couldn't tell me why it put it on another line xD. Fixed

turbaszek · 2020-03-04T09:11:12Z

airflow/providers/amazon/aws/hooks/base_aws.py

+                        requests_gssapi.DISABLED)
+                else:
+                    raise NotImplementedError(
+                        'mutual_authentication=%s' % mutual_auth)


Same here, the exception does not help the user.

Fixed thanks

turbaszek · 2020-03-04T09:11:25Z

airflow/providers/amazon/aws/hooks/base_aws.py

+            if not saml_assertion:
+                raise ValueError('Invalid SAML Assertion')
+        else:
+            raise NotImplementedError('idp_auth_method=%s' % idp_auth_method)


potiuk · 2020-03-04T11:35:18Z

I am unsure how to unit test this - suggestions welcome :).
I have tested it against my AWS environment and IDP and it is working as intended.

Hello @baolsen -> I have a proposal - we have implemented a lot of "system tests" in GCP and maybe that will be the right time to try the same approach in AWS?

@nuclearpinguin is just implementing some latest fixes to our "reusable" system test class and we have it already pretty well described in the documentation https://github.com/apache/airflow/blob/master/TESTING.rst#airflow-system-tests

Breeze is now very well prepared to run such system tests automatically and I also have an open discussions about backporting the "providers" packages to 1.10 (part of AIP-21) and I want to propose that one of the conditions there is to have system tests semi-automatically that are covering the provider's package functionality. And we want to fully automate it with AIP-4.

Maybe we can work together - me, @nuclearpinguin , @mik-laj and you to add first AWS system tests and be able to run it automatically? Maybe we can also - with your involvement - improve it all and make it easily usable by others?

It's rather straightforward - please take a look at the docs. It boils down to having an "example_dag" that is actually runnable (providing authorisation/configuration information). The dag should perform setup/some operations/teardown on a real external system (GCP or AWS in this case) - we have some useful helpers written to make it easy to encapsulate it in a Pytest test with @pytest.markers.system("amazon") in your case. Then it can be easily run automatically (we will just have to provide authorisation file in the form of files/airflow-breeze-config/variables.env file - with authorisation variables.

It's a bit involved as those tests usually run for a long time - but at the same time you have an environment to test your changes on your own with a real system (something that you do manually now and can be automated for any future changes).

What do you think? We would love to help with that!

baolsen · 2020-03-04T13:04:52Z

Hey @potiuk . Sure, I'd be willing to give it a try :) Probably best to create a Slack channel... It is blocked at my work, but I can use it via mobile to keep in touch.
Should we add the changes to this JIRA?
Perhaps better to create another, just for the AWS system test scaffolding, and rebase this one later?

potiuk · 2020-03-04T13:14:18Z

Yeah - cool. That would be great. I will create a channel for that. I think it should be ok to proceed with that one without system tests and merge it and then add the system test as separate PR as some refactoring might be needed

baolsen · 2020-03-05T05:12:32Z

Build can be restarted - test timed out :)

turbaszek · 2020-03-05T12:27:55Z

Build can be restarted - test timed out :)

Restarted :)

baolsen · 2020-03-05T13:40:36Z

Timed out again... Please re-restart :)

turbaszek · 2020-03-07T10:36:14Z

@baolsen can you rebase onto actual master? We've got some system test failing (should not be run at all).

feluelle

The _assume_role_with_saml method looks a bit complex to me, because you are importing so many different libraries and you are doing a lot of semantic blocks which are showing that it could be useful to split it - So I would probably refactor it.

In my opinion the whole _get_credentials is too complex. Maybe it is time to add a sub-class AwsCredentialsExtractor or similar where we can split this whole function into smaller pieces we also fully test.

But in general it really looks good to me. 👍 You added the documentation to the how-to which is great :)

feluelle · 2020-03-10T11:31:50Z

airflow/providers/amazon/aws/hooks/base_aws.py

+            # requests_gssapi will need paramiko > 2.6 since you'll need
+            # 'gssapi' not 'python-gssapi' from PyPi.
+            # https://github.com/paramiko/paramiko/pull/1311


In my opinion this would be better to have in the commit message and/or the PR description.

But it's fine I guess - not that important to me.

Added to the PR description; will quickly add it to the how-to documentation as well :)

baolsen · 2020-03-12T05:23:39Z

The _assume_role_with_saml method looks a bit complex to me, because you are importing so many different libraries and you are doing a lot of semantic blocks which are showing that it could be useful to split it - So I would probably refactor it.

In my opinion the whole _get_credentials is too complex. Maybe it is time to add a sub-class AwsCredentialsExtractor or similar where we can split this whole function into smaller pieces we also fully test.

But in general it really looks good to me. 👍 You added the documentation to the how-to which is great :)

It is getting quite complex. I think I'd rather leave it for now, until the AWS system tests are in place and then refactoring and unit testing will be a possibility :)

codecov-io · 2020-03-12T07:11:32Z

Codecov Report

Merging #7619 into master will decrease coverage by 0.61%.
The diff coverage is 23.21%.

@@            Coverage Diff             @@
##           master    #7619      +/-   ##
==========================================
- Coverage   86.76%   86.15%   -0.62%     
==========================================
  Files         897      904       +7     
  Lines       42819    43787     +968     
==========================================
+ Hits        37153    37725     +572     
- Misses       5666     6062     +396

Impacted Files	Coverage Δ
airflow/providers/amazon/aws/hooks/base_aws.py	`59.41% <23.21%> (-17.90%)`	⬇️
...flow/providers/apache/cassandra/hooks/cassandra.py	`21.51% <0.00%> (-72.16%)`	⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py	`35.84% <0.00%> (-64.16%)`	⬇️
airflow/kubernetes/volume_mount.py	`44.44% <0.00%> (-55.56%)`	⬇️
airflow/providers/redis/operators/redis_publish.py	`50.00% <0.00%> (-50.00%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0.00%> (-47.06%)`	⬇️
airflow/providers/mongo/sensors/mongo.py	`53.33% <0.00%> (-46.67%)`	⬇️
airflow/kubernetes/pod_launcher.py	`47.18% <0.00%> (-45.08%)`	⬇️
airflow/providers/mysql/operators/mysql.py	`55.00% <0.00%> (-45.00%)`	⬇️
airflow/providers/redis/sensors/redis_key.py	`61.53% <0.00%> (-38.47%)`	⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b39468d...2e12bf6. Read the comment docs.

baolsen · 2020-03-12T12:02:11Z

@nuclearpinguin Ready to merge :)

boring-cyborg bot added area:docs provider:amazon-aws AWS/Amazon - related issues labels Mar 4, 2020

turbaszek removed the area:docs label Mar 4, 2020

turbaszek requested a review from feluelle March 4, 2020 09:06

turbaszek requested changes Mar 4, 2020

View reviewed changes

boring-cyborg bot added the area:docs label Mar 4, 2020

baolsen requested a review from turbaszek March 5, 2020 05:12

turbaszek approved these changes Mar 5, 2020

View reviewed changes

Bjorn Olsen added 2 commits March 9, 2020 12:48

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML

bc154e4

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML CODE REVIEW

a95e8c6

feluelle approved these changes Mar 10, 2020

View reviewed changes

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML CODE REVIEW 2

2e12bf6

turbaszek merged commit 3bb60af into apache:master Mar 13, 2020

snazzyfox mentioned this pull request Apr 14, 2020

Support Duration in AwsHook when using assume role #8301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML #7619

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML #7619

baolsen commented Mar 4, 2020 •

edited

Loading

baolsen commented Mar 4, 2020 •

edited

Loading

turbaszek Mar 4, 2020

baolsen Mar 4, 2020

turbaszek Mar 4, 2020

baolsen Mar 5, 2020

turbaszek Mar 4, 2020

baolsen Mar 5, 2020

turbaszek Mar 4, 2020

baolsen Mar 5, 2020

turbaszek Mar 4, 2020

baolsen Mar 5, 2020

potiuk commented Mar 4, 2020

baolsen commented Mar 4, 2020

potiuk commented Mar 4, 2020

baolsen commented Mar 5, 2020

turbaszek commented Mar 5, 2020

baolsen commented Mar 5, 2020

turbaszek commented Mar 7, 2020

feluelle left a comment

feluelle Mar 10, 2020

baolsen Mar 12, 2020

baolsen commented Mar 12, 2020

codecov-io commented Mar 12, 2020 •

edited

Loading

baolsen commented Mar 12, 2020

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML #7619

[AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML #7619

Conversation

baolsen commented Mar 4, 2020 • edited Loading

requests_gssapi will need paramiko > 2.6 since you'll need

'gssapi' not 'python-gssapi' from PyPi.

baolsen commented Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Mar 4, 2020

baolsen commented Mar 4, 2020

potiuk commented Mar 4, 2020

baolsen commented Mar 5, 2020

turbaszek commented Mar 5, 2020

baolsen commented Mar 5, 2020

turbaszek commented Mar 7, 2020

feluelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baolsen commented Mar 12, 2020

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

baolsen commented Mar 12, 2020

baolsen commented Mar 4, 2020 •

edited

Loading

baolsen commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading