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 alternate option to forward plugin #6681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dihmandrake
Copy link

Allows the forward plugin to execute the next plugin based on the return code. Similar to the externally mainted alternate plugin https://github.com/coredns/alternate

Based on the idea of chrisohaver@ in #6549 (comment)

I am having issues adding a proper test for functionality. Primarily, I do not know the code base enough and having multiple dnstest.NewServer with ResponseWriter does not work. From my testing these are "Singletons'' and only the last defined response writer is used for all servers

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

This PR introduces an alternate option within the CoreDNS forward plugin, eliminating the need for separate compiled plugins. This option proves particularly beneficial for Kubernetes cluster operators managing internal DNS servers. These servers resolve specific domains locally, ensuring requests remain within the designated network or cluster. This approach enhances fault tolerance and serves other purposes. Additionally, the internal DNS servers may leverage tools like cert-manager, which necessitates external TXT record generation and resolution for the DNS-01 challenge mechanism.

The new alternate option within the forward plugin allows resolving entries exclusively available on public DNS servers. This enables mechanisms like cert-manager to function correctly without requiring operators to maintain self-compiled and hosted versions of CoreDNS. This approach simplifies deployment and maintenance while ensuring proper functionality.

2. Which issues (if any) are related?

PR: #6549

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

Added as part of this PR

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

No

Copy link
Member

@chrisohaver chrisohaver left a comment

Choose a reason for hiding this comment

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

Thanks! I think next might be a better name for the option from an intuitive sense.
There is the history of the external plugin being called alternate ... and before that I think it was called fallback, but I dont think its important from a clarity pov to maintain the name going forward.

What do you think?

plugin/forward/forward.go Outdated Show resolved Hide resolved
Allows the forward plugin to execute the next plugin based on the return code. Similar to the externally mainted alternate plugin https://github.com/coredns/alternate

Based on the idea of chrisohaver@ in coredns#6549 (comment)
Also incoperated the request to rename `alternate` to `next` as an option

I am having issues adding a proper test for functionality. Primarily, I do not know the code base enough and having multiple `dnstest.NewServer` with ResponseWriter does not work. From my testing these are "Singletons'' and only the last defined response writer is used for all servers

Signed-off-by: Jasper Bernhardt <[email protected]>
@dihmandrake
Copy link
Author

dihmandrake commented Jun 5, 2024

Thanks! I think next might be a better name for the option from an intuitive sense. There is the history of the external plugin being called alternate ... and before that I think it was called fallback, but I dont think its important from a clarity pov to maintain the name going forward.

What do you think?

Hi,

thank you for the review and the suggestion. I do not have a strong feeling about naming. alternate might be more consistent while I understand that next might appear clearer. Since I do not have preference, I have changed the config value to next and noted also in the Readme that this will only work if the next handler is a forward plugin.
The only thing where I left alternate is in the variable name for alternateRcodes as I think it helps possible in the future with clarity.

Would you mind taking another look @chrisohaver

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 59.20%. Comparing base (93c57b6) to head (386f068).
Report is 1211 commits behind head on master.

Current head 386f068 differs from pull request most recent head 7163bc6

Please upload reports for the commit 7163bc6 to get more accurate results.

Files Patch % Lines
plugin/forward/forward.go 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6681      +/-   ##
==========================================
+ Coverage   55.70%   59.20%   +3.50%     
==========================================
  Files         224      252      +28     
  Lines       10016    13485    +3469     
==========================================
+ Hits         5579     7984    +2405     
- Misses       3978     4908     +930     
- Partials      459      593     +134     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

2 participants