I'm not very familiar with these options, but my thinking is that it's best for --publish-rm to support a syntax that's consistent with --publish-add. It seems like a common point of confusion that people can't use --publish-add 80:81 and then --publish-rm 80:81.

Are there any obstacles to supporting --publish-rm 80:81?

When I raised #1396 I didn't mean to suggest adding a validation, but allowing --publish-rm to remove only one binding. Currently it's just not possible, and I want to remove only one binding, I need to remove all of them and re-add all the other ones.. what is not optimal?

I vote the same as @aaronlehmann (to fix that behaviour).

@aaronlehmann @sheerun

Currently the --publish-add takes the form of ip:published:target/proto, and in case of --publish-add 80 it represents ip:published:80/tcp so it matches the existing interpretation of --publish-rm target/proto.

Technically it is possible to extend the --publish-rm to take published port without breaking the backward-compatibility.

cc @thaJeztah @dnephin @dperny

LGTM

Not 100% sure I understand this, because I'd think I'd be able to use the full port spec to remove... e.g. 80:80/tcp, however it looks like from the test this would be rejected.

@cpuguy83 The current (1.12) behavior for --publish-rm is to only accept <TargetPort>/<Protocol> (No published port):

      --publish-rm value               Remove a published port by its target port (default [])

So --publish-rm 80:80/tcp will be considered as an invalid input.

However, in 1.12 there is no error output and the invalid input 80:80/tcp will be ignored silently (which caused confusion).

There are some related discussions:
moby/swarmkit#1396
#25200 (comment)
#25338 (comment)

This pull request is to return an error message (not changing 1.12 behavior) so it is easy for end user to understand.

We could change the behavior to accept --publish-rm 80:80/tcp if it is agreed.

Ping for additional feedback for the PR (rebased).

ping @aboch

Ping @mrjana @aboch.

@yongtang
I only have a minor comment on the function name. Up to you if you want to change it.

And I have a question just for my understanding: Why isn't the validation being done in the in the swarmkit code instead, which owns the feature, so that it takes care of validating the remote API.
Is it because there is no remote API counterpart to the --publish-add/--publish-rm cli options, so this is peculiar to the cli only ?
Thanks.

Is it because there is no remote API counterpart to the --publish-add/--publish-rm cli options, so this is peculiar to the cli only ?

@aboch correct; the daemon has no option to "remove" or "add" an option; the client sends the full spec after applying the changes

Thanks @aboch for the review. The validatePort has been renamed to validatePublishRemove for better description.

As was mentioned by @thaJeztah, the swarmkit will only take a desired state of the service spec, so add or remove checking has to be done at the client side.

Thanks @thaJeztah and @yongtang for the explanation.

Changes look good to me.

Ping. Any additional issues need to address?

LGTM but needs a rebase.

Design wise this looks good.

Thanks all for the review. The PR has been rebased.

hm, doing some more testing, this does catch the invalid format (1234:456/tcp), but I wonder if this will lead to the expectation that any incorrect value will produce an error.

Basically, the docker service update command is still using the "reconciliation" pattern; in other words; removing something that didn't exist, doesn't produce an error, because it didn't exist in the first place.

Some examples;

With the service below;

$docker service create --publish 8080:80/udp --name web nginx:alpine
86q0pdryba8xj9k6e7vbkhz1l

Using source-port does not produce an error;

$ docker service update --publish-rm 8080 web
web

Using source-port/protocol does not produce an error;

docker service update --publish-rm 8080/udp web
web

Using a non-existing port also doesn't produce an error;

docker service update --publish-rm 1234 web
web

Perhaps it's not a blocker for this PR, because it does catch the incorrect format

$ docker service update --publish-rm 8080:80/udp web
invalid argument "8080:80/udp" for --publish-rm: invalid port format: '8080:80', should be <TargetPort>[/<Protocol>] (e.g., 80, 80/tcp, 53/udp)
See 'docker service update --help'.

just wondering if we should change that behavior.

discussing this in the maintainers meeting, and we can keep it for a separate discussion.