Skip to content
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(kinesisanalytics-flink): VPC support for Flink applications #24442

Merged
merged 13 commits into from
Mar 8, 2023

Conversation

mitchlloyd
Copy link
Contributor

@mitchlloyd mitchlloyd commented Mar 3, 2023

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

Mitchell Lloyd added 8 commits March 3, 2023 08:26
@github-actions 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 aws-cdk-automation requested a review from a team March 3, 2023 14:23
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review March 3, 2023 14:32

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

Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Choose which VPC subnets to use.
*
* @default SubnetType.PRIVATE_WITH_EGRESS subnets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Deploy the Flink application in a VPC.
*
* @default no VPC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default no VPC
* @default - no VPC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -854 to +909
constructor(scope: Construct, id: string, attrs: { applicationArn: string, applicationName: string }) {
constructor(scope: Construct, id: string, attrs: { applicationArn: string, securityGroups?: ec2.ISecurityGroup[] }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why has applicationName been removed?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -862 to +921
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mergify mergify bot dismissed comcalvi’s stale review March 6, 2023 19:33

Pull request has been modified.

@mitchlloyd
Copy link
Contributor Author

@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.

Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, nice work!

Comment on lines -163 to +164
* @default sum over 5 minutes
* @default - sum over 5 minutes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all of these!

Comment on lines -854 to +909
constructor(scope: Construct, id: string, attrs: { applicationArn: string, applicationName: string }) {
constructor(scope: Construct, id: string, attrs: { applicationArn: string, securityGroups?: ec2.ISecurityGroup[] }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2023

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
Copy link
Collaborator

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 mergify bot merged commit 7c7ad6d into aws:main Mar 8, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2023

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).

homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
…#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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-kinesisanalytics-flink: Add support to deploy in VPC
3 participants