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

Refactor aws_stack.mock_aws_request_headers() #9509

Merged
merged 9 commits into from Nov 2, 2023

Conversation

viren-nadkarni
Copy link
Member

@viren-nadkarni viren-nadkarni commented Oct 30, 2023

Motivation

The aws_stack.mock_aws_request_headers() utility is used in our codebase to generate headers for AWS requests. Among these headers include Content-Type, Accept-Encoding, date and especially the Authorization header.

The issue this PR is addressing is the optional nature of the access key and region name arguments for this function, in which the get_aws_access_key_id() and aws_stack.get_region() are used. We already know that these are problematic functions that rely on thread context storage and are not guaranteed to return the correct values. Furthermore they must not be used in tests, indirectly which is exactly what is happening.

Implementation

This PR makes the access key ID and region name field mandatory.

@viren-nadkarni viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Oct 30, 2023
Comment on lines 146 to 151
auth_header = (
auth_header
or aws_stack.generate_aws_request_headers(
service_name, aws_access_key_id=DEFAULT_AWS_ACCOUNT_ID, region_name=AWS_REGION_US_EAST_1
)["Authorization"]
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is dead code but updated anyway

@github-actions
Copy link

github-actions bot commented Oct 30, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 9m 3s ⏱️
2 316 tests 1 743 ✔️ 573 💤 0
2 317 runs  1 743 ✔️ 574 💤 0

Results for commit 351458d.

♻️ This comment has been updated with latest results.

@viren-nadkarni viren-nadkarni changed the title Refactor aws_stack.mock_aws_request_headers Refactor aws_stack.mock_aws_request_headers() Oct 30, 2023
@coveralls
Copy link

Coverage Status

coverage: 82.467% (-0.02%) from 82.488% when pulling 351458d on refactor-mock-aws-headers-helper into 6f976c7 on master.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for churning over all these refactorings to properly use the account ID and regions everywhere! 🧹 🚀

@viren-nadkarni viren-nadkarni merged commit ebf3c2a into master Nov 2, 2023
29 checks passed
@viren-nadkarni viren-nadkarni deleted the refactor-mock-aws-headers-helper branch November 2, 2023 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants