Skip to content

Support domain names and port numbers in dns_nameservers#2376

Open
yadij wants to merge 6 commits intosquid-cache:masterfrom
yadij:arc-dns-port-1
Open

Support domain names and port numbers in dns_nameservers#2376
yadij wants to merge 6 commits intosquid-cache:masterfrom
yadij:arc-dns-port-1

Conversation

@yadij
Copy link
Contributor

@yadij yadij commented Feb 18, 2026

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.

@yadij
Copy link
Contributor Author

yadij commented Feb 18, 2026

Old cache.log would show:

2026/02/18 22:08:12 kid1| DNS IPv6 socket created at [::], FD 6
2026/02/18 22:08:12 kid1| DNS IPv4 socket created at 0.0.0.0, FD 7
2026/02/18 22:08:12 kid1| Adding nameserver ::1 from squid.conf
2026/02/18 22:08:12.271 kid1| 78,3| /dns_internal.cc(349) idnsAddNameserver: Added nameserver #0 ([::1]:53)
2026/02/18 22:08:12 kid1| Adding nameserver [::2] from squid.conf
2026/02/18 22:08:12 kid1| WARNING: rejecting '[::2]' as a name server, because it is not a numeric IP address
2026/02/18 22:08:12 kid1| Adding nameserver 127.0.0.1:52 from squid.conf
2026/02/18 22:08:12 kid1| WARNING: rejecting '127.0.0.1:52' as a name server, because it is not a numeric IP address
2026/02/18 22:08:12 kid1| Adding nameserver [::3]:81 from squid.conf
2026/02/18 22:08:12 kid1| WARNING: rejecting '[::3]:81' as a name server, because it is not a numeric IP address
2026/02/18 22:08:12 kid1| Adding nameserver example.com:56 from squid.conf
2026/02/18 22:08:12 kid1| WARNING: rejecting 'example.com:56' as a name server, because it is not a numeric IP address

new cache.log looks like:

2026/02/18 21:43:57 kid1| DNS IPv6 socket created at [::], FD 6
2026/02/18 21:43:57 kid1| DNS IPv4 socket created at 0.0.0.0, FD 7
2026/02/18 21:43:57 kid1| Added nameserver #0 ([::1]:53) from squid.conf dns_nameservers ::1
2026/02/18 21:43:57 kid1| Added nameserver #1 ([::2]:53) from squid.conf dns_nameservers  [::2]
2026/02/18 21:43:57 kid1| Added nameserver #2 (127.0.0.1:52) from squid.conf dns_nameservers 127.0.0.1:52
2026/02/18 21:43:57 kid1| Added nameserver #3 ([::3]:81) from squid.conf dns_nameservers [::3]:81
2026/02/18 21:43:57 kid1| Added nameserver #4 (104.18.26.120:56) from squid.conf dns_nameservers example.com:56

@yadij yadij added the feature maintainer needs documentation updates for merge label Feb 18, 2026
@rousskov rousskov changed the title Support DNS nameserver with custom ports Support domain names and port numbers in dns_nameservers Feb 18, 2026
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setup should be done by the Dns::ConfigRr runner. However that refactoring project for startup runners is stuck awaiting reviewer on a number of prerequisites already.

For now this PR keeps the status-quo behaviour when receiving invalid configurations: log and ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done the Dns::ConfigRr changes in PR #2378. Order of merge does not matter for these two.

Have been experimenting today with what is needed to parse the dns_nameservers and validate immediately instead of just storing for later validation. I am not seeing any particular reason why that change cannot be done as a followup - there is only a few lines of overlap with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for multiple dns_nameservers parameters on the same directive line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8aa7366

Copy link
Contributor

@rousskov rousskov Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_nameservers are resolved without using (previously configured) dns_nameservers;
  • how repeated/duplicate dns_nameservers directive entries are interpreted;
  • how multiple dns_nameservers directives 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in d904ba0

try {
static const CharacterSet hostChars = CharacterSet("host", "._-") + CharacterSet::ALPHA + CharacterSet::DIGIT;
static const CharacterSet ip6Chars = CharacterSet("ip6", ":") + CharacterSet::HEXDIG;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not add a yet another code snippet parsing host[:port].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no choice at the moment, the other parsers we have a) do not check the requirements properly, or b) do not parse all the valid cases, or c) are hidden privately inside code that cannot be made a dependency of DNS startup.
The refactor project to update the AnyP::Uri interface for a generically useful parser are stuck awaiting reviewer.

}

// Squid extension for servers with custom ports
int64_t foundPort = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use std::optional<AnyP::KnownPort> instead of a magic number to handle unspecified ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how exactly do you propose we access the non-existent value inside this std::optional container in order to pass it to the tokenizer for initialization?
I see your approach in AnyP::Uri::parsePort() uses a temporary local with magic number rawPort = 0 as well in exactly this same way as here.

@yadij yadij added S-waiting-for-author author action is expected (and usually required) S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Feb 19, 2026
@yadij yadij requested a review from rousskov February 20, 2026 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature maintainer needs documentation updates for merge S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants