plugin/forward: Enable multiple forward declarations #5127

chrisohaver · 2022-01-26T20:16:19Z

Signed-off-by: Chris O'Haver [email protected]

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

This PR enables multiple forward plugin definitions in a server block.
When processing a query, each forward instance is evaluated in their order in the Corefile.

This implementation chains the multiple forward plugin instances to each other via the existing forward.Next. This keeps the amount of code change to a minimum.

An alternate solution is to create a slice of Forward structs, and loop through each one, essentially acting as a single plugin that checks multiple Forward instances.

2. Which issues (if any) are related?

This limitation comes up now and then in Q&A.

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

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

no

codecov-commenter · 2022-04-14T16:53:43Z

Codecov Report

Merging #5127 (579980c) into master (93c57b6) will increase coverage by 1.29%.
The diff coverage is 59.46%.

@@            Coverage Diff             @@
##           master    #5127      +/-   ##
==========================================
+ Coverage   55.70%   56.99%   +1.29%     
==========================================
  Files         224      236      +12     
  Lines       10016    14806    +4790     
==========================================
+ Hits         5579     8439    +2860     
- Misses       3978     5855    +1877     
- Partials      459      512      +53

Impacted Files	Coverage Δ
core/dnsserver/address.go	`100.00% <ø> (ø)`
core/dnsserver/config.go	`0.00% <ø> (ø)`
core/dnsserver/onstartup.go	`0.00% <0.00%> (ø)`
core/dnsserver/server_grpc.go	`8.16% <0.00%> (+2.10%)`	⬆️
plugin/auto/auto.go	`0.00% <0.00%> (ø)`
plugin/auto/zone.go	`78.94% <ø> (+0.68%)`	⬆️
plugin/azure/azure.go	`10.61% <0.00%> (-0.57%)`	⬇️
plugin/backend_lookup.go	`0.00% <0.00%> (ø)`
plugin/bind/bind.go	`50.00% <0.00%> (-50.00%)`	⬇️
plugin/bufsize/bufsize.go	`100.00% <ø> (ø)`
... and 311 more

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 d7f8202...579980c. Read the comment docs.

chrisohaver · 2022-06-23T20:30:15Z

FWIW - A case came up again in a recent support issue, where allowing multiple forward declarations in the root server block would enable a better solution.

Tantalor93

@chrisohaver could you extend README of forward plugin and add mention that this plugin can be specified multiple times in a single server block (thats not common pattern in CoreDNS => worth mentioning in README), what is the behaviour in such case and maybe some usecases? Also please add an example into the README of Corefile with multiple forward declarations?

chrisohaver · 2022-06-23T22:37:19Z

Thanks for reviewing!

thats not common pattern in CoreDNS

Actually, it is a common pattern in CoreDNS, but worth illustrating with an example. I think plugins that enforce only one per server block are the exception, hence those plugins having a note in their README, "This plugin can only be used once per Server Block."

Also please add an example into the README of Corefile with multiple forward declarations?

Agreed. I will add an example that illustrate a use case for multiple instances.

chrisohaver · 2022-06-23T23:07:38Z

@Tantalor93, It's been a looong time since I looked at this (January).

I want to build it and play with it a bit to make sure it works as intended before merging.

plugin/forward/README.md

chrisohaver · 2022-06-27T14:25:45Z

It's been a looong time since I looked at this (January). I want to build it and play with it a bit to make sure it works as intended ...

OK, I have tested using local forwarding and the forwarding aspect works as expected.

I've also added a note about plugin order in the Corefile, since (as is the case in some other multi-instance plugins) relative order of the instances in the server block matters.

Tantalor93

LGTM, thanks!

Tantalor93

I just noticed that parseForward is not used anymore in production code (it is replaced by parseMultiForward), could you remove it?

chrisohaver · 2022-07-10T16:23:20Z

I just noticed that parseForward is not used anymore in production code (it is replaced by parseMultiForward), could you remove it?

IIRC it is used by the alternate external plugin. So for now, I was going to leave it in.

chrisohaver · 2022-07-10T16:32:29Z

I just noticed that parseForward is not used anymore in production code (it is replaced by parseMultiForward), could you remove it?

IIRC it is used by the alternate external plugin. So for now, I was going to leave it in.

Hmm. Maybe not that plugin, but I thought an external plugin used it? I should have commented why I left it in. This PR had been sitting waiting for a review for so long I forgot the details.

Thanks for dedicating the time to reviewing PRs!

Tantalor93 · 2022-07-10T16:41:04Z

I just noticed that parseForward is not used anymore in production code (it is replaced by parseMultiForward), could you remove it?

IIRC it is used by the alternate external plugin. So for now, I was going to leave it in.

Hmm. Maybe not that plugin, but I thought an external plugin used it? I should have commented why I left it in. This PR had been sitting waiting for a review for so long I forgot the details.

I think that parseForward cannot be directly used by any other plugin, as it is not exported function of forward package and therefore not visible from other GO packages and any other plugin

Thanks for dedicating the time to reviewing PRs!

no problem, this feature looks very useful 👍

chrisohaver · 2022-07-10T16:42:51Z

but I thought an external plugin used it?

it couldn’t have been used by another package because it’s a private function. Oh. I think I remember why I thought this. I think it was once a public function, and it was used by an external plugin, but later it was made private in a refactor, breaking the plugin. And the other plugin was refactored.

TLDR, I can remove it.

chrisohaver · 2022-07-19T18:49:57Z

@Tantalor93, How do I interpret the linter failure? It says there exists trailing whitespace but doesn't say where.

chrisohaver · 2022-07-19T19:06:21Z

rebasing, to see if that helps the cryptic linter fail... no it didnt.

chrisohaver · 2022-07-19T19:15:15Z

rebasing, to see if that helps the cryptic linter fail... no it didnt.

I think I figured it out - golinter makes comments in-line in the commits.
I guess since the offending files are outside of this PR, they didn't boil up into the main PR page. Or maybe they are always buried like that. I don't know.

Signed-off-by: Chris O'Haver <[email protected]>

Tantalor93 · 2022-07-19T20:06:34Z

@chrisohaver yea, golangci-lint annotates code directly (see golangci/golangci-lint-action#5)

chrisohaver · 2022-07-19T20:13:01Z

@chrisohaver yea, golangci-lint annotates code directly (see golangci/golangci-lint-action#5)

Yeah - weird that those comments don't show up in the main discussion. That means for a large commit, you'd have to go hunting for them.

* enable multiple declarations of forward plugin Signed-off-by: Chris O'Haver <[email protected]>

chrisohaver marked this pull request as ready for review January 26, 2022 21:04

chrisohaver requested review from johnbelamaric and miekg as code owners January 26, 2022 21:04

chrisohaver marked this pull request as draft May 4, 2022 15:57

chrisohaver marked this pull request as ready for review May 4, 2022 16:00

Tantalor93 reviewed Jun 23, 2022

View reviewed changes

chrisohaver force-pushed the multi-forward2 branch from 0737946 to 26a9df2 Compare June 23, 2022 23:03

Tantalor93 reviewed Jun 24, 2022

View reviewed changes

plugin/forward/README.md Outdated Show resolved Hide resolved

chrisohaver requested a review from Tantalor93 July 8, 2022 16:05

Tantalor93 approved these changes Jul 10, 2022

View reviewed changes

Tantalor93 reviewed Jul 10, 2022

View reviewed changes

zouyee approved these changes Jul 15, 2022

View reviewed changes

chrisohaver force-pushed the multi-forward2 branch from 2f3ca52 to acabf6f Compare July 19, 2022 19:04

chrisohaver added 5 commits July 19, 2022 15:27

handle multiple forwards

7439c54

Signed-off-by: Chris O'Haver <[email protected]>

add test

ad9c51c

Signed-off-by: Chris O'Haver <[email protected]>

fix import order

61e0d7e

Signed-off-by: Chris O'Haver <[email protected]>

fix import order

a45a190

Signed-off-by: Chris O'Haver <[email protected]>

update docs

cc12572

Signed-off-by: Chris O'Haver <[email protected]>

chrisohaver added 4 commits July 19, 2022 15:27

add multi instance example

2b9a891

Signed-off-by: Chris O'Haver <[email protected]>

fix readme typos

4bdf2e1

Signed-off-by: Chris O'Haver <[email protected]>

add note about plugin order

b0d5ddb

Signed-off-by: Chris O'Haver <[email protected]>

remove previous parseForward; rename parseMultiForward to parseForward

37c45c0

Signed-off-by: Chris O'Haver <[email protected]>

chrisohaver force-pushed the multi-forward2 branch from acabf6f to 37c45c0 Compare July 19, 2022 19:27

Tantalor93 approved these changes Jul 19, 2022

View reviewed changes

chrisohaver merged commit 513f27b into coredns:master Jul 20, 2022

gonzalop pushed a commit to gonzalop/coredns that referenced this pull request Jan 14, 2023

plugin/forward: Enable multiple forward declarations (coredns#5127)

adfbfa1

* enable multiple declarations of forward plugin Signed-off-by: Chris O'Haver <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin/forward: Enable multiple forward declarations #5127

plugin/forward: Enable multiple forward declarations #5127

chrisohaver commented Jan 26, 2022 •

edited

Loading

codecov-commenter commented Apr 14, 2022 •

edited

Loading

chrisohaver commented Jun 23, 2022

Tantalor93 left a comment

chrisohaver commented Jun 23, 2022

chrisohaver commented Jun 23, 2022

chrisohaver commented Jun 27, 2022 •

edited

Loading

Tantalor93 left a comment

Tantalor93 left a comment

chrisohaver commented Jul 10, 2022

chrisohaver commented Jul 10, 2022

Tantalor93 commented Jul 10, 2022 •

edited

Loading

chrisohaver commented Jul 10, 2022

chrisohaver commented Jul 19, 2022

chrisohaver commented Jul 19, 2022

chrisohaver commented Jul 19, 2022 •

edited

Loading

Tantalor93 commented Jul 19, 2022

chrisohaver commented Jul 19, 2022

plugin/forward: Enable multiple forward declarations #5127

plugin/forward: Enable multiple forward declarations #5127

Conversation

chrisohaver commented Jan 26, 2022 • edited Loading

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

2. Which issues (if any) are related?

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

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

codecov-commenter commented Apr 14, 2022 • edited Loading

Codecov Report

chrisohaver commented Jun 23, 2022

Tantalor93 left a comment

Choose a reason for hiding this comment

chrisohaver commented Jun 23, 2022

chrisohaver commented Jun 23, 2022

chrisohaver commented Jun 27, 2022 • edited Loading

Tantalor93 left a comment

Choose a reason for hiding this comment

Tantalor93 left a comment

Choose a reason for hiding this comment

chrisohaver commented Jul 10, 2022

chrisohaver commented Jul 10, 2022

Tantalor93 commented Jul 10, 2022 • edited Loading

chrisohaver commented Jul 10, 2022

chrisohaver commented Jul 19, 2022

chrisohaver commented Jul 19, 2022

chrisohaver commented Jul 19, 2022 • edited Loading

Tantalor93 commented Jul 19, 2022

chrisohaver commented Jul 19, 2022

chrisohaver commented Jan 26, 2022 •

edited

Loading

codecov-commenter commented Apr 14, 2022 •

edited

Loading

chrisohaver commented Jun 27, 2022 •

edited

Loading

Tantalor93 commented Jul 10, 2022 •

edited

Loading

chrisohaver commented Jul 19, 2022 •

edited

Loading