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

plugin/hosts: Modifies NODATA handling #3536

Merged
merged 9 commits into from Feb 24, 2020
Merged

plugin/hosts: Modifies NODATA handling #3536

merged 9 commits into from Feb 24, 2020

Conversation

@ykhr53
Copy link
Collaborator

ykhr53 commented Dec 12, 2019

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

Bug fixing for the issue #2564.
If hosts file has only A record and this plugin receives a query for AAAA, NODATA should be returned.
Now, this plugin fallthrough the query and a client going to get NXDOMAIN.

2. Which issues (if any) are related?

#2564

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

I don't think so.

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

No.

Signed-off-by: ykhr53 <ykhr53@yokohei.com>
@ykhr53 ykhr53 requested review from johnbelamaric and pmoroney as code owners Dec 12, 2019
@@ -52,15 +52,15 @@ func (h Hosts) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
answers = aaaa(qname, h.options.ttl, ips)
}

if len(answers) == 0 {
// This condition hooks only NXDOMAIN.

This comment has been minimized.

Copy link
@miekg

miekg Dec 12, 2019

Member

comment looks off, maybe something like:
Only on NXDOMAIN we will fallthrough

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 12, 2019

Codecov Report

Merging #3536 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3536      +/-   ##
==========================================
+ Coverage   56.62%   56.66%   +0.04%     
==========================================
  Files         222      222              
  Lines       11044    11043       -1     
==========================================
+ Hits         6254     6258       +4     
+ Misses       4311     4307       -4     
+ Partials      479      478       -1
Impacted Files Coverage Δ
plugin/hosts/hosts.go 88.52% <100%> (+9.49%) ⬆️
plugin/clouddns/clouddns.go 82.72% <0%> (-2.73%) ⬇️
plugin/file/reload.go 75% <0%> (+5.55%) ⬆️

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 c95e7f2...22c00c2. Read the comment docs.

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Dec 12, 2019

needs a test as well

@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Dec 12, 2019

Thank you for the review!
The original code has some tests which cover this change, that's why I didn't add tests.
Do you think we need to add a new test for more specific case?

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Dec 12, 2019

@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Dec 12, 2019

Ok, thanks. I modified two tests to ensure to get NODATA instead of NXDOMAIN.

if !h.otherRecordsExist(qname) {
return dns.RcodeServerFailure, nil
}
return dns.RcodeRefused, nil

This comment has been minimized.

Copy link
@miekg

miekg Dec 13, 2019

Member

I meant changing the comment, not the return code itself

This comment has been minimized.

Copy link
@miekg

miekg Dec 13, 2019

Member

also making this change without failing tests means this is lacking coverage. As you are touching this code; please also add a test for this :)

This comment has been minimized.

Copy link
@ykhr53

ykhr53 Dec 13, 2019

Author Collaborator

OK, I'm going to add some new tests which will cover this line and my new code!

@@ -82,11 +82,11 @@ var hostsTestCases = []test.Case{
},
{
Qname: "example.org.", Qtype: dns.TypeAAAA,
Answer: []dns.RR{},
Answer: []dns.RR{}, Rcode: dns.RcodeSuccess,

This comment has been minimized.

Copy link
@miekg

miekg Dec 13, 2019

Member

I would like to see a new test that is failing with the current code and working with the new

@ykhr53 ykhr53 force-pushed the ykhr53:hosts-modify branch from f023062 to 64ba72c Dec 13, 2019
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Dec 13, 2019

I made a bit wide change to the hosts_test.go because we need to take care the cases whether fallthrough is enabled or not.
The last test will be fail in the current code, and work in the new code.
I hope it's good and enough :)

@ykhr53 ykhr53 requested a review from miekg Dec 13, 2019
@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Jan 2, 2020

@miekg @pmoroney @johnbelamaric

Hi, could you review this code?

},
}
h.hmap = h.parse(strings.NewReader(hostsExample))
for i, tc := range hostsTestCases {

This comment has been minimized.

Copy link
@miekg

miekg Jan 3, 2020

Member

adding the test is good, but why did you amend an existing test? Better to just have a small contained test with only 1 use case: one that fails now and is fixed with this PR

This comment has been minimized.

Copy link
@ykhr53

ykhr53 Jan 3, 2020

Author Collaborator

the Case structure doesn't have any optional fields, so I need to extend it to use Fallthrough flag in func TestLookupA, I could not add Fallthrough only for the last test case.
So... should I add a new function like func TestFallthrough which checks only one case?
I don't have an idea which one is better, I'm going to follow your advice.
Thanks!

@ykhr53 ykhr53 force-pushed the ykhr53:hosts-modify branch from 22c00c2 to 4fb848f Jan 13, 2020
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
@ykhr53 ykhr53 force-pushed the ykhr53:hosts-modify branch from 4fb848f to 8a6f184 Jan 13, 2020
@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Jan 13, 2020

I updated the test code to have a small contained test for failing only the current code.
We need to check RCODE to fix and test this bug, but as you said we don't want to change the existing cases.
So in this code, just check RCODE only the case fallthrough is enabled.
(I would like to add test index like the other plugins, but in this PR, I'm focusing on fixing this bug.)

Current Code

--- FAIL: TestLookupA (0.00s)
    /Users/yukihira/develop/coredns/plugin/hosts/hosts_test.go:48: Expected rcode is 0, but got 3
FAIL
FAIL	github.com/coredns/coredns/plugin/hosts	0.018s
Error: Tests failed.

New Code by this PR

ok  	github.com/coredns/coredns/plugin/hosts	(cached)
Success: Tests passed.
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Jan 14, 2020

The CircleCI test was failed because of the following reason.

    --- FAIL: TestKubernetesA/dns-version.cluster.local._TXT (0.44s)
        address_test.go:173: RR 0 should have a Txt of "1.1.0", but has "1.0.1"
        address_test.go:176: coredns log: .:53

I found some same error cases, but to be honest I am not sure this error is related to hosts plugin changes.
Much appreciated if anyone takes a look at it. Thanks.
https://circleci.com/gh/coredns/coredns/1411
https://circleci.com/gh/coredns/coredns/1409

@chrisohaver

This comment has been minimized.

Copy link
Member

chrisohaver commented Jan 14, 2020

The CircleCI test was failed because of the following reason. ...

Updating your master branch and rebasing this branch against master should fix it.

@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Jan 14, 2020

@chrisohaver Thank you for the reply and giving me a solution! I'm gonna do that.

ykhr53 added 4 commits Dec 12, 2019
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
Signed-off-by: ykhr53 <ykhr53@yokohei.com>
@ykhr53 ykhr53 requested a review from miekg Jan 15, 2020
@miekg
miekg approved these changes Feb 24, 2020
@miekg miekg merged commit 813cc5d into coredns:master Feb 24, 2020
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.59% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@ykhr53 ykhr53 deleted the ykhr53:hosts-modify branch Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.