feat(core): enable additional metadata collection (under feature flag) #32827
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR does not currently change CLI functionality. The function `convertToCliArgs` is not used in the CLI yet which is why this PR is not fixing a regression. It will eventually be used to strongly-type cli arguments. Previously, aliased commands, like `cdk ack` instead of `cdk acknowledge` would fall through the cracks of the generated convert function. The switch statement was only switching on command names so we would not store any options associated with an aliased command. Specifically, `cdk synth --exclusively` would _not_ store the `exclusively` flag in the ensuing `CliArguments` object because `synth` is an alias. Now we do. This is an additional step forward to being able to use `CliArguments` in `cli.ts` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR does not change the functionality of the CLI (yet)
It does however articulate a schema for what `cdk.json` should look like
in the future. I'm aware that we honor a slightly different set of rules
in `cdk.json` that _is not documented anywhere_, and we will have to
honor those rules ad-hoc. However, this will hopefully move us towards a
strongly-typed future where `cdk.json` contents mirror CLI argument
options.
- global options are specified at the base level of `cdk.json`
- command specific options will be prefixed by their command name. NOTE:
some options are honored at the base level today. I will have to, in a
separate PR, find each of these instances and take care of them but
ensuring we still map them to the correct place in `CliArguments`.
```json
{
"app": "npx ts-node -P tsconfig.json --prefer-ts-exts src/main.ts",
"output": "cdk.out",
"build": "npx projen bundle",
"watch": {
"exclude": [
"README.md",
"cdk*.json",
"**/*.d.ts",
"**/*.js",
"tsconfig.json",
"package*.json",
"yarn.lock",
"node_modules"
]
}
```
This will turn into the following `CliArgument` object:
```ts
{
globalOptions: {
app: 'npx ts-node -P tsconfig.json --prefer-ts-exts src/main.ts',
output: 'cdk.out',
build: 'npx projen bundle',
watch: {
exclude: [
"README.md",
"cdk*.json",
"**/*.d.ts",
"**/*.js",
"tsconfig.json",
"package*.json",
"yarn.lock",
"node_modules",
],
},
};
```
----
*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*
---------
Co-authored-by: Momo Kornher <[email protected]>
### Issue # (if applicable) None ### Reason for this change AWS AppConfig environment supports [deletion protection](https://docs.aws.amazon.com/appconfig/latest/userguide/deletion-protection.html) and this feature is not configurable from AWS CDK. ### Description of changes - Add `DeletionProtectionCheck` enum - Add `deletionProtectionCheck` prop to `EnvironmentOption` There are two entities, `EnvironmentOptions` and `EnvironmentProps`, where `EnvironmentProps` is designed as an extension of `EnvironmentOptions` with the addition of an `application` prop. ```ts export interface EnvironmentProps extends EnvironmentOptions { /** * The application to be associated with the environment. */ readonly application: IApplication; } abstract class ApplicationBase extends cdk.Resource implements IApplication, IExtensible { public addEnvironment(id: string, options: EnvironmentOptions = {}): IEnvironment { return new Environment(this, id, { application: this, ...options, }); } ``` Therefore, the current argument addition has also been made to `EnvironmentOptions`. ### Describe any new or updated permissions being added None ### Description of how you validated changes Add both unit and integ test. ### 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*
GavinZZ
changed the base branch from
yuanhaoz/feat/metadata-collection
to
main
|
Temporarily change the target branch to Note that adding import statements and |
… does not allow IPv6 inbound traffic (under feature flag) (#32765) ### Issue # (if applicable) Closes #32197 . ### Reason for this change Default generated security group ingress rules for open, dual-stack-without-public-ipv4 ALB does not allow IPv6 traffic. Only a rule for IPv4 ingress traffic is added to the security group rules currently. ### Description of changes Introduced a new feature flag which is enabled by default so that default generated security group ingress rules now have an additional rule that allows IPv6 ingress from anywhere. ### Describe any new or updated permissions being added No new IAM permissions. Added IPv6 security group ingress rules for open, internet-facing ALBs if IP address type is `dual-stack-without-public-ipv4` and feature flag is set to `true` (default). ### Description of how you validated changes Added unit test which checks the security group rules for both cases where feature flag is enabled/disabled. Updated integration test snapshot. ### 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) ---- Co-authored-by: Clare Liguori <[email protected]> *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR does not change CLI functionality because we are not using `CliArguments` yet. This PR includes the following related changes: - `CliArguments` are renamed `UserInput` to reflect what the schema represents -- they are options available to be specified via CLI options or `cdk.json`. - the tool previously known as `cli-arg-gen` is now named `user-input-gen` to reflect this change. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…32823) As a product we have standardized on `cdk synth`. This change reflects that while not changing behavior of the CDK CLI at all. It will however matter for the behavior of a future feature where we allow defaults specified in a schematic way in `cdk.json`. The payoff is that instead of requiring `synthesize: { ... }` we instead will require `synth: { ... }`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This does not change CLI functionality because `UserInput` is not in use yet. Since we have full control over `UserInput`, i.e. we control the input functions from the CLI or `cdk.json`, we can rename properties however we like. `_` is a relic of `yargs`, we do not need to maintain that convention. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
### Issue #32210 Closes #32210 ### Reason for this change Incorrect grammar in a SnapStart warning that appears during cdk deployment. ### Description of changes Corrected the line: `SnapStart only support published Lambda versions. Ignore if function already have published versions` to: `SnapStart only supports published Lambda versions. Ignore if function already has published versions.` ### Describe any new or updated permissions being added No permissions changes. ### Description of how you validated changes No testing needed, only changed text in a warning. ### 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*
GavinZZ
force-pushed
the
yuanhaoz/feat/metadata-collection-1
branch
from
ae763b0 to
f9f14d4
Compare
aws-cdk-automation
added
the
pr/needs-maintainer-review
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
moelasmar
reviewed
Jan 10, 2025
| @@ -0,0 +1,96 @@ | |||
| { | |||
There was a problem hiding this comment.
I could not see the metadata resource in this template, although you enabled the ENABLE_ADDITIONAL_METADATA_COLLECTION FF. Is this expected
There was a problem hiding this comment.
This is expected as all of our integration test have analyticsReporting: false to avoid contaminate the actual DB. I enabled to flag to make sure that it does deploy on my local test machine by overriding analyticsReporting to true.
1 task
moelasmar
reviewed
Jan 10, 2025
| nested: { foo: '*' }, | ||
| arr: ['*', '*', '*'], | ||
| str: '*', | ||
| arrOfObjects: [{ foo: { hello: '*' } }, { myFunc: '*' }], |
There was a problem hiding this comment.
add a TODO here to change this test case with your change to only collect data for our types.
moelasmar
approved these changes
Jan 10, 2025
aws-cdk-automation
previously requested changes
Jan 10, 2025
There was a problem hiding this comment.
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.
GavinZZ
added
the
pr-linter/exempt-integ-test
|
Verified that this build successfully. Going to ask Core team for a review and set the target branch to feature branch to merge in the change (instead of merging to main). |
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
GavinZZ
changed the base branch from
main
to
yuanhaoz/feat/metadata-collection
GavinZZ
merged commit 54167b5
into
yuanhaoz/feat/metadata-collection
16 of 20 checks passed
|
Comments on closed issues and PRs are hard for our team to see. |
aws-cdk-automation
requested changes
Jan 13, 2025
There was a problem hiding this comment.
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.
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue # (if applicable)
Closes #.
Reason for this change
Expand the scope of usage data collected by the AWS CDK to better inform CDK development and improve communication for security concerns and emerging issues. Currently, for those that opt in, the CDK collects usage data on your CDK version and which L2 constructs you use. For more information on current CDK behavior, see Version Reporting.
This proposal expands the scope of usage data collection to include the following from L2 constructs in CDK applications:
Description of changes
Update CDK synthesis code to additionally handle resource metadata.
On feature flag set to true, synthesis will not only inject Metadata usage like version and construct name, it will additionally look for any construct/method/feature flag metadata injected during resource creation.
Note that this PR is only part one so we will have follow up PRs to add metadata injection during resource creation.
On feature flag set to false, it should be the same as before.
Describe any new or updated permissions being added
N/A
Description of how you validated changes
New unit tests added.
New integration tests added.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license