fix: (aws-ec2): fix vpc endpoint incorrect issue in China region #16139

readybuilderone · 2021-08-20T09:14:21Z

fix:(aws-ec2): fix vpc endpoint incorrect issue in China region

This PR fix the issue that can't create interface vpc endpoint for 40+ services like ecr, ec2, athena etc

Closes: #9864

Considerations

In cn-north-1 region, there are:

2 services whose endpoint service begin with "aws.sagemaker" prefix;
41 services whose endpoint service begin with "cn.com.amazonaws" prefix;
21 services whose endpoint service begin with "com.amazonaws" prefix;
Details: https://gist.github.com/readybuilderone/d355f9f8f0f2b66379a10742b3c67cc7

in cn-northwest-1 region, there are:

2 services whose endpoint service begin with "aws.sagemaker" prefix;
44 services whose endpoint service begin with "cn.com.amazonaws" prefix;
21 services whose endpoint service begin with "com.amazonaws" prefix;
Details: https://gist.github.com/readybuilderone/a79f2c5e6fa02aae1699bf674b08be7c

So, In cn-north-1 and cn-northwest-1, the vpc endpoint prefix is both region and service related.

At first, I found the vpc endpoint prefix could be fetched via AWS API , however the javascript sdk supports only asynchronous call, currently I didn't find any pattern in CDK to make live calls in object construction. Since the exception rules won't ever change ( only be added to), and the rate of change should be small in the grand scheme, I just kept the exception prefix lists in a map as fact data.

Known Issues

Since the interface vpc endpoints prefixs are not region agnostic, it requires to set the {region} in the environment to create stacks in cn-north-1, and cn-northwest-1. The methods could be found in the offical guide.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…ws#9864

gitpod-io · 2021-08-20T09:14:23Z

mergify · 2021-08-20T09:14:40Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

pahud · 2021-08-21T16:48:02Z

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

@@ -326,9 +326,32 @@ export class InterfaceVpcEndpointAwsService implements IInterfaceVpcEndpointServ
    const region = Lazy.uncachedString({
      produce: (context) => Stack.of(context.scope).region,
    });
-    this.name = `${prefix || 'com.amazonaws'}.${region}.${name}`;
+    const regionPrefix =Lazy.uncachedString({


Suggested change

const regionPrefix =Lazy.uncachedString({

const defaultEndpointPrefix =Lazy.uncachedString({

How about just call it defaultEndpointPrefix ?

Thanks @pahud, that's a good suggestion, I'll update it.

pahud · 2021-08-21T16:48:37Z

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

+      'cn-north-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'iotsitewise.api', 'iotsitewise.data', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'xray'],
+      'cn-northwest-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'workspaces', 'xray'],


Let's wrap the lines to make it more readable.

good suggestions, thanks Pahud

pahud · 2021-08-21T16:50:13Z

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

+  /**
+   * Get the endpoint prefix for the service in the specified region
+   * because the prefix for some of the services in cn-north-1 and cn-northwest-1 are different
+   */


Maybe we can add an AWS CLI sample in the doc comments showing how to get all endpoints from a region. That would be helpful.

ok, I'll add the AWS CLI sample to the comments

pahud · 2021-08-21T16:57:35Z

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

+      'cn-north-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'iotsitewise.api', 'iotsitewise.data', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'xray'],
+      'cn-northwest-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'workspaces', 'xray'],
+    };
+    if (VPC_ENDPOINT_SERVICE_EXCEPTIONS.hasOwnProperty(region) && VPC_ENDPOINT_SERVICE_EXCEPTIONS[region].includes(name)) {


Suggested change

if (VPC_ENDPOINT_SERVICE_EXCEPTIONS.hasOwnProperty(region) && VPC_ENDPOINT_SERVICE_EXCEPTIONS[region].includes(name)) {

if (VPC_ENDPOINT_SERVICE_EXCEPTIONS[region] && VPC_ENDPOINT_SERVICE_EXCEPTIONS[region].includes(name)) {

I believe this should be working.

It will be more concise, thanks Pahud for your suggestion, I'll update it

pahud

just some minor suggested changes.

…CEPTIONS has the property of region

…efix function

…readybuilderone/aws-cdk into fix-vpc-endpoint-issue-in-china

readybuilderone · 2021-08-22T14:35:05Z

Thanks @pahud for review my PR, I have updated my code according to your suggestions.
@njlynch can you help to assign a SDE to review my PR? Thanks.

njlynch

Thanks @readybuilderone , and thanks @pahud for the first-round review!

I think it's worth getting the transcribe edge case right before we merge this; otherwise it looks great.

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

njlynch · 2021-08-23T09:32:34Z

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

+        'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder',
+        'iotsitewise.api', 'iotsitewise.data', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring',
+        'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime',
+        'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn',


! I think transcribe.cn is an additional edge case. Based on the data here, it looks like it's the single case of a region-specific suffix. As-is, the name input to this function will never be transcribe.cn, so the exception will never be properly picked up. I guess we could create a corresponding getDefaultEndpointSuffix with just transcribe as the exception? What do you think?

You're right, the .cn suffix should be handled for transcribe in China region.

this.name = `${prefix || defaultEndpointPrefix}.${region}.${name}${defaultEndpointSuffix}`;

I'll implement the getDefaultEndpointSuffix function and add the relative unit tests.

I have updated my implementation and added the unit tests. Am I doing the right way? @njlynch

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Pull request has been modified.

@njlynch

It's really a great way to use "?.includes" to check the property exist, I have learned it, thanks @njlynch! Co-authored-by: Nick Lynch <nlynch@amazon.com>

packages/@aws-cdk/aws-ec2/test/vpc-endpoint.test.ts

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

njlynch

Looks great, thanks!

Pull request has been modified.

pahud · 2021-08-24T05:14:28Z

Looks like there is a validate-pr waiting for approval for first-time contributor and probably need @njlynch kick it off.

readybuilderone · 2021-08-24T06:15:59Z

Looks like there is a validate-pr waiting for approval for first-time contributor and probably need @njlynch kick it off.

Yes, this is my first PR to CDK, thanks @pahud and @njlynch so much!
I'm not quite clear about the process, so just want to confirm with @njlynch , is there anything I should do before the approval?

mergify · 2021-08-24T09:14:03Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

njlynch · 2021-08-24T09:15:18Z

is there anything I should do before the approval?

Nope, you're good. I just had to re-click the approve button, as the review was dismissed after the merge. It's in the queue to be automatically merged now, and should be merged shortly.

aws-cdk-automation · 2021-08-24T10:08:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 105846d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-08-24T10:09:06Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…#16139) fix:(aws-ec2): fix vpc endpoint incorrect issue in China region This PR fix the issue that can't create interface vpc endpoint for 40+ services like ecr, ec2, athena etc Closes: aws#9864 ---- # Considerations 1. In cn-north-1 region, there are: - 2 services whose endpoint service begin with "aws.sagemaker" prefix; - 41 services whose endpoint service begin with "cn.com.amazonaws" prefix; - 21 services whose endpoint service begin with "com.amazonaws" prefix; Details: https://gist.github.com/readybuilderone/d355f9f8f0f2b66379a10742b3c67cc7 2. in cn-northwest-1 region, there are: - 2 services whose endpoint service begin with "aws.sagemaker" prefix; - 44 services whose endpoint service begin with "cn.com.amazonaws" prefix; - 21 services whose endpoint service begin with "com.amazonaws" prefix; Details: https://gist.github.com/readybuilderone/a79f2c5e6fa02aae1699bf674b08be7c So, In cn-north-1 and cn-northwest-1, the vpc endpoint prefix is both region and service related. At first, I found the vpc endpoint prefix could be fetched via [AWS API](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeVpcEndpoints.html) , however the javascript sdk supports only asynchronous call, currently I didn't find any pattern in CDK to make live calls in object construction. Since the exception rules won't ever change ( only be added to), and the rate of change should be small in the grand scheme, I just kept the exception prefix lists in a map as fact data. # Known Issues Since the interface vpc endpoints prefixs are not region agnostic, it requires to set the {region} in the environment to create stacks in cn-north-1, and cn-northwest-1. The methods could be found in the [offical guide](https://docs.aws.amazon.com/cdk/latest/guide/environments.html). *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix:(aws-ec2): fix vpc endpoint incorrect issue in China region. Fix a…

c796fc6

…ws#9864

readybuilderone changed the title ~~fix:(aws-ec2): fix vpc endpoint incorrect issue in China region~~ fix: (aws-ec2): fix vpc endpoint incorrect issue in China region Aug 20, 2021

readybuilderone and others added 2 commits August 20, 2021 17:56

fix unit test of vpc interface endpoint creation

66e623b

Merge branch 'master' into fix-vpc-endpoint-issue-in-china

e2e20a4

pahud reviewed Aug 21, 2021

View reviewed changes

pahud requested changes Aug 21, 2021

View reviewed changes

readybuilderone and others added 6 commits August 22, 2021 09:29

rename variable regionPrefix to defaultEndpointPrefix

c191933

update to a more concise way to check whether VPC_ENDPOINT_SERVICE_EX…

9aded26

…CEPTIONS has the property of region

add comments to getDefaultEndpointPrefix, and wrapped the lines

a27acc1

remove traing spaces and adjust the indention in getDefaultEndpointPr…

4fac703

…efix function

Merge branch 'fix-vpc-endpoint-issue-in-china' of https://github.com/…

63f167a

…readybuilderone/aws-cdk into fix-vpc-endpoint-issue-in-china

Merge branch 'master' into fix-vpc-endpoint-issue-in-china

fe93aa8

njlynch previously requested changes Aug 23, 2021

View reviewed changes

Update packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

b37b4c9

Co-authored-by: Nick Lynch <nlynch@amazon.com>

readybuilderone and others added 2 commits August 23, 2021 19:50

Update packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts

f606f18

It's really a great way to use "?.includes" to check the property exist, I have learned it, thanks @njlynch! Co-authored-by: Nick Lynch <nlynch@amazon.com>

Add logic to handle suffix '.cn' for transcribe service in China region

138c50f

njlynch reviewed Aug 23, 2021

View reviewed changes

packages/@aws-cdk/aws-ec2/test/vpc-endpoint.test.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-ec2/lib/vpc-endpoint.ts Outdated Show resolved Hide resolved

Minor tweaks

88999ab

njlynch previously approved these changes Aug 23, 2021

View reviewed changes

Merge branch 'master' into fix-vpc-endpoint-issue-in-china

20f6008

njlynch approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into fix-vpc-endpoint-issue-in-china

105846d

mergify bot merged commit 0d0db38 into aws:master Aug 24, 2021

This was referenced Jan 28, 2022

fix(aws-ec2): update vpc service endpoints in china regions #14721

Closed

[aws-ec2] The static vars defined in ec2.InterfaceVpcEndpointAwsService do not work in 'cn' regions #11140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (aws-ec2): fix vpc endpoint incorrect issue in China region #16139

fix: (aws-ec2): fix vpc endpoint incorrect issue in China region #16139

readybuilderone commented Aug 20, 2021 •

edited

gitpod-io bot commented Aug 20, 2021

mergify bot commented Aug 20, 2021

pahud Aug 21, 2021

readybuilderone Aug 22, 2021

pahud Aug 21, 2021

readybuilderone Aug 22, 2021

pahud Aug 21, 2021

readybuilderone Aug 22, 2021

pahud Aug 21, 2021

readybuilderone Aug 22, 2021

pahud left a comment

readybuilderone commented Aug 22, 2021

njlynch left a comment

njlynch Aug 23, 2021

readybuilderone Aug 23, 2021

readybuilderone Aug 23, 2021

njlynch left a comment

pahud commented Aug 24, 2021

readybuilderone commented Aug 24, 2021

mergify bot commented Aug 24, 2021

njlynch commented Aug 24, 2021

aws-cdk-automation commented Aug 24, 2021

mergify bot commented Aug 24, 2021

	const regionPrefix =Lazy.uncachedString({
	const defaultEndpointPrefix =Lazy.uncachedString({

		'cn-north-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'iotsitewise.api', 'iotsitewise.data', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'xray'],
		'cn-northwest-1': ['application-autoscaling', 'athena', 'autoscaling', 'awsconnector', 'cassandra', 'cloudformation', 'codedeploy-commands-secure', 'databrew', 'dms', 'ebs', 'ec2', 'ecr.api', 'ecr.dkr', 'elasticbeanstalk', 'elasticfilesystem', 'elasticfilesystem-fips', 'execute-api', 'imagebuilder', 'kinesis-streams', 'lambda', 'license-manager', 'monitoring', 'rds', 'redshift', 'redshift-data', 's3', 'sagemaker.api', 'sagemaker.featurestore-runtime', 'sagemaker.runtime', 'servicecatalog', 'sms', 'sqs', 'states', 'sts', 'synthetics', 'transcribe.cn', 'transcribestreaming', 'transfer', 'workspaces', 'xray'],

	if (VPC_ENDPOINT_SERVICE_EXCEPTIONS.hasOwnProperty(region) && VPC_ENDPOINT_SERVICE_EXCEPTIONS[region].includes(name)) {
	if (VPC_ENDPOINT_SERVICE_EXCEPTIONS[region] && VPC_ENDPOINT_SERVICE_EXCEPTIONS[region].includes(name)) {

fix: (aws-ec2): fix vpc endpoint incorrect issue in China region #16139

fix: (aws-ec2): fix vpc endpoint incorrect issue in China region #16139

Conversation

readybuilderone commented Aug 20, 2021 • edited

Considerations

Known Issues

gitpod-io bot commented Aug 20, 2021

mergify bot commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pahud left a comment

Choose a reason for hiding this comment

readybuilderone commented Aug 22, 2021

njlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlynch left a comment

Choose a reason for hiding this comment

pahud commented Aug 24, 2021

readybuilderone commented Aug 24, 2021

mergify bot commented Aug 24, 2021

njlynch commented Aug 24, 2021

aws-cdk-automation commented Aug 24, 2021

AWS CodeBuild CI Report

mergify bot commented Aug 24, 2021

readybuilderone commented Aug 20, 2021 •

edited