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

resolve: remove server 'large' level #20528

Merged
merged 1 commit into from Dec 7, 2021
Merged

Conversation

ddstreet
Copy link
Contributor

This removes the DNS_SERVER_FEATURE_LEVEL_LARGE, and sets the EDNS0
advertised max packet size as if always in 'large' mode.

Without this, we always send out EDNS0 opts that limit response sizes
to 512 bytes, thus the remote server will never send anything larger
and will always truncate responses larger than 512 bytes, forcing us
to drop from EDNS0 down to TCP, even though one of the primary benefits
of EDNS0 is larger packet sizes.

@ddstreet
Copy link
Contributor Author

rebased on main, just to keep this up to date.

@yuwata could you take a look at this if you have time?

@yuwata
Copy link
Member

yuwata commented Sep 10, 2021

rebased on main, just to keep this up to date.

@yuwata could you take a look at this if you have time?

I'd like to forward this to @poettering or @keszybz. I am not familiar with DNS protocol...

@ddstreet
Copy link
Contributor Author

just rebased on main to keep this up to date.

@technicianted
Copy link

Can we expedite this? the issue causes hostname failures if tcp dns is not available.

This removes the DNS_SERVER_FEATURE_LEVEL_LARGE, and sets the EDNS0
advertised max packet size as if always in 'large' mode.

Without this, we always send out EDNS0 opts that limit response sizes
to 512 bytes, thus the remote server will never send anything larger
and will always truncate responses larger than 512 bytes, forcing us
to drop from EDNS0 down to TCP, even though one of the primary benefits
of EDNS0 is larger packet sizes.

Fixes: systemd#20993
@ddstreet
Copy link
Contributor Author

rebased, and also opened #20993 for this

@fredflev
Copy link

fredflev commented Nov 3, 2021

the issue causes hostname failures if tcp dns is not available.

I can confirm ^^ '

Using feature level UDP+EDNS0 for transaction 49776.
Using DNS server 168.63.129.16 for transaction 49776.
Sending query packet with id 49776.
Processing query...
Processing incoming packet on transaction 49776. (rcode=SUCCESS)
Reply truncated, retrying via TCP.
Using feature level UDP+EDNS0 for transaction 49776.
Timeout reached on transaction 49776.
Retrying transaction 49776.

@keszybz
Copy link
Member

keszybz commented Nov 5, 2021

Sorry for the delay. The changes to the code look correct.

@poettering, review?

@poettering
Copy link
Member

Did anyone actually do a review of this? I totally don#t follow here. @keszybz you merged this, what's going on here?

The original idea was that we'd start out with "large" mode, and if that fails, will fall back to simple edns0 (and thus still get better error codes and stuff, but no large packets) mode. Now you basically removed the latter? Why though?

The thing is that whether edns0 works or not is primarily a question of the server implementation. While the packet size is a lot more a question of the gateways in between... Hence it makes sense to discern edns0 mode from large mode, and still to edns0 if the large stuff doesn't work.

Anyway, I don't understand what the rationale for removing this altogether is. There's no rationale for that given.. Only this:

"Without this, we always send out EDNS0 opts that limit response sizes
to 512 bytes, thus the remote server will never send anything larger
and will always truncate responses larger than 512 bytes, forcing us
to drop from EDNS0 down to TCP, even though one of the primary benefits
of EDNS0 is larger packet sizes."

But that sounds like an issue that the large level is not sued on the offending system at all?

I mean, I am not questioning that there's a bug, but it seems a bit drastic to remove the middle level altogether.

So, what's the rationale here?

@poettering
Copy link
Member

I filed #22368 as a revert for this. Not necessarily something to merge, but just so that we can track this, because this doesn't look right to me, and this requires further discussion.

(Sorry for catching up so late, I have been on parental leave the past months)

@ddstreet ddstreet deleted the edns-larger-size branch September 15, 2022 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

6 participants