Handle UDP responses that overflow with TC bit with test case #6100

SriHarsha001 · 2023-05-15T18:52:51Z

1. Why is this pull request needed and what does it do?

Description in this PR - #6003

Code changes in plugin/pkg/proxy/connect.go file is copied from above PR - #6003

2. Which issues (if any) are related?

#5953
#5998

3. Which documentation changes (if any) need to be made?

Not sure. I didn't see any.

4. Does this introduce a backward incompatible change or deprecation?

No.

chrisohaver · 2023-05-15T19:20:03Z

Copy of description from #6003

Please just make a reference to the original PR instead of copy-pasting the original PR description. In the description, please make it clear that you are copying the code of from @phealy's PR, which appears to have stalled. Please also review all outstanding feedback on the original PR, and list the changes you have made to address that feedback. Thanks!

gcs278

thanks for the PR

plugin/pkg/proxy/connect.go

gcs278 · 2023-05-17T18:05:25Z

plugin/pkg/proxy/connect.go

+			// Instead of returning an error, return an empty response with TC bit set. This will make the
+			// client retry over TCP (if that's supported) or at least receive a clean
+			// error. The connection is still good so we break before the close.
+			if proto == "udp" && strings.Contains(err.Error(), "overflow") {


I had a question regarding if we should create types for these errors, searching for the string "overflow" seems a bit dangerous to me: https://github.com/coredns/coredns/pull/6003/files#r1156333895

I have create an issue here - miekg/dns#1463 and will have to wait before I can update this code - if proto == "udp" && ((strings.Contains(err.Error(), "overflow")) || dnsErrBufOccured)

Created a PR for this - miekg/dns#1465

Thanks! In that PR, maybe put a blank line between ErrTime and ErrOverflowUnpackingA, that way indentation doesn't change on existing set of errors, and the diff looks cleaner.

@chrisohaver , @gcs278 - While waiting for this PR miekg/dns#1465 to be reviewed, I wanted to check with you if I need to check for all the overflow errors exported here - https://github.com/miekg/dns/pull/1465/files. Because Overflow error due to 512 cache size can occur due to any of the reasons.

For example -

if errors.Is(err, dns.ErrOverflowHeaderSize)
if errors.Is(err, dns.ErrOverflowPackingUint8)
if errors.Is(err, dns.ErrBuf) .... so on

@miekg - Any suggestions will be appreciated. I will look forward to hearing from you.

@chrisohaver - Do you have any suggestions here ?

@chrisohaver @miekg - I kindly request your review and suggest on what we can do here to move forward on this change. We need to fix this to move forward with upcoming releases.

Miek is on indefinite sabbatical from the CoreDNS project, for about a year now.

Looks like miekg/dns#1465 was rejected.

Based on the comment, I would suggest re-submitting it with following changes:

Don't create new class variables for every possible overflow case.
Create a single public overflow error type that implements error, and raise this error for overflows.
You can make the error text different per overflow case, but the error type would be the same for all.

Thank you @chrisohaver - I have submitted a PR with the changes here - miekg/dns#1475, waiting for it to be reviewed.

chrisohaver

Thanks for picking this up!

In the description, please make it clear that you are copying the code of from @phealy's PR, and the reason (it appears to have stalled)
Please also review all outstanding feedback on the original PR, and list the changes you have made to address that feedback. (e.g. I see that you added a unit test, but there was also other feedback that does not appear to be addressed)

plugin/pkg/proxy/connect.go

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

chrisohaver · 2023-06-26T18:05:22Z

plugin/pkg/proxy/connect.go

@@ -142,6 +143,10 @@ func (p *Proxy) Connect(ctx context.Context, state request.Request, opts Options

 				// Clear AD bit in case request had set the AD bit. The empty response is not authenticated.
 				newRet.AuthenticatedData = false
+
+				// Clear AA bit in case request had set the AA bit.


Why do you clear the AA bit?

that means that this request isn't an authoritative (i.e. that server owns the final answer) response. Since it's not a complete answer, it's good to keep it set to false. @chrisohaver - Please let me know if you see any concerns here.

Cool, no concerns. Please add that description to the existing comment.

it's good to keep it set to false

Actually, are you basing that on common sense, or an RFC requirement?
We may want to follow what is done for actual truncated responses.

If RFC says AA should be set to false when replying with a truncated response, then we should definitely do that here.
if not, then we need to discuss pros/cons. E.g. what's is the negative of leaving AA bit as it was.

I couldn't find any specific RFC that directly addresses the handling of the authorization bit (AA) in truncated DNS responses. There is no specific handling or interpretation defined for the authorization bit (AA) in truncated responses

RFC 1035 does not specifically cover the handling of the AA bit in truncated responses
https://tools.ietf.org/html/rfc1035
https://datatracker.ietf.org/doc/html/rfc1035#section-4.2.1

When a response is truncated, the focus is on addressing the truncation itself and ensuring the complete response is obtained. The authorization bit is not directly related to handling truncation but rather serves as an indicator of authority.
The authorization bit is considered meaningful only in non-truncated responses, where it signifies the authoritative nature of the responding server.

AFAIK - It is better to keep it set to false here.

SriHarsha001 requested review from miekg, chrisohaver, johnbelamaric, yongtang and stp-ip as code owners May 15, 2023 18:52

gcs278 reviewed May 17, 2023

View reviewed changes

chrisohaver requested changes May 18, 2023

View reviewed changes

Tantalor93 added the needs update Please update the PR label May 26, 2023

chrisohaver reviewed Jun 6, 2023

View reviewed changes

plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved

SriHarsha001 requested review from inigohu, zouyee, Tantalor93, bradbeam, dilyevsky, jameshartig, greenpau, Isolus, pmoroney, rajansandeep and SuperQ as code owners June 6, 2023 20:14

SriHarsha001 and others added 5 commits June 6, 2023 13:18

Add a test case for CoreDNSOverflow scenario

624cef2

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

Add a test case for CoreDNSOverflow scenario

6dc1e5c

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

Add a test case for CoreDNSOverflow scenario

d92f287

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

Add a test case for CoreDNSOverflow scenario

0ad2324

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

Clearing AA bit

35ab6e3

Signed-off-by: Sri Harsha <sharsha@microsoft.com>

chrisohaver reviewed Jun 26, 2023

View reviewed changes

chrisohaver mentioned this pull request Jun 30, 2023

CoreDNS resiliency with non-compliant upstream #5953

Closed

SriHarsha001 closed this by deleting the head repository Aug 10, 2023

SriHarsha001 mentioned this pull request Aug 17, 2023

Handle UDP responses that overflow with TC bit #6277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle UDP responses that overflow with TC bit with test case #6100

Handle UDP responses that overflow with TC bit with test case #6100

SriHarsha001 commented May 15, 2023 •

edited

Loading

chrisohaver commented May 15, 2023

gcs278 left a comment

gcs278 May 17, 2023

SriHarsha001 Jun 6, 2023

SriHarsha001 Jun 7, 2023

chrisohaver Jun 7, 2023

SriHarsha001 Jun 8, 2023

SriHarsha001 Jun 26, 2023

SriHarsha001 Jul 10, 2023

SriHarsha001 Jul 12, 2023

chrisohaver Jul 12, 2023 •

edited

Loading

SriHarsha001 Jul 20, 2023

chrisohaver left a comment •

edited

Loading

chrisohaver Jun 26, 2023

SriHarsha001 Jul 10, 2023 •

edited

Loading

chrisohaver Jul 12, 2023

chrisohaver Jul 12, 2023

SriHarsha001 Jul 20, 2023 •

edited

Loading

Handle UDP responses that overflow with TC bit with test case #6100

Handle UDP responses that overflow with TC bit with test case #6100

Conversation

SriHarsha001 commented May 15, 2023 • edited Loading

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

chrisohaver commented May 15, 2023

gcs278 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisohaver Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisohaver left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SriHarsha001 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SriHarsha001 Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

SriHarsha001 commented May 15, 2023 •

edited

Loading

chrisohaver Jul 12, 2023 •

edited

Loading

chrisohaver left a comment •

edited

Loading

SriHarsha001 Jul 10, 2023 •

edited

Loading

SriHarsha001 Jul 20, 2023 •

edited

Loading