feat: add support for AWS China regions#794
feat: add support for AWS China regions#794danxie1999 wants to merge 3 commits intoawslabs:masterfrom
Conversation
e833280 to
ee3d295
Compare
ee3d295 to
ba03038
Compare
a64c2fa to
72f4e02
Compare
sbkok
left a comment
There was a problem hiding this comment.
Thank you for contributing this feature.
Could you please look at the changes requested and accept/explain or improve those?
Thanks for your help on this.
| CloudTrail](https://docs.aws.amazon.com/organizations/latest/userguide/orgs_incident-response.html) | ||
| for AWS Organizations can only be acted upon in the us-east-1 (North Virginia) | ||
| region. | ||
| or cn-northwest-1 region. |
There was a problem hiding this comment.
Is this correct? This is the first time you reference cn-northwest-1. Everywhere else where us-east-1 was stated in the docs you referenced cn-north-1. Can you please check if this is the right one to use here?
| You don't need to include the main region in this list. For example, if you | ||
| use the example values for the default region and target regions, it will allow | ||
| pipelines to deploy to `eu-west-1`, `eu-central-`, and `us-east-1`. | ||
| pipelines to deploy to `eu-west-1`, `eu-central-`, `cn-north-1` and `us-east-1`. |
There was a problem hiding this comment.
AWS does not support deploying cross multiple partitions as far as I know.
Listing cn-north-1 here makes it look like ADF does support this. I would recommend to remove cn-north-1 in this line, and fix the missing number on eu-central- with this suggestion:
| pipelines to deploy to `eu-west-1`, `eu-central-`, `cn-north-1` and `us-east-1`. | |
| pipelines to deploy to `eu-west-1`, `eu-central-1`, and `us-east-1`. |
| # fix the china org service endpoint issue | ||
| _china_region_extra_deploy() |
There was a problem hiding this comment.
| # fix the china org service endpoint issue | |
| _china_region_extra_deploy() | |
| if REGION_DEFAULT == CHINA_PRIMARY_REGION: | |
| # Fix China partition organization service endpoint issue | |
| _china_region_extra_deploy() |
| def _china_region_extra_deploy(): | ||
| if REGION_DEFAULT == CHINA_PRIMARY_REGION: | ||
| parameters = [ | ||
| { | ||
| 'ParameterKey': 'AccountBootstrapingStateMachineArn', | ||
| 'ParameterValue': ACCOUNT_BOOTSTRAPPING_STATE_MACHINE_ARN, | ||
| 'UsePreviousValue': False, | ||
| }, | ||
| { | ||
| 'ParameterKey': 'AdfLogLevel', | ||
| 'ParameterValue': ADF_LOG_LEVEL, | ||
| 'UsePreviousValue': False, | ||
| }, | ||
| ] | ||
| try: | ||
| s3_china = S3( | ||
| region=REGION_DEFAULT, | ||
| bucket=S3_BUCKET_NAME | ||
| ) | ||
| cloudformation = CloudFormation( | ||
| region=CHINA_SECONDARY_REGION, | ||
| deployment_account_region=CHINA_SECONDARY_REGION, | ||
| role=boto3, | ||
| wait=True, | ||
| stack_name=ADF_REGIONAL_BASE_CHINA_EXTRA_STACK_NAME, | ||
| s3=s3_china, | ||
| s3_key_path='adf-build', | ||
| account_id=MANAGEMENT_ACCOUNT_ID, | ||
| template_file_prefix=CHINA_SECONDARY_REGION_DEPLOY_TEMP, | ||
| parameters=parameters | ||
|
|
||
| ) | ||
| cloudformation.create_stack() | ||
| except Exception as error: | ||
| LOGGER.error( | ||
| "China extra stack adf-regional-base-china-extra deployment failed in region " | ||
| "%(region)s, please check following error: %(error)s", | ||
| { | ||
| "region": CHINA_SECONDARY_REGION, | ||
| "error": str(error), | ||
| }, | ||
| ) | ||
| sys.exit(2) |
There was a problem hiding this comment.
Moving the if statement to the location where we call this method.
| def _china_region_extra_deploy(): | |
| if REGION_DEFAULT == CHINA_PRIMARY_REGION: | |
| parameters = [ | |
| { | |
| 'ParameterKey': 'AccountBootstrapingStateMachineArn', | |
| 'ParameterValue': ACCOUNT_BOOTSTRAPPING_STATE_MACHINE_ARN, | |
| 'UsePreviousValue': False, | |
| }, | |
| { | |
| 'ParameterKey': 'AdfLogLevel', | |
| 'ParameterValue': ADF_LOG_LEVEL, | |
| 'UsePreviousValue': False, | |
| }, | |
| ] | |
| try: | |
| s3_china = S3( | |
| region=REGION_DEFAULT, | |
| bucket=S3_BUCKET_NAME | |
| ) | |
| cloudformation = CloudFormation( | |
| region=CHINA_SECONDARY_REGION, | |
| deployment_account_region=CHINA_SECONDARY_REGION, | |
| role=boto3, | |
| wait=True, | |
| stack_name=ADF_REGIONAL_BASE_CHINA_EXTRA_STACK_NAME, | |
| s3=s3_china, | |
| s3_key_path='adf-build', | |
| account_id=MANAGEMENT_ACCOUNT_ID, | |
| template_file_prefix=CHINA_SECONDARY_REGION_DEPLOY_TEMP, | |
| parameters=parameters | |
| ) | |
| cloudformation.create_stack() | |
| except Exception as error: | |
| LOGGER.error( | |
| "China extra stack adf-regional-base-china-extra deployment failed in region " | |
| "%(region)s, please check following error: %(error)s", | |
| { | |
| "region": CHINA_SECONDARY_REGION, | |
| "error": str(error), | |
| }, | |
| ) | |
| sys.exit(2) | |
| def _china_region_extra_deploy(): | |
| parameters = [ | |
| { | |
| 'ParameterKey': 'AccountBootstrapingStateMachineArn', | |
| 'ParameterValue': ACCOUNT_BOOTSTRAPPING_STATE_MACHINE_ARN, | |
| 'UsePreviousValue': False, | |
| }, | |
| { | |
| 'ParameterKey': 'AdfLogLevel', | |
| 'ParameterValue': ADF_LOG_LEVEL, | |
| 'UsePreviousValue': False, | |
| }, | |
| ] | |
| try: | |
| s3_china = S3( | |
| region=REGION_DEFAULT, | |
| bucket=S3_BUCKET_NAME | |
| ) | |
| cloudformation = CloudFormation( | |
| region=CHINA_SECONDARY_REGION, | |
| deployment_account_region=CHINA_SECONDARY_REGION, | |
| role=boto3, | |
| wait=True, | |
| stack_name=ADF_REGIONAL_BASE_CHINA_EXTRA_STACK_NAME, | |
| s3=s3_china, | |
| s3_key_path='adf-build', | |
| account_id=MANAGEMENT_ACCOUNT_ID, | |
| template_file_prefix=CHINA_SECONDARY_REGION_DEPLOY_TEMP, | |
| parameters=parameters | |
| ) | |
| cloudformation.create_stack() | |
| except Exception as error: | |
| LOGGER.error( | |
| "China extra stack adf-regional-base-china-extra deployment failed in region " | |
| "%(region)s, please check following error: %(error)s", | |
| { | |
| "region": CHINA_SECONDARY_REGION, | |
| "error": str(error), | |
| }, | |
| ) | |
| sys.exit(2) |
| self.template_url_from_template_file_prefix = self.s3.fetch_s3_url( | ||
| self._create_template_path(self.s3_key_path, template_file_prefix) | ||
| ) \ | ||
| if template_file_prefix else None |
There was a problem hiding this comment.
| self.template_url_from_template_file_prefix = self.s3.fetch_s3_url( | |
| self._create_template_path(self.s3_key_path, template_file_prefix) | |
| ) \ | |
| if template_file_prefix else None | |
| self.template_url_from_template_file_prefix = ( | |
| self.s3.fetch_s3_url( | |
| self._create_template_path(self.s3_key_path, template_file_prefix), | |
| ) if template_file_prefix else None | |
| ) |
| - ssm:GetParameter | ||
| Resource: | ||
| - !Sub "arn:${AWS::Partition}:ssm:*:${AWS::AccountId}:parameter/adf/*" | ||
| - !Sub "arn:${AWS::Partition}:ssm:*:${AWS::AccountId}:parameter/*" |
There was a problem hiding this comment.
This should not be changed. Please change the specific parameter you need to read to match the path it is granted access to instead.
| - !Sub "arn:${AWS::Partition}:ssm:*:${AWS::AccountId}:parameter/*" | |
| - !Sub "arn:${AWS::Partition}:ssm:*:${AWS::AccountId}:parameter/adf/*" |
| - Effect: "Allow" | ||
| Action: | ||
| - iam:* | ||
| Resource: | ||
| - "*" |
There was a problem hiding this comment.
I assume this was added to debug something that didn't work.
We cannot add this, that would be way too permissive.
| To check the current version of ADF that you have deployed, go to the management | ||
| account in us-east-1. Check the CloudFormation stack output or tag of the | ||
| `serverlessrepo-aws-deployment-framework` Stack. | ||
| account in us-east-1 or cn-north-1. Check the CloudFormation stack |
There was a problem hiding this comment.
Instead of adding cn-north-1 everywhere where us-east-1 is used, I would like to suggest we add a mapping table at the top to explain what values to use for each partition.
Otherwise we should also add the us-gov region everywhere, in case that partition is used.
For the US-Gov partition, the region they should deploy to is us-gov-west-1.
|
|
||
| - Install AWS Control Tower in `eu-central-1`. | ||
| - Install ADF in `us-east-1`. | ||
| - Install ADF in `us-east-1` or `cn-north-1`. |
There was a problem hiding this comment.
Does that work, having AWS Control Tower deployed in eu-central-1 while deploying ADF in cn-north-1?
| # Assert | ||
| # Remove this line as it's incorrect - we're not calling the mock directly | ||
| # mock_cloudformation.assert_called_once() | ||
|
|
||
| # Instead, check that create_stack was called on the mock instance | ||
| mock_cloudformation.create_stack.assert_called_once() |
There was a problem hiding this comment.
| # Assert | |
| # Remove this line as it's incorrect - we're not calling the mock directly | |
| # mock_cloudformation.assert_called_once() | |
| # Instead, check that create_stack was called on the mock instance | |
| mock_cloudformation.create_stack.assert_called_once() | |
| # Assert | |
| mock_cloudformation.create_stack.assert_called_once() |
Why?
Now the we cannot deploy the AWS Deployment Framework (ADF) in AWS China regions. This pull request introduces support for the AWS China Regions in ADF.
AWS China now have two regions which are cn-north-1 and cn-northwest-1. While the cn-north-1 is the primary region, the AWS Organization events only available in cn-northwest-1.
From this perspective, additional event forwarder is required in cn-northwest-1 (aws-deployment-framework/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/china-support).
Additionally, In China region, the S3 URL is in following format: https://{self.bucket}.s3.{self.region}.{self.domain_suffix}/{key} (aws-deployment-framework/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/python/s3.py ).
What?
Description of changes: