feat: add LocalDNS forward health knobs - #9174
Open
Saewon Kwak (saewoni) wants to merge 1 commit into
Open
Conversation
Saewon Kwak (saewoni)
requested review from
Chou Hu (AbelHu),
Devinwong,
Sri Harsha (SriHarsha001),
Nishchay (awesomenix),
Calvin S. (calvin197),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
Ganeshkumar Ashokavardhanan (ganeshkumarashok),
Karen Chen (karenychen),
lilypan26,
Mark Ibrahim (mxj220),
Peter Damianov (pdamianov-dev),
Patrick W. Healy (phealy),
r2k1,
Runzhen (runzhen),
sulixu,
Tim Wright (timmy-wright),
Thibault Cohen (titilambert),
Xu Xue (xuexu6666) and
Zachary (zachary-bailey)
as code owners
August 11, 2026 17:23
Contributor
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Contributor
Windows Unit Test Results 3 files 12 suites 45s ⏱️ Results for commit 02661e6. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends LocalDNS configuration to support CoreDNS forward plugin upstream health behavior by adding new knobs to the config model/proto and wiring them into the generated LocalDNS Corefile template.
Changes:
- Add
failfast_all_unhealthy_upstreamsandhealth_checkconfiguration fields to LocalDNS overrides (datamodel + aks-node-controller proto). - Render the new
forwardplugin directives in the LocalDNS Corefile template for both VnetDNS and KubeDNS override blocks. - Add a parser unit test covering
health_check+failfast_all_unhealthy_upstreamsoutput and regenerate protobuf Go bindings.
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/agent/datamodel/types.go | Adds datamodel structs/fields for LocalDNS forward health knobs. |
| aks-node-controller/proto/aksnodeconfig/v1/localdns_config.proto | Extends the LocalDNS overrides proto schema with failfast + health check message. |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go | Regenerated bindings for the updated LocalDNS proto. |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go | Regenerated bindings (import ordering change). |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/config.pb.go | Regenerated bindings (header formatting). |
| aks-node-controller/parser/templates/localdns.toml.gtpl | Wires new forward plugin directives into Corefile rendering. |
| aks-node-controller/parser/helper_test.go | Adds unit test validating Corefile output for health_check + failfast. |
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (1)
aks-node-controller/parser/templates/localdns.toml.gtpl:121
- Same issue as the earlier forward block: checking
.Interval/.Domainpointer fields can render empty strings, producing invalidhealth_checkarguments. Check getter return values (strings) so empty values are treated as unset.
{{- if $override.HealthCheck}}
health_check {{if $override.HealthCheck.Interval}}{{$override.HealthCheck.GetInterval}}{{else}}0.5s{{end}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+48
to
+50
| {{- if $override.HealthCheck}} | ||
| health_check {{if $override.HealthCheck.Interval}}{{$override.HealthCheck.GetInterval}}{{else}}0.5s{{end}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}} | ||
| {{- end}} |
Contributor
Author
There was a problem hiding this comment.
Addressed in the latest update. Both VnetDNS and KubeDNS template blocks now guard with GetDuration/GetDomain, so empty duration omits health_check and empty domain omits the domain clause.
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 20:33
2e8f898 to
d18bb92
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (2)
aks-node-controller/parser/templates/localdns.toml.gtpl:50
- The health_check rendering logic checks pointer presence (Interval/Domain) rather than non-empty values. If the config explicitly sets interval/domain to an empty string (non-nil pointer), this will emit
health_check(blank interval) and/ordomain(blank domain), which can produce an invalid CoreDNS Corefile and break DNS.
{{- if $override.HealthCheck}}
health_check {{if $override.HealthCheck.Interval}}{{$override.HealthCheck.GetInterval}}{{else}}0.5s{{end}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
aks-node-controller/parser/templates/localdns.toml.gtpl:121
- Same issue as above for the cluster listener block: checking Interval/Domain pointer presence can emit empty tokens when the pointer is set to "". Prefer checking the resolved string values (GetInterval/GetDomain) so empty strings fall back to defaults/omission.
{{- if $override.HealthCheck}}
health_check {{if $override.HealthCheck.Interval}}{{$override.HealthCheck.GetInterval}}{{else}}0.5s{{end}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 20:42
d18bb92 to
88ccb0b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (3)
aks-node-controller/parser/templates/localdns.toml.gtpl:121
- This condition only checks that Duration is non-nil; if Duration is set to an empty string, the template will emit an invalid
health_checkdirective (missing duration) and CoreDNS will fail to parse the forward block. Consider also checking that the duration value is non-empty, and similarly avoid emitting an emptydomainvalue.
{{- if and $override.HealthCheck $override.HealthCheck.Duration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
pkg/agent/datamodel/types.go:2624
- The new LocalDNSOverrides knobs (FailfastAllUnhealthyUpstreams / HealthCheck) are added to the datamodel but aren’t consumed anywhere in the AgentBaker localdns Corefile generation path (GenerateLocalDNSCoreFile renders localDNSCoreFileTemplateString in pkg/agent/baker.go, which currently doesn’t reference these fields). As a result, setting these fields via the AgentBaker API won’t change the generated Corefile, so the feature is only partially wired.
// Determines the handling of requests when all upstream servers are unhealthy.
FailfastAllUnhealthyUpstreams *bool `json:"failfastAllUnhealthyUpstreams,omitempty"`
// Configures CoreDNS forward plugin health checking behavior for upstream servers.
HealthCheck *LocalDNSHealthCheck `json:"healthCheck,omitempty"`
aks-node-controller/parser/templates/localdns.toml.gtpl:50
- This condition only checks that Duration is non-nil; if Duration is set to an empty string, the template will emit an invalid
health_checkdirective (missing duration) and CoreDNS will fail to parse the forward block. Consider also checking that the duration value is non-empty, and similarly avoid emitting an emptydomainvalue.
This issue also appears on line 119 of the same file.
{{- if and $override.HealthCheck $override.HealthCheck.Duration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 20:49
88ccb0b to
4666fe8
Compare
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 20:50
4666fe8 to
2780ba7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (2)
aks-node-controller/parser/templates/localdns.toml.gtpl:121
- Same issue as the node listener block above:
duration: ""(ordomain: "") will still satisfy the current pointer-based condition and render an invalidhealth_checkline (ordomainwith an empty value). Guard against empty strings to avoid generating an invalid Corefile.
{{- if and $override.HealthCheck $override.HealthCheck.Duration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
aks-node-controller/parser/templates/localdns.toml.gtpl:50
- The template guards on
$override.HealthCheck.Durationbeing non-nil, but does not guard against the duration being an empty string. If a caller setsduration: ""(non-nil pointer), this will renderhealth_checkwith an empty first argument, which CoreDNS will treat as an invalid Corefile. Similarly, an explicitly-empty domain would emitdomain.
Prefer checking the computed strings are non-empty before rendering the directive/optional domain segment.
This issue also appears on line 119 of the same file.
{{- if and $override.HealthCheck $override.HealthCheck.Duration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 21:06
2780ba7 to
f7055da
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (4)
aks-node-controller/parser/templates/localdns.toml.gtpl:121
- Same issue as above in the KubeDNS overrides block: checking
$override.HealthCheck.Domainwill renderdomaineven when the value is an empty string (non-nil pointer), producing invalid CoreDNS syntax. Check.GetDomaininstead.
{{- if and $override.HealthCheck $override.HealthCheck.GetDuration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
aks-node-controller/parser/templates/localdns.toml.gtpl:50
domainrendering guard checks pointer presence ($override.HealthCheck.Domain) rather than a non-empty value. Ifdomainis explicitly set to an empty string, this will emitdomainand produce an invalidhealth_checkdirective. Prefer checking.GetDomain(non-empty) to match thedurationhandling.
This issue also appears on line 119 of the same file.
{{- if and $override.HealthCheck $override.HealthCheck.GetDuration}}
health_check {{$override.HealthCheck.GetDuration}}{{if $override.HealthCheck.GetNoRec}} no_rec{{end}}{{if $override.HealthCheck.Domain}} domain {{$override.HealthCheck.GetDomain}}{{end}}
{{- end}}
pkg/agent/datamodel/types.go:2624
- These new LocalDNS override fields are added to the datamodel, but there are no references to them elsewhere in
pkg/agent(including the in-repo LocalDNS Corefile template inpkg/agent/baker.go). As-is, AgentBaker's rendered LocalDNS Corefile will ignorehealthCheckandfailfastAllUnhealthyUpstreams, which appears to contradict the PR description about Corefile rendering updates. Either wire these fields into the AgentBaker Corefile template/rendering path, or clarify that only the aks-node-controller renderer is expected to honor them.
// Determines the handling of requests when all upstream servers are unhealthy.
FailfastAllUnhealthyUpstreams *bool `json:"failfastAllUnhealthyUpstreams,omitempty"`
// Configures CoreDNS forward plugin health checking behavior for upstream servers.
HealthCheck *LocalDNSHealthCheck `json:"healthCheck,omitempty"`
aks-node-controller/parser/helper_test.go:2055
- The matrix covers nil/missing duration cases, but it doesn't cover the edge case where
domainis explicitly set to an empty string (non-nil pointer). Given the template currently guardsdomainby pointer presence in some places, adding a test forDomain: to.Ptr("")(with duration set) helps prevent emittingdomainin the generated Corefile.
{
name: "duration and domain",
healthCheck: &aksnodeconfigv1.LocalDnsHealthCheck{
Duration: to.Ptr("1s"),
Domain: to.Ptr("health.local."),
},
wantContains: "health_check 1s domain health.local.",
wantNotContains: []string{"no_rec"},
},
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 21:47
f7055da to
d0703f1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (4)
pkg/agent/baker.go:2387
- Same issue as the earlier forward block:
FailfastAllUnhealthyUpstreamsis a*bool, so pointer presence makes this condition truthy even when the underlying value isfalse. Switch the template to use a bool getter (e.g.,GetFailfastAllUnhealthyUpstreams()) so explicitfalseomits the directive.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/baker.go:2316
- This
if $override.FailfastAllUnhealthyUpstreamscheck will be true for any non-nil*bool, including when the value is explicitly set tofalse, sofailfast_all_unhealthy_upstreamsmay render incorrectly. Use a bool-returning accessor (e.g.,GetFailfastAllUnhealthyUpstreams()) or otherwise compare the underlying bool value rather than pointer presence.
This issue also appears on line 2385 of the same file.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/datamodel/types.go:2643
FailfastAllUnhealthyUpstreamsis a*bool, but templates can’t dereference it. Without aGetFailfastAllUnhealthyUpstreams()accessor, call sites (like the legacy Corefile template) will treat any non-nil pointer as truthy and may renderfailfast_all_unhealthy_upstreamseven when explicitly set to false. Add a getter that returns false when the pointer is nil and the pointed value otherwise, mirroring the generated proto API.
ServeStale string `json:"serveStale,omitempty"`
// Determines the handling of requests when all upstream servers are unhealthy.
FailfastAllUnhealthyUpstreams *bool `json:"failfastAllUnhealthyUpstreams,omitempty"`
// Configures CoreDNS forward plugin health checking behavior for upstream servers.
HealthCheck *LocalDNSHealthCheck `json:"healthCheck,omitempty"`
pkg/agent/baker_test.go:813
- The legacy Corefile renderer test only exercises
FailfastAllUnhealthyUpstreams=true. Add a sub-case withFailfastAllUnhealthyUpstreams=falseand assertfailfast_all_unhealthy_upstreamsis omitted, to prevent regressions (and to catch pointer-truthiness rendering bugs in templates).
It("renders localdns forward health knobs", func() {
config.AgentPoolProfile.LocalDNSProfile = &datamodel.LocalDNSProfile{
EnableLocalDNS: true,
VnetDNSOverrides: map[string]*datamodel.LocalDNSOverrides{".": {
QueryLogging: "Log", Protocol: "PreferUDP", ForwardDestination: "VnetDNS", ForwardPolicy: "Sequential",
MaxConcurrent: to.Int32Ptr(1000), CacheDurationInSeconds: to.Int32Ptr(3600), ServeStaleDurationInSeconds: to.Int32Ptr(3600), ServeStale: "Immediate",
FailfastAllUnhealthyUpstreams: to.BoolPtr(true), HealthCheck: &datamodel.LocalDNSHealthCheck{Duration: to.StringPtr("1s"), NoRec: to.BoolPtr(true), Domain: to.StringPtr("health.local.")},
}},
KubeDNSOverrides: map[string]*datamodel.LocalDNSOverrides{".": {
QueryLogging: "Error", Protocol: "PreferUDP", ForwardDestination: "ClusterCoreDNS", ForwardPolicy: "Sequential",
MaxConcurrent: to.Int32Ptr(1000), CacheDurationInSeconds: to.Int32Ptr(3600), ServeStaleDurationInSeconds: to.Int32Ptr(3600), ServeStale: "Immediate",
HealthCheck: &datamodel.LocalDNSHealthCheck{Duration: to.StringPtr("2s"), Domain: to.StringPtr("")},
}},
}
localDNSCoreFile, err := GenerateLocalDNSCoreFile(config, config.AgentPoolProfile, false)
Expect(err).To(BeNil())
Expect(localDNSCoreFile).To(ContainSubstring("health_check 1s no_rec domain health.local."))
Expect(localDNSCoreFile).To(ContainSubstring("failfast_all_unhealthy_upstreams"))
Expect(localDNSCoreFile).To(ContainSubstring("health_check 2s"))
Expect(localDNSCoreFile).ToNot(ContainSubstring("domain \n"))
})
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 11, 2026 21:57
d0703f1 to
a8cf296
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (3)
pkg/agent/baker.go:2387
- Same issue as above:
FailfastAllUnhealthyUpstreamsis a*bool, so thisifwill be true whenever the pointer is non-nil (including when it points tofalse). Use a bool-returning getter and call it here to avoid renderingfailfast_all_unhealthy_upstreamswhen explicitly disabled.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/baker.go:2316
FailfastAllUnhealthyUpstreamsis a*bool, but in Go templates any non-nil pointer is truthy regardless of its underlying value. Thisifwill therefore renderfailfast_all_unhealthy_upstreamseven when the field is explicitly set tofalse.
Fix by dereferencing via a bool-returning getter (nil/false => omit, true => render) and use that getter in the template (as the aks-node-controller template does).
This issue also appears on line 2385 of the same file.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/datamodel/types.go:2644
- Add a
GetFailfastAllUnhealthyUpstreams()helper onLocalDNSOverrides(matching the protobuf getter style) so templates can safely treat this tri-state*boolas a boolean. Without a getter, template code will tend to test the pointer itself, which is truthy for bothtrueandfalsewhen non-nil.
// Determines the handling of requests when all upstream servers are unhealthy.
FailfastAllUnhealthyUpstreams *bool `json:"failfastAllUnhealthyUpstreams,omitempty"`
// Configures CoreDNS forward plugin health checking behavior for upstream servers.
HealthCheck *LocalDNSHealthCheck `json:"healthCheck,omitempty"`
}
Comment on lines
+793
to
+813
| It("renders localdns forward health knobs", func() { | ||
| config.AgentPoolProfile.LocalDNSProfile = &datamodel.LocalDNSProfile{ | ||
| EnableLocalDNS: true, | ||
| VnetDNSOverrides: map[string]*datamodel.LocalDNSOverrides{".": { | ||
| QueryLogging: "Log", Protocol: "PreferUDP", ForwardDestination: "VnetDNS", ForwardPolicy: "Sequential", | ||
| MaxConcurrent: to.Int32Ptr(1000), CacheDurationInSeconds: to.Int32Ptr(3600), ServeStaleDurationInSeconds: to.Int32Ptr(3600), ServeStale: "Immediate", | ||
| FailfastAllUnhealthyUpstreams: to.BoolPtr(true), HealthCheck: &datamodel.LocalDNSHealthCheck{Duration: to.StringPtr("1s"), NoRec: to.BoolPtr(true), Domain: to.StringPtr("health.local.")}, | ||
| }}, | ||
| KubeDNSOverrides: map[string]*datamodel.LocalDNSOverrides{".": { | ||
| QueryLogging: "Error", Protocol: "PreferUDP", ForwardDestination: "ClusterCoreDNS", ForwardPolicy: "Sequential", | ||
| MaxConcurrent: to.Int32Ptr(1000), CacheDurationInSeconds: to.Int32Ptr(3600), ServeStaleDurationInSeconds: to.Int32Ptr(3600), ServeStale: "Immediate", | ||
| HealthCheck: &datamodel.LocalDNSHealthCheck{Duration: to.StringPtr("2s"), Domain: to.StringPtr("")}, | ||
| }}, | ||
| } | ||
| localDNSCoreFile, err := GenerateLocalDNSCoreFile(config, config.AgentPoolProfile, false) | ||
| Expect(err).To(BeNil()) | ||
| Expect(localDNSCoreFile).To(ContainSubstring("health_check 1s no_rec domain health.local.")) | ||
| Expect(localDNSCoreFile).To(ContainSubstring("failfast_all_unhealthy_upstreams")) | ||
| Expect(localDNSCoreFile).To(ContainSubstring("health_check 2s")) | ||
| Expect(localDNSCoreFile).ToNot(ContainSubstring("domain \n")) | ||
| }) |
Contributor
Author
There was a problem hiding this comment.
Addressed. The legacy pkg/agent template now uses GetFailfastAllUnhealthyUpstreams instead of pointer truthiness, and I added coverage for explicit false to ensure failfast_all_unhealthy_upstreams is omitted. Package tests compile and pass.
Contributor
Clawpilot AgentBaker Linux gate detective
|
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 12, 2026 18:30
a8cf296 to
c9ff53f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
Suppressed comments (4)
pkg/agent/baker.go:2393
- Same issue as above in the KubeDNS override block: checking the
*boolpointer directly will renderfailfast_all_unhealthy_upstreamseven when the value is explicitly set to false. Use a boolean getter instead.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/baker_test.go:817
- This test covers the enabled case for
failfast_all_unhealthy_upstreams, but it doesn't cover the important edge-case where the field is explicitly set tofalse(non-nil pointer). Given the template uses Go template truthiness, adding an explicit-false assertion will prevent regressions.
Expect(localDNSCoreFile).To(ContainSubstring("health_check 1s no_rec domain health.local."))
Expect(localDNSCoreFile).To(ContainSubstring("failfast_all_unhealthy_upstreams"))
Expect(localDNSCoreFile).To(ContainSubstring("health_check 2s"))
Expect(localDNSCoreFile).ToNot(ContainSubstring("domain \n"))
pkg/agent/baker.go:2319
FailfastAllUnhealthyUpstreamsis a*bool, but the template currently checks the pointer itself. In Go templates any non-nil pointer is truthy, so setting this tofalse(non-nil) would still renderfailfast_all_unhealthy_upstreams. Switch the condition to a boolean getter that dereferences the pointer safely.
This issue also appears on line 2391 of the same file.
{{- if $override.FailfastAllUnhealthyUpstreams}}
failfast_all_unhealthy_upstreams
{{- end}}
pkg/agent/datamodel/types.go:2644
FailfastAllUnhealthyUpstreamsis optional (*bool) butLocalDNSOverridesdoesn't currently expose a getter like the generated proto does. AddingGetFailfastAllUnhealthyUpstreams()avoids Go-template pointer-truthiness bugs and keeps the legacy renderer consistent with the aks-node-controller template usage.
// Determines the handling of requests when all upstream servers are unhealthy.
FailfastAllUnhealthyUpstreams *bool `json:"failfastAllUnhealthyUpstreams,omitempty"`
// Configures CoreDNS forward plugin health checking behavior for upstream servers.
HealthCheck *LocalDNSHealthCheck `json:"healthCheck,omitempty"`
}
Saewon Kwak (saewoni)
force-pushed
the
sakwa/localdns-forward-knobs
branch
from
August 12, 2026 18:39
c9ff53f to
02661e6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go: Generated file
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change summary
Adds CoreDNS forward plugin health/failfast knobs to LocalDNS node configuration and Corefile rendering.
This intentionally avoids inventing a default health_check duration in AgentBaker. If healthCheck is nil or duration is unset/empty, the templates omit health_check and leave CoreDNS default behavior intact.
Testing