fix(ecs): nat network mode for windows tasks #4317

kolomied · 2019-10-01T14:37:23Z

Add NAT network mode to support Windows container networking.
This is the only network mode supported by Windows.
See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/windows_task_IAM_roles.html for additional details.

fixes #4272

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

Add NAT network mode to support Windows container networking. This is the only network mode supported by Windows. See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/windows_task_IAM_roles.html for additional details. fixes aws#4272

mergify · 2019-10-01T14:37:59Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-01T14:46:40Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-01T15:27:36Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

kolomied · 2019-10-01T16:17:08Z

@SoManyHs I'm afraild I need you help here - here is the error from the build:

Installing from NPM...
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @aws-cdk/aws-ec2@^1.10.1.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget 
npm ERR! notarget It was specified as a dependency of '@aws-cdk/aws-servicediscovery'
npm ERR! notarget 

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2019-10-01T15_27_25_592Z-debug.log

[Container] 2019/10/01 15:27:25 Command did not exit successfully /bin/bash ./build.sh exit status 1
[Container] 2019/10/01 15:27:25 Phase complete: BUILD State: FAILED
[Container] 2019/10/01 15:27:25 Phase context status code: COMMAND_EXECUTION_ERROR Message: Error while executing command: /bin/bash ./build.sh. Reason: exit status 1

Just successfully re-run ./build.sh locally without any errors. I'm puzzled because I didn't change ec2 module at all.

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

iamhopaul123 · 2019-10-01T17:02:20Z

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

+  }
+
+  private renderNetworkMode(networkMode: NetworkMode): string | undefined {
+    // 'NAT' is the only supported network mode for Windows containers,


Since it is also explained in the ENUM def, I think you can combine those lines of comment into the ENUM one.

Sure, makes sense. Thanks!

aws-cdk-automation · 2019-10-02T08:27:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-02T08:33:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk · 2019-10-02T17:32:40Z

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

@@ -428,6 +428,14 @@ export class TaskDefinition extends TaskDefinitionBase {
  private findContainer(containerName: string): ContainerDefinition | undefined {
    return this.containers.find(c => c.containerName === containerName);
  }
+
+  private isEphemeralPortMappingSupported(networkMode: NetworkMode): boolean {
+    return networkMode === NetworkMode.BRIDGE || networkMode === NetworkMode.NAT;


nit: personally, I'd prefer these conditions in the if statement itself, just to allow for ease of debugging.

No problem, will address that

aws-cdk-automation · 2019-10-02T18:05:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-02T18:46:53Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-02T20:41:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

…k into kolomied/nat-networkmode

aws-cdk-automation · 2019-10-02T21:46:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk

Ship it! @kolomied, thanks for taking the time to fix this issue!

aws-cdk-automation · 2019-10-03T16:55:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-10-03T16:55:45Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-03T17:27:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

kolomied requested a review from SoManyHs as a code owner October 1, 2019 14:37

remove unnecessary whitespaces

f50e3ac

piradeepk self-assigned this Oct 1, 2019

iamhopaul123 reviewed Oct 1, 2019

View reviewed changes

Dmitry Kolomiets and others added 2 commits October 2, 2019 08:55

PR review feedback - consolidate the comments.

c6812c3

Merge branch 'master' into kolomied/nat-networkmode

1be7f87

rix0rrr self-assigned this Oct 2, 2019

rix0rrr requested a review from piradeepk October 2, 2019 08:24

piradeepk reviewed Oct 2, 2019

View reviewed changes

Merge branch 'master' into kolomied/nat-networkmode

6eae70a

Merge branch 'master' into kolomied/nat-networkmode

cb71a29

piradeepk added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Oct 2, 2019

Merge branch 'master' into kolomied/nat-networkmode

63e09ff

Dmitry Kolomiets added 2 commits October 2, 2019 22:12

PR feedback - simplify ephemeral port mapping logic

fd83aef

Merge branch 'kolomied/nat-networkmode' of github.com:kolomied/aws-cd…

88eedd3

…k into kolomied/nat-networkmode

Merge branch 'master' into kolomied/nat-networkmode

0fc3e07

piradeepk approved these changes Oct 3, 2019

View reviewed changes

Merge branch 'master' into kolomied/nat-networkmode

c6a1ea7

mergify bot merged commit 9ceb995 into aws:master Oct 3, 2019

kolomied deleted the kolomied/nat-networkmode branch October 3, 2019 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs): nat network mode for windows tasks #4317

fix(ecs): nat network mode for windows tasks #4317

kolomied commented Oct 1, 2019

mergify bot commented Oct 1, 2019

aws-cdk-automation commented Oct 1, 2019

aws-cdk-automation commented Oct 1, 2019

kolomied commented Oct 1, 2019

iamhopaul123 Oct 1, 2019

kolomied Oct 1, 2019

aws-cdk-automation commented Oct 2, 2019

aws-cdk-automation commented Oct 2, 2019

piradeepk Oct 2, 2019

kolomied Oct 2, 2019

aws-cdk-automation commented Oct 2, 2019

aws-cdk-automation commented Oct 2, 2019

aws-cdk-automation commented Oct 2, 2019

aws-cdk-automation commented Oct 2, 2019

piradeepk left a comment

aws-cdk-automation commented Oct 3, 2019

mergify bot commented Oct 3, 2019

aws-cdk-automation commented Oct 3, 2019

fix(ecs): nat network mode for windows tasks #4317

fix(ecs): nat network mode for windows tasks #4317

Conversation

kolomied commented Oct 1, 2019

mergify bot commented Oct 1, 2019

aws-cdk-automation commented Oct 1, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 1, 2019

AWS CodeBuild CI Report

kolomied commented Oct 1, 2019

iamhopaul123 Oct 1, 2019

Choose a reason for hiding this comment

kolomied Oct 1, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

piradeepk Oct 2, 2019

Choose a reason for hiding this comment

kolomied Oct 2, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

piradeepk left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 3, 2019

AWS CodeBuild CI Report

mergify bot commented Oct 3, 2019

aws-cdk-automation commented Oct 3, 2019

AWS CodeBuild CI Report