Skip to content
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

API: Autofill Service.Address with Node.Address if Service.Address is empty #4599

Open
johncowen opened this issue Aug 28, 2018 · 4 comments
Labels
theme/catalog Relating to the catalog of nodes, services, and checks theme/service-metadata Anything related to management/tracking of service metadata type/enhancement Proposed improvement or new feature

Comments

@johncowen
Copy link
Contributor

Feature Description

Whenever the Consul API reports a ServiceAddress or Service.Address, if this value is blank/empty, default to the address of the Node instead of leaving the value blank.

We currently document that a blank value is expected behaviour in the catalog endpoint documentation, although changing this could improve DX and is therefore a feature request.

https://www.consul.io/api/catalog.html#serviceaddress

ServiceAddress is the IP address of the service host — if empty, node address should be used

Use Case(s)

Make it easier for API consumers/clients to retrieve the IP address of a service, even when there is no need for the registration of the service to provide an IP.

Also see:

hashicorp/consul-template#587 (comment)
#4579

@mkeeler this is re: offline chat

@johncowen johncowen changed the title API: Autofill Service.Address with Node.Address is Service.Address is empty API: Autofill Service.Address with Node.Address if Service.Address is empty Aug 28, 2018
@sandstrom
Copy link
Contributor

sandstrom commented Sep 12, 2018

This would be good!

  1. Current response is somewhat confusing, and leaves less flexibility. If we instruct a service consumer to use the Address response value, and later change things up with a dedicated service address, then the consumer would fail.

  2. It would align better with ServicePort

@pearkes pearkes added theme/ui Anything related to the UI type/enhancement Proposed improvement or new feature labels Sep 12, 2018
@johncowen johncowen removed the theme/ui Anything related to the UI label Mar 15, 2019
@pierresouchay
Copy link
Contributor

I agree with you on the initial issue, but I think that many tools are now doing assumptions based on this value being present or not, see my comment: #7782 (comment)

I wonder if correct fix would not be to provide such wrappers in consul-template or various SDKs (and describe what should be the correct implementation).

Note that is also applies for instance to Check(s) status(es) for Health endpoint, see the helpers we added in our templating system (service_address, status, current_weight...): https://github.com/criteo/consul-templaterb/blob/master/TemplateAPI.md#helpers

@jf
Copy link

jf commented Sep 9, 2020

I agree with you on the initial issue, but I think that many tools are now doing assumptions based on this value being present or not, see my comment: #7782 (comment)

I'm late to this issue but... what tools would these be, and why would they care about it being empty (""?) vs being filled with an actual IP address? I fail to see why having the actual IP address reflected in Service.Address is a problem vs it's current "" value

@jsosulska
Copy link
Contributor

Hello All!

Apologies for the long tail. We closed #7782 based on internal discussion. There are docs updates that can be performed that can better clear this ambiguity.

There is, however, a potential need for a new API to properly service both the service discovery and service management use cases and it will require a more holistic look at our API to determine what needs to be done. There'll be a follow up to this message with a bit more on that once a solution has been considered.

In the meantime, to clear the ambiguity:

  • docs/discovery/services Link - The section beginning with "The address field ..." should include information about when to set this registration and the default behavior of assuming node IP when unset. We can be more explicit here, point out the IPv4/6 cases and other formats more explictly.

  • api-docs/agent/service Link We could include a note at the top of this page that describes the unset value being returned in a respons.

  • We will update the FAQ on Consul to reference this Issue + docs cleanup associate with this.

@jsosulska jsosulska added theme/catalog Relating to the catalog of nodes, services, and checks theme/service-metadata Anything related to management/tracking of service metadata labels May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/catalog Relating to the catalog of nodes, services, and checks theme/service-metadata Anything related to management/tracking of service metadata type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants