Skip to content

feat: support AKS LocalDNS profiles - #255

Open
hbc (bcho) wants to merge 18 commits into
mainfrom
feature/aks-localdns-profile
Open

feat: support AKS LocalDNS profiles#255
hbc (bcho) wants to merge 18 commits into
mainfrom
feature/aks-localdns-profile

Conversation

@bcho

Copy link
Copy Markdown
Member

Summary

  • consume the official AKS LocalDNS profile shape under networking.localDNS
  • translate vnetDNSOverrides and kubeDNSOverrides into an Unbounded full replacement Corefile template
  • support Required, Preferred, and Disabled modes
  • preserve the AKS cluster DNS service IP while enabling the nspawn LocalDNS runtime
  • enable LocalDNS in the MSI Azure e2e node and validate service, resolver, listeners, NOTRACK rules, reboot, and repave behavior
  • document configuration using the official AKS LocalDNS example

Dependency

This PR consumes the Unbounded LocalDNS implementation from Azure/unbounded#554 at commit 291de68b. It should move to the merged/tagged Unbounded version before this PR is finalized.

Official configuration reference

https://learn.microsoft.com/azure/aks/localdns-custom

Validation

  • make check
  • bash -n hack/e2e/lib/node-join-msi.sh hack/e2e/lib/validate.sh
  • PR Azure e2e enables LocalDNS on the MSI Flex Node and performs host/nspawn validation

Comment thread pkg/config/localdns.go Outdated
Comment thread pkg/config/localdns.go Outdated
Comment thread pkg/config/localdns.go Outdated
Comment thread pkg/config/localdns.go
@bcho
hbc (bcho) force-pushed the feature/aks-localdns-profile branch from 5626e4b to 6b2fe2c Compare August 4, 2026 20:53
@bcho
hbc (bcho) force-pushed the feature/aks-localdns-profile branch from 6b2fe2c to e6d87db Compare August 4, 2026 21:33
@bcho
hbc (bcho) requested a balanced review from Copilot August 7, 2026 21:00
Copilot AI review requested due to automatic review settings August 7, 2026 21:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (8)

pkg/config/internal/localdns/localdns.go:223

  • An omitted protocol is defaulted to ForceTCP for every zone, while the official AKS default is ForceTCP only for cluster.local and PreferUDP elsewhere. Partial/root overrides that omit this field therefore silently change DNS forwarding to TCP. Make this default depend on the zone.
}

func isClusterLocalZone(zone string) bool {

hack/e2e/lib/upgrade-drift.sh:247

  • LocalDNS creates and validates these rules in the native nftables table ip unbounded_localdns; iptables -t raw -S does not inspect that dedicated table. A stale LocalDNS table therefore passes this cleanup assertion. Check that the nftables table itself is absent.
! nft list table ip unbounded_localdns >/dev/null 2>&1

pkg/config/internal/localdns/localdns.go:45

  • The official AKS contract defaults an omitted mode to Preferred, but this condition rejects such profiles. That makes valid official-shape input fail instead of taking the documented validation-only path. Accept an empty mode as Preferred (or apply that default before validation).

This issue also appears on line 221 of the same file.

	if p.Mode != LocalDNSModeRequired && p.Mode != LocalDNSModePreferred && p.Mode != LocalDNSModeDisabled {

pkg/config/internal/localdns/localdns.go:56

  • A zone containing : passes validation, but rendering then appends :53; for example, example.com:1053 becomes the invalid Corefile key example.com:1053:53. Reject colons in map keys so Preferred mode actually catches this malformed configuration before repave.
		if class == "kubeDNSOverrides" {
			defaultDestination = "ClusterCoreDNS"

pkg/config/internal/localdns/localdns.go:152

  • AKS defines both . and cluster.local as default server blocks. Creating only the root VNet block causes default-policy cluster.local queries to match . and go to VNet DNS instead of ClusterCoreDNS. Seed both official blocks (and their zone-specific protocols) when overrides are omitted.
		return "", err
	}
	if !p.Enabled() {
		return "", nil
	}

	vnet := p.VnetDNSOverrides
	if len(vnet) == 0 {

docs/usages/configuration.md:157

  • This is the kube root (.) block, whose official AKS default protocol is PreferUDP; ForceTCP belongs to a separate cluster.local block. Both cluster.local blocks are omitted from this example, so it does not match the official example claimed by the PR. Copy the complete official profile here.
      "kubeDNSOverrides": {
        ".": {
          "queryLogging": "Error",
          "protocol": "ForceTCP",
          "forwardDestination": "ClusterCoreDNS",

hack/e2e/lib/validate.sh:201

  • The delete is asynchronous, yet the same pod names are recreated immediately. Re-running validation after an interrupted run (or while the prior successful cleanup is still terminating) can fail with AlreadyExists. Wait for deletion so this validator is reliably repeatable.
    hack/e2e/lib/upgrade-drift.sh:260
  • The PR says the Azure E2E validates LocalDNS reboot and repave behavior, but this flow never reboots the MSI host and never reruns validate_localdns_status after the enabled MSI repave. Add a reboot/wait/status cycle and rerun the LocalDNS validator after the MSI repave before testing disablement.
upgrade_drift_all() {
  log_section "Controller Machine Repave (all modes)"
  upgrade_drift_mode msi
  upgrade_drift_mode token
  upgrade_drift_mode kubeadm
  localdns_disable_repave_msi

Copilot AI review requested due to automatic review settings August 7, 2026 21:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

hack/e2e/lib/validate.sh:256

  • This second accidental leading + is also parsed as a +# command rather than a comment. Remove it so the validation library can be sourced successfully.
+# validate_all_nodes - Check all MSI, token, offline, and kubeadm VMs joined

pkg/config/internal/localdns/localdns.go:45

  • The official AKS profile makes mode optional and defaults it to Preferred, but this rejects an omitted mode. A profile accepted by the AKS contract therefore fails FlexNode startup validation; accept the empty value as the Preferred default.
	if p.Mode != LocalDNSModeRequired && p.Mode != LocalDNSModePreferred && p.Mode != LocalDNSModeDisabled {

pkg/config/internal/localdns/localdns.go:112

  • Preferred is incorrectly treated as disabled. AKS defines this mode as enabling LocalDNS when the runtime supports it, and this runtime now does; only Disabled should turn it off. This currently makes an explicitly Preferred profile validation-only instead of activating LocalDNS.
func (p *LocalDNSProfile) Enabled() bool {
	return p != nil && p.Mode == LocalDNSModeRequired
}

docs/usages/configuration.md:133

  • This documents Preferred as validation-only, but the official AKS contract says it prefers enabling LocalDNS when supported. FlexNode includes a supported LocalDNS runtime, so this statement would lead users to expect behavior different from AKS and from the corrected implementation.
fields and values. `Required` enables LocalDNS, `Disabled` disables it through
repave, and `Preferred` validates the profile without enabling the service.

Comment thread hack/e2e/lib/validate.sh Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 21:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/config/internal/localdns/localdns.go:47

  • The official AKS LocalDNSProfile declares mode optional with a default of Preferred. This rejects an omitted mode, so an otherwise valid official profile such as {} fails Flex Node validation instead of behaving as validation-only. Treat the empty value as Preferred before checking the enum.
	if p.Mode != LocalDNSModeRequired && p.Mode != LocalDNSModePreferred && p.Mode != LocalDNSModeDisabled {

pkg/config/internal/localdns/localdns.go:245

  • This rejects an AKS-supported override key: the official AKS 2025-10-01 LocalDNS example uses *.example.com, but the alphanumeric endpoint check rejects the * label. As a result, a profile accepted by AKS fails Flex Node startup despite the stated official-profile compatibility. Permit * as a complete leftmost label.
		if len(label) == 0 || len(label) > 63 || !isASCIIAlphaNumeric(label[0]) || !isASCIIAlphaNumeric(label[len(label)-1]) {

Copilot AI review requested due to automatic review settings August 7, 2026 22:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/config/internal/localdns/localdns.go:155

  • Partial override maps discard the required default zones. For example, kubeDNSOverrides containing only cluster.local produces no . block on the cluster listener, so ClusterFirst lookups for external names have no matching CoreDNS server and fail. Initialize both maps with the default ./cluster.local entries and overlay the supplied overrides (or reject partial maps) instead of defaulting only when the whole map is empty.
	vnet := p.VnetDNSOverrides
	if len(vnet) == 0 {
		vnet = defaultLocalDNSOverrides()
	}
	kube := p.KubeDNSOverrides

pkg/config/internal/localdns/localdns.go:71

  • Validation still accepts an external VNet override such as example.com -> ClusterCoreDNS, although the AKS LocalDNS contract rejects external zones forwarded to ClusterCoreDNS from vnetDNSOverrides. Such a profile would be accepted here despite not being valid under the official shape this adapter claims to consume; reject this combination while allowing cluster.local suffixes.
			if class == "vnetDNSOverrides" && zone == "." && resolved.ForwardDestination == "ClusterCoreDNS" {
				errs = append(errs, fmt.Errorf("%s[%q]: forwardDestination must not be ClusterCoreDNS", class, zone))
			}
			if isClusterLocalZone(zone) && resolved.ForwardDestination == "VnetDNS" {
				errs = append(errs, fmt.Errorf("%s[%q]: forwardDestination must not be VnetDNS", class, zone))

Copilot AI review requested due to automatic review settings August 7, 2026 22:37
@bcho
hbc (bcho) deployed to e2e-testing August 7, 2026 22:40 — with GitHub Actions Active

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/config/internal/localdns/localdns.go:112

  • Preferred is currently indistinguishable from Disabled: Enabled() can only return true for Required. The AKS contract defines Preferred as enabling LocalDNS when the current orchestrator supports it, and this PR pins an Unbounded version that provides the implementation. As written, a valid Preferred profile silently disables LocalDNS instead of applying the capability-gated fallback semantics. Please either implement that gate/fallback or reject Preferred rather than claiming support for it.
// Enabled reports whether the profile requires LocalDNS installation.
func (p *LocalDNSProfile) Enabled() bool {
	return p != nil && p.Mode == LocalDNSModeRequired
}

pkg/config/internal/localdns/localdns.go:46

  • The official AKS profile declares mode optional with a default of Preferred, but this validation rejects the omitted value (""). Consequently, { "networking": { "localDNS": {} } } is accepted by the AKS contract but fails FlexNode startup. Apply the same default before validation and ensure Enabled()/rendering use the effective mode.
	if p.Mode != LocalDNSModeRequired && p.Mode != LocalDNSModePreferred && p.Mode != LocalDNSModeDisabled {
		return fmt.Errorf("mode must be Required, Preferred, or Disabled")

pkg/config/internal/localdns/localdns.go:225

  • This suffix check is neither label-boundary-aware nor case-insensitive. It misclassifies a valid custom zone such as notcluster.local as Kubernetes DNS, while failing to recognize the equivalent CLUSTER.LOCAL; those errors then affect validation, defaults, and upstream routing. Normalize case and require either exact cluster.local or a .cluster.local suffix.
func isClusterLocalZone(zone string) bool {
	return strings.HasSuffix(strings.TrimSuffix(zone, "."), "cluster.local")
}

Copilot AI review requested due to automatic review settings August 7, 2026 22:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/config/internal/localdns/localdns.go:46

  • The AKS ARM contract makes mode optional and defaults it to Preferred, but an omitted field unmarshals to "" and is rejected here. This means a valid official profile such as {"localDNS": {}} cannot be consumed. Preserve field presence or apply the Preferred default before validation so omission retains AKS semantics.
	if p.Mode != LocalDNSModeRequired && p.Mode != LocalDNSModePreferred && p.Mode != LocalDNSModeDisabled {
		return fmt.Errorf("mode must be Required, Preferred, or Disabled")

pkg/config/internal/localdns/localdns.go:36

  • These non-pointer fields collapse an omitted value and an explicit 0. The official contract models them as optional int32 values whose defaults apply on omission, while withLocalDNSDefaults currently rewrites every explicit zero to 1000/3600. As a result, a requested zero cache/stale duration is silently changed. Use optional values and default only when absent, while retaining nonnegative/range validation.
	MaxConcurrent               int    `json:"maxConcurrent,omitempty"`
	CacheDurationInSeconds      int    `json:"cacheDurationInSeconds,omitempty"`
	ServeStaleDurationInSeconds int    `json:"serveStaleDurationInSeconds,omitempty"`

docs/usages/configuration.md:125

  • This value is shown in the Default column, but the implementation never creates a LocalDNS profile when the property is omitted, so LocalDNS is disabled rather than Required. The AKS contract also defaults an omitted mode within an existing profile to Preferred. Document this as not set/disabled instead of presenting the sample as the runtime default.
| `networking.localDNS` | object | Optional AKS LocalDNS profile using the same `mode`, `vnetDNSOverrides`, and `kubeDNSOverrides` shape accepted by `az aks nodepool --localdns-config`. | `{ "mode": "Required" }` |

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.

integrate with local dns configuration

3 participants