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

Add fallback plugin (DNS endpoints only) #1398

Closed
wants to merge 1 commit into from
Closed

Add fallback plugin (DNS endpoints only) #1398

wants to merge 1 commit into from

Conversation

yongtang
Copy link
Member

1. What does this pull request do?

This fix adds a preliminary fallback plugin so that failed DNS queries could be sent to other endpoints.

This fix only accept DNS endpoints at the moment.

2. Which issues (if any) are related?

This fix is part of the #1382.

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

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This fix adds a preliminary fallback plugin so that failed
DNS queries could be sent to other endpoints.

This fix only accept DNS endpoints at the moment.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@codecov-io
Copy link

Codecov Report

Merging #1398 into master will decrease coverage by 0.07%.
The diff coverage is 41.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
- Coverage   51.51%   51.44%   -0.08%     
==========================================
  Files         173      175       +2     
  Lines        8357     8421      +64     
==========================================
+ Hits         4305     4332      +27     
- Misses       3719     3753      +34     
- Partials      333      336       +3
Impacted Files Coverage Δ
plugin/fallback/fallback.go 0% <0%> (ø)
core/dnsserver/server.go 10.37% <0%> (ø) ⬆️
plugin/fallback/setup.go 50.94% <50.94%> (ø)

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 dd9fc89...e9c055b. Read the comment docs.

Copy link
Contributor

@fturib fturib left a comment

Choose a reason for hiding this comment

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

I am not a CoreDNS core dev -- very far from there :)
I like the way it is implement - I find it 'light", but also a bit scary as it is a centralized function that is overwritten.
I hope my comments will help !

~~~ corefile
. {
fallback example.org {
on NXDOMAIN 10.10.10.10:53
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe exemple with several failures cases - to show the full usage.

on NXDOMAIN
on SERVFAIL
on .. ???

is there a list of all events that can happen and we can use ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes : SERVFAIL, NXDOMAIN, ERROR

Copy link
Contributor

Choose a reason for hiding this comment

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

should NXDOMAIN be DENIAL ? or is there a NODATA ?

}

c = caddy.NewTestController("dns", `fallback example.org {
on NXDOMAIN 10.10.10.10:100 8.8.8.8:53
Copy link
Contributor

Choose a reason for hiding this comment

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

test with several "on XXXX" ?
test with file (/etc/resolv.conf)

state := request.Request{W: w, Req: r}
qname := state.Name()
log.Printf("[INFO] Send fallback %q to %q", qname, p.endpoint)
_, err := dns.Exchange(r, p.endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand we use directly Exchange as most of plugin are using Proxy.newLookup(...).
What I see is that in proxy (that is calling exchange), there is a management of the TTL of the query. We do not care here in case of error ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Knowing that this ErrorFunc can now be called whenever in the ServeDNS chain, including before any plugin is called, would it be safe to verify that the request is correctly defined ?

see in server.ServeDNS:

	if r == nil || len(r.Question) == 0 {
		DefaultErrorFunc(w, r, dns.RcodeServerFailure)
		return
	}

@yongtang
Copy link
Member Author

yongtang commented Jan 18, 2018

I think there are some miscommunications there. Let's figure out what is the purpose of fallback. I realized multiple endpoints does not make much sense. Each endpoint could return different errors NXDOMAIN/SERVFAIL/REFUSED again and the logic for errors in the middle will mess up the logic again.

@yongtang
Copy link
Member Author

yongtang commented Jan 18, 2018

I also initially though fallback was to send the failed query to some where else (thus no further handling).

Now it looks like the purpose is to ignore the failure (like SERVFAIL)of certain queries and switch to another proxy endpoint.

@yongtang yongtang closed this Jan 18, 2018
@yongtang yongtang deleted the fallback branch January 18, 2018 04:23
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

3 participants