Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Absolute DNS name are supported #229

Open
jjsaunier opened this issue Jan 6, 2025 · 3 comments
Open

Absolute DNS name are supported #229

jjsaunier opened this issue Jan 6, 2025 · 3 comments
Labels

Comments

@jjsaunier
Copy link

The UdpTransportExecutor does not correctly handle FQDN with a trailing dot, such as google.com.. This notation is often used for in-cluster DNS queries and ensures efficiency by telling the resolver not to search using local domain suffixes. When passing such names, the resolution fails.

use React\Dns\Model\Message;
use React\Dns\Query\Query;
use React\Dns\Query\UdpTransportExecutor;

$executor = new UdpTransportExecutor('8.8.8.8:53');

$name = $argv[1] ?? 'www.google.com.';

$ipv4Query = new Query($name, Message::TYPE_A, Message::CLASS_IN);
$ipv6Query = new Query($name, Message::TYPE_AAAA, Message::CLASS_IN);

$executor->query($ipv4Query)->then(function (Message $message) {
    foreach ($message->answers as $answer) {
        echo 'IPv4: ' . $answer->data . PHP_EOL;
    }
}, 'printf');
$executor->query($ipv6Query)->then(function (Message $message) {
    foreach ($message->answers as $answer) {
        echo 'IPv6: ' . $answer->data . PHP_EOL;
    }
}, 'printf');

I tested on php8.4, react/socket v1.16.0, react/dns v1.13.0

@jjsaunier jjsaunier added the bug label Jan 6, 2025
@WyriHaximus
Copy link
Member

We should add this, even if it's just to trim the trailing period so it works.

However, we currently don't support the search directive in resolve.conf so supporting the trailing period and adding that would mean a BC break in how this package behaves. I'm, more than happy to get this in, but it would be at v3. What are your thoughts on this @clue?

@jjsaunier
Copy link
Author

No rush on this; I discovered it while testing your bunny version 0.6 for rabbitmq which reworks the async part and relies on react/socket, react/dns and this broke during the upgrade test (and yes trimming the ending dot hotfix it)

If the search directive isn’t supported, adding trailing dot support should not cause a BC break since all queries are already treated as absolute. Trimming the dot aligns with DNS standards without altering existing behavior until search is implemented (if I understand correctly)

WyriHaximus added a commit to WyriHaximus-labs/dns that referenced this issue Jan 21, 2025
As brought up in reactphp#229 we're already treating all domains as absolute,
so adding direct support for it is a small bug fix. That said, fully
support for search domains requires a different handling of this and
will be done in a different PR.
@WyriHaximus
Copy link
Member

Just filed #230 to move that hot fix here. Running Bunny inside Kubernetes so having search support in there would make configuring it a little bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants