plugin/file: fix local CNAME lookup #1866
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue #1864 explains it will, when we serve the child zone as well we should just recursive into ourself (upstream self). Thus relax the IsSubDomain check in file/lookup.go and just query (even if the query will hit a remote server). I've looped over all other plugins that do something similar (CNAME resolving) and they didn't do the IsSubDomain check; therefor I've removed it from *file* as well. Added test in file_upstream_test that shows this failed before but now results in a reply. Fixes #1864
|
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 superq (via |
SuperQ
reviewed
Jun 9, 2018
| @@ -380,7 +374,7 @@ func cnameForType(targets []dns.RR, origQtype uint16) []dns.RR { | |||
| func (z *Zone) externalLookup(state request.Request, target string, qtype uint16) []dns.RR { | |||
| m, e := z.Upstream.Lookup(state, target, qtype) | |||
| if e != nil { | |||
| // TODO(miek): debugMsg for this as well? Log? | |||
| // TODO(miek): Log, or return error here? | |||
There was a problem hiding this comment.
Increment an error counter here. 😄
Codecov Report
@@ Coverage Diff @@
## master #1866 +/- ##
==========================================
+ Coverage 54.41% 54.49% +0.08%
==========================================
Files 189 189
Lines 9310 9307 -3
==========================================
+ Hits 5066 5072 +6
+ Misses 3851 3843 -8
+ Partials 393 392 -1
Continue to review full report at Codecov.
|
|
Thanks for reviewing.
Increment an error counter here. 😄
Could point. And probably should, but that requires more code as we don't have a
metrics to use here. We can't use the normal metrics (i.e return an error),
because there may not be a CNAME target; meaning this isn't error.
Proxy itself might need to export fail/errors
/Miek
…--
Miek Gieben
|
fturib
pushed a commit
to fturib/coredns
that referenced
this pull request
Jun 13, 2018
* plugin/file: fix local CNAME lookup Issue coredns#1864 explains it will, when we serve the child zone as well we should just recursive into ourself (upstream self). Thus relax the IsSubDomain check in file/lookup.go and just query (even if the query will hit a remote server). I've looped over all other plugins that do something similar (CNAME resolving) and they didn't do the IsSubDomain check; therefor I've removed it from *file* as well. Added test in file_upstream_test that shows this failed before but now results in a reply. Fixes coredns#1864 * self does not need to be exported * Fix test We don't know if we had a valid reply. Check this.
fturib
pushed a commit
to fturib/coredns
that referenced
this pull request
Jun 13, 2018
* plugin/file: fix local CNAME lookup Issue coredns#1864 explains it will, when we serve the child zone as well we should just recursive into ourself (upstream self). Thus relax the IsSubDomain check in file/lookup.go and just query (even if the query will hit a remote server). I've looped over all other plugins that do something similar (CNAME resolving) and they didn't do the IsSubDomain check; therefor I've removed it from *file* as well. Added test in file_upstream_test that shows this failed before but now results in a reply. Fixes coredns#1864 * self does not need to be exported * Fix test We don't know if we had a valid reply. Check this.
Jason-ZW
pushed a commit
to rancher/coredns
that referenced
this pull request
Apr 17, 2019
* plugin/file: fix local CNAME lookup Issue coredns#1864 explains it will, when we serve the child zone as well we should just recursive into ourself (upstream self). Thus relax the IsSubDomain check in file/lookup.go and just query (even if the query will hit a remote server). I've looped over all other plugins that do something similar (CNAME resolving) and they didn't do the IsSubDomain check; therefor I've removed it from *file* as well. Added test in file_upstream_test that shows this failed before but now results in a reply. Fixes coredns#1864 * self does not need to be exported * Fix test We don't know if we had a valid reply. Check this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #1864 explains it will, when we serve the child zone as well we
should just recursive into ourself (upstream self). Thus relax the
IsSubDomain check in file/lookup.go and just query (even if the query
will hit a remote server).
I've looped over all other plugins that do something similar (CNAME
resolving) and they didn't do the IsSubDomain check; therefor I've
removed it from file as well.
Added test in file_upstream_test that shows this failed before but now
results in a reply.
No documentation changes are needed.
Fixes #1864