--publish-rm on service update should error if the port isn't a target port

[jhorwit2]

Output of docker version:

Client:
 Version:      1.12.0
 API version:  1.24
 Go version:   go1.6.3
 Git commit:   8eab29e
 Built:        Thu Jul 28 21:15:28 2016
 OS/Arch:      darwin/amd64

Server:
 Version:      1.12.0
 API version:  1.24
 Go version:   go1.6.3
 Git commit:   8eab29e
 Built:        Thu Jul 28 21:15:28 2016
 OS/Arch:      linux/amd64

Output of docker info:

Containers: 20
 Running: 3
 Paused: 0
 Stopped: 17
Images: 223
Server Version: 1.12.0
Storage Driver: aufs
 Root Dir: /var/lib/docker/aufs
 Backing Filesystem: extfs
 Dirs: 201
 Dirperm1 Supported: true
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
 Volume: local
 Network: overlay bridge host null
Swarm: active
 NodeID: eq2uc7oz4ge5dx0ooc7k2wflz
 Is Manager: true
 ClusterID: bzgt4x9ylks43eu4q6km1o80z
 Managers: 1
 Nodes: 1
 Orchestration:
  Task History Retention Limit: 10
 Raft:
  Snapshot interval: 10000
  Heartbeat tick: 1
  Election tick: 3
 Dispatcher:
  Heartbeat period: 5 seconds
 CA configuration:
  Expiry duration: 3 months
 Node Address: 192.168.65.2
Runtimes: runc
Default Runtime: runc
Security Options: seccomp
Kernel Version: 4.4.15-moby
Operating System: Alpine Linux v3.4
OSType: linux
Architecture: x86_64
CPUs: 4
Total Memory: 1.954 GiB
Name: moby
ID: K2ME:GK6I:GEAO:454Y:I6XE:M63Q:PVR2:4EPP:UY7U:S6JI:5OZT:FKN2
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): true
 File Descriptors: 75
 Goroutines: 171
 System Time: 2016-08-02T16:37:32.670912274Z
 EventsListeners: 1
No Proxy: *.local, 169.254/16
Username: jhorwit2
Registry: https://index.docker.io/v1/
Insecure Registries:
 127.0.0.0/8

Additional environment details (AWS, VirtualBox, physical, etc.):

Steps to reproduce the issue:

1. docker swarm init
2. docker network create -d overlay backend
3. docker service create --name nginx --network backend --publish 8088:80 nginx
4. docker service update --publish-rm 8088 nginx

Describe the results you received:

$ docker service update --publish-rm 8088 test
test

Describe the results you expected:

Since 8088 is the published port, not the target port that --publish-rm expects it should error out instead of acting like it's working but not.

$ docker service update --publish-rm 8088 test
<some error about port 8088 not being able to be removed>

[thaJeztah]

Thanks for opening!

I think what we need, is a design decision (and document) about how we want these to act, so that we can be consistent. Given the overall design of Swarm mode, making actions idempotent could also make sense (the desired state is to un-publish port 8080).

(Similarly, there have been discussions about producing errors for invalid configuration changes; this is tricky because that can only be done for configuration that can never become valid, which may depend on the platform the daemon runs on)

/cc @aluzzardi @vieux

[dnephin]

Yes, I think this should be idempotent. We could add a warning to inform users, but I don't think it should error.

[thaJeztah]

Perhaps we should add a generic message "nothing to update" if an update action didn't lead to changes in the service definition.

Interesting; I think we should fix this:

$ docker service update --publish-add 80:80 web
web

$ docker service update --publish-add 80:80 web
Error response from daemon: rpc error: code = 3 desc = EndpointSpec: duplicate ports provided

[thaJeztah]

Opened an issue for that: #25375

[yongtang]

For --publish-rm I am thinking maybe it makes sense to allow user to specify the protocol as well, e.g.:

--publish-rm 80/tcp
--publish-rm 53/udp

?

[thaJeztah]

@yongtang We should be very careful with adding a new syntax; we've been bitten by bad decisions in that area before (e.g. the completely overloaded syntax on --volume). Possibly using the same format as --mount could be an option (--publish-rm hostport=80,proto=tcp). It's a lot more verbose, but less likely to result in issues. Just some quick thoughts, but happy to hear others on that

[yongtang]

@thaJeztah Yes we definitely should be very careful about the UI/UX. I noticed only very recently that there are some potential confusions with respect to --publish-rm

• --publish-rm <TargetPort> only TargetPort (80) will be accepted. It will not do anything if --publish-rm 80:80 is specified because it is no a valid <TargetPort>. Many people may assume 80:80 is the correct format because of the format --publish-add 80:80. (I thought the same before).
• --publish-rm <TargetPort> will remove any mappings as long as match. No check for <PublishedPort> and <Protocol>.

I think having a verbose (and less confusion) option is much better. That should help address the issue of the above (e.g., remove UDP vs TCP inadvertently.)

[yongtang]

@thaJeztah Here is an related issue (moby/swarmkit#1396) in swarmkit.

[dnephin]

For --publish-rm I am thinking maybe it makes sense to allow user to specify the protocol as well

Yes, I think that already works, it just defaults to tcp. See https://github.com/docker/docker/blob/master/api/client/service/update.go#L435-L438

[yongtang]

Thanks @dnephin! I didn't notice the equalPort before. It looks like equalPort may not cover the case where port.Protocol is "" yet targetPort.Proto() is tcp.

I created a pull request #25860 to update the equalPort. It also adds the error checking for --publish-rm so that if the user provides an incorrect format (like --publish-rm 80:80) then at least they will be notified.

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