feat(kinesisanalytics-flink): VPC support for Flink applications #24442

mitchlloyd · 2023-03-03T14:23:15Z

The Kinesis Data Analytics team added support for deploying Flink applications in a VPC. This feature is also available in CloudFormation. Deploying Flink in a VPC allows the application to reach services like Redis and other databases.

This PR adds support for configuring VpcConfigurations with vpcSubets (subnetSelection) and securityGroups following similar patterns for resources like lambda.Function that support optional deployment in a VPC.

Some design decisions:

Name the subnet selection prop vpcSubnets. Some resources call the subnet selection property subnetSelection but vpcSubnets seemed more popular and is used by the Lambda and ECS modules.
Only support passing an array of security groups. Some resources support adding a single SecurityGroup or SecurityGroupId properties but it appears this usage is deprecated in favor of always passing an array of SecurityGroups.
I added a fromApplicationAttributes factory that includes securityGroups. This seemed like an appropriate time to add this method given there was another property to pass besides ARN and name. However I didn't go down the path of including a role in fromApplicationAttributes yet in order to keep this PR focused.
I thought about adding a section to the readme about using VPCs, but I didn't notice a section like that in the Lambda readme for instance. My current thinking is that the conventions for VPC-bound resources are so consistent it probably doesn't warrant more documentation @aws-cdk-automation did not buy this rational.

I'd like to follow-up with a PR to move code into more files as the > 1K lines of code in application.ts is getting a little unweildy. I wanted to avoid moving code around in this PR to make it easier to review.

Closes #21104.

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

Looking around the code base vpcSubnets seems to be the most popular name for this property, used by ecs Fargate and Lambda.

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.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

comcalvi

I have a few questions about the design, but overall looks pretty good. Please remove all changes to yarn.lock from this PR, we have automation to handle those.

comcalvi · 2023-03-06T18:23:33Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

+  /**
+   * Choose which VPC subnets to use.
+   *
+   * @default SubnetType.PRIVATE_WITH_EGRESS subnets


Suggested change

* @default SubnetType.PRIVATE_WITH_EGRESS subnets

* @default - SubnetType.PRIVATE_WITH_EGRESS subnets

comcalvi · 2023-03-06T18:23:40Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

+  /**
+   * Deploy the Flink application in a VPC.
+   *
+   * @default no VPC


Suggested change

* @default no VPC

* @default - no VPC

I just pushed a change which adds hyphens for these kinds of @default phrases to the entire file.

comcalvi · 2023-03-06T18:23:48Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

+  /**
+   * Security groups to use with a provided VPC.
+   *
+   * @default a new security group is created for this application.


Suggested change

* @default a new security group is created for this application.

* @default - a new security group is created for this application.

comcalvi · 2023-03-06T18:25:36Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

-  constructor(scope: Construct, id: string, attrs: { applicationArn: string, applicationName: string }) {
+  constructor(scope: Construct, id: string, attrs: { applicationArn: string, securityGroups?: ec2.ISecurityGroup[] }) {


why has applicationName been removed?

I'd rather avoid removing this arg entirely, as doing so is a breaking change

Import is an internal class that has never been exported so I don't think it's a breaking change. I think you may have missed that this code is in the Import class because the diff view isn't wide enough to show the class Import declaration.

ahh I see, yep I'd missed that. Thanks

comcalvi · 2023-03-06T18:26:01Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

-    this.applicationName = attrs.applicationName;
+    const applicationName = core.Stack.of(scope).splitArn(attrs.applicationArn, core.ArnFormat.SLASH_RESOURCE_NAME).resourceName;
+    if (!applicationName) {
+      throw new Error(`applicationArn for fromApplicationArn (${attrs.applicationArn}) must include resource name`);
+    }
+    this.applicationName = applicationName;


why are we getting this from the arn now? This worked when it was in the Import class, because that ARN was always an actual ARN. This ARN is almost certainly not an actual string, it's instead a Token, which means we can't validate this at synth time

Before this code was in the Application.fromApplicationName static method and now it's in the Import constructor. So I think this will still work. I think this is the same code path as before but with a slight refactoring.

Also I have tests for fromApplicationName and applicationArn that didn't change in this PR. Let me know if there is some edge case I'm missing.

Pull request has been modified.

mitchlloyd · 2023-03-06T19:54:19Z

@comcalvi, thanks for the review!

I added some changes to:

Add hyphens when using @default followed by some phrase
Rest the lock file to what's on the main branch this PR is based on

And I replied to your comments about the changes to the Import class. This is ready for another look when you get a chance.

comcalvi

lgtm, nice work!

comcalvi · 2023-03-08T21:17:17Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

-   * @default sum over 5 minutes
+   * @default - sum over 5 minutes


thanks for all of these!

comcalvi · 2023-03-08T21:17:47Z

packages/@aws-cdk/aws-kinesisanalytics-flink/lib/application.ts

-  constructor(scope: Construct, id: string, attrs: { applicationArn: string, applicationName: string }) {
+  constructor(scope: Construct, id: string, attrs: { applicationArn: string, securityGroups?: ec2.ISecurityGroup[] }) {


ahh I see, yep I'd missed that. Thanks

mergify · 2023-03-08T21:22:46Z

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-cdk-automation · 2023-03-08T22:02:31Z

AWS CodeBuild CI Report

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

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

mergify · 2023-03-08T22:02:54Z

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-cdk-automation

…#24442) The Kinesis Data Analytics team added support for [deploying Flink applications in a VPC](https://docs.aws.amazon.com/kinesisanalytics/latest/java/vpc.html). This feature is also available in CloudFormation. Deploying Flink in a VPC allows the application to reach services like Redis and other databases. This PR adds support for configuring `VpcConfigurations` with `vpcSubets` (subnetSelection) and securityGroups following similar patterns for resources like `lambda.Function` that support optional deployment in a VPC. Some design decisions: - Name the subnet selection prop `vpcSubnets`. Some resources call the subnet selection property `subnetSelection` but `vpcSubnets` seemed more popular and is used by the Lambda and ECS modules. - Only support passing an array of security groups. Some resources support adding a single SecurityGroup or SecurityGroupId properties but it appears this [usage is deprecated](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-lambda/lib/function.ts#L170) in favor of always passing an array of SecurityGroups. - I added a `fromApplicationAttributes` factory that includes `securityGroups`. This seemed like an appropriate time to add this method given there was another property to pass besides ARN and name. However I didn't go down the path of including a role in `fromApplicationAttributes` yet in order to keep this PR focused. - ~~I thought about adding a section to the readme about using VPCs, but I didn't notice a section like that in the [Lambda readme](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-lambda/README.md) for instance. My current thinking is that the conventions for VPC-bound resources are so consistent it probably doesn't warrant more documentation~~ @aws-cdk-automation did not buy this rational. I'd like to follow-up with a PR to move code into more files as the > 1K lines of code in `application.ts` is getting a little unweildy. I wanted to avoid moving code around in this PR to make it easier to review. Closes aws#21104. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Mitchell Lloyd added 8 commits March 3, 2023 08:26

Add vpc prop

04464cb

Add securityGroups option

88f03af

Add vpcSubnets option

985d40a

Looking around the code base vpcSubnets seems to be the most popular name for this property, used by ecs Fargate and Lambda.

Add connections

c0a1318

Validate vpc prop combinations

a6acadc

Add connection error tests

ed87d70

Clean up lint errors and refine wording

e5110f1

Add vpc integ test

6432960

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Mar 3, 2023

aws-cdk-automation requested a review from a team March 3, 2023 14:23

aws-cdk-automation previously requested changes Mar 3, 2023

View reviewed changes

Update README

82e453e

Add ec2 to Rosetta fixture

5cf2e59

comcalvi previously requested changes Mar 6, 2023

View reviewed changes

Add many more hyphens in JSDoc

8b6ed6f

mitchlloyd force-pushed the flink-vpc branch from e56d680 to 8b6ed6f Compare March 6, 2023 19:43

Reset yarn.lock changes

6042760

comcalvi approved these changes Mar 8, 2023

View reviewed changes

Merge branch 'main' into flink-vpc

4f3db20

mergify bot merged commit 7c7ad6d into aws:main Mar 8, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kinesisanalytics-flink): VPC support for Flink applications #24442

feat(kinesisanalytics-flink): VPC support for Flink applications #24442

mitchlloyd commented Mar 3, 2023 •

edited

Loading

aws-cdk-automation left a comment

comcalvi left a comment

comcalvi Mar 6, 2023

comcalvi Mar 6, 2023

mitchlloyd Mar 6, 2023

comcalvi Mar 6, 2023

comcalvi Mar 6, 2023

mitchlloyd Mar 6, 2023

comcalvi Mar 8, 2023

comcalvi Mar 6, 2023

mitchlloyd Mar 6, 2023

mitchlloyd commented Mar 6, 2023

comcalvi left a comment

comcalvi Mar 8, 2023

comcalvi Mar 8, 2023

mergify bot commented Mar 8, 2023

aws-cdk-automation commented Mar 8, 2023

mergify bot commented Mar 8, 2023

	* @default SubnetType.PRIVATE_WITH_EGRESS subnets
	* @default - SubnetType.PRIVATE_WITH_EGRESS subnets

	* @default a new security group is created for this application.
	* @default - a new security group is created for this application.

		constructor(scope: Construct, id: string, attrs: { applicationArn: string, applicationName: string }) {
		constructor(scope: Construct, id: string, attrs: { applicationArn: string, securityGroups?: ec2.ISecurityGroup[] }) {

		* @default sum over 5 minutes
		* @default - sum over 5 minutes

feat(kinesisanalytics-flink): VPC support for Flink applications #24442

feat(kinesisanalytics-flink): VPC support for Flink applications #24442

Conversation

mitchlloyd commented Mar 3, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

comcalvi left a 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

mitchlloyd commented Mar 6, 2023

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 8, 2023

aws-cdk-automation commented Mar 8, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 8, 2023

mitchlloyd commented Mar 3, 2023 •

edited

Loading