Updating NetworkName to return proper name for user defined networks #88
Updating NetworkName to return proper name for user defined networks #88calavera merged 1 commit intodocker-archive-public:masterfrom msabansal:master
Conversation
|
LGTM |
|
Can we please get this merged? I need to start my review for docker daemon changes after this. |
|
@msabansal to speed up the process; the |
|
@thaJeztah Thanks. Done that. |
|
Great, I saw you opened the PR, thanks! |
|
ping @calavera @thaJeztah David/Sebastiaan - any chance we can get this merged so that we can proceed with the revendoring into docker/docker and the integration of HNS on Windows? Thanks 🐕 |
| // IsBridge indicates whether container uses the bridge network stack | ||
| // in windows it is given the name NAT | ||
| func (n NetworkMode) IsBridge() bool { | ||
| return n == "NAT" |
There was a problem hiding this comment.
Oooh - msabansal, should this be lowercase too?
There was a problem hiding this comment.
👍 yes, I think that should be changed
There was a problem hiding this comment.
Doing this. This will however require a minor libnetwork change aswell. Making that aswell.
|
LGTM once that string is changed |
…in windows and added missing APIs to the windows hostconfig file Signed-off-by: msabansal <sabansal@microsoft.com>
Updating NetworkName to return proper name for user defined networks
|
merged! 🎉 |
|
There have been some changes in engine-api that might make your life slightly more complicated than expected. You might want to wait until moby/moby#20565 is merged. |
|
Thanks @calavera. @msabansal Over to you for the next one :) |
| return false | ||
| } | ||
|
|
||
| // IsBridge indicates whether container uses the bridge network stack |
There was a problem hiding this comment.
From a networking perspective, this is one of the most confusing statements I have ever read. Why would bridge and nat be the same thing?
There was a problem hiding this comment.
How so? The windows equivalent of the bridge mode is nat and is enabled through the WinNAT service.
There was a problem hiding this comment.
A bridge is a very specific network device that "bridges" layer two packets over an interface, creating a single, layer 2 broadcast domain. A nat actually translates IP addresses between two separate networks, operating mostly at layer 3. There are protocols that work with one and not the other. For example, standard DHCP might run fine over a bridge but would require a relay or helper to work across a NAT boundary.
It sounds like windows doesn't really have the concept of a bridge. It would be better to leave out the concept of bridge than to have something claim it is a bridge but isn't.
I'm not privy to the implications of this behavior here, but it seems unwise to blur these concepts, as they are not at all the same. Even if they are two approaches to solve the same problem in docker, they have different trade offs and behavior.
The network api doesn't return the proper network name for user defined networks and it causes issues with windows libnetwork integration. This fixes that and brings parity with Linux implementation.