From e38463b2779f455b4173171d5a1fdb115180a7e9 Mon Sep 17 00:00:00 2001 From: Madhu Venugopal Date: Mon, 25 Jan 2016 19:55:18 -0800 Subject: [PATCH] Move port-mapping ownership closer to Sandbox (from Endpoint) https://github.com/docker/libnetwork/pull/810 provides the more complete solution for moving the Port-mapping ownership away from endpoint and into Sandbox. But, this PR makes the best use of existing libnetwork design and get a step closer to the gaol. Signed-off-by: Madhu Venugopal --- container/container_unix.go | 87 ++++++++++++++++--------- daemon/container_operations_unix.go | 4 +- integration-cli/docker_cli_port_test.go | 21 ++++++ 3 files changed, 81 insertions(+), 31 deletions(-) diff --git a/container/container_unix.go b/container/container_unix.go index 9510a247c4..6fa72151ea 100644 --- a/container/container_unix.go +++ b/container/container_unix.go @@ -129,18 +129,26 @@ func (container *Container) buildPortMapInfo(ep libnetwork.Endpoint) error { return derr.ErrorCodeEmptyNetwork } + if len(networkSettings.Ports) == 0 { + pm, err := getEndpointPortMapInfo(ep) + if err != nil { + return err + } + networkSettings.Ports = pm + } + return nil +} + +func getEndpointPortMapInfo(ep libnetwork.Endpoint) (nat.PortMap, error) { + pm := nat.PortMap{} driverInfo, err := ep.DriverInfo() if err != nil { - return err + return pm, err } if driverInfo == nil { // It is not an error for epInfo to be nil - return nil - } - - if networkSettings.Ports == nil { - networkSettings.Ports = nat.PortMap{} + return pm, nil } if expData, ok := driverInfo[netlabel.ExposedPorts]; ok { @@ -148,30 +156,45 @@ func (container *Container) buildPortMapInfo(ep libnetwork.Endpoint) error { for _, tp := range exposedPorts { natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port))) if err != nil { - return derr.ErrorCodeParsingPort.WithArgs(tp.Port, err) + return pm, derr.ErrorCodeParsingPort.WithArgs(tp.Port, err) } - networkSettings.Ports[natPort] = nil + pm[natPort] = nil } } } mapData, ok := driverInfo[netlabel.PortMap] if !ok { - return nil + return pm, nil } if portMapping, ok := mapData.([]types.PortBinding); ok { for _, pp := range portMapping { natPort, err := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port))) if err != nil { - return err + return pm, err } natBndg := nat.PortBinding{HostIP: pp.HostIP.String(), HostPort: strconv.Itoa(int(pp.HostPort))} - networkSettings.Ports[natPort] = append(networkSettings.Ports[natPort], natBndg) + pm[natPort] = append(pm[natPort], natBndg) } } - return nil + return pm, nil +} + +func getSandboxPortMapInfo(sb libnetwork.Sandbox) nat.PortMap { + pm := nat.PortMap{} + if sb == nil { + return pm + } + + for _, ep := range sb.Endpoints() { + pm, _ = getEndpointPortMapInfo(ep) + if len(pm) > 0 { + break + } + } + return pm } // BuildEndpointInfo sets endpoint-related fields on container.NetworkSettings based on the provided network and endpoint. @@ -265,7 +288,7 @@ func (container *Container) BuildJoinOptions(n libnetwork.Network) ([]libnetwork } // BuildCreateEndpointOptions builds endpoint options from a given network. -func (container *Container) BuildCreateEndpointOptions(n libnetwork.Network, epConfig *network.EndpointSettings) ([]libnetwork.EndpointOption, error) { +func (container *Container) BuildCreateEndpointOptions(n libnetwork.Network, epConfig *network.EndpointSettings, sb libnetwork.Sandbox) ([]libnetwork.EndpointOption, error) { var ( portSpecs = make(nat.PortSet) bindings = make(nat.PortMap) @@ -294,10 +317,29 @@ func (container *Container) BuildCreateEndpointOptions(n libnetwork.Network, epC createOptions = append(createOptions, libnetwork.CreateOptionDisableResolution()) } - // Other configs are applicable only for the endpoint in the network + // configs that 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" && container.HostConfig.NetworkMode.IsDefault()) { + // Ideally all these network-specific endpoint configurations must be moved under + // container.NetworkSettings.Networks[n.Name()] + if n.Name() == container.HostConfig.NetworkMode.NetworkName() || + (n.Name() == "bridge" && container.HostConfig.NetworkMode.IsDefault()) { + if container.Config.MacAddress != "" { + mac, err := net.ParseMAC(container.Config.MacAddress) + if err != nil { + return nil, err + } + + genericOption := options.Generic{ + netlabel.MacAddress: mac, + } + + createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(genericOption)) + } + } + + // Port-mapping rules belong to the container & applicable only to non-internal networks + portmaps := getSandboxPortMapInfo(sb) + if n.Info().Internal() || len(portmaps) > 0 { return createOptions, nil } @@ -357,19 +399,6 @@ func (container *Container) BuildCreateEndpointOptions(n libnetwork.Network, epC libnetwork.CreateOptionPortMapping(pbList), libnetwork.CreateOptionExposedPorts(exposeList)) - if container.Config.MacAddress != "" { - mac, err := net.ParseMAC(container.Config.MacAddress) - if err != nil { - return nil, err - } - - genericOption := options.Generic{ - netlabel.MacAddress: mac, - } - - createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(genericOption)) - } - return createOptions, nil } diff --git a/daemon/container_operations_unix.go b/daemon/container_operations_unix.go index c78fa9aa59..997a17657b 100644 --- a/daemon/container_operations_unix.go +++ b/daemon/container_operations_unix.go @@ -775,7 +775,8 @@ func (daemon *Daemon) connectToNetwork(container *container.Container, idOrName controller := daemon.netController - createOptions, err := container.BuildCreateEndpointOptions(n, endpointConfig) + sb := daemon.getNetworkSandbox(container) + createOptions, err := container.BuildCreateEndpointOptions(n, endpointConfig, sb) if err != nil { return err } @@ -801,7 +802,6 @@ func (daemon *Daemon) connectToNetwork(container *container.Container, idOrName return err } - sb := daemon.getNetworkSandbox(container) if sb == nil { options, err := daemon.buildSandboxOptions(container, n) if err != nil { diff --git a/integration-cli/docker_cli_port_test.go b/integration-cli/docker_cli_port_test.go index 0d9306f4ad..a4361f2eaa 100644 --- a/integration-cli/docker_cli_port_test.go +++ b/integration-cli/docker_cli_port_test.go @@ -293,3 +293,24 @@ func (s *DockerSuite) TestPortExposeHostBinding(c *check.C) { // Port is still bound after the Container is removed c.Assert(err, checker.NotNil, check.Commentf("out: %s", out)) } + +func (s *DockerSuite) TestPortBindingOnSandbox(c *check.C) { + testRequires(c, DaemonIsLinux, NotUserNamespace) + dockerCmd(c, "network", "create", "--internal", "-d", "bridge", "internal-net") + dockerCmd(c, "run", "--net", "internal-net", "-d", "--name", "c1", + "-p", "8080:8080", "busybox", "nc", "-l", "-p", "8080") + c.Assert(waitRun("c1"), check.IsNil) + + _, _, err := dockerCmdWithError("run", "--net=host", "busybox", "nc", "localhost", "8080") + c.Assert(err, check.NotNil, + check.Commentf("Port mapping on internal network is expected to fail")) + + // Connect container to another normal bridge network + dockerCmd(c, "network", "create", "-d", "bridge", "foo-net") + dockerCmd(c, "network", "connect", "foo-net", "c1") + + _, _, err = dockerCmdWithError("run", "--net=host", "busybox", "nc", "localhost", "8080") + c.Assert(err, check.IsNil, + check.Commentf("Port mapping on the new network is expected to succeed")) + +}