windows: explicitly provide the network name override to nat when using bridge network mode #3564
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ng bridge mode On Linux, the default network mode is bridge wherein all the containers are attached to a common docker bridge network. On Windows, this default mode translates to "nat" or "default" network. When we use "bridge" as the network mode, then docker daemon error out due to "bridge" network not present itself. Therefore, we need to do an override for Windows such that whenever "bridge" mode is specified, we would translate it to "nat" network. This commit adds support for the same.
arun-annamalai
approved these changes
Feb 8, 2023
jterry75
approved these changes
Feb 8, 2023
singholt
approved these changes
Feb 8, 2023
chienhanlin
approved these changes
Feb 8, 2023
ofiliz
approved these changes
Feb 10, 2023
| @@ -105,6 +109,13 @@ func (task *Task) platformHostConfigOverride(hostConfig *dockercontainer.HostCon | |||
| hostConfig.MemoryReservation = 0 | |||
| } | |||
|
|
|||
| // On Windows, bridge mode equivalent for Linux is "default" or "nat" network. | |||
| // We need to perform explicit conversion for the same. | |||
There was a problem hiding this comment.
A mental note on the comment above and how to think about these concepts:
The "bridge" networkMode here is ECS' network mode, not docker's. The fact that docker also has a networking mode called "bridge" should be treated as coincidental. Today ECS' bridge is implemented with docker's bridge under the hood, but that can change in the future.
Therefore, instead of saying "bridge mode equivalent is 'nat'", we should say "On Windows, ECS' bridge mode is implemented with "nat" mode."
|
|
||
| // windowsDefaultNetworkName is the name of the docker network to which the containers in | ||
| // default mode are attached to. | ||
| windowsDefaultNetworkName = "nat" |
There was a problem hiding this comment.
Minor: Do you need to prefix the variable with "windows" here, since this file is already Windows-specific?
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
On Linux, the default network mode is
bridgewherein all the containers are attached to a common docker bridge network. On Windows, this default mode translates tonatordefaultdocker network.When we use
bridgeas the network mode, then docker daemon errors out due tobridgenetwork not present itself. Therefore, we need to do an override for Windows such that wheneverbridgemode is specified in HostConfig, we would translate it tonatnetwork.This PR adds support for the same.
Implementation details
We already have a method named
platformHostConfigOverrideintask_windows.gowhich performs platform specific overrides in the HostConfig. We have added the translation logic in the same method.Testing
A custom binary of the agent was built and was tested out.
Ran a bunch of tasks with both
bridgeanddefaultmode.New tests cover the changes: Yes
Description for the changelog
windows: explicitly provide the network name override to nat when using bridge network mode
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.