Add mDNS provider option using hashicorp/mdns library

[aaronhurt]

Adding an mDNS provider option for us on a private lan segment. I plan to leverage this in an upcoming PR to add mDNS advertisements to consul agents in server mode via the same library.

[aaronhurt]

Any thoughts on this?

[pearkes]

This is interesting, @leprechau! I think the stated goal of go-discover is to support specifically cloud environments, where I believe mdns is not usually possible. Conceptually though, it does seem similar to me so I get the addition. I will try to track someone down to give you a more authoritative answer on the subject!

[aaronhurt]

@pearkes I get the cloud focus, however this is also the discovery library used within Consul. I think having the ability for Consul nodes to discover each other via mDNS on a lan segment or within an isolated Docker network could be very useful.

[pearkes]

Okay @leprechau -- after a bit of discussion within HashiCorp I think we should figure out how to get this in 😄. Do you think integration testing similar to how we have for the cloud providers would make sense here?

https://github.com/hashicorp/go-discover/tree/master/test/tf

Another housekeeping note, it'd be great to update the readme with the mDNS provider listed as part of this. We'll come back around for a formal review.

[aaronhurt]

@pearkes I updated the readme to document the mDNS option.

To the integration tests, I thought about that when I first built the PR. It should be possible to spin up a docker container running an mDNS server and exec discover within that container to pull back a service record. I wasn't sure if that would be sufficient or beneficial though.

[aaronhurt]

Ideally I'd like to see this merged and then submit another PR to Consul to allow it to publish it's own mDNS records. We could then build an integration test that spun up consul in a container and have discover pull the service record advertised by Consul.

[pearkes]

@leprechau fair, it'd be great to follow the pattern we have currently of being able to test it independent of HashiCorp tools (I can imagine Nomad wanting to integrate this as well as Consul!). If we can figure out a decent way that'd be great.

In the meantime we'll get a review done here soon!

[aaronhurt]

@pearkes thank you. I added a simple integration test into the provider package. It now validates config and spins up a test local mDNS server and then uses the provider to query for the service.

[aaronhurt]

@pearkes any updates on getting this merged? If the above comment is the only feedback on the overall PR I consider it done.

[pearkes]

@leprechau Haven't had a chance to review the implementation or experiment with it which should be brief but is something we should do prior to merging. Definitely in the queue to look at in the next couple of weeks.

[aaronhurt]

@pearkes Thank you sir, appreciate the update.

[smurfix]

Any progress? there have been a lot of "couple of" weeks since May 1st … ;-)

[aaronhurt]

I'm still interested in getting this in and adding mDNS server/broadcast support to Consul in a follow-up PR once we have the go-discover side done.

[euskadi31]

Any progress?

[aaronhurt]

@pearkes Anything I can do to help get this moved along?

[aaronhurt]

@alvin-huang are you supporting go-discover now?

[pearkes]

Great job. Couldn't find any concerns. We are interested in seeing what the upstream agent integration looks like in Consul.

Apologies it took us so long to get this merged, notably with little feedback. I think initially there was some concern for committing to a non-standard "cloud" discovery option. Clearly this was too risk-averse, and the potential impact is quite low of getting this in master. Thank you for your contribution and importantly, your patience!

We welcome docs in upstream Consul, with the implementation of listening for requests in the agent as a more involved review. If you have questions about that, feel free to open an issue there.