api/types/container, runconfig: NetworkMode align Windows and Unix implementations#48010
api/types/container, runconfig: NetworkMode align Windows and Unix implementations#48010thaJeztah wants to merge 2 commits intomoby:masterfrom
Conversation
thaJeztah
added
area/api
runconfig/hostconfig.go
Outdated
|
|
||
| // IsPreDefinedNetwork indicates if a network is predefined by the daemon | ||
| func IsPreDefinedNetwork(network string) bool { | ||
| return !container.NetworkMode(network).IsUserDefined() |
There was a problem hiding this comment.
For a container network, IsUserDefined() -> false, so IsPredefinedNetwork() -> true.
It was false before, which seems right ... I think this needs an extra check?
There was a problem hiding this comment.
Hm... all the booleans make my head spin. Hm.. so curious why the Windows implementation had it included.
So this function is used on (e.g.) network create to prevent one of these networks from being created; that part should still be correct when trying to create a container:foo network? So .. wondering if the Linux implementation was faulty; wdyt?
There was a problem hiding this comment.
Yes, these predicates are fiddly ... I don't think the create/delete functions get called for a container network (there's no separate network), so that's why things are working.
Similarly, I think filterNetworkByUse and filterNetworkByType will only be called for the underlying networks. From Cluster.populateNetworkID and Daemon.localNetworksPrune there's probably no call for container networks, because the container network doesn't really exist as its own thing.
So, the change probably wouldn't break anything. But I'd want to stare-at/play-with it some more to be sure, and the behaviour of the function could do with clarifying/unit-testing.
There was a problem hiding this comment.
Yeah, I think we need to look at cleaning up some of this. Perhaps a single "is reserved name" utility. We currently allow networks on Linux that are reserved names on Windows; I've been thinking about NOT allowing that, and have a single list of reserved ones.
I ran into that case on the CLI side, where some code used "IsUserDefined" on the client-side, but that meant that a Windows CLI connected to a Linux daemon could attempt to create a network with a reserved name. That would of course fail because the API would reject it (so I think I removed that check, and let it be only handled by the API).
There was a problem hiding this comment.
I've been thinking about NOT allowing that, and have a single list of reserved ones.
Someone's bound to have a Linux bridge network called "nat". (Maybe someone called Nathan or Natalie!)
There was a problem hiding this comment.
Yes, that's what kept me from doing so 😂
|
Let me move this to draft while we look into "what's correct". I'll move the non-controversial commits to a separate PR |
thaJeztah
force-pushed
the
cleanup_networkmodes
branch
from
e549c07 to
e8ddd42
Compare
thaJeztah
force-pushed
the
cleanup_networkmodes
branch
from
e8ddd42 to
474bfac
Compare
The Windows and non-Windows implementations where identical, with the exception of the Windows variant not checking for IsHost() (which is always "false" on Windows, so a redundant check). This patch unifies them to prevent them from getting out of sync. Signed-off-by: Sebastiaan van Stijn <[email protected]>
The Windows and non-Windows implementations used a different approach;
the Unix implementation checked for any of the known pre-defined networks,
whereas the Windows implementation checked for the network not being
a user-defined one.
While both are valid, and the Unix approach ever-so-slightly more
performant in case additional parsing was needed ("container:<id>"),
the performance differences are likely neglectable, and the Windows
implementation makes it less likely to diverge from "IsUserDefined".
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
cleanup_networkmodes
branch
from
474bfac to
253b9a5
Compare
3 participants
api/tpes/container: unify NetworkMode.IsUserDefined
The Windows and non-Windows implementations where identical, with the
exception of the Windows variant not checking for IsHost() (which is
always "false" on Windows, so a redundant check).
This patch unifies them to prevent them from getting out of sync.
runconfig: use single implementation for IsPreDefinedNetwork
The Windows and non-Windows implementations used a different approach;
the Unix implementation checked for any of the known pre-defined networks,
whereas the Windows implementation checked for the network not being
a user-defined one.
While both are valid, and the Unix approach ever-so-slightly more
performant in case additional parsing was needed ("container:"),
the performance differences are likely neglectable, and the Windows
implementation makes it less likely to diverge from "IsUserDefined".
- A picture of a cute animal (not mandatory but encouraged)