Fix mapping a range of host ports to a single container port

[sfluor]

- What I did
fixes #1074

- How I did it
Splitting the host port range in a list of host ports

- How to verify it
run docker service create --name multi -p 8080-8081:80 nginx:alpine

- Description for the changelog
fix #1074 by splitting host port range into a list of host ports

- A picture of a cute animal (not mandatory but encouraged)

[silvin-lubecki]

Thank you for your PR!
I think this code can be simplified a little (and avoid the double iteration):

for _, binding := range portBindings[port] {
    if binding.HostIP != "" && binding.HostIP != "0.0.0.0" {
        logrus.Warnf("ignoring IP-address (%s:%s:%s) service will listen on '0.0.0.0'", binding.HostIP, binding.HostPort, port)
    }
    startHostPort, endHostPort, err := nat.ParsePortRange(binding.HostPort)
    if err != nil && binding.HostPort != "" {
        return nil, fmt.Errorf("invalid hostport binding (%s) for port (%s)", binding.HostPort, port.Port())
    }
    for i := startHostPort; i <= endHostPort; i++ {
        ports = append(ports, swarm.PortConfig{
            //TODO Name: ?
            Protocol:      swarm.PortConfigProtocol(strings.ToLower(port.Proto())),
            TargetPort:    uint32(port.Int()),
            PublishedPort: uint32(hostPort),
            PublishMode:   swarm.PortConfigPublishModeIngress,
        })
    }
}

Does it sounds good to you?
And is it possible to add some tests on this feature in opts/port_test.go?

[sfluor]

Hello 😄 !

I just removed the for loop as you suggested and added test cases for this behavior.

[silvin-lubecki]

I think this test is quite redundant with the previous one (value: "80-82:8080/udp").

[sfluor]

I just removed the redundant test ^^

[silvin-lubecki]

Thank you @sfluor ! LGTM
cc @vdemeester @thaJeztah

[mat007]

nit: extra line?

[sfluor]

I remove it 👍

[thaJeztah]

LGTM, thanks!

[thaJeztah]

Just thinking of this (haven't tried), but does this also work for docker stack deploy? (if not, we may have to add additional changes)

@sfluor could you squash your commits? I think its ok to have the tests and code-changes in the same commit.

[thaJeztah]

Oh, and we should check if we can / want to support this notation for the long syntax; moby/moby#32551

[sfluor]

Hello !

I just checked and the change also work for docker stack deploy

I did the following:

docker stack deploy -c docker-compose.yml multi

With docker-compose.yml being:

version: "3.5"
services:
  nginx:
    image: nginx:alpine
    ports: 
    - "8080-8081:80"

docker service ls then shows:

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
ra6q1sz8s1fq        multi_nginx         replicated          1/1                 nginx:alpine       *:8080-8081->80/tcp

And the redirection works as expected when doing a curl.

I also squashed both commits 👍.

I would be glad to look at the long syntax issue.

[vdemeester]

LGTM 🐯