service update --publish-rm unable to remove pair of ports

[sheerun]

--publish-rm 82:80 fails when I published before with the same port pair. Full commands are:

docker service update --publish-add 80:80 root_nginx
docker service update --publish-add 81:80 root_nginx
docker service update --publish-rm 81:80 root_nginx

I'd expect that only 80:80 is left, but the only thing that works is --publish-rm 80 but that removes both bindings instead of only one. Probably removing only single binding pair is not implemented.

[aaronlehmann]

ping @yongtang

[dperny]

From #1348

[dperny]

Tested and verified this is a problem. Also worth noting is that --publish-rm 81 does not remove any ports.

[yongtang]

@sheerun That is "expected" behavior of the current --publish-rm. Currently, the usage of --publish-rm is

--publish-rm <TargetPort>[/Protocol]

It will remove any mappings that matches <TargetPort> (not taking <PublishedPort> or <Protocol> into consideration). And unfortunately, it will not report any error if you pass an incorrect format.

So the --publish-rm 81:80 will not take effect. And --publish-rm 80 will remove both.

The current --publish-rm definitely could be improved. Though I think it makes sense to decide what the expected behavior would be. For example:

• Should we error out if --publish-rm takes an incorrect <TargetPort> (e.g., 80:80)
• Or when user specify 80:80, we match both TargetPort and PublishedPort.
• We haven't take <Protocol> (udp or tcp) into the consideration, should we allow user to specify <Protocol> as well.

See processing of --publish-rm:
https://github.com/docker/docker/blob/37302bbb3f4889e9de2a95d5ea018acdab9e4447/api/client/service/update.go#L420-L429

I can work on improving the --publish-rm but may want some guidance on the above listed items.

[yongtang]

Also see some other discussions on --publish-rm:
moby/moby#25200 (comment)
moby/moby#25338 (comment)

[yongtang]

@sheerun @aaronlehmann @dperny It looks like --publish-rm already allows the user to specify a protocol, so both --publish-rm 80 and --publish-rm 80/tcp are valid.

However, --publish-rm 80:80 is still not valid and will silently ignore the incorrectly provided format.

I created a pull request moby/moby#25860 to return an error if the invalid format (like --publish-rm 80:80) is provided.

Please take a look and let me know if there are any issues, or if there are better ways to handle it.

[tiborvass]

@thaJeztah and I think it makes sense to be able to have --publish-rm 81:80/udp remove only the 81:80/udp port binding.

[tianon]

+1; can confirm that the first time I tried to use --publish-rm, I gave it the same format as --publish-add and was quite surprised to see that it didn't actually do anything (despite telling me it was successful) and I had to figure out which port it was actually wanting me to specify in order to remove the mapping