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

async signing support #659

Merged
merged 36 commits into from
Mar 31, 2020
Merged

async signing support #659

merged 36 commits into from
Mar 31, 2020

Conversation

thehesiod
Copy link
Collaborator

@thehesiod thehesiod commented Jan 5, 2019

This starts the large work of supporting #619

Large changes:

  • (api breaking) the result ofcreate_client is now a required async context class
  • generate_presigned_url is now an async call
  • Credentials.[access_key/secret_key/token] now raise NotImplementedError because they won't call refresh like botocore. Instead should use get_frozen_credentials async method

In general, this is going to increase our footprint significantly.

TODO

  • get tests/checks passing
  • ensured that all the events.emit calls have been properly wrapped (all the way up call chain)
  • add patches for new methods
  • port credential classes and callers, partial list: IMDSFetcher, ContainerMetadataFetcher, AssumeRoleWithWebIdentityCredentialFetcher, ProcessProvider, AssumeRoleCredentialFetcher
  • add tests for coverage (ports from botocore)
  • look into botocore.utils.S3RegionRedirector.redirect_from_error
  • adhoc test using real aws creds

@thehesiod
Copy link
Collaborator Author

with the changes as-is we support the simple testcase I list in #658

@thehesiod thehesiod changed the title async signing and stream support [WIP] async signing and stream support Jan 21, 2019
@terricain
Copy link
Collaborator

terricain commented Jun 21, 2019

This'll help with #707 (and terricain/aioboto3#173) as we essentially want to make botocore.utils.S3RegionRedirector.redirect_from_error async

@thehesiod
Copy link
Collaborator Author

I'm going to try working on this this weekend

@terricain
Copy link
Collaborator

@thehesiod hows it going?

@thehesiod
Copy link
Collaborator Author

@terrycain under a mountain of work with new baby almost here 🤯 :)

@terricain
Copy link
Collaborator

Np man, congrats 😄

@stalkerg
Copy link

I am very interesting in async generate_presigned_url, how I can help to merge this PR?

@thehesiod
Copy link
Collaborator Author

hello @stalkerg , you can take a look at this PR. We need to change create_client from being an async method to returning an async context class as mentioned in the description. Also need to merge from master, and ensure all the emit methods that touch these changes are async. In general need to do a lot of research to ensure this system will work, which means we probably need a lot of more testing. This PR probably should be the place we do adaptation of the underlying botocore unittests.

# Conflicts:
#	aiobotocore/client.py
#	aiobotocore/endpoint.py
#	aiobotocore/session.py
#	tests/test_patches.py
@thehesiod
Copy link
Collaborator Author

not sure how this happened, but we're out of sync with botocore, I'm working on cleaning this up

@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2019

This pull request introduces 4 alerts when merging 8b055ee into 957c91b - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Missing call to __init__ during object initialization
  • 1 for Module-level cyclic import

@thehesiod thehesiod changed the title [WIP] async signing and stream support [WIP] async signing support Oct 26, 2019
@aio-libs aio-libs deleted a comment from lgtm-com bot Oct 26, 2019
@aio-libs aio-libs deleted a comment from lgtm-com bot Oct 26, 2019
@aio-libs aio-libs deleted a comment from lgtm-com bot Oct 26, 2019
@thehesiod
Copy link
Collaborator Author

omg, the whole credentials file needs to be ported! I give up for awhile again, so much work!

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 5 alerts and fixes 1 when merging d43ff58 into 904332b - view on LGTM.com

new alerts:

  • 3 for Unreachable code
  • 1 for Missing call to __init__ during object initialization
  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@thehesiod
Copy link
Collaborator Author

ok, I think we're good in terms of emitters, now to fix tests again

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 5 alerts and fixes 1 when merging b9dd4f3 into 904332b - view on LGTM.com

new alerts:

  • 3 for Unreachable code
  • 1 for Missing call to __init__ during object initialization
  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@thehesiod
Copy link
Collaborator Author

ok fixed tests, and set access_key+ secret_key + token properties to raise NotImplementedError, instead callers should use get_frozen_credentials. thoughts?

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 5 alerts and fixes 1 when merging 7177429 into 904332b - view on LGTM.com

new alerts:

  • 3 for Unreachable code
  • 1 for Missing call to __init__ during object initialization
  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@terricain
Copy link
Collaborator

Ok so according to something I read in the botocore source/docs somewhere, everything is supposed to call get_frozen_credentials, lemme have a look

@terricain
Copy link
Collaborator

Yeah I think were good. So tests are failing because of an old pyyaml version having a CVE, pretty sure aws-cli is the culprit as they seem to have moved to using ruamel.yaml.

Should we just do the usual bump botocore version stuff here and then merge?

@terricain
Copy link
Collaborator

Also tests run fine for me so the access_key changes are good

@thehesiod
Copy link
Collaborator Author

ya lets bump and merge! We're hitting this CVE at work too so bump would be awesome, if you don't get to in in a few hours I'll do it and merge. 🎆 🍾

@thehesiod
Copy link
Collaborator Author

ok doing it now

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 5 alerts and fixes 1 when merging 5b61a6c into 904332b - view on LGTM.com

new alerts:

  • 3 for Unreachable code
  • 1 for Missing call to __init__ during object initialization
  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@thehesiod thehesiod changed the title [WIP] async signing support async signing support Mar 31, 2020
@thehesiod thehesiod merged commit 6f07726 into master Mar 31, 2020
@thehesiod thehesiod deleted the thehesiod/streaming branch March 31, 2020 16:57
@terricain
Copy link
Collaborator

Yay 😄

@thehesiod
Copy link
Collaborator Author

@terrycain thanks so much for your help! Wouldn't have been possible without you, at least for a long time :]

@terricain
Copy link
Collaborator

No problem ;-). Just realized we need to override session._set_credentials() as otherwise it breaks when access and secret key are passed explicitly :(. Will pr in morning if you don't get there first, am too tired now :D

@thehesiod
Copy link
Collaborator Author

Hmm I left the set, just not the get. Too tired to look now too though :)

@terricain terricain mentioned this pull request Apr 2, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants