plugin/rewrite: add rcode as a rewrite option

[pschou]

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

Adds the ability to mask server failure replies from a proxied DNS server. This change makes CoreDNS "just work" in broken/firewalled environments.

For example, an IPv6 incompatible DNS appliance-- enables a stand-in proxy fix where IPv6 is not fully supported yet. This option allows you to "ignore errors" and reply with an empty record. When docker or Kubernetes sits behind a firewall appliance that blocks AAAA records from being resolved, the docker API sits and waits seconds to do multiple tries in hopes of getting a SUCCESS reply. This flag masks this issue go away, and thus, the infrastructure runs faster.

In gist, it takes any SERVER-FAIL replies and switches them to SUCCESS to give the application a "no records found" reply.

2. Which issues (if any) are related?

None (that I know of)

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

The plugin README has been updated.

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

No

[chrisohaver]

Thanks for contributing!

When docker or Kubernetes sits behind a firewall appliance that blocks AAAA records from being resolved, the docker API sits and waits seconds to do multiple tries

There may be other ways to solve the issue, with existing plugins such as acl or template.
e.g. you can make coredns respond to all AAAA requests with empty response (aka nodata) with acl plugin using filter type AAAA.

[pschou]

Thanks for contributing!

Thank you for the consideration and reply!

There may be other ways to solve the issue, with existing plugins such as acl or template. e.g. you can make coredns respond to all AAAA requests with empty response (aka nodata) with acl plugin using filter type AAAA.

I agree that this is a potential avenue of resolution if we want all AAAA records to be taken away, but I still want IPv6 to be functional and enabled as much as possible-- so turning off all IPv6 goes against the plan to make IPv6 work where ever possible.

The SERVER-FAIL is something that the DNS appliance keeps throwing back. It seems the DNS administrators think that SERVER-FAIL reply is the right course of action, but this keeps tripping up my Kubernetes deployments because they just stall whenever I try to deploy on a system.

[palmerrb2000]

This fixed a longstanding problem for me. A sincere thank you to all who contribute!

[chrisohaver]

Conceptually, this class of feature is a better fit for the rewrite plugin. It's a plugin that rewrites/overrides aspects of request and response. E.g. It could be implemented by adding a new FIELD type option to the rewrite plugin: code FROM TO.

Also if implemented in the rewrite plugin, the response code override could be applied to a response from any plugin, not just forward plugin. So it would be more utilitarian, applicable to more situations.

[pschou]

Conceptually, this class of feature is a better fit for the rewrite plugin. It's a plugin that rewrites/overrides aspects of request and response. E.g. It could be implemented by adding a new FIELD type option to the rewrite plugin: code FROM TO.

Also if implemented in the rewrite plugin, the response code override could be applied to a response from any plugin, not just forward plugin. So it would be more utilitarian, applicable to more situations.

This is a very good point and I concur that a rewrite type would be a better fit for more diverse use cases. I have amended the PR to realize this suggestion.

[pschou]

@chrisohaver Do you think the documentation should use OLD/NEW or FROM/TO? I am considering switching it, but I would like to see what you think.

[chrisohaver]

@chrisohaver Do you think the documentation should use OLD/NEW or FROM/TO? I am considering switching it, but I would like to see what you think.

Best to be consistent with existing wording. I think it uses from/to.

[chrisohaver]

Thanks!

[wondywang]

This PR also addresses a particular scenario I encountered. A big thank you to all contributors!

[chrisohaver]

Fix import ordering and LGTM. Deferring final approval/merge to the rewrite plugin codeowners.

[github-actions]

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days

[greenpau]

@pschou , please address the go coverage errors. Otherwise, LGTM.

[greenpau]

LGTM. Please address presubmit check.

[greenpau]

@pschou , there are some other test failing too:

• DCO Action required after 1s — DCO

• ci/circleci: kubernetes-tests — Your tests failed on CircleCI

[pschou]

@greenpau, this build process is fragile.

The pull approval was delayed all because of a single new line character, "\n", the test to verify that the imports are in the right format fail-- ok, I understand that consistency is important. So, I add a single new line character to space the imports apart, and now other checks that previously passed are failing due to a curl command and because of an external API change at go.dev. This issue is not related to this pull request, out of scope.

When pulling down this endpoint, one will find two lines are returned:

$ curl 'https://go.dev/VERSION?m=text'
go1.21.0
time 2023-08-04T20:14:06Z

The error in the kubernetes check looks like this:

LATEST=$(curl -s https://go.dev/VERSION?m=text)
curl https://dl.google.com/go/${LATEST}.linux-amd64.tar.gz | sudo tar xz -C $GOROOT --strip-components=1
curl: (6) Could not resolve host: time
curl: (3) URL using bad/illegal format or missing URL

Maybe a proposed fix would look like this:

LATEST=($(curl -s https://go.dev/VERSION?m=text))
curl "https://dl.google.com/go/${LATEST[0]}.linux-amd64.tar.gz" | sudo tar xz -C $GOROOT --strip-components=1

Note that every other check passed before this external API response change:
https://app.circleci.com/pipelines/github/coredns/coredns/6794/workflows/4c459d4c-0fb8-4049-a554-743276e69d85/jobs/6867

Now it is failing due to the external API change:
https://app.circleci.com/pipelines/github/coredns/coredns/6951/workflows/336e30ed-1b21-4de5-bcc5-8b83de43df91/jobs/7025

[chrisohaver]

Please rebase (the go version check was fixed a few weeks ago)

[pschou]

Ok, done 👍

[codecov]

Codecov Report

Merging #6204 (1e86cd3) into master (93c57b6) will increase coverage by 2.66%.
Report is 1026 commits behind head on master.
The diff coverage is 55.36%.

@@            Coverage Diff             @@
##           master    #6204      +/-   ##
==========================================
+ Coverage   55.70%   58.36%   +2.66%     
==========================================
  Files         224      252      +28     
  Lines       10016    16509    +6493     
==========================================
+ Hits         5579     9636    +4057     
- Misses       3978     6282    +2304     
- Partials      459      591     +132     

Files Changed	Coverage Δ
core/dnsserver/config.go	0.00% <ø> (ø)
core/dnsserver/register.go	13.27% <0.00%> (-4.71%)	⬇️
plugin/auto/auto.go	0.00% <0.00%> (ø)
plugin/auto/xfr.go	0.00% <0.00%> (ø)
plugin/auto/zone.go	78.94% <ø> (+0.68%)	⬆️
plugin/autopath/autopath.go	73.61% <ø> (-1.39%)	⬇️
plugin/azure/azure.go	10.61% <0.00%> (-0.57%)	⬇️
plugin/azure/setup.go	56.36% <0.00%> (-4.14%)	⬇️
plugin/backend_lookup.go	0.00% <0.00%> (ø)
plugin/bind/bind.go	50.00% <0.00%> (-50.00%)	⬇️
... and 77 more

... and 170 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[greenpau]

LGTM