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

Include params set in provide-client-param event handlers in dynamic context params for endpoint resolution #2920

Merged
merged 7 commits into from
May 11, 2023

Conversation

jonemo
Copy link
Contributor

@jonemo jonemo commented Apr 26, 2023

This PR moves the provide-client-params.* and before-parameter-build.* from immediately after endpoint resolution to immediately before endpoint resolution.

This addresses the fact that for endpoint rulesets that accept dynamic context parameters (currently s3, s3-control, kinesis, and events) any parameters changed or updated in an event handler for the provide-client-params.* were not accounted for in endpoint resolution because the event was only emitted after endpoint resolution.

@jonemo jonemo requested a review from nateprewitt April 26, 2023 23:01
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (3f49661) 93.44% compared to head (d8ed89d) 93.41%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2920      +/-   ##
===========================================
- Coverage    93.44%   93.41%   -0.04%     
===========================================
  Files           63       63              
  Lines        13551    13572      +21     
===========================================
+ Hits         12663    12678      +15     
- Misses         888      894       +6     
Impacted Files Coverage Δ
botocore/client.py 97.84% <100.00%> (ø)
botocore/signers.py 96.06% <100.00%> (+0.01%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dlm6693 dlm6693 self-requested a review May 5, 2023 14:06
Copy link
Contributor

@dlm6693 dlm6693 left a comment

Choose a reason for hiding this comment

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

Just a couple of stylistic comments, but otherwise LGTM!

botocore/client.py Show resolved Hide resolved
Comment on lines +915 to +919
api_params = self._emit_api_params(
api_params=api_params,
operation_model=operation_model,
context=request_context,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be called from within _resolve_endpoint_ruleset at the very start of it? I'm thinking in purely organizational terms just because _make_api_call is a rather long, complex function already.

@@ -663,7 +663,11 @@ def generate_presigned_url(
context,
ignore_signing_region=(not bucket_is_arn),
)

params = self._emit_api_params(
Copy link
Contributor

Choose a reason for hiding this comment

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

The only concern I have with this PR is we're moving this into multiple places now making any future changes harder to keep consistent.

It would be nice to keep these consolidated with _convert_to_request_dict, but I don't see an immediate solution for that.

Copy link
Contributor

@dlm6693 dlm6693 May 5, 2023

Choose a reason for hiding this comment

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

I think using my suggestion of putting this in _resolve_endpoint_ruleset would accomplish that no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or does this still need to happen immediately after endpoint resolution for presigned URLs?

@jonemo
Copy link
Contributor Author

jonemo commented May 8, 2023

@dlm6693 @nateprewitt Added two commits that consolidate the previously duplicated code. This comes at the cost of a change to the API of the _convert_to_request_dict method. The signature remains backwards compatible, but the endpoint_url argument is no longer used inside the function. There is a small chance that this will break anyone who relies on this "private" internal interface.

While making this change I also fixed the presign functions which were actually still affected by the original bug and added test coverage for this oversight.

@jonemo jonemo requested review from nateprewitt and dlm6693 May 8, 2023 23:17
@jonemo
Copy link
Contributor Author

jonemo commented May 9, 2023

After reviewing potential impact of the API change contained in bd44524, @nateprewitt and I decided to revert to the less well organized state that has been discussed above. Note that f8e01a6 is not an exact line-for-line revert because it keeps the bugfix described in #2920 (comment).

@jonemo jonemo force-pushed the event-ordering-params-endpoints branch from f8e01a6 to 878cd84 Compare May 9, 2023 21:03
@dlm6693 dlm6693 self-requested a review May 10, 2023 13:57
botocore/signers.py Outdated Show resolved Hide resolved
@dlm6693
Copy link
Contributor

dlm6693 commented May 11, 2023

@jonemo still LGTM after the latest commit 👍

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

:shipit:

@jonemo jonemo merged commit 4f5bc57 into boto:develop May 11, 2023
@jonemo jonemo deleted the event-ordering-params-endpoints branch May 11, 2023 16:38
aws-sdk-python-automation added a commit that referenced this pull request May 11, 2023
* release-1.29.133:
  Bumping version to 1.29.133
  Update to latest partitions and endpoints
  Update to latest models
  Update black formatting
  Include params set in provide-client-param event handlers in dynamic context params for endpoint resolution (#2920)
  Addresses admonition accessibility issue (#2928)
  Update pre-commit hooks versions
kdaily added a commit to kdaily/aws-cli that referenced this pull request May 23, 2024
Emit `provide-client-params.*` and
`before-parameter-build.*` before endpoint
resolution.

This ports the following botocore pull request:

boto/botocore#2920

One update to the endpoint name in the
test_signers.py unit test was applied to use the
region name in the s3 endpoint, consistent with
existing CLI v2 tests.
kdaily added a commit to kdaily/aws-cli that referenced this pull request May 23, 2024
Emit `provide-client-params.*` and
`before-parameter-build.*` before endpoint
resolution.

This ports the following botocore pull request:

boto/botocore#2920

One update to the endpoint name in the
test_signers.py unit test was applied to use the
region name in the s3 endpoint, consistent with
existing CLI v2 tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants