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

EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in the task namespace. #2876

Conversation

rawahars
Copy link
Contributor

@rawahars rawahars commented May 21, 2021

Summary

The changes introduced in this PR includes-

  • gMSA support for tasks running in awsvpc network mode on Windows.
  • Support for adding additional local routes in task namespace using ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES environment variable
  • Support for adding explicit IMDS route in task namespace if ECS_AWSVPC_BLOCK_IMDS is false
  • Rectified a minor bug which was causing panic in the agent.

Implementation details

The changes are implemented as -

  • For gMSA support: We store the instance ENI's DNS server list using Win32 IpHelper API call. The same is used while creating CNI plugin configuration. This is possible because both instance ENI and task ENI belong to the same customer VPC.
  • For Additional local routes: We create an add route command and append it to the existing list of commands to be executed inside pause namespace.
  • Similarly IMDS explicit route is created.

Testing

A custom binary was tested for gMSA, additional local routes and IMDS.

New tests cover the changes:
Yes

Description for the changelog

Added gMSA support for tasks in awsvpc network mode and create additional local routes in task namespace.

Licensing

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

@rawahars rawahars requested review from a team May 21, 2021 01:25
Harsh Rawat added 2 commits May 23, 2021 16:42
This is not required as we can use the dns settings of the primary instance ENI.
@rawahars rawahars marked this pull request as draft May 24, 2021 18:56
@rawahars rawahars force-pushed the feature/ec2-awsvpc-windows-task-setup branch 3 times, most recently from f0ce968 to 6785d8d Compare May 24, 2021 23:00
@rawahars rawahars changed the title EC2 task networking on Windows: Add IMDS route to task namespace EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in task namespace May 24, 2021
We use the instance ENIs DNS settings since both the ENIs would be in the same VPC and would be additionally beneficial during domain join.
@rawahars rawahars force-pushed the feature/ec2-awsvpc-windows-task-setup branch from 6785d8d to 7871ec0 Compare May 24, 2021 23:14
@rawahars rawahars marked this pull request as ready for review May 24, 2021 23:20
@rawahars rawahars requested review from angelcar and fenxiong May 24, 2021 23:20
agent/ecscni/namespace_helper_windows.go Outdated Show resolved Hide resolved
agent/app/agent_windows.go Show resolved Hide resolved
agent/app/agent_windows.go Show resolved Hide resolved
agent/ecscni/namespace_helper_windows.go Show resolved Hide resolved
agent/ecscni/namespace_helper_windows.go Show resolved Hide resolved
agent/eni/networkutils/utils_windows.go Show resolved Hide resolved
agent/eni/netwrapper/netwrapper_windows.go Outdated Show resolved Hide resolved
Harsh Rawat added 3 commits June 3, 2021 16:40
…k stats.

Initially, without any network, pause container returns nil network stats. When container stats are collected with pause namespace in place, lastStats.NetworkStats is nil which causes agent to crash and restart.
@rawahars rawahars changed the title EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in task namespace EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in task namespace, and building the vpc-eni plugin for Windows. Jun 4, 2021
@rawahars rawahars requested review from angelcar and fenxiong June 4, 2021 00:53
@rawahars rawahars force-pushed the feature/ec2-awsvpc-windows-task-setup branch from da10ba8 to 6045511 Compare June 5, 2021 19:18
@rawahars rawahars changed the title EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in task namespace, and building the vpc-eni plugin for Windows. EC2 task networking on Windows: gMSA support, adding additional local routes and IMDS route in the task namespace. Jun 5, 2021
@rawahars
Copy link
Contributor Author

rawahars commented Jun 5, 2021

Decoupled CNI plugin build logic from this PR as it was making the entire PR complex.
Those changes will be part of another PR.

Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

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

Changes look good. Left a few minor nits and suggestions.

qq: What happens if the agent starts on an instance before the container instance is part of any valid Active Directory setup? Will AD/gMSA and task networking work in unison?

agent/app/agent_windows.go Outdated Show resolved Hide resolved
agent/app/agent_windows.go Outdated Show resolved Hide resolved
agent/config/types.go Outdated Show resolved Hide resolved
agent/ecscni/namespace_helper_windows.go Show resolved Hide resolved
agent/ecscni/plugin_windows.go Outdated Show resolved Hide resolved
@rawahars
Copy link
Contributor Author

rawahars commented Jun 7, 2021

Changes look good. Left a few minor nits and suggestions.

