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

Handle UDP responses that overflow with TC bit #6277

Merged
merged 17 commits into from
Sep 7, 2023
Merged

Handle UDP responses that overflow with TC bit #6277

merged 17 commits into from
Sep 7, 2023

Conversation

SriHarsha001
Copy link
Contributor

@SriHarsha001 SriHarsha001 commented Aug 17, 2023

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

This PR - #6100 was closed by mistake. So submitting a new PR here with the same code changes.

Getting this PR miekg/dns#1475 reviewed is taking time. Can we get the bug fix merged here with "overflow" string comparison. And we can address this "To accurately identify the error by type rather than string compares" as a follow up PR while I work with maintainers of miekg package.

2. Which issues (if any) are related?

#5953
#5998

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

Not sure.

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

No.

@SriHarsha001
Copy link
Contributor Author

@chrisohaver - I apologize, I had accidently deleted my fork which closed this PR - #6100 , hence submitted this PR with the same code changes.

As trying to get this PR miekg/dns#1475 reviewed and merged is taking a long time, can we get the bug fix merged here with "overflow" string comparison.

Can we address this comment "To accurately identify the error by type rather than string compares" as a follow up PR while I work with maintainers of miekg package.

@SriHarsha001
Copy link
Contributor Author

@chrisohaver - When you get a chance, I kindly request you to take a look at this request.

@chrisohaver
Copy link
Member

Is there anything new to review here vs where #6100 was left off?

@SriHarsha001
Copy link
Contributor Author

@chrisohaver - Thank you for your reply. I wanted to check if we get the bug fix merged here with "overflow" string comparison. And we can address this "To accurately identify the error by type rather than string compares" as a follow up PR while I work with maintainers of miekg package on this PR - miekg/dns#1475

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #6277 (a89212e) into master (93c57b6) will increase coverage by 2.77%.
Report is 1035 commits behind head on master.
The diff coverage is 55.36%.

@@            Coverage Diff             @@
##           master    #6277      +/-   ##
==========================================
+ Coverage   55.70%   58.47%   +2.77%     
==========================================
  Files         224      252      +28     
  Lines       10016    16537    +6521     
==========================================
+ Hits         5579     9670    +4091     
- Misses       3978     6278    +2300     
- Partials      459      589     +130     
Files Changed Coverage Δ
core/dnsserver/config.go 0.00% <ø> (ø)
core/dnsserver/register.go 13.27% <0.00%> (-4.71%) ⬇️
plugin/auto/auto.go 0.00% <0.00%> (ø)
plugin/auto/xfr.go 0.00% <0.00%> (ø)
plugin/auto/zone.go 78.94% <ø> (+0.68%) ⬆️
plugin/autopath/autopath.go 73.61% <ø> (-1.39%) ⬇️
plugin/azure/azure.go 10.61% <0.00%> (-0.57%) ⬇️
plugin/azure/setup.go 56.36% <0.00%> (-4.14%) ⬇️
plugin/backend_lookup.go 0.00% <0.00%> (ø)
plugin/bind/bind.go 50.00% <0.00%> (-50.00%) ⬇️
... and 77 more

... and 170 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chrisohaver
Copy link
Member

@chrisohaver - Thank you for your reply. I wanted to check if we get the bug fix merged here with "overflow" string comparison. And we can address this "To accurately identify the error by type rather than string compares" as a follow up PR while I work with maintainers of miekg package on this PR - miekg/dns#1475

Aside from that, were there other outstanding issues that needed to be addressed? It looks like there were ... relating to AA bit? Maybe elsewhere? Can you review the feedback and requested changes in #6100?

@SriHarsha001
Copy link
Contributor Author

@chrisohaver - yes yes, we had discussed about setting AA bit to false here - #6100 (comment)

Please let me know, if there is anything outstanding that needs to be address.

@chrisohaver
Copy link
Member

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

I think in that case we should not alter the AA bit from the original.

plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/proxy_test.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
Copy link
Member

@chrisohaver chrisohaver left a comment

Choose a reason for hiding this comment

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

requested changes may be confusing... I think all the error/overflow/truncation logic can be moved into the if proto == "udp" { ... } block.

plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
plugin/pkg/proxy/connect.go Outdated Show resolved Hide resolved
SriHarsha001 and others added 9 commits September 7, 2023 09:31
Co-authored-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Co-authored-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
@chrisohaver chrisohaver merged commit 4c69549 into coredns:master Sep 7, 2023
13 checks passed
@chrisohaver chrisohaver changed the title Handle UDP responses that overflow with TC bit with test case Handle UDP responses that overflow with TC bit Oct 27, 2023
@SuperQ SuperQ mentioned this pull request Jan 8, 2024
gcs278 pushed a commit to gcs278/coredns that referenced this pull request Jan 23, 2024
Handle UDP responses that overflow with TC bit with test case (coredns#6277)

Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/coredns that referenced this pull request Jan 24, 2024
Handle UDP responses that overflow with TC bit with test case (coredns#6277)

Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
gcs278 pushed a commit to gcs278/coredns that referenced this pull request Jan 25, 2024
Handle UDP responses that overflow with TC bit with test case (coredns#6277)

Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Modified-by: Grant Spence <gspence@redhat.com>
gcs278 pushed a commit to gcs278/coredns that referenced this pull request Jan 25, 2024
Handle UDP responses that overflow with TC bit with test case (coredns#6277)

Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Modified-by: Grant Spence <gspence@redhat.com>
gcs278 pushed a commit to gcs278/coredns that referenced this pull request Jan 25, 2024
Handle UDP responses that overflow with TC bit with test case (coredns#6277)

Signed-off-by: SriHarshaBS001 <SriHarshaBS009@gmail.com>
Modified-by: Grant Spence <gspence@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants