Trim the suffix dot from the srv.Target for etcd-client DNS lookup #13712
Conversation
ahrtr
force-pushed
the
correct_dns_etcd_client
branch
from
170f017 to
71022a6
Compare
Codecov Report
@@ Coverage Diff @@
## main #13712 +/- ##
==========================================
- Coverage 72.85% 72.78% -0.08%
==========================================
Files 465 465
Lines 37880 37881 +1
==========================================
- Hits 27599 27573 -26
- Misses 8508 8538 +30
+ Partials 1773 1770 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ahrtr
force-pushed
the
correct_dns_etcd_client
branch
2 times, most recently
from
9958bf0 to
09038ea
Compare
Thanks for the quick review, resolved. |
ahrtr
force-pushed
the
correct_dns_etcd_client
branch
from
09038ea to
7c70526
Compare
ahrtr
force-pushed
the
correct_dns_etcd_client
branch
from
7c70526 to
5fc0092
Compare
|
Resolved all comments, PTAL @serathius |
| @@ -106,9 +106,10 @@ func GetClient(service, domain string, serviceName string) (*SRVClients, error) | |||
| return err | |||
| } | |||
| for _, srv := range addrs { | |||
| shortHost := strings.TrimSuffix(srv.Target, ".") | |||
There was a problem hiding this comment.
I thought the trailing dot meant the dns name was canonical and shouldn't have dns search paths appended to it. Does trimming the dot mean local dns search paths are used and the resolved address could end up being different?
There was a problem hiding this comment.
If you keep reading the following couple of lines of code, you will find we just trim the trailing dot when constructing the network address.
There was a problem hiding this comment.
if the SRV records are being returned with trailing dots, and we're turning that into client URLs without trailing dots, doesn't that mean DNS search paths are used to resolve those URLs to IPs, contrary to what the trailing dot in the srv record indicated?
There was a problem hiding this comment.
The addrs is already the results of dns lookup. The urls/endpoints will be delivered to gRPC
There was a problem hiding this comment.
won't a second DNS lookup be done to resolve the hostnames to IPs? don't we still need the trailing . to make that DNS resolution avoid using locally configured DNS search paths?
There was a problem hiding this comment.
I verified this change is incorrect, opened #13948 with details of the implications for DNS lookup.
There was a problem hiding this comment.
that also probably might mean the server-side SRV record transformation this was based on is incorrect -
Line 72 in e814f6f
There was a problem hiding this comment.
Please see my response in 13948#issuecomment-1100588121)
| {Target: "a.example.com", Port: 2480}, | ||
| {Target: "b.example.com", Port: 2480}, | ||
| {Target: "a.example.com.", Port: 2480}, | ||
| {Target: "b.example.com.", Port: 2480}, | ||
| {Target: "c.example.com", Port: 2480}, | ||
| }, | ||
| []*net.SRV{}, |
There was a problem hiding this comment.
this unit test is a good example of what looks incorrect at first glance... with a client connection of []string{"https://a.example.com:2480", "https://b.example.com:2480", "https://c.example.com:2480"}, won't local DNS search paths be attempted for these URLs (depending on ndots settings)?
Usually the SRV records returned by DNS lookup have a trailing dot but URL shouldn't, so we should trim the suffix dot. We have already trimmed the suffix dot when bootstrapping etcd server ( see srv.go#L72 ), but not for etcd client.
cc @serathius @spzala @ptabor