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

plugin/forward: respond with REFUSED when max_concurrent is exceeded #4326

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

chrisohaver
Copy link
Member

Signed-off-by: Chris O'Haver cohaver@infoblox.com

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

Respond with REFUSED when max_concurrent is exceeded to avoid caching the repsonse. REFUSED responses are typified as OTHERERROR, which are not cached by the cache plugin. Currently, the response code used is SERVFAIL, which is cached. If max_concurrent exceeded responses are cached, those answer are potentially cached beyond the time the load has dropped, barring those clients from getting a valid answers for some time after the limit has dropped below the threshold. So, yes, REFUSED works better that SERVFAIL.

2. Which issues (if any) are related?

This issue was raised tangentially in #4324

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

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

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@codecov-io
Copy link

Codecov Report

Merging #4326 (d720fd6) into master (6bbb48d) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4326   +/-   ##
=======================================
  Coverage   54.95%   54.95%           
=======================================
  Files         223      223           
  Lines        9917     9917           
=======================================
  Hits         5450     5450           
  Misses       4022     4022           
  Partials      445      445           
Impacted Files Coverage Δ
plugin/forward/forward.go 42.16% <0.00%> (ø)

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 6bbb48d...d720fd6. Read the comment docs.

@zouyee
Copy link
Member

zouyee commented Dec 14, 2020

/lgtm

Copy link

@corbot corbot bot left a comment

Choose a reason for hiding this comment

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

Approved by zouyee

@miekg
Copy link
Member

miekg commented Dec 15, 2020

REFUSED is actually also the better rcode I think

@miekg miekg merged commit 9cb5348 into coredns:master Dec 15, 2020
@chrisohaver chrisohaver deleted the max_exceeded_no_cache branch January 9, 2021 14:45
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.

4 participants