This PR fixes etcd library update issue in 8492 where additional lint fix
is needed to take the latest etcd dependency.
This PR supersede 8492.
This PR closes 8492.
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
A CNAME whose target is its own owner name is chased by externalLookup
until the depth cap, appending the same record on every pass. The reply
was NOERROR with the CNAME repeated ten times.
Self-referential DNAME already returns SERVFAIL, as do wildcard CNAME
loops. Return SERVFAIL here too. The check runs on the CNAME chase path
only, so normal responses are unaffected.
Fixes#6421
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
Add a max_streams option to the *https* plugin to limit the number of
concurrent HTTP/2 streams per DoH connection. This lets operators cap
per-connection concurrency (guarding against resource exhaustion) or
raise it above the Go default for high-fan-in clients that multiplex
many requests over a single connection.
Semantics match the existing *https3* plugin's max_streams:
- omitted -> Go HTTP/2 server default is used
- 0 -> use the underlying HTTP/2 transport default
- positive -> advertise exactly that many concurrent streams
- negative -> rejected at config parse time
The limit is applied via the standard library http.Server.HTTP2
(HTTP2Config.MaxConcurrentStreams) so it is advertised in the server's
SETTINGS frame.
Signed-off-by: Mekias Yohannes <mmyohannes@gmail.com>
* plugin/autopath: Fixes a nil pointer dereference panic in autopath during search path walk
When a plugin later in the chain returns a ClientWrite rcode without
writing a response (for example acl's drop action, which returns
(dns.RcodeSuccess, nil) without calling WriteMsg), autopath dereferences
a nil nw.Msg at nw.Msg.Rcode and panics. The final fallback
w.WriteMsg(firstReply) can also receive a nil firstReply for the same
reason.
Skip search path elements that produced no message, and only write the
first reply when it is non-nil. This mirrors the nil guards recently
added in plugin/minimal (#8506), plugin/dns64 (#8511) and plugin/cache
(#8512).
Signed-off-by: zjncs <18910855655@163.com>
* plugin/autopath: silence unused-parameter lint and assert no client write
Address review feedback on #8517: rename the unused 'w' parameter in
TestAutoPathNilMsgFromNext to '_w' so the revive unused-parameter check
passes, and assert that the recorder receives no message so the intended
drop/no-client-write behavior is explicit.
Signed-off-by: zjncs <18910855655@163.com>
---------
Signed-off-by: zjncs <18910855655@163.com>
* plugin/k8s_external: Fixes a nil pointer dereference panic when upstream lookup returns no response
When a CNAME-hosted service is resolved, k8s_external performs internal
upstream lookups for the target name. Upstream.Lookup can return a nil
message with a nil error when the internal self-query's plugin chain
returns a ClientWrite rcode without writing a response (for example
acl's drop action), and the a/aaaa/srv handlers then dereference
resp.Answer on a nil resp and panic.
Guard all four lookups with err == nil && resp != nil, matching the nil
checks already used in plugin/backend_lookup.go and the guard recently
added to plugin/dns64 (#8511).
Signed-off-by: zjncs <18910855655@163.com>
* plugin/k8s_external: silence unused-parameter lint in nil upstream test
The CI lint flagged the test handler's unused 'w' parameter. Rename it
to '_' so golangci-lint (revive unused-parameter) passes. No behavior
change.
Signed-off-by: zjncs <18910855655@163.com>
---------
Signed-off-by: zjncs <18910855655@163.com>
ResponseReverter.WriteMsg calls res1.Copy() without checking res1 for
nil, so any plugin further down the chain that writes a nil response
(for example a handler returning (dns.RcodeSuccess, nil) after
w.WriteMsg(nil)) panics here.
Return an error instead, mirroring the nil guard recently added to
plugin/cache's ResponseWriter.WriteMsg (#8512).
Signed-off-by: zjncs <18910855655@163.com>
This PR fixes a nil pointer dereference panic in dns64 during response,
when the internal A-record upstream re-lookup returns a nil response.
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
bufio.Scanner stops at the first line longer than its 64KiB default
buffer and reports bufio.ErrTooLong from Err(). parse() never checked
Err(), so that line and every entry after it were dropped silently: the
hosts file simply looked shorter than it is, with nothing in the log.
Raise the scanner's limit to 1MiB (the scanner still grows its buffer
lazily, so nothing is preallocated up front) and log an error if the
scan does stop early, so the truncation is at least visible.
Signed-off-by: Paco Cartones <pacocartones@users.noreply.github.com>
Co-authored-by: Paco Cartones <pacocartones@users.noreply.github.com>
Store the normalized key name in the request context only after successful TSIG verification. This lets downstream plugins distinguish unsigned requests from authenticated requests and authorize by key without relying on the stripped TSIG RR or exposing secret material.
Signed-off-by: houyuwushang <liuluoqianqiu@outlook.com>
* plugin/cache: stop setting AA on answers served from cache
toMsg hardcoded m1.Authoritative = true, so a reply rebuilt from a cache entry claimed authority the answer that populated it never had.
The hardcoding was a workaround for legacy stub resolvers that dropped non-authoritative answers, but it only ever ran on the cache hit path: the same query still returned AA=0 on every miss and after every TTL expiry, so those clients were never actually protected.
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
* plugin/cache: pin the AA=1 side of the cache round trip
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
* plugin/cache: assert AA=0 on verified stale refresh and prove the cache hit
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
* plugin/cache: count backend calls in TestCachePreservesAA
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
---------
Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>
* perf(loadbalance): fast-path zero-allocation roundRobin for homogeneous record sets
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* plugin/loadbalance: copy before shuffling in the fast path
The fast path shuffled the caller's slice in place, which is not safe.
roundRobin must not modify its input: a backend may hand back a slice it
owns rather than one built for the response. plugin/file does exactly that
- Lookup returns elem.Type(qtype), which is the zone tree's own []dns.RR -
so an in-place shuffle reorders the zone itself, visible to every other
query and racing with the ones running concurrently.
Copy the records into a fresh slice and shuffle that instead. This is still
a single allocation rather than the four slices the partitioning path builds,
so most of the gain is kept:
name old time/op new time/op delta
RoundRobin 353 ns 244 ns -31%
name old alloc/op new alloc/op delta
RoundRobin 118 B 54 B -54%
name old allocs/op new allocs/op delta
RoundRobin 5 4 -20%
Also reorder the type check so a response led by a CNAME is rejected on the
first record instead of scanning the whole answer section first.
TestRoundRobinDoesNotMutateInput pins the contract; it fails against the
in-place version.
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
---------
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
remapStringRewriter matches a record name against orig and its sub domains. The
sub domain check was strings.HasSuffix(src, "."+r.orig), which built the
dot-prefixed string on every call and threw it away. Match the label boundary by
index instead: src is a sub domain of orig when orig sits at the end of src with
a "." immediately before it, which is exactly what the HasSuffix call tested.
Go concatenates short strings into a 32-byte stack buffer, so "."+orig only
reached the heap once orig passed 31 bytes. Below that the temporary was free
and this saves a few ns per record. Above it, 48 B was allocated per record.
Kubernetes service names are past the threshold -
my-service.my-namespace.svc.cluster.local. is 42 bytes - and those are the names
an auto rule rewrites to when a Corefile maps an external name onto an
in-cluster one. orig is the name the question was rewritten to, so whether a
deployment sees the allocation is a property of its Corefile, not its queries.
Caching "."+orig on the rewriter instead does not work: responseRuleFor
constructs a new rewriter for every request an auto name rule rewrites, so the
concatenation would run once per request rather than once per rule, and escapes
to the heap from there. That buys per-record work with a per-request allocation
and regresses every response short enough not to amortize it.
Per record, one rewriteString call on an existing rewriter:
name master this PR
RemapStringRewriter/short/match 186.6n 16 B/1 120.4n 16 B/1 -35%
RemapStringRewriter/short/nomatch 61.5n 0 B/0 16.5n 0 B/0 -73%
RemapStringRewriter/long/match 381.9n 72 B/2 165.3n 24 B/1 -57%
RemapStringRewriter/long/nomatch 219.1n 48 B/1 18.7n 0 B/0 -91%
Per request - rewrite the question, build the response rules, apply them to the
answer:
name master this PR
AutoNameRuleResponse/exact 935n 96 B/4 945n 96 B/4 ~
AutoNameRuleResponse/subdomain 1.248µ 112 B/5 1.242µ 112 B/5 ~
AutoNameRuleResponse/nomatch 1.016µ 96 B/4 945n 96 B/4 -7%
AutoNameRuleResponse/subdomain-8 3.376µ 224 B/12 2.891µ 224 B/12 -14%
AutoNameRuleResponse/k8s/subdomain 1.611µ 176 B/6 1.380µ 128 B/5 -14%
AutoNameRuleResponse/k8s/subdomain-8 5.144µ 672 B/20 3.305µ 288 B/12 -36%
benchstat over 8 runs, i7-1065G7. With short names this is flat at the request
level: one rewriteString call is small next to the four allocations that
building the rules costs. The saving is per record and per byte of name, so it
shows up where responses carry several records and the rewritten-to name is
long.
This applies to exact, prefix, substring and regex name rules with answer auto.
suffix rules build a suffixStringRewriter instead and are not affected.
TestRemapStringRewriter pins the label boundary semantics the index arithmetic
now carries, notably that notexample.com. is not a sub domain of example.com.
It passes against the previous implementation too.
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* plugin/rewrite: apply rcode rewrites to record-less responses
An rcode rewrite rewrites the message-level RCODE, but the reverter only ran
response rules from inside the per-record loops in WriteMsg. When a response
carries no answer, authority or additional records - for example a bare
SERVFAIL that a downstream plugin returns to a non-EDNS client - none of the
loops iterate, so the rcode rewrite was silently skipped and the client
received the original RCODE.
Apply message-level response rules once when the response has no records, using
a small marker interface that mirrors the existing requestExtraRevertRule
pattern. This fixes the plugin's documented SERVFAIL-to-NOERROR use case for
responses without records.
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
* plugin/rewrite: apply fallback rcode rewrites for continue
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
---------
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
* plugin/cache: add prefer_positive stale policy
Add an opt-in serve_stale_policy that prefers an eligible success-cache
answer over denial-cache entries while serve_stale is enabled. Preserve the
existing ncache-first behavior when the policy is absent.
Also classify SOA-backed CNAME NODATA responses in the cache so incomplete
answers cannot be selected as positive stale responses.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6
Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com>
* plugin/cache: retain last-known-good positive answers
Keep an answering success-cache item reachable when a later NOERROR or
referral response overwrites the visible cache key without answering the
question. This lets prefer_positive survive empty responses, referrals, and
additional-only data while leaving policy-off lookup behavior unchanged.
Return the exact accepted verify refresh item instead of re-reading an
ambiguous cache key, avoiding expired TTL wraparound for uncacheable replies.
Add regression coverage for non-answer refreshes, NODATA, SERVFAIL, NOTIMP,
stale-window expiry, and bounded verify reply shaping.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6
Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com>
* plugin/cache: validate preferred stale answers
Reject truncated, DNSSEC-expired, mismatched-class, unrelated ANY, and ambiguous CNAME refreshes before replacing a stale last-known-good answer. Precompute answer eligibility when cache items are created so prefer_positive hits avoid repeated CNAME walks.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6
Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com>
---------
Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com>
Co-authored-by: Nitin Nizhawan <nnizhawan@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6
* plugin/kubernetes: copy Labels in Pod.DeepCopyObject
Pod.DeepCopyObject built the copy field by field and left Labels out, so
every copy came back unlabelled. Every other object in this package copies
all of its fields; Pod was the only one missing any.
Labels feeds the kubernetes/client-label/<key> metadata, so anything that
reached a pod through a deep copy would see no labels at all rather than an
error. Nothing does today - DefaultProcessor stores the converted object
straight into the indexer without copying it, which is why this has not
surfaced - so this is a latent bug rather than a live one.
Clone the map instead of assigning it, so the copy does not alias the
original. maps.Clone returns nil for a nil map, so an unlabelled pod stays
unlabelled and a round trip does not turn a nil map into an empty one.
The test covers every runtime.Object in the package rather than just Pod,
and refuses to pass if a fixture leaves a field at its zero value. Adding a
field to any of these types therefore fails the test until the fixture sets
it, which is what makes the round-trip assertion cover the new field too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* plugin/kubernetes/object: deep copy the Service and ServiceImport ports
The deepcopy tests claimed a package-wide guarantee that a copy shares nothing
with its original, but only checked it for Pod and a hand-written Endpoints
value. Both api.ServicePort and mcs.ServicePort hold an AppProtocol *string, so
the elementwise copy(s1.Ports, s.Ports) in Service.DeepCopyObject and
ServiceImport.DeepCopyObject left the copy pointing at the original's string.
Mutating it through either side was visible from the other, and the tests still
passed.
Copy the ports with the generated DeepCopyInto so the copy owns everything it
can reach, and make the guarantee real: TestDeepCopyObjectIsDeep now runs over
every case in deepCopyCases, mutates every value reachable through a pointer,
slice, or map, and requires the copy to still equal a pristine fixture. A type
that gains a reference-bearing field is covered as soon as assertAllFieldsSet
forces the fixture to populate it, rather than needing a new hand-written case.
Failure messages render as JSON, because %+v prints an aliased pointer field as
an address and hides the value that actually differs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
---------
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Use net.JoinHostPort rather than string concatenation to support
both ipv4, ipv6, and hostname service endpoints for the trace
plugin. Previously, ipv6 bind addresses in the coredns configuration
would fail to be parsed, as the ipv6 address was not surrounded in
brackets.
Signed-off-by: Michael Wolf <mwolf@cloudflare.com>
Closes#8409
Co-authored-by: Michael Wolf <mwolf@cloudflare.com>
* test: add benchmark cases for Request IP/Port and parseRequest
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* test(kubernetes): add BenchmarkServices and BenchmarkServicesHeadless
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* perf(hosts): pre-convert Origins to plugin.Zones in hosts setup
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* style: fix gofmt trailing line formatting
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
---------
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
externalLookup, which resolves the chase for ordinary CNAME, wildcard
CNAME, and DNAME answers, returned a nil additional section. So a chased
SRV/MX/SVCB/HTTPS answer with an in-bailiwick target was missing the
target's A/AAAA glue, unlike the direct path.
Run additionalProcessing at externalLookup's return points so all three
callers add the glue, and add ordinary-CNAME, wildcard-CNAME, and DNAME
regression cases.
Fixes#6628
Signed-off-by: Saleh <root@lr0.org>
Default to two connect attempts per configured upstream so fast failures cannot spin until the request deadline. Track whether max_connect_attempts was explicitly configured so zero still opts into the legacy unbounded behavior.
Fixes#7723
Signed-off-by: houyuwushang <liuluoqianqiu@outlook.com>
* plugin/kubernetes: skip zone serial bump on DNS neutral pod updates
In pods verified mode every pod update event bumped the zone modified
timestamp, even when the pod IP did not change. Pod records only depend
on the pod IP, so routine status churn (conditions, container statuses,
labels) caused spurious SOA serial changes and needless zone transfer
activity, even though pod records are not part of transfers at all.
Only bump the modified timestamp when the pod IP changes, mirroring how
service and endpoint updates are already filtered. Also update the pods
verified documentation to describe the actual overhead: modest memory
for a stripped down pod object, plus watch load on the API server.
Ref #8043
Signed-off-by: Karan V <karanvknarayanan@gmail.com>
* plugin/kubernetes: keep pods verified cost description neutral
Avoid characterizing the memory overhead as modest until benchmark
data quantifies it. State only what the code does: the watch requires
additional memory in CoreDNS and adds load to the API server.
Signed-off-by: Karan V <karanvknarayanan@gmail.com>
---------
Signed-off-by: Karan V <karanvknarayanan@gmail.com>
* test: add benchmark cases for Request IP/Port and parseRequest
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* test(kubernetes): add BenchmarkServices and BenchmarkServicesHeadless
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
* perf(forward): fast-path string comparison in isAllowedDomain
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
---------
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
RFC 4592 permits a wildcard source of synthesis to exist as an empty non-terminal. Track wildcard names proven by descendant records and return NODATA when such a source is selected.
Fixes#4256
Signed-off-by: houyuwushang <liuluoqianqiu@outlook.com>
Adds two regression tests for #8234 that the existing suite does not cover:
- an OPT-less upstream reply that carries a record, so the per-record
response rules run while the request OPT is reused by ScrubWriter;
- a request that already carries the option, where "set ... revert" must
put the client's original value back rather than just drop the option.
Both fail against the tree before #8235 and pass on current master.
Signed-off-by: maximilize <3752128+maximilize@users.noreply.github.com>
This PR fixes multi-primary AXFR zone contamination. It
use a fresh candidate zone for each primary so records from failed transfers cannot leak into later.
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
