Support domain names and port numbers in dns_nameservers#2376
Support domain names and port numbers in dns_nameservers#2376yadij wants to merge 4 commits intosquid-cache:masterfrom
Conversation
|
Old cache.log would show: new cache.log looks like: |
rousskov
left a comment
There was a problem hiding this comment.
I adjusted PR title to more prominently disclose that this PR adds domain name support and to mention the affected directive name for context.
I plan to come back to this PR once the backlog is dealt with.
There was a problem hiding this comment.
Before we make this code more complex, I recommend changing/fixing this code error handling so that configuration errors trigger an exception (and, hence, become fatal at startup). That change should be done in a dedicated prerequisite PR. It will affect how this code is structured and called/used.
There was a problem hiding this comment.
Please add a test for multiple dns_nameservers parameters on the same directive line.
There was a problem hiding this comment.
Please adjust dns_nameservers description in cf.data.pre to avoid the current possible implication that port numbers are not supported and to disclose that domain names are supported (in addition to IP addresses).
It is probably a good idea to also note
- that domain names specified in
dns_nameserversare resolved without using (previously configured)dns_nameservers; - how repeated/duplicate
dns_nameserversdirective entries are interpreted; - how multiple
dns_nameserversdirectives are interpreted; and - what affect does the order of dns_nameservers` directive entries have.
Edit: GitHub hit this change request while I was writing my review, so I added another, similar one. I deleted the latter after I noticed that this change request was resurrected.
| try { | ||
| static const CharacterSet hostChars = CharacterSet("host", "._-") + CharacterSet::ALPHA + CharacterSet::DIGIT; | ||
| static const CharacterSet ip6Chars = CharacterSet("ip6", ":") + CharacterSet::HEXDIG; | ||
|
|
There was a problem hiding this comment.
Please do not add a yet another code snippet parsing host[:port].
| // TODO: support host/FQDN that resolves to multiple IPs | ||
| // TODO: support configured append_domain search for non-IPs | ||
|
|
||
| // XXX: performance regression. c_str() copies |
There was a problem hiding this comment.
This code is not performance-sensitive enough for this XXX. I recommend removing it:
| // XXX: performance regression. c_str() copies |
| } | ||
|
|
||
| // Squid extension for servers with custom ports | ||
| int64_t foundPort = 0; |
There was a problem hiding this comment.
Please use std::optional<AnyP::KnownPort> instead of a magic number to handle unspecified ports.
Upgrade the DNS nameserver configuration parser
to better validate IP/hostname labels.
Add support for IPv6 with brackets and optional
port values. These are not valid for /etc/resolv.conf
content, but may be used by admin in squid.conf
dns_nameservers and Windows registry settings.
Add support for named servers. OS resolver is used
to lookup the configured host name. Currently only
one IP address per host is supported.
Also, polish cache.log output to avoid false claims
of adding a nameserver which is then ignored.
Configuration errors are now displayed as "ERROR"
and each added entry has its origin name/label
logged on the same output line.