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

Specify global endpoint and signature region for other partitions #3274

Merged
merged 5 commits into from
Jun 16, 2020

Conversation

workeitel
Copy link
Contributor

@workeitel workeitel commented May 24, 2020

So far the global endpoint had us-east-1 for normal partition
hard-coded and it did not work in other partitions like China or us-gov.

#3192

Route53 endpoint pattern (source)

Partition Endpoint Region
aws route53.amazonaws.com us-east-1
aws-cn route53.amazonaws.com.cn cn-northwest-1

IAM endpoint pattern (source)

Partition Endpoint Region
aws iam.amazonaws.com us-east-1
aws-cn iam.cn-north-1.amazonaws.com.cn cn-north-1
aws-us-gov iam.us-gov.amazonaws.com us-gov-west-1
testing

script:

var iam_aws = new AWS.IAM({ region: 'me-south-1', credentials: creds_aws });
iam_aws.listRoles(function(err, data) { console.log({ p: 'aws', data: data, err: err }); });
var route53_aws = new AWS.Route53({ region: 'me-south-1', credentials: creds_aws });
route53_aws.listHostedZones(function(err, data) { console.log({ p: 'aws', data: data, err: err }); });

var iam_aws_cn = new AWS.IAM({ region: 'cn-northwest-1', credentials: creds_aws_cn });
iam_aws_cn.listRoles(function(err, data) { console.log({ p: 'aws-cn', data: data, err: err }); });
var route53_aws_cn = new AWS.Route53({ region: 'cn-north-1', credentials: creds_aws_cn });
route53_aws_cn.listHostedZones(function(err, data) { console.log({ p: 'aws-cn', data: data, err: err }); });

var iam_aws_us_gov = new AWS.IAM({ region: 'us-gov-east-1', credentials: creds_aws_us_gov });
iam_aws_us_gov.listRoles(function(err, data) { console.log({ p: 'aws-us-gov', data: data, err: err }); });
var route53_aws_us_gov = new AWS.Route53({ region: 'us-gov-east-1', credentials: creds_aws_us_gov });
route53_aws_us_gov.listHostedZones(function(err, data) { console.log({ p: 'aws-us-gov', data: data, err: err }); });

before:

{
  p: 'aws-cn',
  err: Error: getaddrinfo ENOTFOUND iam.cn-northwest-1.amazonaws.com.cn
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:66:26) {
    errno: -3008,
    code: 'NetworkingError',
    syscall: 'getaddrinfo',
    hostname: 'iam.cn-northwest-1.amazonaws.com.cn',
    region: 'cn-northwest-1',
    retryable: true,
    time: 2020-05-24T16:46:11.006Z
  }
}
{
  p: 'aws-us-gov',
  err: InvalidClientTokenId: The security token included in the request is invalid
      ...
      at Request.callListeners (/Volumes/unix/aws-sdk-js/lib/sequential_executor.js:116:18) {
    code: 'InvalidClientTokenId',
    time: 2020-05-24T16:46:11.204Z,
    requestId: '0b8388dc-2ce9-40b1-85f4-40d25d5097eb',
    statusCode: 403,
    retryable: false,
    retryDelay: 57.79329267683981
  }
}
{
  p: 'aws-cn',
  err: Error: getaddrinfo ENOTFOUND route53.cn-north-1.amazonaws.com.cn
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:66:26) {
    errno: -3008,
    code: 'NetworkingError',
    syscall: 'getaddrinfo',
    hostname: 'route53.cn-north-1.amazonaws.com.cn',
    region: 'cn-north-1',
    retryable: true,
    time: 2020-05-24T16:46:11.244Z
  }
}
{ p: 'aws', err: null }
{
  p: 'aws-us-gov',
  err: SignatureDoesNotMatch: Credential should be scoped to a valid region, not 'us-gov-east-1'. 
      ...
      at Request.callListeners (/Volumes/unix/aws-sdk-js/lib/sequential_executor.js:116:18) {
    code: 'SignatureDoesNotMatch',
    time: 2020-05-24T16:46:11.483Z,
    requestId: '85819340-2931-414c-931d-59c2996763a6',
    statusCode: 403,
    retryable: false,
    retryDelay: 94.41183963355235
  }
}
{ p: 'aws', err: null }

after:

{ p: 'aws', err: null }
{ p: 'aws-us-gov', err: null }
{ p: 'aws', err: null }
{ p: 'aws-us-gov', err: null }
{ p: 'aws-cn', err: null }
{ p: 'aws-cn', err: null }
Checklist
  • npm run test passes
  • changelog is added, npm run add-change

So far the global endpoint had `us-east-1` for normal partition
hard-coded and it did not work in other partitions like China or us-gov.

aws#3192
@aws-sdk-js-automation

This comment has been minimized.

@aws-sdk-js-automation

This comment has been minimized.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-v2-github
  • Commit ID: 7b98622
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@AllanZhengYP AllanZhengYP self-requested a review June 1, 2020 16:35
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Very solid PR. It's a good start. I have a few questions in the comment

lib/region_config.js Outdated Show resolved Hide resolved
lib/region_config_data.json Show resolved Hide resolved
lib/request.js Outdated
Comment on lines 316 to 317
if (service.isGlobalEndpoint) {
if (service.signatureRegion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

signingRegion can be applicable for non-global regions right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the service is non-global the signingRegion should be equal to the region passed to the client. Either the detected region or the explicitly specified one.

lib/request.js Outdated
if (service.isGlobalEndpoint) region = 'us-east-1';
if (service.isGlobalEndpoint) {
if (service.signatureRegion) {
region = service.signatureRegion;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to fix in this PR but we need to expost signingRegion to the client config later.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-v2-github
  • Commit ID: f52d69a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@trivikr trivikr requested a review from AllanZhengYP June 9, 2020 16:32
@ajredniwja ajredniwja added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jun 11, 2020
@github-actions
Copy link

This PR has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the PR from automatically closing.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 12, 2020
@workeitel
Copy link
Contributor Author

Hey bot! yes, the ticket is still important and all comments addressed. Please don't auto close it

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Jun 13, 2020
@zxkane
Copy link

zxkane commented Jun 15, 2020

Thanks for this PR. It also resolves #3279 and #3296.

@AllanZhengYP
Copy link
Contributor

@workeitel Awesome! I'm going to approve this PR. Can you add the changelog to the PR?

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-v2-github
  • Commit ID: 4e60d66
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the work!

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.

None yet

5 participants