Skip to content
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

AWS Hook - allow IDP HTTP retry (#12639) #16612

Merged
merged 3 commits into from
Jun 28, 2021
Merged

AWS Hook - allow IDP HTTP retry (#12639) #16612

merged 3 commits into from
Jun 28, 2021

Conversation

baolsen
Copy link
Contributor

@baolsen baolsen commented Jun 23, 2021

When doing AssumeRoleWithSAML there is an HTTP request made to an IDP endpoint to obtain the SAML Assertion. This PR allows the IDP request to have a configurable Retry, which makes this authentication mechanism a bit more robust.

Tested with my own Airflow environment which uses this auth method, and it is working as intended.
It's working with and without the new configuration.

closes: 12639

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jun 23, 2021
self.log.info("idp_url= %s", idp_url)

session = requests.Session()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example configuration, can be set on the Connection extra along with the other attributes needed for assume-role-with-saml:

"assume_role_with_saml": {
   [...]
   "idp_request_retry_kwargs": {
      "total": 10,
      "backoff_factor":1,
      "status":10,
      "status_forcelist": [400, 429, 500, 502, 503, 504]
   },
   [...]
}

The Retry strategy is triggers when the IDP responds with any HTTP status code which is in status_forcelist.
Up to 10 retries are attempted, with exponential backoff.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, done.

@baolsen
Copy link
Contributor Author

baolsen commented Jun 23, 2021

Seem to be getting CI-related issues, will try again later

Updates:

1 Build is getting pylint issues which other builds are also experiencing. Seems unrelated to my changes then.
2 Latest build magically did not have pylint errors but failed (also magically :) )on quarantined tests. I can't locate an error message in the CI so I've retriggered it, presumably still a CI issue.
3 Unrelated Pylint failures magically back, retrying
4 Different pylint failures this time in CI, also appear to be unrelated

@baolsen baolsen requested a review from vikramkoka as a code owner June 24, 2021 09:13
@baolsen baolsen requested a review from mik-laj June 25, 2021 07:39
@baolsen
Copy link
Contributor Author

baolsen commented Jun 25, 2021

Hey @mik-laj
CI on this PR is failing due to unrelated issues.
Have tagged you for review, if you like.
Otherwise I'm happy to wait with this one until the CI issues get resolved, and retry it later. Have tried a few times though.

@potiuk
Copy link
Member

potiuk commented Jun 25, 2021

Otherwise I'm happy to wait with this one until the CI issues get resolved, and retry it later. Have tried a few times though.

We got finally fed up with the pylint-induced problems. I think we are just about to decide to get rid of pylint (precisely for the kind of problems you are experiencing @baolsen ). You can wait for it (I hope on Monday the voting will finish) or add disables to speed it up :).

BTW. If you are as frustrated as we are with this pylint random errors - feel free to chime-in here https://lists.apache.org/thread.html/r9e2cc385db8737ec0874ad09872081bd083593ee29e8303e58d21efb%40%3Cdev.airflow.apache.org%3E :)

@potiuk potiuk merged commit 0d80383 into apache:main Jun 28, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants