cli icon indicating copy to clipboard operation
cli copied to clipboard

display port name field in 'service ls'

Open aep opened this issue 1 year ago • 3 comments

- What I did

in docker service ls we durrently format the port as "*:8080->9000" with the asterisk being hardcoded, it would be useful if the api server could return a dns name on the port, for api servers that can expose ports directly as ingress.

what we're currently doing instead is abusing the protocol field, but that breaks some assumptions of other api consumers, and is on the wrong side of the arrow

aep@whale: ~ docker -c kraud.aep service ls                                                                        
ID             NAME    MODE         REPLICAS      IMAGE    PORTS
a0199c59d51f   nginx   replicated   1/1           nginx    *:443->80/38pvc43th.kraud.cloud

- How I did it

the least invasive way to implement that appears to be to use the existing Name field. We could add an additional field, if that's easy to upstream. That would have the benefit of not changing anything unless the new field is set, but the downside is that it touches the api

- How to verify it

aep@whale: ~ docker -c kraud.aep service ls                                                                        
ID             NAME    MODE         REPLICAS      IMAGE    PORTS
a0199c59d51f   nginx   replicated   1/1           nginx    38pvc43th.kraud.cloud:443->80/https

- Description for the changelog

display port name field in 'service ls'

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

stickers

aep avatar Aug 01 '22 17:08 aep

I'm not sure if the information shown is accurate; the *: is printed to indicate for services that they're published on the ingress network, and because of that listening on all IP addresses of the swarm cluster. IIUC, in your case, there's additional code or an alternative API that uses this information to register services in a DNS, and those services (and ports) to be accessible using that, which would not be the default behavior, and exposing the Name here would not work for regular uses.

The Name field here originates from SwarmKit (added in https://github.com/moby/swarmkit/pull/353, but corresponding changes appear to have been in a separate PR), and is described as;

// Name for the port. If provided the port information can
// be queried using the name as in a DNS SRV query.

thaJeztah avatar Aug 08 '22 11:08 thaJeztah

Codecov Report

Merging #3724 (2f652fa) into master (e198123) will decrease coverage by 0.00%. The diff coverage is 41.66%.

:exclamation: Current head 2f652fa differs from pull request most recent head b043380. Consider uploading reports for the commit b043380 to get more accurate results

@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
- Coverage   59.12%   59.11%   -0.01%     
==========================================
  Files         289      289              
  Lines       24669    24678       +9     
==========================================
+ Hits        14585    14589       +4     
- Misses       9212     9216       +4     
- Partials      872      873       +1     

codecov-commenter avatar Aug 08 '22 11:08 codecov-commenter

and because of that listening on all IP addresses of the swarm cluster.

yes. this change would let the cluster indicate that it's not all IP addresses, but a specific one (or name)

exposing the Name here would not work for regular uses.

not sure i understand what you mean by that. The existing behavior will not change. At least i haven't seen Name actually be used.

Of course if you would prefer the alternative option of adding a new field, i could do that.

how does Endpoints []string sound?

aep avatar Aug 08 '22 12:08 aep