qq: What happens if the agent starts on an instance before the container instance is part of any valid Active Directory setup? Will AD/gMSA and task networking work in unison?

Whenever the instance is domain joined, the instance is restarted and therefore, the agent on startup will have the updated DNS server list.

Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

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

🚀

@rawahars rawahars force-pushed the feature/ec2-awsvpc-windows-task-setup branch from d2416dd to e8fc5bb Compare June 7, 2021 22:34
@fenxiong fenxiong merged commit da4f2da into aws:feature/awsvpc-windows Jun 9, 2021
rawahars added a commit to rawahars/amazon-ecs-agent that referenced this pull request Jun 23, 2021
… routes and IMDS route in the task namespace. (aws#2876)

* Add IMDS route to the task namespace if task is allowed to have IMDS access.c

* Add support for adding additional local routes in awsvpc network mode on Windows.

* Removed the workflow for getting Primary ipv4 address of the vpc.

This is not required as we can use the dns settings of the primary instance ENI.

* Added gSMA support while using awsvpc network mode.

We use the instance ENIs DNS settings since both the ENIs would be in the same VPC and would be additionally beneficial during domain join.

* PR review changes: Reverted netwrapper to platform agnostic package

* Changes to json format for vpc-eni plugin

* Bug fix to rectify the scenario when pause container sends nil network stats.

Initially, without any network, pause container returns nil network stats. When container stats are collected with pause namespace in place, lastStats.NetworkStats is nil which causes agent to crash and restart.

* Minor changes: Comment changes, logfile path consolidation and variable name change
rawahars added a commit to rawahars/amazon-ecs-agent that referenced this pull request Jun 24, 2021
… routes and IMDS route in the task namespace. (aws#2876)

* Add IMDS route to the task namespace if task is allowed to have IMDS access.c

* Add support for adding additional local routes in awsvpc network mode on Windows.

* Removed the workflow for getting Primary ipv4 address of the vpc.

This is not required as we can use the dns settings of the primary instance ENI.

* Added gSMA support while using awsvpc network mode.

We use the instance ENIs DNS settings since both the ENIs would be in the same VPC and would be additionally beneficial during domain join.

* PR review changes: Reverted netwrapper to platform agnostic package

* Changes to json format for vpc-eni plugin

* Bug fix to rectify the scenario when pause container sends nil network stats.

Initially, without any network, pause container returns nil network stats. When container stats are collected with pause namespace in place, lastStats.NetworkStats is nil which causes agent to crash and restart.

* Minor changes: Comment changes, logfile path consolidation and variable name change
rawahars added a commit to rawahars/amazon-ecs-agent that referenced this pull request Jun 29, 2021
… routes and IMDS route in the task namespace. (aws#2876)

* Add IMDS route to the task namespace if task is allowed to have IMDS access.c

* Add support for adding additional local routes in awsvpc network mode on Windows.

* Removed the workflow for getting Primary ipv4 address of the vpc.

This is not required as we can use the dns settings of the primary instance ENI.

* Added gSMA support while using awsvpc network mode.

We use the instance ENIs DNS settings since both the ENIs would be in the same VPC and would be additionally beneficial during domain join.

* PR review changes: Reverted netwrapper to platform agnostic package

* Changes to json format for vpc-eni plugin

* Bug fix to rectify the scenario when pause container sends nil network stats.

Initially, without any network, pause container returns nil network stats. When container stats are collected with pause namespace in place, lastStats.NetworkStats is nil which causes agent to crash and restart.

* Minor changes: Comment changes, logfile path consolidation and variable name change
rawahars added a commit to rawahars/amazon-ecs-agent that referenced this pull request Jun 29, 2021
… routes and IMDS route in the task namespace. (aws#2876)

* Add IMDS route to the task namespace if task is allowed to have IMDS access.c

* Add support for adding additional local routes in awsvpc network mode on Windows.

* Removed the workflow for getting Primary ipv4 address of the vpc.

This is not required as we can use the dns settings of the primary instance ENI.

* Added gSMA support while using awsvpc network mode.

We use the instance ENIs DNS settings since both the ENIs would be in the same VPC and would be additionally beneficial during domain join.

* PR review changes: Reverted netwrapper to platform agnostic package

* Changes to json format for vpc-eni plugin

* Bug fix to rectify the scenario when pause container sends nil network stats.

Initially, without any network, pause container returns nil network stats. When container stats are collected with pause namespace in place, lastStats.NetworkStats is nil which causes agent to crash and restart.

* Minor changes: Comment changes, logfile path consolidation and variable name change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants