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

Fix handling truncated responses in forward #3110

Merged
merged 2 commits into from Aug 12, 2019
Merged

Conversation

@ameshkov
Copy link
Contributor

ameshkov commented Aug 11, 2019

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

Currently, forward plugin does not handle truncated responses as it was supposed to because there's no error returned when a truncated response is received

2. Which issues (if any) are related?

None

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

None

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

On the contrary

@corbot corbot bot requested a review from johnbelamaric Aug 11, 2019
@corbot

This comment has been minimized.

Copy link

corbot bot commented Aug 11, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked johnbelamaric (via plugin/forward/OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 11, 2019

Codecov Report

Merging #3110 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3110      +/-   ##
==========================================
+ Coverage   55.51%   55.52%   +0.01%     
==========================================
  Files         207      207              
  Lines       10337    10335       -2     
==========================================
  Hits         5739     5739              
+ Misses       4183     4182       -1     
+ Partials      415      414       -1
Impacted Files Coverage Δ
plugin/forward/forward.go 54.76% <ø> (-1.06%) ⬇️
plugin/file/reload.go 74.28% <0%> (+5.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebe6a41...b4e384e. Read the comment docs.

@rdrozhdzh

This comment has been minimized.

Copy link
Contributor

rdrozhdzh commented Aug 12, 2019

@ameshkov, could you clarify your use case? I guess the only problematic case here is when user always sends DNS queries by TCP (e.g. via TLS connection) and forward plugin is configured with prefer_udp option.
In other cases, the response should get back to user with TC flag set and user should resend the query over TCP.

@ameshkov

This comment has been minimized.

Copy link
Contributor Author

ameshkov commented Aug 12, 2019

@rdrozhdzh that's right, when prefer_udp is specified, forward plugin was supposed to handle it and repeat the request automatically, and I guess it was working okay until some recent changes in miekg/dns -- when ErrTruncated was deprecated.

@UladzimirTrehubenka

This comment has been minimized.

Copy link
Contributor

UladzimirTrehubenka commented Aug 12, 2019

ErrTruncated was deprecated in miekg/dns#815 (27 Nov 2018) - delivered in v 1.1.0 (29 Nov 2018)

@ameshkov

This comment has been minimized.

Copy link
Contributor Author

ameshkov commented Aug 12, 2019

So not so recent then:). It has been broken since then.

plugin/forward/forward.go Outdated Show resolved Hide resolved
Copy link
Contributor

rdrozhdzh left a comment

looks good to me

@miekg miekg merged commit 1ef24a8 into coredns:master Aug 12, 2019
4 checks passed
4 checks passed
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.52% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.