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

feat: support per-request region and service override in signer #1444

Merged
merged 2 commits into from Aug 19, 2020

Conversation

AllanZhengYP
Copy link
Contributor

Description of changes:

The PR makes change to SigV4 signer to support per-request region and service override. Specifically, users can specify signingRegion and signingService when signing a request, string, or event. The signingRegion and signingService will override the region and service configuration specified in SigV4 constructor. The override is only effective for one sign invocation.

In the @aws-sdk/middleware-signing, the signing middleware will inspect the value in the HandlerExecutionContext looking for signing_region and signing_service value. If the values are set by previous middleware, it mean the signer should use a different signing region or signing service for the specified request. So the middleware will call the sign() with the overriden region or service.

This change is useful in infering region or signing service from the operation parameters, e.g. ARN supplied in s3 bucket parameter

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2020

Codecov Report

Merging #1444 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
- Coverage   79.69%   79.65%   -0.05%     
==========================================
  Files         302      296       -6     
  Lines       11402    11392      -10     
  Branches     2438     2435       -3     
==========================================
- Hits         9087     9074      -13     
- Misses       2315     2318       +3     
Impacted Files Coverage Δ
packages/middleware-retry/src/configurations.ts 90.62% <0.00%> (-9.38%) ⬇️
protocol_tests/aws-ec2/endpoints.ts 81.48% <0.00%> (ø)
protocol_tests/aws-json/endpoints.ts 81.48% <0.00%> (ø)
protocol_tests/aws-query/endpoints.ts 81.48% <0.00%> (ø)
protocol_tests/aws-ec2/runtimeConfig.ts 100.00% <0.00%> (ø)
protocol_tests/aws-restxml/endpoints.ts 81.48% <0.00%> (ø)
packages/signature-v4/src/SignatureV4.ts 100.00% <0.00%> (ø)
protocol_tests/aws-json/runtimeConfig.ts 100.00% <0.00%> (ø)
protocol_tests/aws-restjson/endpoints.ts 81.48% <0.00%> (ø)
protocol_tests/aws-query/runtimeConfig.ts 100.00% <0.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5478012...bb36243. Read the comment docs.

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

LGTM, some minor nits

signingService,
} = options;
const credentials = await this.credentialProvider();
const region = signingRegion || (await this.regionProvider());
Copy link
Member

Choose a reason for hiding this comment

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

Use Nullish coalescing operator (??)?

Suggested change
const region = signingRegion || (await this.regionProvider());
const region = signingRegion ?? (await this.regionProvider());

Ditto on other places when || is used: 137, 178, 180 etc

packages/signature-v4/src/SignatureV4.ts Outdated Show resolved Hide resolved
@AllanZhengYP
Copy link
Contributor Author

The codebuild succeeds after retry. See log

@AllanZhengYP AllanZhengYP merged commit a2635af into aws:master Aug 19, 2020
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants