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.

Sign up for GitHub

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

Jump to bottom

feat(events-targets): support assignPublicIp flag to EcsTask #25660

Merged
merged 8 commits into from
Jun 12, 2023
Merged

feat(events-targets): support assignPublicIp flag to EcsTask #25660

merged 8 commits into from
Jun 12, 2023

Conversation

ymhiroki
Copy link
Contributor

@ymhiroki ymhiroki commented May 20, 2023 •
edited
Loading

This feature supports assignPublicIp to EcsTask.

It specifies whether the task's elastic network interface receives a public IP address.
You can enable it only when LaunchType is FARGATE.
In this commit, the choice logic of the LaunchType keeps the backwards compatibility.

Closes #9233

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

@gitpod-io
Copy link

gitpod-io bot commented May 20, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team May 20, 2023 09:08
@github-actions github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels May 20, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 20, 2023
corymhall
corymhall previously requested changes May 30, 2023
View reviewed changes
packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-events-targets/test/ecs/event-rule-target.test.ts Outdated
@@ -537,74 +537,6 @@ test('Can use same fargate taskdef multiple times in a rule', () => {
}))).not.toThrow();
});

test('Isolated subnet does not have AssignPublicIp=true', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not need to remove any tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I mistakenly removed the test. It's recovered.

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts Outdated
@@ -201,14 +211,19 @@ export class EcsTask implements events.IRuleTarget {
const tagList = this.tags;

const subnetSelection = this.props.subnetSelection || { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS };
const assignPublicIp = subnetSelection.subnetType === ec2.SubnetType.PUBLIC ? 'ENABLED' : 'DISABLED';

const assignPublicIp = (this.assignPublicIp ?? subnetSelection.subnetType == ec2.SubnetType.PUBLIC) ? 'ENABLED' : 'DISABLED';
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the case where assignPublicIp is set to true, but the subnetSelection is not public? Should we throw an error in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The public IP addresses can be enabled even in the private subnets. (of course, it's impossible to communicate with the Internet and a meaningless operation.)
I think we shouldn't throw an error in the case since it's the valid setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if the user explicitly sets assignPublicIp=true then we should throw an error if using private subnets. The user is clearly intending to have a public service or is making a misconfiguration. In either case we should throw an error.

@corymhall corymhall self-assigned this May 30, 2023
@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 1, 2023
@mergify mergify bot dismissed corymhall’s stale review June 2, 2023 05:39

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 2, 2023
corymhall
corymhall previously requested changes Jun 5, 2023
View reviewed changes
packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts Outdated
@@ -201,14 +211,19 @@ export class EcsTask implements events.IRuleTarget {
const tagList = this.tags;

const subnetSelection = this.props.subnetSelection || { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS };
const assignPublicIp = subnetSelection.subnetType === ec2.SubnetType.PUBLIC ? 'ENABLED' : 'DISABLED';

const assignPublicIp = (this.assignPublicIp ?? subnetSelection.subnetType == ec2.SubnetType.PUBLIC) ? 'ENABLED' : 'DISABLED';
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
const assignPublicIp = (this.assignPublicIp ?? subnetSelection.subnetType == ec2.SubnetType.PUBLIC) ? 'ENABLED' : 'DISABLED';
const assignPublicIp = (this.assignPublicIp ?? subnetSelection.subnetType === ec2.SubnetType.PUBLIC) ? 'ENABLED' : 'DISABLED';

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts Outdated

const assignPublicIp = (this.assignPublicIp ?? subnetSelection.subnetType == ec2.SubnetType.PUBLIC) ? 'ENABLED' : 'DISABLED';
const launchType = this.taskDefinition.isEc2Compatible ? 'EC2' : 'FARGATE';
if (assignPublicIp == 'ENABLED' && launchType != 'FARGATE') {
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
if (assignPublicIp == 'ENABLED' && launchType != 'FARGATE') {
if (assignPublicIp === 'ENABLED' && launchType !== 'FARGATE') {

@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 5, 2023
@aws-cdk-automation
Copy link
Collaborator

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.

2 similar comments
@aws-cdk-automation
Copy link
Collaborator

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

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.

@mergify mergify bot dismissed corymhall’s stale review June 8, 2023 02:56

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 8, 2023
@ymhiroki ymhiroki requested a review from corymhall June 8, 2023 03:58
@mergify
Copy link
Contributor

mergify bot commented Jun 12, 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: 184a2eb
  • 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 37f1eb0 into aws:main Jun 12, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 12, 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).

@filletofish filletofish mentioned this pull request Jul 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corymhall corymhall corymhall approved these changes

Assignees

@corymhall corymhall

Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[aws-events-targets] Add flag for Auto-assign public IP to EcsTask
3 participants
@ymhiroki @aws-cdk-automation @corymhall