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.

Sign up for GitHub

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

Jump to bottom

Fix race condition in AWS request signers #470

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Fix race condition in AWS request signers #470

merged 3 commits into from
Aug 10, 2023

Conversation

lattwood
Copy link
Contributor

Closes #423

@lattwood lattwood force-pushed the lattwood/fix_refreshable_credentials branch 2 times, most recently from 9293581 to 106f3b8 Compare August 10, 2023 15:50
@lattwood
Fix race condition in AWS request signers
8c69ad3 
Signed-off-by: Logan Attwood <logan.attwood@gmail.com>
@lattwood
Conditionally expose get_frozen_credentials on mock_session
dd325ed 
Signed-off-by: Logan Attwood <logan.attwood@gmail.com>
@lattwood lattwood force-pushed the lattwood/fix_refreshable_credentials branch from 106f3b8 to dd325ed Compare August 10, 2023 15:52
lattwood
lattwood commented Aug 10, 2023
View reviewed changes
CHANGELOG.md
[2.3.0]: https://github.com/opensearch-project/opensearch-py/compare/v2.2.0...v2.3.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This showed up because my local vim doesn't like it when a file doesn't end in a newline- I'm guessing most the devs on this project are using VSCode

Copy link
Member

Choose a reason for hiding this comment

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

It's fine.

@lattwood lattwood mentioned this pull request Aug 10, 2023
Closed
@codecov
Copy link

codecov bot commented Aug 10, 2023 •
edited
Loading

Codecov Report

Merging #470 (32074ec) into main (8485606) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #470   +/-   ##
=======================================
  Coverage   70.91%   70.92%           
=======================================
  Files          81       81           
  Lines        7730     7732    +2     
=======================================
+ Hits         5482     5484    +2     
  Misses       2248     2248
Files Changed Coverage Δ
opensearchpy/helpers/asyncsigner.py 95.83% <100.00%> (+0.18%) ⬆️
opensearchpy/helpers/signer.py 94.73% <100.00%> (+0.14%) ⬆️

dblock
dblock reviewed Aug 10, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good!

There's a bit of code duplication, but I suppose it's ok here. LMK if you want to leave things as is in the test, and I'll merge?

CHANGELOG.md
[2.3.0]: https://github.com/opensearch-project/opensearch-py/compare/v2.2.0...v2.3.0
Copy link
Member

Choose a reason for hiding this comment

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

It's fine.

test_opensearchpy/test_async/test_signer.py Outdated
access_key = uuid.uuid4().hex
secret_key = uuid.uuid4().hex
token = uuid.uuid4().hex
dummy_session = Mock()
dummy_session.access_key = access_key
dummy_session.secret_key = secret_key
dummy_session.token = token
dummy_session.get_frozen_credentials = Mock(return_value=dummy_session)

if disable_get_frozen:
Copy link
Member

Choose a reason for hiding this comment

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

There's probably a cleaner way to do this, if you care, by inheriting TestAsyncSigner into a TestAsyncSignerWithFrozenCredentials(TestAsyncSigner) or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm. The issue is the checks for the get_frozen_credentials method in the signer implementations will always evaluate to True if I don't explicitly delete the attribute because of how eager Mock is to mock :)

@lattwood
Copy link
Contributor Author

@dblock unless you can think of a better solution to Mock being overeager around the get_frozen_credentials method (that I'd be happy to implement), I'd say go ahead and merge it. Anything I need to do for backports?

@lattwood
Copy link
Contributor Author

Figured it out, another commit will be forthcoming

@lattwood lattwood force-pushed the lattwood/fix_refreshable_credentials branch from 28ea160 to 5cba13f Compare August 10, 2023 16:51
@lattwood
Use inheritance to make tests cleaner
32074ec 
Signed-off-by: Logan Attwood <logan.attwood@gmail.com>
@lattwood
Copy link
Contributor Author

doing one more force push to retrigger that failing integration test

@lattwood lattwood force-pushed the lattwood/fix_refreshable_credentials branch from 5cba13f to 32074ec Compare August 10, 2023 16:57
@dblock
Copy link
Member

dblock commented Aug 10, 2023

doing one more force push to retrigger that failing integration test

This failure has been so annoying. Hope someone can make the test more reliable. Wink wink.

@lattwood
Copy link
Contributor Author

doing one more force push to retrigger that failing integration test

This failure has been so annoying. Hope someone can make the test more reliable. Wink wink.

Looks like it's related to docker networking stuff 😅

@dblock
Copy link
Member

dblock commented Aug 10, 2023

Anything I need to do for backports?

AFAIK no, we release from main, @VachaShah yes?

@lattwood
Copy link
Contributor Author

Anything I need to do for backports?

AFAIK no, we release from main, @VachaShah yes?

image

was why I asked (CONTRIBUTING.md)

@dblock dblock merged commit fca8bd3 into opensearch-project:main Aug 10, 2023
@dblock
Copy link
Member

dblock commented Aug 10, 2023 •
edited
Loading

Good work @lattwood ! Also great GitHub profile blurb. You're talking to an LLM btw here ;)

@lattwood lattwood deleted the lattwood/fix_refreshable_credentials branch August 10, 2023 17:37
@VachaShah
Copy link
Collaborator

Anything I need to do for backports?

AFAIK no, we release from main, @VachaShah yes?

Yeah no backports are required currently as we release from main since there have been no breaking changes in this client, so main = 2.x.

@lattwood lattwood mentioned this pull request Aug 14, 2023
Closed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.3 2.3
# Navigate to the new working tree
cd .worktrees/backport-2.3
# Create a new branch
git switch --create backport/backport-470-to-2.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fca8bd3d0e7f42f2458e924c36ac6a135a6bc622
# Push it to GitHub
git push --set-upstream origin backport/backport-470-to-2.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.3

Then, create a pull request where the base branch is 2.3 and the compare/head branch is backport/backport-470-to-2.3.

@saimedhi saimedhi mentioned this pull request Aug 15, 2023
Merged
@dacevedo12 dacevedo12 mentioned this pull request Nov 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@axeoman axeoman Awaiting requested review from axeoman axeoman is a code owner

@deztructor deztructor Awaiting requested review from deztructor

@Shephalimittal Shephalimittal Awaiting requested review from Shephalimittal Shephalimittal is a code owner

@saimedhi saimedhi Awaiting requested review from saimedhi saimedhi is a code owner

Assignees
No one assigned
Labels
backport 2.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] AWSV4SignerAuth doesn't freeze credentials before signing, leading to a race condition
4 participants
@lattwood @dblock @VachaShah @saimedhi