Skip to content

Add LocalDNS live patching handler - #9137

Open
Saewon Kwak (saewoni) wants to merge 2 commits into
feature/knead-security-patchingfrom
localdns-lps-on-knead
Open

Add LocalDNS live patching handler#9137
Saewon Kwak (saewoni) wants to merge 2 commits into
feature/knead-security-patchingfrom
localdns-lps-on-knead

Conversation

@saewoni

@saewoni Saewon Kwak (saewoni) commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR stacks on #8952 and wires LocalDNS into the generic knead live-patching loop.

End-to-end flow:

knead detects localDNS goal hash change
→ updateLocalDNS calls aks-node-controller fetch-localdns-config
→ aks-node-controller fetches localDNS config from LPS
→ writes /opt/azure/containers/localdns/livepatched.localdns.corefile
→ localdns.sh prefers the livepatched Corefile when generating updated.localdns.corefile
→ restart localdns.service when a new Corefile is applied

Important behavior

  • LPS is the source of truth for LocalDNS config; the knead-dispatched payload is only used to trigger the LocalDNS handler.
  • localdns.corefile remains the baked/CSE-generated fallback source.
  • livepatched.localdns.corefile stores the LPS-provided desired source.
  • updated.localdns.corefile remains the runtime-generated Corefile that CoreDNS starts with after VNet DNS replacement.
  • notFound from LPS is intentionally fail-open/no-op so nodes keep existing LocalDNS config when no LPS config is published yet.
  • noCorefileData is treated as failure because LPS returned an unusable LocalDNS payload.
  • The LocalDNS background live-patching-status writer is opt-in now; knead owns kubernetes.azure.com/live-patching-status for runtime patching.

Dependencies

  • Stacked on Chase's AgentBaker PR feat: update snapshot to be generic #8952 (feature/knead-security-patching).
  • Depends on aks-rp/LPS work to publish and serve the localDNS config.
  • End-to-end validation depends on LPC/LPS rollout in E2E/staging.

Validation

  • go test . ./common ./helpers ./parser ./pkg/nodeconfigutils ./utils in aks-node-controller
  • docker run --rm -t -v "$PWD:/src" shellspec-docker --shell bash spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.sh
  • docker run --rm -t -v "$PWD:/src" shellspec-docker --shell bash spec/parts/linux/cloud-init/artifacts/localdns_spec.sh

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed❌ failed (0)Aug 5, 2026, 7:50 PM

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

This PR wires LocalDNS into the generic “knead” live-patching reconciliation loop by adding a LocalDNS handler that fetches the desired Corefile from LPS via aks-node-controller, writes it to the livepatched Corefile location, and restarts localdns.service when applicable. It also refactors the existing Ubuntu snapshot update script into a generic reconciler + securityPatch handler split, and introduces an aks-live-patching proto module plus Buf CI updates.

Changes:

  • Add LocalDNS live-patching fetch/apply path via aks-node-controller fetch-localdns-config, plus LocalDNS Corefile livepatched source selection and optional legacy status writer.
  • Split Ubuntu snapshot update into a generic reconciler loop (ubuntu-snapshot-update.sh) and a securityPatch handler (security-update.sh), and stage/deliver the new handler in VHD + hotfix/customdata.
  • Add/expand ShellSpec coverage, VHD content validation, and an E2E scenario for LocalDNS LPS bootstrap patching; introduce aks-live-patching proto module and update Buf workflow.

Reviewed changes

Copilot reviewed 34 out of 38 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
vhdbuilder/packer/vhd-image-builder-cvm.json Stage security-update.sh into packer inputs for Ubuntu images.
vhdbuilder/packer/vhd-image-builder-base.json Stage security-update.sh into packer inputs for Ubuntu images.
vhdbuilder/packer/vhd-image-builder-arm64-gen2.json Stage security-update.sh into packer inputs for Ubuntu images.
vhdbuilder/packer/vhd-image-builder-arm64-gb.json Stage security-update.sh into packer inputs for Ubuntu images.
vhdbuilder/packer/test/linux-vhd-content-test.sh Validate knead/security patching assets exist + permissions.
vhdbuilder/packer/packer_source.sh Copy security-update.sh into /opt/azure/containers/ on Ubuntu VHDs.
spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.sh Expand tests to cover generic knead reconciliation behavior and LocalDNS dispatch.
spec/parts/linux/cloud-init/artifacts/snapshot-update-service_spec.sh New tests validating packer inputs + service command expectations + hotfix delivery behavior.
spec/parts/linux/cloud-init/artifacts/security-update_spec.sh New tests for security-update.sh handler behavior and edge cases.
spec/parts/linux/cloud-init/artifacts/localdns_spec.sh Add tests for LPS fetch behavior and livepatched Corefile selection.
pkg/agent/variables.go Add securityUpdateScript cloud-init variable injection.
pkg/agent/const.go Add const mapping for security-update.sh.
pkg/agent/baker.go Add reload directive to generated LocalDNS Corefile templates.
pkg/agent/baker_test.go Update expected LocalDNS Corefile snapshots to include reload.
parts/linux/cloud-init/nodecustomdata.yml Deliver both generic updater + security handler together for Ubuntu via write_files.
parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh Refactor into generic knead reconciler + add LocalDNS handler dispatch.
parts/linux/cloud-init/artifacts/ubuntu/security-update.sh New securityPatch handler extracted from prior Ubuntu snapshot updater logic.
parts/linux/cloud-init/artifacts/localdns.sh Prefer livepatched Corefile when present; add LPS fetch + optional legacy status writer.
hotfix/hotfix_generate.py Include ubuntu/security-update.sh in hotfix inventory mapping.
e2e/scenario_localdns_hosts_test.go Add E2E scenario validating LocalDNS LPS bootstrap patching behavior.
CODEOWNERS Add ownership for new security update + new specs.
aks-node-controller/parser/templates/localdns.toml.gtpl Add reload directive to generated LocalDNS Corefile template.
aks-node-controller/parser/helper.go Export GenerateLocalDNSCorefileFromAKSNodeConfig for reuse.
aks-node-controller/parser/helper_test.go Update expected LocalDNS Corefile snapshots to include reload.
aks-node-controller/localdnsconfig.go Add LocalDNS LPS fetch + Corefile apply logic and outcomes.
aks-node-controller/localdnsconfig_test.go Add unit tests for LocalDNS LPS fetch/apply outcome handling.
aks-node-controller/go.sum Update sums for added grpc/proto dependencies and related indirects.
aks-node-controller/go.mod Add grpc/proto deps and replace for new aks-live-patching submodule.
aks-node-controller/app.go Add fetch-localdns-config and apply-localdns-config CLI commands.
aks-live-patching/protoc.Dockerfile Add a dockerized protoc toolchain image for generation.
aks-live-patching/proto/akslivepatching/v1/live_patching.proto Define LivePatchingService proto API.
aks-live-patching/pkg/gen/akslivepatching/v1/live_patching.pb.go Generated protobuf Go code.
aks-live-patching/pkg/gen/akslivepatching/v1/live_patching_grpc.pb.go Generated gRPC Go code.
aks-live-patching/Makefile Add proto generate + lint targets (buf + protoc container).
aks-live-patching/go.sum New module dependency lockfile.
aks-live-patching/go.mod New Go module for live-patching proto artifacts.
aks-live-patching/buf.yaml New buf config for the aks-live-patching module.
.github/workflows/buf.yaml Extend Buf CI to run on aks-live-patching and reuse buf version pin.
Files not reviewed (2)
  • aks-live-patching/pkg/gen/akslivepatching/v1/live_patching.pb.go: Generated file
  • aks-live-patching/pkg/gen/akslivepatching/v1/live_patching_grpc.pb.go: Generated file

Comment on lines +130 to +145
outcome, err := a.fetchAndApplyLocalDNSConfig(ctx, outputPath)
level := helpers.EventLevelInformational
if outcome == outcomeLocalDNSConfigFailed {
level = helpers.EventLevelError
}
message := fmt.Sprintf("fetch-localdns-config outcome=%s", outcome)
if err != nil {
message = fmt.Sprintf("%s error=%s", message, err.Error())
slog.Warn("fetch-localdns-config completed with error (fail-open)", "outcome", outcome, "error", err)
} else {
slog.Info("fetch-localdns-config completed", "outcome", outcome)
}
if a.eventLogger != nil {
a.eventLogger.LogEvent("FetchLocalDNSConfig", message, level, startTime, time.Now())
}
return nil

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 280c137. The runtime LocalDNS handler now uses apply-localdns-config with the dispatched component payload, so the command emits the expected outcome for updateLocalDNS.

Comment on lines +245 to +250
if err != nil {
if code := status.Code(err); code == codes.NotFound || code == codes.PermissionDenied || code == codes.Unauthenticated {
return "", &lpsUnavailableError{statusCode: int(code)}
}
return "", fmt.Errorf("get %s component config: %w", localDNSLivePatchingComponentName, err)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d4fdefb. The LocalDNS gRPC path now maps NotFound/PermissionDenied/Unauthenticated to the HTTP-style 404/403/401 values expected by lpsUnavailableError, matching the existing hotfix handler semantics.

Comment thread e2e/scenario_localdns_hosts_test.go Outdated
Comment on lines +148 to +156
if [ -z "$output" ]; then
echo "missing --output" >&2
exit 1
fi
cp "$output" "$output.tmp"
cat <<'CORE' >> "$output.tmp"
# ` + marker + `
CORE
mv "$output.tmp" "$output"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in acfe482. The fake LocalDNS LPS fetcher now creates the temp output file when the livepatched Corefile does not exist yet, so fresh-node runs no longer fail at cp before appending the marker.

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 28 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (2)

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:296

  • PR description states the knead LocalDNS handler calls aks-node-controller fetch-localdns-config and that the knead-dispatched payload is only a trigger. In the current implementation, updateLocalDNS pipes the knead payload into aks-node-controller apply-localdns-config, which applies that payload as the desired config. This discrepancy makes it unclear what the source of truth is (LPS vs ConfigMap payload) and can confuse future maintenance/troubleshooting.
    if ! outcome="$(printf '%s' "${component_payload}" | /opt/azure/containers/aks-node-controller apply-localdns-config \
        --config-file - \
        --output /opt/azure/containers/localdns/livepatched.localdns.corefile)"; then

e2e/scenario_localdns_hosts_test.go:138

  • This E2E test waits for components.localDNS.current to appear in the kubernetes.azure.com/live-patching-status annotation, but localdns.sh now only runs the legacy status writer when LOCALDNS_ENABLE_LEGACY_LIVEPATCH_STATUS=true. Nothing in this scenario enables that flag, so the poll will likely time out even if the Corefile patch succeeds.
sudo chmod +x ` + ancBinary + `
sudo systemctl restart localdns.service
`

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 28 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (2)

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:298

  • updateLocalDNS calls aks-node-controller apply-localdns-config and then handles outcomes like notFound/noCorefileData as if the command is fetching from LPS. However, apply-localdns-config only applies the JSON passed via stdin (no LPS fetch), so notFound cannot occur here, and a “trigger-only” payload (as described in the PR) would likely hit noCorefileData and fail the component.

This also contradicts the PR description flow (updateLocalDNSfetch-localdns-config → LPS). Either switch this handler to use fetch-localdns-config (and decide how to detect/apply changes), or ensure the dispatched nodeConfig always includes actual Corefile/profile content so apply-localdns-config can render a Corefile.

    if ! outcome="$(printf '%s' "${component_payload}" | /opt/azure/containers/aks-node-controller apply-localdns-config \
        --config-file - \
        --output /opt/azure/containers/localdns/livepatched.localdns.corefile)"; then
        echo "localDNS config apply failed"
        return 1

parts/linux/cloud-init/artifacts/localdns.sh:339

  • refresh_localdns_corefile_from_lps treats aks-node-controller fetch-localdns-config as a success/failure signal, but fetch-localdns-config is implemented as fail-open and always exits 0 (it returns nil even when it logs an error). That makes the else branch effectively unreachable and the “Completed LocalDNS LPS config fetch.” log misleading when the fetch fails.

Consider either (1) making fetch-localdns-config return a non-zero exit code on genuine errors (while the caller remains fail-open), or (2) adjusting this function to not imply it can detect fetch failures via exit status (e.g., log based on whether the corefile/version file was updated).

    # Write the LPS-provided Corefile to the livepatched source file; VNET DNS replacement
    # later derives UPDATED_LOCALDNS_CORE_FILE from this file before CoreDNS starts.
    if "${AKS_NODE_CONTROLLER_BINARY}" fetch-localdns-config --output "${LIVEPATCHED_LOCALDNS_CORE_FILE}"; then
        echo "Completed LocalDNS LPS config fetch."
        return 0
    fi

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 28 out of 29 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

aks-node-controller/localdnsconfig.go:39

  • After limiting the TLS client to ALPN "h2" (required for gRPC), the custom ALPN constant becomes dead code and should be removed to avoid unused-identifier compile failures.
	localDNSAgentPoolLabel            = "kubernetes.azure.com/agentpool"
	localDNSLPSALPNProto              = "aks-live-patching"
	localDNSALPNH2Proto               = "h2"

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:283

  • localDNSIsCurrent() compares the entire desired vs current payload strings. If the ConfigMap includes multiple agent pools, any change for a different pool will force every node to re-run the LocalDNS handler (and call aks-node-controller), even when its own pool is unchanged. Comparing only the current node’s agent pool checksum avoids unnecessary work and reduces churn.
localDNSIsCurrent() {
    local desired_payload="$1"
    local current_payload="$2"

    [ "${desired_payload}" = "${current_payload}" ]

Comment on lines +226 to +232
tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
RootCAs: rootCAs,
NextProtos: []string{localDNSLPSALPNProto, localDNSALPNH2Proto},
InsecureSkipVerify: true, //nolint:gosec // SNI stays on the apiserver FQDN for ALPN routing; chain and hostname are verified below.
VerifyPeerCertificate: localDNSVerifyChainAgainstPool(rootCAs, lpsSNIHost),
}
Comment thread e2e/scenario_localdns_hosts_test.go Outdated
Comment on lines +166 to +167
sudo chmod +x ` + ancBinary + `
sudo systemctl restart localdns.service

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 28 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (2)

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:307

  • updateLocalDNS calls aks-node-controller apply-localdns-config, but the outcome switch still handles notFound and logs messages implying the handler talks to LPS directly. apply-localdns-config cannot return notFound (it never calls LPS), so that branch is dead code and the logging is misleading. Please remove the unreachable notFound case and adjust the messages to describe the dispatched config payload rather than LPS availability.
    case "$(printf '%s
' "${outcome}" | tail -n 1)" in
        applied)
            if ! systemctl restart localdns.service; then
                echo "failed to restart localdns.service"

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:296

  • PR description says updateLocalDNS calls aks-node-controller fetch-localdns-config, but the knead handler currently calls apply-localdns-config with the dispatched component payload. Can you update the PR description (or the intended design) to reflect the actual runtime flow (bootstrap fetch via localdns.sh vs knead apply via apply-localdns-config), so operators/debuggers don’t follow the wrong command path?

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

    if ! outcome="$(printf '%s' "${component_payload}" | /opt/azure/containers/aks-node-controller apply-localdns-config \
        --config-file - \
        --output /opt/azure/containers/localdns/livepatched.localdns.corefile)"; then

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 28 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (2)

parts/linux/cloud-init/artifacts/ubuntu/security-update.sh:165

  • This error message refers to unattended_upgrade, but the function invoked here is security_patch_unattended_upgrade. Keeping the message aligned with the actual operation will make logs easier to interpret.
        echo "unattended_upgrade failed"

e2e/scenario_localdns_hosts_test.go:152

  • Test_LocalDNSLPSBootstrapPatch asserts that components.localDNS.current is stamped in kubernetes.azure.com/live-patching-status, but localdns.sh only runs the annotation writer when LOCALDNS_ENABLE_LEGACY_LIVEPATCH_STATUS=true. This E2E drop-in doesn’t set that environment variable, so the status annotation update won’t happen and the poll will time out.
func localDNSBranchScriptDropIn() string {
	return `[Service]
ExecStartPre=/bin/bash -c 'base64 -d ` + localDNSBranchScriptArchivePath + ` | gzip -d > /opt/azure/containers/localdns.sh && chmod 0544 /opt/azure/containers/localdns.sh'
`
}

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 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

e2e/scenario_localdns_hosts_test.go:151

  • Test_LocalDNSLPSBootstrapPatch validates that the node annotation eventually contains components.localDNS.current, but localdns.sh only writes that annotation when LOCALDNS_ENABLE_LEGACY_LIVEPATCH_STATUS=true. The systemd drop-in used by this test doesn't set that env var, so the test will fail unless something else in the image happens to enable it.
func localDNSBranchScriptDropIn() string {
	return `[Service]
ExecStartPre=/bin/bash -c 'base64 -d ` + localDNSBranchScriptArchivePath + ` | gzip -d > /opt/azure/containers/localdns.sh && chmod 0544 /opt/azure/containers/localdns.sh'
`

parts/linux/cloud-init/artifacts/localdns.sh:343

  • refresh_localdns_corefile_from_lps branches on the exit code of fetch-localdns-config, but that command is implemented as fail-open and always returns success (it logs outcome/errors internally). As a result, the else path here is effectively unreachable and the log line always implies success even when no config was applied. Consider making this unconditional (or basing messaging on file state) while still remaining fail-open.
    # Write the LPS-provided Corefile to the livepatched source file; VNET DNS replacement
    # later derives UPDATED_LOCALDNS_CORE_FILE from this file before CoreDNS starts.
    if "${AKS_NODE_CONTROLLER_BINARY}" fetch-localdns-config --output "${LIVEPATCHED_LOCALDNS_CORE_FILE}"; then
        echo "Completed LocalDNS LPS config fetch."
        return 0

e2e/scenario_localdns_hosts_test.go:146

  • mustReadCompressedLocalDNSArtifact compresses the original localdns.sh into buf and then immediately discards it (buf.Reset()), so the first gzip pass is dead work. Also, if the AKS_NODE_CONTROLLER_BINARY=... string ever changes, the replacement would silently fail and the test would exercise the real binary path instead of the wrapper. Compress once and assert the replacement actually happened.
func mustReadCompressedLocalDNSArtifact(t *testing.T) string {
	t.Helper()
	data, err := os.ReadFile("../parts/linux/cloud-init/artifacts/localdns.sh")
	require.NoError(t, err)
	var buf bytes.Buffer
	zw := gzip.NewWriter(&buf)
	_, err = zw.Write(data)
	require.NoError(t, err)
	require.NoError(t, zw.Close())

Copilot AI review requested due to automatic review settings August 18, 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.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Saewon Kwak (saewoni) and others added 2 commits August 18, 2026 23:41
Add AgentBaker LocalDNS live-patching support for the LPS bootstrap path and runtime knead dispatcher. The bootstrap path fetches LocalDNS nodeConfig from LPS, renders it through aks-node-controller, and feeds the generated Corefile into the existing updated.localdns.corefile flow before kubelet starts. The runtime path applies dispatched LocalDNS payloads with apply-localdns-config. Update focused unit, shellspec, and E2E coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Force GitHub to rebuild the stale PR merge ref after replacing the stacked branch with a clean LocalDNS commit on top of feature/knead-security-patching.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:311

  • The printf format strings here are split across lines, which embeds a newline (and indentation spaces) into the format string. That makes the trailing tail -n 1 return whitespace instead of applied|alreadyCurrent|..., so the case statement will fall through to unexpected localDNS apply outcome even on success.
    printf '%s
' "${outcome}"

    case "$(printf '%s
' "${outcome}" | tail -n 1)" in

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:304

  • PR description says updateLocalDNS calls aks-node-controller fetch-localdns-config, but the implementation calls apply-localdns-config. Please update the PR description (or adjust naming/docs) so the end-to-end flow matches what actually runs on nodes.

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

    if ! outcome="$(printf '%s' "${component_payload}" | /opt/azure/containers/aks-node-controller apply-localdns-config \
        --config-file - \
        --output /opt/azure/containers/localdns/livepatched.localdns.corefile)"; then
        echo "localDNS config apply failed"

parts/linux/cloud-init/artifacts/localdns.sh:342

  • fetch-localdns-config is explicitly fail-open and returns exit code 0 even when it can’t fetch/apply config. Because of that, this if ...; then branch is effectively always taken and the "fetch failed" branch is unreachable/misleading. Consider having the command print a machine-readable outcome (like apply-localdns-config does) and branch on that, or adjust the messaging here to reflect that this is only a best-effort invocation (not a success signal).
    # Write the LPS-provided Corefile to the livepatched source file; VNET DNS replacement
    # later derives UPDATED_LOCALDNS_CORE_FILE from this file before CoreDNS starts.
    if "${AKS_NODE_CONTROLLER_BINARY}" fetch-localdns-config --output "${LIVEPATCHED_LOCALDNS_CORE_FILE}"; then
        echo "Completed LocalDNS LPS config fetch."
        return 0
    fi

    echo "LocalDNS LPS config fetch failed; continuing with existing corefile."
    return 0

e2e/scenario_localdns_hosts_test.go:141

  • This test mutates localdns.sh by string replacement, but it doesn’t assert that the replacement actually happened. If the target string changes (even whitespace), this will silently produce an unpatched script and the scenario will fail later in a harder-to-debug way. Add an assertion that content differs from the original.
	content := strings.ReplaceAll(string(data), `AKS_NODE_CONTROLLER_BINARY="/opt/azure/containers/aks-node-controller"`, `AKS_NODE_CONTROLLER_BINARY="`+localDNSFetcherPath+`"`)
	buf.Reset()
	zw = gzip.NewWriter(&buf)

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 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aks-node-controller/localdnsconfig.go:232

  • The gRPC TLS config doesn’t set ServerName, so the client may omit SNI or use the apiserver host by default. That’s likely to break the envoy routing pattern used elsewhere in this repo (check-hotfix pins SNI to lpsSNIHost while dialing the apiserver front), and it also makes the current InsecureSkipVerify comment misleading.

To align with buildLPSHTTPClient in checkhotfix.go (which uses ServerName: lpsSNIHost without InsecureSkipVerify), set ServerName: lpsSNIHost here as well and drop the custom verification if it’s no longer needed.

	tlsConfig := &tls.Config{
		MinVersion:            tls.VersionTLS12,
		RootCAs:               rootCAs,
		NextProtos:            []string{localDNSLPSALPNProto, localDNSALPNH2Proto},
		InsecureSkipVerify:    true, //nolint:gosec // SNI stays on the apiserver FQDN for ALPN routing; chain and hostname are verified below.

parts/linux/cloud-init/artifacts/ubuntu/ubuntu-snapshot-update.sh:304

  • updateLocalDNS currently shells out to aks-node-controller apply-localdns-config and feeds it the knead-dispatched payload via stdin. But apply-localdns-config applies the provided JSON as-is (it does not fetch from LPS), so this only works if the knead payload contains an actual Corefile / localDnsProfile.

This conflicts with the PR description (“LPS is the source of truth; knead payload only triggers the handler”) and will fail-open in the wrong direction if the payload is checksum-only (likely returning noCorefileData and marking the component Failed).

Either (1) switch the handler to invoke fetch-localdns-config (and have it emit an actionable outcome for the handler), or (2) ensure the knead payload includes the LPS LocalDNS config data that apply-localdns-config expects, and update the PR description/tests accordingly.

    if ! outcome="$(printf '%s' "${component_payload}" | /opt/azure/containers/aks-node-controller apply-localdns-config \
        --config-file - \
        --output /opt/azure/containers/localdns/livepatched.localdns.corefile)"; then
        echo "localDNS config apply failed"

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.

3 participants