-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(core): automatic cross stack, cross region references (under feature flag) #22008
Conversation
This PR adds the ability to automatically create references in cross-region stacks. You can now do something like ```ts const stack1 = new Stack(app, 'Stack1', { env: { region: 'us-east-1' } }); const cert = new certificatemanager.Certificate(stack1, 'Cert', {...}); const stack2 = new Stack(app, 'Stack2', { env: { region: 'us-east-2' } }); new cloudfront.Distribution(stack2, 'Distro', { certificate: cert, }) ``` To accomplish this, I've added a new construct `ExportsReader` which creates a Lambda backed custom resource. This custom resource will return all of the CloudFormation exports in a given region. Given the above example, this would create an output in `stack1` ```json { "Outputs": { "ExportsOutputRefCert5C9FAEC110AE5C6A": { "Value": { "Ref": "Cert5C9FAEC1" }, "Export": { "Name": "East1:ExportsOutputRefCert5C9FAEC110AE5C6A" } } } } ``` And then an "import" in `stack2` which references the `ExportReader` ```json { "Resources": { "Distro87EBE6BA": { "Type": "AWS::CloudFront::Distribution", "Properties": { "DistributionConfig": { "ViewerCertificate": { "AcmCertificateArn": { "Fn::GetAtt": [ "ExportsReaderuseast1D746CBDB", "East1:ExportsOutputRefCert5C9FAEC110AE5C6A" ] } } } } } } ``` Currently this will create a single ExportsReader per region, but we could potentially update this to just use a single ExportsReader which can list exports from a list of regions. Future extensions: - Could be updated to do cross account references as well - Could be used to implement weak references
@@ -0,0 +1,65 @@ | |||
import { Queue, IQueue } from '@aws-cdk/aws-sqs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the integration test here since we can't have integration tests in core
and there is a precedent for doing it this way.
|
||
const artifact = new Artifact(); | ||
const pipeline = new Pipeline(stack2, 'Pipeline', { | ||
crossRegionReplicationBuckets: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created this integration test based off of a common cross region scenario
aws-cdk/packages/@aws-cdk/aws-codepipeline/README.md
Lines 320 to 374 in 1f03e8c
### Creating an encrypted replication bucket | |
If you're passing a replication bucket created in a different stack, | |
like this: | |
```ts | |
// Passing a replication bucket created in a different stack. | |
const app = new App(); | |
const replicationStack = new Stack(app, 'ReplicationStack', { | |
env: { | |
region: 'us-west-1', | |
}, | |
}); | |
const key = new kms.Key(replicationStack, 'ReplicationKey'); | |
const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', { | |
// like was said above - replication buckets need a set physical name | |
bucketName: PhysicalName.GENERATE_IF_NEEDED, | |
encryptionKey: key, // does not work! | |
}); | |
// later... | |
new codepipeline.Pipeline(replicationStack, 'Pipeline', { | |
crossRegionReplicationBuckets: { | |
'us-west-1': replicationBucket, | |
}, | |
}); | |
``` | |
When trying to encrypt it | |
(and note that if any of the cross-region actions happen to be cross-account as well, | |
the bucket *has to* be encrypted - otherwise the pipeline will fail at runtime), | |
you cannot use a key directly - KMS keys don't have physical names, | |
and so you can't reference them across environments. | |
In this case, you need to use an alias in place of the key when creating the bucket: | |
```ts | |
// Passing an encrypted replication bucket created in a different stack. | |
const app = new App(); | |
const replicationStack = new Stack(app, 'ReplicationStack', { | |
env: { | |
region: 'us-west-1', | |
}, | |
}); | |
const key = new kms.Key(replicationStack, 'ReplicationKey'); | |
const alias = new kms.Alias(replicationStack, 'ReplicationAlias', { | |
// aliasName is required | |
aliasName: PhysicalName.GENERATE_IF_NEEDED, | |
targetKey: key, | |
}); | |
const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', { | |
bucketName: PhysicalName.GENERATE_IF_NEEDED, | |
encryptionKey: alias, | |
}); | |
``` |
packages/@aws-cdk/core/lib/custom-resource-provider/export-reader-provider.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/core/lib/custom-resource-provider/export-reader-provider.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/core/lib/custom-resource-provider/export-reader-provider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pull Request Linter fails with the following errors:
❌ Features must contain a change to a README file.
PRs must pass status checks before we can provide a meaningful review.
Pull Request updated. Dissmissing previous PRLinter Review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pull Request Linter fails with the following errors:
❌ Features must contain a change to a README file.
PRs must pass status checks before we can provide a meaningful review.
Pull Request updated. Dissmissing previous PRLinter Review.
const account = process.env.CDK_INTEG_ACCOUNT ?? process.env.CDK_DEFAULT_ACCOUNT; | ||
const hostedZoneId = process.env.CDK_INTEG_HOSTED_ZONE_ID ?? process.env.HOSTED_ZONE_ID; | ||
if (!hostedZoneId) throw new Error('For this test you must provide your own HostedZoneId as an env var "HOSTED_ZONE_ID"'); | ||
const hostedZoneName = process.env.CDK_INTEG_HOSTED_ZONE_NAME ?? process.env.HOSTED_ZONE_NAME; | ||
if (!hostedZoneName) throw new Error('For this test you must provide your own HostedZoneName as an env var "HOSTED_ZONE_NAME"'); | ||
const domainName = process.env.CDK_INTEG_DOMAIN_NAME ?? process.env.DOMAIN_NAME; | ||
if (!domainName) throw new Error('For this test you must provide your own Domain Name as an env var "DOMAIN_NAME"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof! Can we not create a zone as part of the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to have a valid public zone one way or another. We could create one in the test, but in order for it to be valid you would have to get the NS records and update the parent hosted zone.
packages/@aws-cdk/core/README.md
Outdated
You can enable the feature flag `@aws-cdk/core:enableCrossRegionReferencesUsingCustomResources` | ||
in order to access resources in a different stack _and_ region. With this feature flag | ||
enabled it is possible to do something like creating a CloudFront distribution in `us-east-2` and | ||
an ACM certificate in `us-east-1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the CloudFront README (as well)
packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-ssm-reader-handler/index.ts
Outdated
Show resolved
Hide resolved
case 'Create': | ||
console.info('Tagging SSM Parameter imports'); | ||
await addTags(ssm, imports, keyName); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need to read the value THROUGH a { Fn::GetAtt }
of this resource, to make sure the tagging happens before the value is consumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that we want to tag the resource after it is consumed. That way if it is not consumed (maybe the stack operation fails) we don't add the tags. Although I guess in that case we would rollback this resource as well which should remove the tags.
Also, if we read the value through this resource then in order for updates to be consumed then this resource needs to be updated. Right now I guess that is not a problem since we do not allow updates to exported values, but if we introduced weak references in the future that might be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alth ough I guess in that case we would rollback this resource as well which should remove the tags.
👆 this
If consuming and tagging are not connected, then we might have a window where a parameter is consumed and not tagged, which is dangerous because it might be replaced in that window.
Also, if we read the value through this resource then in order for updates to be consumed then this resource needs to be updated.
We could solve that by sending the SSM values in as Properties
-- that way if the value changes, the CR is invalidated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could solve that by sending the SSM values in as Properties -- that way if the value changes, the CR is invalidated.
genius!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this is updated.
packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-ssm-reader-handler/index.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-ssm-reader-handler/index.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-ssm-reader-handler/index.ts
Outdated
Show resolved
Hide resolved
await putParameters(ssm, newExports); | ||
return; | ||
case 'Delete': | ||
// consuming stack will delete parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be safe to delete parameters that are not in use, right? I'm afraid we might leak them otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would leak them until the consuming stack is deleted since the consuming stack will delete all of it's parameters (by path prefix). As it is we wouldn't have a list of all parameters that this resource has ever created, just what it had on the last update. We could store the state somewhere (another parameter?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would be able to delete the parameters from the producing stack if we only ever supported strong references (since the stack deployment will fail if you try to delete an imported parameter), but if we want to one day support weak references we need to "leak" the parameters from the producing stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know which references are strong or weak beforehand, so we can apply different rules for them.
Right now, if the producing stack fails to create for some reason and rolls back (or the consumer fails to create) we never get to deploy the consumer and we will leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I reworked this. Since we want to fail the stack operation if we are trying to update/delete exports that are in use, i've moved the deletion to the ExportWriter
. The ExportReader
no longer deletes parameters, it will only add/remove tags.
- moving feature flag to stack property - renaming some things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final change request: turn optInToCrossRegionReferences
into crossRegionReferences
?
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Where does it say what the feature flag is named? |
This sounds great. Could something like this also help us with cross-stage dependencies? |
@corymhall Standard CloudFormation exports also cannot be deleted if they are in use. (If you try, the entire stack update/delete is immediately aborted.) However, since your approach models these exports as custom resources, that means they won't be deleted until the end of the deployment. Furthermore, CloudFormation basically ignores failures that occur during the UPDATE_COMPLETE_CLEANUP_IN_PROGRESS phase, meaning that the custom resource (and anything it manages) will just get leaked. Is this analysis correct? On a related note, I think you may need something like |
I take it this does not apply if the two cross-region stacks are deployed through separate CDK apps ? |
This PR adds the ability to automatically create references in cross-region stacks. You can now do something like
The above is a good example of the motivation behind this feature. A CloudFront distribution is a global resource and can be created in a CloudFormation stack in any region. Other resources, like the ACM certificate, that need to be attached to the CloudFront distribution can only be created in us-east-1. Another example is the
CloudFront.EdgeFunction
where we use a support stack and a custom resource to lookup the value.To accomplish this, I've added two new constructs
ExportsWriter
&ExportReader
. These constructs create Lambda backed custom resources.ExportWriter
responsibilitiesExportReader
responsibilitiesI am currently using
/cdk/exports/${consumingStackName}/
as the SSM path prefix to create all the exports.Given the above example, this would create an output in
stack1
And then an "import" in
stack2
which is a dynamic ssm reference.Currently this will create a single ExportsWriter per region, but we could potentially update this to just use a single ExportsWriter which can write exports to a list of regions.
Future extensions:
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license