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

[bug?] Fabio uses "global" Consul ServiceID's #383

Closed
ygersie opened this issue Nov 7, 2017 · 4 comments
Closed

[bug?] Fabio uses "global" Consul ServiceID's #383

ygersie opened this issue Nov 7, 2017 · 4 comments

Comments

@ygersie
Copy link

ygersie commented Nov 7, 2017

Fairly serious "bug?" if looking at how Consul implements service registration. By default if you register a Consul service the ServiceID will be set to the ServiceName if not explicitly passed ( https://www.consul.io/api/agent/service.html#id )
This is allowed as a ServiceID only has to be unique per agent, not globally (in the catalog).

Although Consul deals with serviceIDs like that (unique per node) Fabio doesn't, leading to very surprising/disturbing results.. Register a service with the same ServiceName (/ServiceID) on 2 separate nodes. Put 1 of them in maintenance mode (consul maint -enable -service=<serviceID>) and all routes disappear. Put the entire node into maintenance mode (consul maint -enable) and the routes associated with the node do not disappear at all. Let the healthcheck on 1 of them fail and the corresponding service route is not removed..

Is this intentional? If so, we need a big warning somewhere as this can cause very nasty issues.

@magiconair
Copy link
Contributor

I haven't fully tested the maintenance use case but in general the ServiceName describes the type of service e.g. UserService and the ServiceID describes the specific instance which should be unique for the cluster and not just the agent. That's why Fabio registers itself as fabio-<host>-<port> and we did that for all other setups. Also, this is the usual cause of errors for new Fabio users which set the name and the id to the same value for multiple instances.

@ygersie
Copy link
Author

ygersie commented Nov 8, 2017

I understand how to workaround it but the thing is, this is Consul spec. A serviceID is optional and will be set to serviceName if unset. Quote from James in gitter.im/hashicorp-consul:

Ah that's interesting - yeah Fabio probably needs to use <node>.<service id> as a key or something similar.
(that's what the Consul servers do internally)

If this behavior is intentional than it should be clearly noted somewhere:
NOTE: Fabio expects a service registration with a "global" unique serviceID
And maybe document what happens if you don't so ppl running into this issue can find the reason for unexpected behavior

@alvaroaleman
Copy link
Contributor

Ill just continue our conversation from #216 here since this issue here is more specific about the same problem.

Can you elaborate why cluster wide unique service ids like I've described are not achievable? I'm curious for the motivation.

Well, it is achievable but the Consul doc explicitly states that this is not required so people won't be doing it. This also means if someone doesn't explicitly test this he or her will only realize on service failure that Fabio expects Consul to be used in a different way than the actual Consul docs say its supposed to be used.

Are you currently working on this? Otherwise I'd like to prepare a PR.

@magiconair
Copy link
Contributor

No. PR would be very welcome :)

alvaroaleman added a commit to alvaroaleman/fabio that referenced this issue Dec 16, 2017
This aligns Fabio with the Consul docs[0] which state that
a serviceID has to be unique per agent but not cluster wide.

Fixes fabiolb#383

[0] https://www.consul.io/api/agent/service.html#id
alvaroaleman added a commit to alvaroaleman/fabio that referenced this issue Dec 16, 2017
This aligns Fabio with the Consul docs[0] which state that
a serviceID has to be unique per agent but not cluster wide.

Fixes fabiolb#383

[0] https://www.consul.io/api/agent/service.html#id
alvaroaleman added a commit to alvaroaleman/fabio that referenced this issue Dec 16, 2017
This aligns Fabio with the Consul docs[0] which state that
a serviceID has to be unique per agent but not cluster wide.

Fixes fabiolb#383

[0] https://www.consul.io/api/agent/service.html#id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants