phoenixframework
Add support for connecting to nodes by their hostname
This PR adds a new option address_type which can be :ip (default) or :hostname to control whether to connect to other nodes via their ip address or hostname.
The resolver lookup function changed from 2-arity to 3-arity, because we now also need to pass the new address_type to it.
To not break backwards compatibility for users with custom resolver implementations (e.g. custom dns servers, DB lookup), the implementation calls lookup/3 if available, calls lookup/3 when address_type == :hostname and otherwise falls back to lookup/2.
Setup
$ dig A app.dev.acme.com +short
192.168.100.1
$ dig AAAA app.dev.acme.com +short
::ffff:192.168.100.1
$ dig SRV app.dev.acme.com +short
1 10 4000 app-1.dev.acme.com
1 10 4000 app-2.dev.acme.com
1 10 4000 app-3.dev.acme.com
$ dig A app-1.dev.acme.com +short
192.168.100.101
$ dig A app-2.dev.acme.com +short
192.168.100.102
$ dig A app-3.dev.acme.com +short
192.168.100.103
Resolving
iex> DNSCluster.Resolver.lookup("app.dev.acme.com", :a)
[{192, 168, 100, 1}]
iex> DNSCluster.Resolver.lookup("app.dev.acme.com", :aaaa)
[{0,0,0,0,0,65535,49320,25601}]
iex> DNSCluster.Resolver.lookup("app.dev.acme.com", :srv)
[{192, 168, 100, 101}, {192, 168, 100, 102}, {192, 168, 100, 103}]
iex> DNSCluster.Resolver.lookup("app.dev.acme.com", {:srv, :ips})
[{192, 168, 100, 101}, {192, 168, 100, 102}, {192, 168, 100, 103}]
iex> DNSCluster.Resolver.lookup("app.dev.acme.com", {:srv, :hostnames})
[~c"app-1.dev.acme.com", ~c"app-2.dev.acme.com", ~c"app-3.dev.acme.com"]
TODOs
- [ ] Add tests
- [ ] Update documentation
Fixes #14
Thank you for the PR! Given address_type :hostnames doesn't make sense for A/AAAA, perhaps we deprecate :srv and instead we have :srv_ips and :srv_hostnames instead? This way we keep compatibility with the resolver API.
@josevalim I've updated the implementation and added :srv_ips and :srv_hostnames. The :srv resource type is still available, but marked as deprecated in the "Options" section. We could also log a warning or similar to tell users to use :srv_ips instead.
One could also think of keeping :srv as is and only add :srv_hostnames. This way we would not need to deprecate anything.
Hrm... sorry for the back and forth. What if we do {:srv, :ips} and {:srv, :hostnames} and we keep :srv as {:srv, :ips} then, with no deprecations?
sorry for the back and forth.
All good! :wink: It wasn't my plan to come with THE implementation. That's why the PR is still marked as a draft, to figure out what is the best way to go.
What if we do
{:srv, :ips}and{:srv, :hostnames}and we keep:srvas{:srv, :ips}then, with no deprecations?
Isn't this the same as what is pushed currently, just with {:srv, :ips} and {:srv, :hostnames} instead of :srv_ips and :srv_hostnames? :thinking: Currently :srv will internally be mapped to :srv_ips. I only "marked" the :srv type as deprecated in the description for the resource_type description. There's no warning or similar.
I am fine with using tuples instead of atoms. In the end, both ways are the same.
Yes, they are the same, but I think it is more elegant to say :srv is shortcut for {:srv, :ips} then a completely different atom. It also makes it clear they are both using the same mechanism (srv) but in different ways.
@josevalim Would it be okay to add a mocking library (mock, mox, mimic, ...) to mock the calls for :inet_res? This would make things more easy, especially because currently the resolver is untested as it is fully replaced in the tests :thinking: If so, any library you prefer?
Another option is to trim down the resolver interface to be resolve_dns, resolve_srv, so we can push as much as the logic possible to outside of the mocked environment. Then the resolver is a thin wrapper around inet_res (which is the thing you would mock anyway).
What do you think? In fact, I believe I would prefer this approach.
@josevalim I somehow have the feeling that this becomes more and more a refactoring :sweat_smile:
Maybe we first make it work (with mocking) and rework it afterwards. WDYT?
@janpieper or maybe we refactor first and then we add the feature, given the feature itself is straight-forward?
Otherwise it feels backwards to add the mocking library simply to remove it after? 🤔