add support for using domain name in configuration

trinity-1686a commented

2021-03-18 02:21:47 +00:00

Owner

We define a static network here which is not considered as a best practise on Docker. The rational is that Garage only supports IP address and not domain names in its configuration, so we need to know the IP address in advance.

While not an insurmountable issue, it's certainly less than ideal. This pull request uses std::net::ToSocketAddrs to resolve domains.
Care should be taken for the domain to provide only the right ip. For instance localhost usually resolves to both [::1] and 127.0.0.1, and the wrong address might get picked.

From [documentation](https://garagehq.deuxfleurs.fr/getting_started/daemon.html) > We define a static network here which is not considered as a best practise on Docker. The rational is that Garage only supports IP address and not domain names in its configuration, so we need to know the IP address in advance. While not an insurmountable issue, it's certainly less than ideal. This pull request uses [`std::net::ToSocketAddrs`](https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html) to resolve domains. Care should be taken for the domain to provide only the right ip. For instance `localhost` usually resolves to both `[::1]` and `127.0.0.1`, and the wrong address might get picked.

👍 1

lx closed this pull request

2021-03-18 18:27:21 +00:00

lx reopened this pull request

2021-03-18 18:36:07 +00:00

lx changed target branch from dev-0.2 to master

2021-03-18 18:36:52 +00:00

lx commented

2021-03-18 18:41:00 +00:00

Owner

I'm not too keen on resolving domain names for the *_bind_addr configuration variables, as they will mostly look something like localhost:3901, which is ambiguous between IPv4 and IPv6, or [::]:3901, which does not have a named equivalent.

This is a great addition for the bootstrap peer list though!

I'm not too keen on resolving domain names for the `*_bind_addr` configuration variables, as they will mostly look something like `localhost:3901`, which is ambiguous between IPv4 and IPv6, or `[::]:3901`, which does not have a named equivalent. This is a great addition for the bootstrap peer list though!

trinity-1686a force-pushed bind-domain from 1bb44fa8a2 to 319ec42805

2021-03-18 18:47:03 +00:00

Compare

trinity-1686a force-pushed bind-domain from 319ec42805 to c8a7ce5cdf

2021-03-18 18:48:00 +00:00

Compare

lx commented

2021-03-18 18:49:53 +00:00

Owner

Also, here's a suggestion: if a domain name resolves to many IPs, the current PR takes only the first one, but what if we put all Garage node IPs in a single domain? If all IPs were taken this would make bootstrap possible with a single domain name!

trinity-1686a commented

2021-03-18 18:52:14 +00:00

Author

Owner

This is possible, and simple enough to implement, however IPs are only resolved at config deserialization, not after, so adding new IPs to the domain name would get detected only by restarting the server

lx commented

2021-03-18 19:12:53 +00:00

Owner

Well bootstrap is done once anyway, at server start only. The Consul module enables regular polling of a Consul service to get new nodes, but in the non-Consul case it is expected that the new nodes will be the one bootstraping by contacting the existing nodes, and not the other way around, so if the DNS contains at least the existing nodes when a new node is launched then it will work.

trinity-1686a added 1 commit 2021-03-18 20:04:40 +00:00

resolve domain to multiple addresses

continuous-integration/drone/pr Build is passing

Details

f17cb6c969

And warn instead of failling when a domain can't be resolved

lx merged commit e19e267b7e into master

2021-03-18 20:26:35 +00:00

lx referenced this pull request from a commit

2021-03-18 20:26:36 +00:00

Merge pull request 'add support for using domain name in configuration' (#46) from trinity-1686a/garage:bind-domain into master

trinity-1686a deleted branch bind-domain

2021-03-18 20:32:37 +00:00

add support for using domain name in configuration #46