-
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: implement node runtime region awareness for cdk vended custom resources #30108
Conversation
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 has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
type: scope.coreInternal | ||
? CORE_INTERNAL_CR_PROVIDER.CustomResourceProviderOptions | ||
: CORE_MODULE.CustomResourceProviderOptions, |
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 is one example of what the ModuleImporter
class helps with. We can maintain one module, e.g., CORE_MODULE
, defined with all of it's types, expressions, etc. rather than needing to define a duplicate module just based on where the module would be imported from, i.e. core vs. aws-lambda. Instead, when registering an import we can just register the same module with a different fromLocation
specified. Now the code looks a lot cleaner and we maintain a single module definition.
/** | ||
* External modules that this class depends on. | ||
*/ | ||
protected abstract readonly externalModules: ExternalModule[]; | ||
|
||
private importExternalModulesInto(scope: HandlerFrameworkModule) { | ||
for (const module of this.externalModules) { | ||
if (!scope.hasExternalModule(module)) { | ||
scope.addExternalModule(module); | ||
this.importExternalModuleInto(scope, module); | ||
} | ||
} | ||
} | ||
|
||
private importExternalModuleInto(scope: HandlerFrameworkModule, module: ExternalModule) { | ||
switch (module.fqn) { | ||
case PATH_MODULE.fqn: { | ||
PATH_MODULE.import(scope, 'path'); | ||
return; | ||
} | ||
case CONSTRUCTS_MODULE.fqn: { | ||
CONSTRUCTS_MODULE.importSelective(scope, ['Construct']); | ||
return; | ||
} | ||
case CORE_MODULE.fqn: { | ||
CORE_MODULE.importSelective(scope, [ | ||
'Stack', | ||
'CustomResourceProviderBase', | ||
'CustomResourceProviderOptions', | ||
]); | ||
return; | ||
} | ||
case CORE_INTERNAL_CR_PROVIDER.fqn: { | ||
CORE_INTERNAL_CR_PROVIDER.importSelective(scope, [ | ||
'CustomResourceProviderBase', | ||
'CustomResourceProviderOptions', | ||
]); | ||
return; | ||
} | ||
case CORE_INTERNAL_STACK.fqn: { | ||
CORE_INTERNAL_STACK.importSelective(scope, ['Stack']); | ||
return; | ||
} | ||
case LAMBDA_MODULE.fqn: { | ||
LAMBDA_MODULE.import(scope, 'lambda'); | ||
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.
Lots of reduced code here due to ModuleImporter
class managing imports
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 overall name of the framework was changed from HandlerFramework to ProviderFramework. This is because the framework is being used to code generate custom resource providers. The custom resource providers execute handler code that this framework has the responsibility of bundling, but calling this a HandlerFramework isn't accurate.
Not in fact true, as far as I'm aware. We call it "custom resource handlers" etc, but I'm pretty sure it also contains Lambda Handlers that don't go into custom resources. Yes, 90% of them are custom resources, but not 100%. So calling this all "custom resources" is a mental shortcut.
/** | ||
* The latest Lambda NodeJS runtime available in a given region. | ||
*/ | ||
public static readonly LATEST_NODE_RUNTIME = 'latestNodeRuntime'; |
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 also would expect this version to be used for lambda.Runtime.LATEST_NODEJS
packages/aws-cdk-lib/custom-resources/lib/provider-framework/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.
Lot's of good improvements to the codegen here that makes it more robust. I'll leave others to approve but I think this handles the move back to a "deploy time" computed runtime value well. 👍🏻
✅ Updated pull request passes all PRLinter validations. Dismissing 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.
Looking good, would like to go over it with you in more depth. Only small thing I noticed is we should add isVariable: true
to the runtime created by the determineLatestNodeRuntime()
function in runtime.ts
.
8305b3a
to
73379cf
Compare
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 has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
73379cf
to
df9727c
Compare
Signed-off-by: Francis <[email protected]>
Signed-off-by: Francis <[email protected]>
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Signed-off-by: Francis <[email protected]>
Signed-off-by: Francis <[email protected]>
Signed-off-by: Francis <[email protected]>
Signed-off-by: Francis <[email protected]>
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). |
…esources (aws#30108) This PR introduces node runtime region awareness into the custom resource provider framework by doing the following: - Adding a `LATEST_NODE_RUNTIME_MAP` fact table used to maintain the latest node runtime available per AWS partition - Introducing `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` functions which determine the latest Lambda node `Runtime` and the latest Lambda node `Runtime` name, respectively - Updating the custom resource provider framework to utilize these two functions when code generating the runtime property for providers using node runtimes This PR can be segmented into the following code changes: - A `LATEST_NODE_RUNTIME_MAP` fact table was added which maintains the latest Lambda node runtime available per AWS partition. - Introduced `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` functions which determine the latest Lambda node `Runtime` and the latest Lambda node `Runtime` name, respectively. - The existing runtime property being code generated via the custom resource provider framework has been altered to now use the appropriate runtime determiner function, i.e., `determineLatestNodeRuntimeName` for `CustomResourceProvider` or `determineLatestNodeRuntime` for `Function` or `SingletonFunction`. Any custom resource provider using a python runtime will not use either of these functions. - To consolidate and manage importing of external modules a `ModuleImporter` class has been created. This class allows external modules to be registered as an import for a target module and prevents duplicate imports for modules that contain multiple framework components. This class also provides the ability to specify different import paths which allows all external modules to be consolidated into a single class rather than having duplicate external modules defined for different import paths. Lastly, this class determines whether or not external modules should be imported selectively or if all targets in the external module should be imported under an alias, i.e., `import { Function } from 'aws-lambda'` vs. `import * as lambda from 'aws-lambda'` - A `CallableExpr` class was created to allow expression proxies to be created from a specified expression name. This allows the new runtime determiner functions and other module specific functions to be called from their specified module and built into a JavaScript object that will mirror the JavaScript operations done to it in an expression tree. Manually tested all individual custom resource handlers for create, update, and delete. I verified that the behavior seen was what was expected based off of the handler code. All impacted integ tests were updated. New unit tests were added to the custom resource handler framework to test that generated code correctly included `determineLatestNodeRuntime` for Lambda based handlers and `determineLatestNodeRuntimeName` for `CfnResource` based handlers. Added new unit tests to test the functionality of `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` for region agnostic stacks, stacks in commercial region, stacks in China regions, stacks in ADC regions, and stacks in GovCloud regions. - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…esources (aws#30108) ### Reason for this change This PR introduces node runtime region awareness into the custom resource provider framework by doing the following: - Adding a `LATEST_NODE_RUNTIME_MAP` fact table used to maintain the latest node runtime available per AWS partition - Introducing `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` functions which determine the latest Lambda node `Runtime` and the latest Lambda node `Runtime` name, respectively - Updating the custom resource provider framework to utilize these two functions when code generating the runtime property for providers using node runtimes ### Description of changes This PR can be segmented into the following code changes: - A `LATEST_NODE_RUNTIME_MAP` fact table was added which maintains the latest Lambda node runtime available per AWS partition. - Introduced `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` functions which determine the latest Lambda node `Runtime` and the latest Lambda node `Runtime` name, respectively. - The existing runtime property being code generated via the custom resource provider framework has been altered to now use the appropriate runtime determiner function, i.e., `determineLatestNodeRuntimeName` for `CustomResourceProvider` or `determineLatestNodeRuntime` for `Function` or `SingletonFunction`. Any custom resource provider using a python runtime will not use either of these functions. - To consolidate and manage importing of external modules a `ModuleImporter` class has been created. This class allows external modules to be registered as an import for a target module and prevents duplicate imports for modules that contain multiple framework components. This class also provides the ability to specify different import paths which allows all external modules to be consolidated into a single class rather than having duplicate external modules defined for different import paths. Lastly, this class determines whether or not external modules should be imported selectively or if all targets in the external module should be imported under an alias, i.e., `import { Function } from 'aws-lambda'` vs. `import * as lambda from 'aws-lambda'` - A `CallableExpr` class was created to allow expression proxies to be created from a specified expression name. This allows the new runtime determiner functions and other module specific functions to be called from their specified module and built into a JavaScript object that will mirror the JavaScript operations done to it in an expression tree. ### Description of how you validated changes Manually tested all individual custom resource handlers for create, update, and delete. I verified that the behavior seen was what was expected based off of the handler code. All impacted integ tests were updated. New unit tests were added to the custom resource handler framework to test that generated code correctly included `determineLatestNodeRuntime` for Lambda based handlers and `determineLatestNodeRuntimeName` for `CfnResource` based handlers. Added new unit tests to test the functionality of `determineLatestNodeRuntime` and `determineLatestNodeRuntimeName` for region agnostic stacks, stacks in commercial region, stacks in China regions, stacks in ADC regions, and stacks in GovCloud regions. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Reason for this change
This PR introduces node runtime region awareness into the custom resource provider framework by doing the following:
LATEST_NODE_RUNTIME_MAP
fact table used to maintain the latest node runtime available per AWS partitiondetermineLatestNodeRuntime
anddetermineLatestNodeRuntimeName
functions which determine the latest Lambda nodeRuntime
and the latest Lambda nodeRuntime
name, respectivelyDescription of changes
This PR can be segmented into the following code changes:
LATEST_NODE_RUNTIME_MAP
fact table was added which maintains the latest Lambda node runtime available per AWS partition.determineLatestNodeRuntime
anddetermineLatestNodeRuntimeName
functions which determine the latest Lambda nodeRuntime
and the latest Lambda nodeRuntime
name, respectively.determineLatestNodeRuntimeName
forCustomResourceProvider
ordetermineLatestNodeRuntime
forFunction
orSingletonFunction
. Any custom resource provider using a python runtime will not use either of these functions.ModuleImporter
class has been created. This class allows external modules to be registered as an import for a target module and prevents duplicate imports for modules that contain multiple framework components. This class also provides the ability to specify different import paths which allows all external modules to be consolidated into a single class rather than having duplicate external modules defined for different import paths. Lastly, this class determines whether or not external modules should be imported selectively or if all targets in the external module should be imported under an alias, i.e.,import { Function } from 'aws-lambda'
vs.import * as lambda from 'aws-lambda'
CallableExpr
class was created to allow expression proxies to be created from a specified expression name. This allows the new runtime determiner functions and other module specific functions to be called from their specified module and built into a JavaScript object that will mirror the JavaScript operations done to it in an expression tree.Description of how you validated changes
Manually tested all individual custom resource handlers for create, update, and delete. I verified that the behavior seen was what was expected based off of the handler code. All impacted integ tests were updated. New unit tests were added to the custom resource handler framework to test that generated code correctly included
determineLatestNodeRuntime
for Lambda based handlers anddetermineLatestNodeRuntimeName
forCfnResource
based handlers. Added new unit tests to test the functionality ofdetermineLatestNodeRuntime
anddetermineLatestNodeRuntimeName
for region agnostic stacks, stacks in commercial region, stacks in China regions, stacks in ADC regions, and stacks in GovCloud regions.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license