Add Consul publisher #100
Add Consul publisher #100
Conversation
Is stale a term of art in Consul? In the normal case, if the publisher knows an endpoint is broken, it shouldn't emit it via Endpoints. If a publisher suspects an endpoint might be broken, then it's a gray area, perhaps best resolved with a configuration setting during construction, or deferring to best practice of the SD system.
Is a hostname sufficient to create a connection to the remote service? If yes, then it's fine. But that means you're carrying the service along implicitly—I can't think of a good use case for that. Do you have one?
Per our discussion yesterday, I'm guessing not. |
|
I've been integrating this code into our migration to using a full publisher/loadbalancer implementation with go-kit and have been very happy with it! One thing I have noticed about the implementation: When reading a service from a Consul health API check there are actually two potential fields the address of a service can appear in, either ServiceEntry.Node.Address or ServiceEntry.Service.Address. The reason these two can differ is as a result of the configuration of a particular Consul datacenter. The service address field is populated if a specific address has been specified for a service configuration on a given node. This is usually a result of loading a service into Consul that cannot have a Consul agent run on its host OS. This results in the Node address referring to the hosting agent, and the service address referring to the actual resolved service location. In our use cases, we treat the service address as an override to the node address, as it is empty when not in use. This is helpful in development environments when you want to run a local Consul server pointing to external dependencies. |
|
@EcofitBrendan Great to hear! Would you be willing to contribute your implementation as a PR when you're happy with it? |
|
@peterbourgon Definitely, I should have something functional available over the next few days. |
|
@EcofitBrendan I'm going to pick this up now, unless you have something ready to go? |
|
This is ready for review, in the meantime I'll work on the README with examples and high-level documentation. As soon as approved I'm going to stash the changes. @EcofitBrendan Thanks for pointing out this intricacy. I added a test case to make sure the publisher accounts for it. |
|
Currently not covered is the case for an update to the entries in Consul. This might be better suited for integration tests. Happy to cover it here as well, if deemed important for acceptance. |
| } | ||
| } | ||
|
|
||
| // GetInstances returns the list of entries for a given service filtered by |
|
Haven't audited for correctness or tested with a real Consul, but code structure LGTM. Do you want me to test in a live environment, or are you confident to merge? |
|
@peterbourgon Would appreciate a real world test, if it doesn't mean too much time consumption for you. |
|
I tested this with a live Consul cluster and it works well. I am happy to merge it. Is there anything else you'd like to do before I click the green button? |
|
@peterbourgon Thanks for taking the time. I squashed the commits and am fine with merging this one. Will work on examples and documentation independently. Unfortunately the tests are failing since rebasing against master, it looks to be a change in the |
|
Yep, I will wait until etcd #246 is fixed and we can get a greenlight on CI before merging. |
|
@peterbourgon Full ack. |
|
Sorry, can you rebase on master and fix the build error? (I think that's what's causing the Travis failure...) |
Implements a Publisher which connects to Consul and returns the Endpoints for the requested service. Addtionally filtered by tags where every tag beyond the first one needs to be filtered in the client.
|
@peterbourgon Done. |
|
Woop woop |
Add Consul publisher
open questions
ip:portor arehostnamesequally valid?todos
The commits in this branch will be cleaned up in time and likely stashed into a single coherent change-set.