feat(ec2): Prefixlist Constructs #24714

watany-dev · 2023-03-21T07:14:14Z

I created this Construct so that I can use AWS::EC2::PrefixList quickly.

The differences from L1 are as follows:

If AddressFamily is not specified, ipv4 is used by default.
If maxEntries is not specified, the number of entries is automatically calculated from the entries array and set as maxEntries.

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

WinterYukky

Thank you for opening the PR. I am commenting on this as needed to brush up on a few things.

Please note that it is currently recommended to go through the RFC to create new L2 constructs in aws-cdk, and this PR may be closed to go through the recommended flow. For more information, please see the following document
https://github.com/aws/aws-cdk/blob/main/docs/NEW_CONSTRUCTS_GUIDE.md

WinterYukky · 2023-03-24T15:21:05Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+  /**
+   * The name of the prefix list.
+   *
+   * @default None
+   *
+   * @remarks
+   * It is not recommended to use an explicit name.
+   */
+  readonly prefixListName?: string;


To avoid the customer having to specify the physical name, generate the physical name in CloudFormation or CDK (if not generated by CloudFormation) if the physical name is not passed.

example that generate in CDK:

constructor(scope: Construct, id: string, props?: PrefixListProps) { super(scope, id, { physicalName: props.prefixListName ?? Lazy.string({ produce: () => Names.uniqueResourceName(this, { maxLength: 255, allowedSpecialCharacters: '.-_' }), }), }); }

thankyou! fixed.

WinterYukky · 2023-03-24T15:52:10Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+  /**
+   * The Name of the Prefix List
+   *
+   */
+  public readonly name: string;


prefixListName

WinterYukky · 2023-03-24T15:52:26Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+   * The ARN of the Prefix List
+   *
+   */
+  public readonly arn: string;


prefixListArn

WinterYukky · 2023-03-24T16:09:16Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+    const prefixList = new CfnPrefixList(this, 'PrefixList', {
+      addressFamily: props.addressFamily || AddressFamily.IP_V4,
+      maxEntries: props.maxEntries || props.entries!.length,
+      prefixListName: this.name,
+      entries: props.entries || [],
+    });
+
+    this.prefixListId = prefixList.attrPrefixListId;
+    this.arn = prefixList.attrArn;
+    this.ownerId = prefixList.attrOwnerId;
+    this.version = prefixList.attrVersion;
+    this.addressFamily = prefixList.addressFamily;
+
+    this.node.defaultChild = prefixList;


The ID of the resource that will be the defaultChild should be 'Resource' and do not set the defaultChild directly as in this.node.defaultChild = prefixList;.

WinterYukky · 2023-03-24T16:10:26Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+      };
+    };
+
+    this.name = props?.prefixListName || id;


this.prefixListName = this.physicalName;

WinterYukky · 2023-03-24T16:13:11Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

+    if (!props?.maxEntries && !props?.entries) {
+      throw new Error('Set maxEntries or enrties.');
+    }


What about setting maxEntry to 1 if neither is provided, I expected to be able to create it without setting anything as props is optional, but an error was thrown and I felt sad :(.

WinterYukky · 2023-03-24T16:17:21Z

packages/@aws-cdk/aws-ec2/test/prefix-list.test.ts

+    expect(() => {
+      new PrefixList(stack, 'prefix-list', {
+        maxEntries: 100,
+        prefixListName: 'ThesupercalifragilisticexpialidociouswordisusedtodescribesomethingthatisextraordinarilygoodItwaspopularizedinthemovieMaryPoppinswhichwasreleasedin1964ThewordismadeupofacombinationofLatinandGreekrootsandithassincebecomeapartofpopularcultureWhileitmaybe difficulttospellitisafunwordtosayandcanbeusedtoimpressyourfriendsSothenexttimeyouwanttodescribesomethingasamazingtryusingthewordsupercalifragilisticexpialidocious',


It is difficult to see that the given prefixListName is more than 256 characters, so try the following.

new PrefixList(stack, 'prefix-list', { maxEntries: 100, prefixListName: 'x'.repeat(256), });

WinterYukky · 2023-03-24T16:22:07Z

packages/@aws-cdk/aws-ec2/README.md

+
+## Managed prefix lists
+
+The following demonstrates how to enable [Detailed Monitoring](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-cloudwatch-new.html) for an EC2 instance. Keep in mind that Detailed Monitoring results in [additional charges](http://aws.amazon.com/cloudwatch/pricing/).


Why is a description of detailed monitoring included?

I'm sorry. This is the "original description". :)

WinterYukky · 2023-03-24T16:27:53Z

packages/@aws-cdk/aws-ec2/lib/prefix-list.ts

Basically, only capitalise the first letter of the sentence within the comment to ensure consistency in naming.

Examples:
The Id of the Prefix List => The ID of the prefix list
The OwnerId of the Prefix List => The owner ID of the prefix list
Prefix List, prefixlist => prefix list

Thank you. I have fixed it as much as I could.

WinterYukky · 2023-03-24T16:30:24Z

packages/@aws-cdk/aws-ec2/README.md

+The following demonstrates how to enable [Detailed Monitoring](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-cloudwatch-new.html) for an EC2 instance. Keep in mind that Detailed Monitoring results in [additional charges](http://aws.amazon.com/cloudwatch/pricing/).
+
+```ts
+new ec2.PrefixList(stack, 'empty-prefix-list', {


ID should use the Pascal case (at least in this document).

aws-cdk-automation · 2023-03-25T00:09:39Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

watany-dev · 2023-03-25T01:35:01Z

Thank you for the great comments that helped to resolve the issue. I have made some minor corrections. I will address the remaining comments later.

aws-cdk-automation · 2023-04-03T00:08:52Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation

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.

aws-cdk-automation · 2023-04-03T13:21:05Z

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

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 Exemption Request and/or Clarification Request.

aws-cdk-automation · 2023-04-03T14:07:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 2bb49bf
Result: SUCCEEDED
Build Logs (available for 30 days)

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

@WinterYukky

I accidentally broke the branch while trying to resolve conflicts #24714 . I have addressed all of the comments made by @WinterYukky who reviewed my code. --- I created this Construct so that I can use AWS::EC2::PrefixList quickly. The differences from L1 are as follows: - If AddressFamily is not specified, ipv4 is used by default. - If maxEntries is not specified, the number of entries is automatically calculated from the entries array and set as maxEntries. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Mar 21, 2023

aws-cdk-automation requested a review from a team March 21, 2023 07:14

WinterYukky suggested changes Mar 24, 2023

View reviewed changes

watany-dev requested a review from WinterYukky March 31, 2023 01:58

watany-dev force-pushed the prefixlist branch from a25f086 to 9fa7ba0 Compare April 3, 2023 13:18

watany-dev closed this Apr 3, 2023

watany-dev force-pushed the prefixlist branch from 9fa7ba0 to 2bb49bf Compare April 3, 2023 13:19

aws-cdk-automation requested changes Apr 3, 2023

View reviewed changes

watany-dev mentioned this pull request Apr 3, 2023

feat(ec2): Prefixlist Constructs #24914

Closed

watany-dev mentioned this pull request Apr 23, 2023

feat(ec2): Prefixlist Constructs #25252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): Prefixlist Constructs #24714

feat(ec2): Prefixlist Constructs #24714

watany-dev commented Mar 21, 2023 •

edited

Loading

WinterYukky left a comment •

edited

Loading

WinterYukky Mar 24, 2023

watany-dev Mar 30, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 30, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

WinterYukky Mar 24, 2023 •

edited

Loading

watany-dev Mar 29, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 30, 2023

WinterYukky Mar 24, 2023

watany-dev Mar 25, 2023

aws-cdk-automation commented Mar 25, 2023

watany-dev commented Mar 25, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation left a comment

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 3, 2023


		## Managed prefix lists

		The following demonstrates how to enable [Detailed Monitoring](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-cloudwatch-new.html) for an EC2 instance. Keep in mind that Detailed Monitoring results in [additional charges](http://aws.amazon.com/cloudwatch/pricing/).

feat(ec2): Prefixlist Constructs #24714

feat(ec2): Prefixlist Constructs #24714

Conversation

watany-dev commented Mar 21, 2023 • edited Loading

WinterYukky left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinterYukky Mar 24, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 25, 2023

watany-dev commented Mar 25, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 3, 2023

AWS CodeBuild CI Report

watany-dev commented Mar 21, 2023 •

edited

Loading

WinterYukky left a comment •

edited

Loading

WinterYukky Mar 24, 2023 •

edited

Loading