Skip to content

display port name field in 'service ls' #3724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

aep
Copy link

@aep aep commented Aug 1, 2022

- 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

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me!
The linter check is failing though, please give gofmt a run :)

Looks like a good improvement over a previous behaviour. Not sure if this is something that shouldn't change due to backwards compatibility though. But since this is CLI and not Engine API - I think it's fine.
@ndeloof @thaJeztah WDYT?

@thaJeztah
Copy link
Member

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 moby/swarmkit#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.

Signed-off-by: Arvid E. Picciani <arvid@kraud.cloud>
@codecov-commenter
Copy link

Codecov Report

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

❗ 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     

@aep
Copy link
Author

aep commented Aug 8, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants