Add LocalDNS live patching handler - #9137
Saewon Kwak (saewoni) wants to merge 19 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
GitHub Actions
Inactive
c9c5b3e to
d27b056
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
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-patchingproto 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
| 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
aks-node-controller/localdnsconfig.go:197
- When applying a Corefile update that does not include
corefileVersion/configChecksum(i.e.,desiredVersionis empty), the existing${outputPath}.versionfile is left untouched. If a previous apply wrote a version file, this can leave a stale version on disk that downstream status reporting may read even though the Corefile content has changed.
if update.desiredVersion != "" {
if err := writeLocalDNSCorefileVersion(versionPath, update.desiredVersion); err != nil {
return outcomeLocalDNSConfigFailed, err
}
}
parts/linux/cloud-init/artifacts/localdns.sh:29
- The comment above
LIVEPATCHED_LOCALDNS_CORE_FILEsays it is the Corefile used by the systemd unit, but that’s nowUPDATED_LOCALDNS_CORE_FILE(derived after VNET DNS replacement). This is misleading when debugging which file CoreDNS actually starts with.
# This is the localdns corefile that has updated UpstreamDNSServerIPs and will be used by the localdns systemd unit.
LIVEPATCHED_LOCALDNS_CORE_FILE="${LOCALDNS_SCRIPT_PATH}/livepatched.localdns.corefile"
UPDATED_LOCALDNS_CORE_FILE="${LOCALDNS_SCRIPT_PATH}/updated.localdns.corefile"
| 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' | ||
| ` | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
aks-node-controller/localdnsconfig.go:239
- The LPS connection here doesn’t follow the established check-hotfix pattern (SNI/authority pinned to lpsSNIHost while dialing the apiserver front). As written, gRPC will use the apiserver FQDN as the target/authority and likely send that as SNI, while certificate verification is manually forced to lpsSNIHost under InsecureSkipVerify. This risks routing to the wrong backend and/or handshake failures, and it weakens TLS unnecessarily.
Consider dialing the apiserver FQDN but setting both TLS ServerName and the gRPC target/authority to lpsSNIHost (similar to aks-node-controller/checkhotfix.go:464-501), and drop InsecureSkipVerify.
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.
aks-node-controller/localdnsconfig.go:197
- If the LPS payload applies a new Corefile but doesn’t include corefileVersion/configChecksum, the old ".version" file will be left in place. That can lead to stale live-patching status reporting (localdns.sh reads livepatched.localdns.corefile.version).
Consider removing the version file when applying a Corefile update with an empty desiredVersion.
if update.desiredVersion != "" {
if err := writeLocalDNSCorefileVersion(versionPath, update.desiredVersion); err != nil {
return outcomeLocalDNSConfigFailed, err
}
}
spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.sh:74
- This spec defines updateLocalDNS after including ubuntu-snapshot-update.sh, which overrides the real updateLocalDNS implementation. The new localDNS tests below expect the real handler behavior (calling aks-node-controller and restarting localdns.service), so this override will prevent the tests from exercising the code under test.
updateLocalDNS() {
echo "updateLocalDNS called with args: $*"
return "${TEST_LOCALDNS_STATUS}"
}
| 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 |
There was a problem hiding this comment.
Thanks — this is by design, and the payload contract guarantees the handler receives full config, not just a trigger.
Two distinct paths, intentionally using different subcommands:
- Bootstrap (
localdns.sh, pre-kubelet): callsfetch-localdns-config, which pulls from LPS directly viaGetComponentConfig("localDNS"). LPS is the source of truth here because no API server / dispatched payload exists yet. This is the path the PR description refers to. - Runtime (
ubuntu-snapshot-update.shupdateLocalDNS): the generic knead loop has already dispatched the component payload, so it callsapply-localdns-config --config-file -with that payload rather than re-fetching. Both subcommands share the samefetchAndApplyLocalDNSConfigWithFetchercore;applyjust substitutes a fetcher that returns the dispatched bytes.
On the noCorefileData concern: the dispatched localDNS payload is not a bare trigger. Per the aks-rp contract (ADO PR 16546985) it is pool-keyed and carries the full localDnsProfile (or corefile), e.g. {"agentPools":{"<pool>":{"corefileVersion":"...","localDnsProfile":{"enableLocalDns":true,...}}}}. This is exactly what the E2E exercises (localDNSLPSPayload in scenario_localdns_hosts_test.go) and what Test_LocalDNSLPSBootstrapPatch validates end-to-end. noCorefileData only occurs for a version/checksum-only payload or a payload targeting a different agent pool — neither is what knead dispatches for this node — and in updateLocalDNS that case is treated as a hard error precisely so a malformed dispatch is surfaced rather than silently ignored.
So the flow is consistent: bootstrap reads LPS; runtime applies the LPS-derived payload knead already dispatched. No change needed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
aks-node-controller/localdnsconfig.go:232
- The gRPC TLS client does not set
tls.Config.ServerNametolpsSNIHost. With the current config, SNI will default to the dial target host (fromfqdn), but certificate verification is pinned tolpsSNIHostviaVerifyPeerCertificate. This combination can cause the envoy ALPN/SNI routing to return a different certificate and makefetch-localdns-configfail on real nodes (even though it is fail-open). Align this withcheckhotfix.goby explicitly settingServerName: lpsSNIHostand using normal TLS verification (noInsecureSkipVerify).
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),
}
e2e/scenario_localdns_hosts_test.go:149
mustReadCompressedLocalDNSArtifactgzips the unmodifiedlocaldns.shintobufand then immediately discards it (buf.Reset()), gzipping the modified content again. This extra compression work is unused and makes the helper harder to follow.
This issue also appears on line 151 of the same file.
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())
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)
_, err = zw.Write([]byte(content))
require.NoError(t, err)
require.NoError(t, zw.Close())
return base64.StdEncoding.EncodeToString(buf.Bytes())
}
e2e/scenario_localdns_hosts_test.go:155
Test_LocalDNSLPSBootstrapPatchpolls for"localDNS":{"current":"..."}in thekubernetes.azure.com/live-patching-statusannotation, butlocaldns.shonly writes that legacy "current" field whenLOCALDNS_ENABLE_LEGACY_LIVEPATCH_STATUS=true. The E2E drop-in installed bylocalDNSBranchScriptDropIn()does not set this env var, so the scenario is likely to flake/fail even when the Corefile is applied successfully.
func localDNSBranchScriptDropIn() string {
return `[Service]
ExecStartPre=/bin/bash -c 'base64 -d ` + localDNSBranchScriptArchivePath + ` | gzip -d > /opt/azure/containers/localdns/localdns.sh && chmod 0544 /opt/azure/containers/localdns/localdns.sh'
`
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
parts/linux/cloud-init/artifacts/localdns.sh:342
fetch-localdns-configis explicitly fail-open and returns exit code 0 even on fetch/apply failures (see aks-node-controller implementation). That means thisif ...; then ... else ...will always take the success path and the failure message branch is effectively dead code. Consider calling the command unconditionally and logging a single "finished (fail-open)" message to avoid misleading output.
# 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
spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.sh:27
TEST_LOCALDNS_STATUSis introduced and exported, but the spec later mocksupdateLocalDNS()(see below). That mock prevents the new LocalDNS dispatch tests from exercising the realupdateLocalDNSimplementation (and makes the added assertions about aks-node-controller/systemctl unreachable). Remove the unusedTEST_LOCALDNS_STATUSwiring so the spec can rely on the production handler.
This issue also appears on line 71 of the same file.
TEST_SECURITY_STATUS=0
TEST_LOCALDNS_STATUS=0
TEST_ANNOTATE_STATUS=0
export KUBECTL KNEAD_COMPONENT_STATE_FILE TEST_COMPONENTS_JSON_FILE TEST_KUBECTL_ARGS_FILE
export TEST_STATUS TEST_GOAL TEST_AGENT_POOL TEST_REPO_SERVICE TEST_SECURITY_STATUS TEST_LOCALDNS_STATUS TEST_ANNOTATE_STATUS
spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.sh:74
- This mock
updateLocalDNS()overrides the productionupdateLocalDNSfunction fromubuntu-snapshot-update.sh, so the new LocalDNS tests that expectaks-node-controller apply-localdns-configandsystemctl restart localdns.servicewill never actually exercise those code paths. Remove this mock (or rename it) so the spec can test the real LocalDNS handler behavior.
updateLocalDNS() {
echo "updateLocalDNS called with args: $*"
return "${TEST_LOCALDNS_STATUS}"
}
| refresh_localdns_corefile_from_lps() { | ||
| if [ -z "${LIVEPATCHED_LOCALDNS_CORE_FILE:-}" ]; then | ||
| echo "LIVEPATCHED_LOCALDNS_CORE_FILE is not set or is empty." | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
aks-node-controller/go.mod:34
- The committed relative
replacedirectives (=> ../and=> ../aks-live-patching) make this module non-reproducible outside the AgentBaker repo checkout (e.g.,go test github.com/Azure/agentbaker/aks-node-controller@<sha>or downstreamgo getwill fail because../won’t exist).
Consider using a workspace (go.work) for local development, or depend on a real module version for aks-live-patching so the checked-in go.mod remains buildable from a clean module cache.
replace github.com/Azure/agentbaker => ../
replace github.com/Azure/agentbaker/aks-live-patching => ../aks-live-patching
replace github.com/coreos/ignition/v2 => github.com/flatcar/ignition/v2 v2.0.0-20250903113522-05b8a773288c
e2e/scenario_localdns_hosts_test.go:148
mustReadCompressedLocalDNSArtifactcurrently gzips the original file contents intobuf(lines 137-141), then immediately discards it withbuf.Reset()and gzips the modified contents. The first compression is dead work and makes the function harder to follow.
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(data)
require.NoError(t, err)
require.NoError(t, zw.Close())
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
aks-node-controller/localdnsconfig.go:240
- The gRPC LocalDNS fetch sets
InsecureSkipVerify: trueand manually verifies the peer cert againstlpsSNIHost, but it never setstls.Config.ServerName(SNI) or the gRPC authority tolpsSNIHost. This is inconsistent with the existing LPS access pattern incheckhotfix.go(forced dial to apiserver front while SNI/Host arelpsSNIHost) and can cause the apiserver front/envoy to route the request incorrectly and/or present a certificate that won’t validate againstlpsSNIHost, breakingfetch-localdns-configon real nodes.
host := fqdn
if h, _, splitErr := net.SplitHostPort(fqdn); splitErr == nil {
host = h
}
target := net.JoinHostPort(host, lpsAPIServerPort)
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),
}
ctx, cancel := context.WithTimeout(ctx, lpsFetchTimeout)
defer cancel()
conn, err := grpc.NewClient(
target,
grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)),
)
if err != nil {
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>
Insert e2e CustomDataWriteFiles before the full aks-node-controller launcher line instead of inside the nohup command, and add tests covering the nohup and systemd launcher cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The LocalDNS LPS bootstrap E2E uses CustomDataWriteFiles, which disabled scriptless local-binary compilation and forced the VM to run the VHD baked-in aks-node-controller. That stale parser predates the Corefile reload change, so its provision-config Corefile differed from the baker-generated nbc-cmd Corefile, tripping compareEnvs/ValidateScriptlessPhase3. Add an opt-in ForceScriptlessCompilation scenario flag so the VM runs the locally-built aks-node-controller, making the parser and baker Corefile output consistent. Enable it for Test_LocalDNSLPSBootstrapPatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
localdns.service runs /opt/azure/containers/localdns/localdns.sh, but the E2E drop-in regenerated the branch script at /opt/azure/containers/localdns.sh (wrong directory). The service therefore ran the VHD baked-in localdns.sh, which never invoked the LPS fetcher, so the fetcher stamp was never created. Write the branch script to the correct localdns/ subdirectory path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fake fetcher wrapper execd /opt/azure/containers/aks-node-controller (the VHD baked-in binary) for apply-localdns-config. That older binary predates the --config-file flag, so apply failed with "flag provided but not defined: -config-file" and the LPS fetch fell open, leaving the corefile .version file unwritten. Prefer the locally-compiled aks-node-controller-hotfix binary (provided by ForceScriptlessCompilation) so apply-localdns-config matches the branch code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
localdns.sh only writes the live-patching-status node annotation when LOCALDNS_ENABLE_LEGACY_LIVEPATCH_STATUS=true; otherwise the generic knead live-patching loop owns it. knead does not drive the localDNS component in this E2E, so the annotation was never written and the validator poll timed out. Set the env var via the localdns.service drop-in so the bootstrap writer path runs and stamps components.localDNS.current. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed provisioning/runtime issues (udev rule matching bug for node-exporter MANA handling and a cse_main.sh exit-path that can fail with a non-numeric exit code) that should be fixed before merge.
Review details
- Files reviewed: 32/33 changed files
- Comments generated: 0 new
- Review effort level: Lite
The knead reconciler (ubuntu-snapshot-update.sh) is shared with the securityPatch component from the stacked security-patching work. LocalDNS does not depend on security-update.sh, its VHD baking, or its cloud-init delivery, so drop them from this PR: - Do not bake security-update.sh into the VHD (revert vhdbuilder/packer/*). - Do not deliver security-update.sh via nodecustomdata; keep only the generic reconciler that hosts the dispatched localDNS handler. - Remove securityUpdateScript wiring from const.go / variables.go. - Drop security-update.sh and its security/service specs from this PR. - Source security-update.sh only when present so the reconciler still runs for localDNS on nodes without the security-update handler baked in. The securityPatch dispatch case is retained in the engine (and its spec is unchanged) to minimize divergence from the security-patching branch.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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. |
There was a problem hiding this comment.
🟡 Changes recommended
The Ubuntu knead reconciler currently treats securityPatch as supported without shipping its handler functions, and the new E2E _test.go files in e2e/ do not build due to mixed package/layout issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 3
- Review effort level: Lite
| package e2e | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "compress/gzip" | ||
| "context" | ||
| "encoding/base64" | ||
| "os" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
| package e2e | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" |
| # Select the handler for this component; unsupported components are ignored. | ||
| case "${component}" in | ||
| securityPatch) | ||
| component_comparator=securityPatchIsCurrent | ||
| component_handler=updateSecurityPatch | ||
| ;; | ||
| localDNS) | ||
| component_comparator=localDNSIsCurrent | ||
| component_handler=updateLocalDNS | ||
| ;; | ||
| *) | ||
| echo "unsupported component: ${component}" | ||
| component_index=$((component_index + 1)) | ||
| continue | ||
| ;; | ||
| esac | ||
|
|
AgentBaker Linux gate detectiveRun: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=180557218 Detective summaryThe E2E job failed before scenario execution during Go package setup, so no �2e/report.xml was produced. The first failing step was go test setup for github.com/Azure/agentbaker/e2e. Likely cause / signature�2e-root-scenario-package-mismatch: Go found mixed packages in �2e/ — main from main.go and �2e from scenario_localdns_hosts_test.go — causing FAIL github.com/Azure/agentbaker/e2e [setup failed]. Timeline, E2E task log, missing test-results warning, build metadata, and PR file list all corroborate this as a PR-change-caused test-code/package layout issue. ConfidenceHigh: PR 9137 adds �2e/scenario_localdns_hosts_test.go, and the fatal setup error names that exact file in the package mismatch. Recommended owner/actionPR author should move the LocalDNS scenario test into the existing scenario package location or change its package declaration/layout to match the �2e root package expectations before re-running the gate. Strongest alternativeTransient Go module/toolchain download failure is the strongest alternative because this job downloads Go and modules, but it is less likely because dependency download completed far enough for the deterministic package mismatch diagnostic. Evidence links |
Summary
Wires LocalDNS into the generic knead live-patching loop so LocalDNS Corefile config can be delivered via LPS (Live Patching Service).
End-to-end flow:
Scope / relationship to the knead security-patching work
This PR previously stacked on #8952 (
feature/knead-security-patching). It has been rebased directly ontomainand is now self-contained:ubuntu-snapshot-update.sh) that hosts the dispatchedlocalDNShandler.aks-live-patchingclient,checkhotfix.gohelpers,errLPSUnavailable) is already inmain(via feat(anc): add check-hotfix subcommand to read hotfix pointer from LPS #8696).securityPatchcomponent,security-update.sh, its VHD baking, or its cloud-init delivery — LocalDNS does not depend on them.securityPatchdispatch case is retained in the engine (and its spec is unchanged) to minimize divergence from the security-patching branch;security-update.shis sourced only when present so the reconciler still runs forlocalDNSon nodes without the security-update handler baked in.Important behavior
localdns.corefileremains the baked/CSE-generated fallback source.livepatched.localdns.corefilestores the LPS-provided desired source.updated.localdns.corefileremains the runtime-generated Corefile that CoreDNS starts with after VNet DNS replacement.localdns.shfetch-localdns-config): a benign LPS "unavailable" response (NotFound/PermissionDenied/Unauthenticated) is intentionally fail-open/no-op so nodes keep existing LocalDNS config when no LPS config is published yet.ubuntu-snapshot-update.shupdateLocalDNS→apply-localdns-config): the payload is supplied inline by knead, sonotFoundis not expected and is treated as a failure to avoid checkpointing an unapplied config as succeeded.noCorefileDatais treated as failure because LPS returned an unusable LocalDNS payload.Validation
go test . ./common ./helpers ./parser ./pkg/nodeconfigutils ./utilsinaks-node-controllergo test ./pkg/agent/...make shellspec(shellspec runs in the repo's Docker image) for:spec/parts/linux/cloud-init/artifacts/ubuntu-snapshot-update_spec.shspec/parts/linux/cloud-init/artifacts/localdns_spec.shE2E
Two scenarios run end-to-end on live nodes: the happy path (
Test_LocalDNSLPSBootstrapPatch) and the LPS-unavailable fallback (Test_LocalDNSLPSUnavailableFallback).livepatched.localdns.corefile+ its.version,localdns.shderivesupdated.localdns.corefile, CoreDNS serves from it, and the node is annotated withkubernetes.azure.com/live-patching-status("localDNS":{"current":...}).localDNSlive-patching version is reported.Additional failure modes (empty/version-only payload, wrong-pool payload, apply failure, unavailable-status mapping) are covered by Go unit tests (
aks-node-controller/localdnsconfig_test.go) and shellspec (ubuntu-snapshot-update_spec.sh).