Allow port mapping only for endpoint created on docker run #17858
Conversation
Signed-off-by: Santhosh Manohar <santhosh@docker.com>
created on network connect. Signed-off-by: Lei Jitang <leijitang@huawei.com> Signed-off-by: Santhosh Manohar <santhosh@docker.com>
|
-1 to boolean arguments, of which it seems there is already one. |
| createOptions, err := container.buildCreateEndpointOptions(n) | ||
| if err != nil { | ||
| return err | ||
| if createOption { |
There was a problem hiding this comment.
Can you check whether this function can derive it is being called for the first network connect ?
Something like:
if len(container.NetworkSettings.Networks) == 1 { program the create option }
If so, caller does not need to pass any bool
| // Other configs are applicable only for the endpoint in the network | ||
| // to which container was connected to on docker run. | ||
| if n.Name() != container.hostConfig.NetworkMode.NetworkName() && | ||
| n.Name() != "bridge" { |
There was a problem hiding this comment.
this check essentially gives a free pass for the default bridge network even if the container is connected to the default bridge network using docker network connect.
But, the intention of the check should be to make sure we allow the rest of endpoint configs only for the network that is same as the NetworkMode.
There was a problem hiding this comment.
The correct fix would be :
container.hostConfig.NetworkMode.IsDefault() && n.Name() == controller.Config().Daemon.DefaultNetwork
There was a problem hiding this comment.
@mavenugo container.hostConfig.NetworkMode.IsDefault() && n.Name() == "bridge" should be ok, avoid passing the controller, see https://github.com/docker/docker/pull/17860/files#r44446197 from @aboch , bridge is hard code
There was a problem hiding this comment.
@coolljt0725 @sanimej okay. Shall we wrap the above check in an utility function ?
|
@mavenugo PTAL. Since the check is done only in one place doing it inline is sufficient I think. |
|
@sanimej Could use the test in #17860 to test the duplicate macaddress and fix the typo in https://github.com/docker/docker/pull/17860/files#diff-832e27741e70367e5566a720c88b8f32L937 so I can close my PR? |
|
@coolljt0725 I picked up your commit and retained the test for the mac-address config. |
|
LGTM |
|
@cpuguy83 PTAL |
|
LGTM |
Allow port mapping only for endpoint created on docker run
|
LGTM |
Fixes #17824
port map config shouldn't be passed for the endpoint created using network connect.
Signed-off-by: Santhosh Manohar santhosh@docker.com