-
Notifications
You must be signed in to change notification settings - Fork 245
feat: scriptless mode in NBC so we can move in phases #7805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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 implements Phase 1 of transitioning to scriptless node bootstrapping by moving LocalDNS configuration from CustomData to CSE environment variables, while maintaining backward compatibility during the migration period.
Changes:
- LocalDNS configuration moved from cloud-init CustomData to CSE environment variables (SHOULD_ENABLE_LOCALDNS, LOCALDNS_CPU_LIMIT, LOCALDNS_MEMORY_LIMIT, LOCALDNS_GENERATED_COREFILE)
- Refactored LocalDNS enablement logic to support both legacy (corefile in VHD) and new (corefile generated at provision time) deployment modes
- Modified GenerateLocalDNSCoreFile to return empty string instead of error when LocalDNS is disabled, allowing graceful handling
- E2E tests updated to use minimal customData placeholder while CSE variables carry the configuration
Reviewed changes
Copilot reviewed 35 out of 133 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/agent/testdata/*/CSECommand | Snapshot tests regenerated with new SHOULD_ENABLE_LOCALDNS, LOCALDNS_CPU_LIMIT, LOCALDNS_MEMORY_LIMIT, LOCALDNS_GENERATED_COREFILE environment variables |
| pkg/agent/baker_test.go | Removed tests expecting errors when LocalDNSProfile is nil or disabled (now returns empty string) |
| pkg/agent/baker.go | Changed GenerateLocalDNSCoreFile to return ("", nil) instead of errors when LocalDNS disabled |
| parts/linux/cloud-init/artifacts/cse_main.sh | Simplified LocalDNS enablement to single conditional path |
| parts/linux/cloud-init/artifacts/cse_config.sh | Refactored enableLocalDNS functions: extracted generateLocalDNSFiles, unified enableLocalDNS logic to handle both legacy and new modes |
| parts/linux/cloud-init/artifacts/cse_cmd.sh | Added 4 new LocalDNS-related environment variables to CSE command |
| e2e/vmss.go | Changed customData to minimal placeholder (base64 "#cloud-config\n") for scriptless testing |
e1f4978 to
6c50700
Compare
6c50700 to
65d3eee
Compare
There was a problem hiding this comment.
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 35 out of 133 changed files in this pull request and generated 2 comments.
| mkdir -p "$(dirname "${LOCALDNS_CORE_FILE}")" | ||
| touch "${LOCALDNS_CORE_FILE}" | ||
| chmod 0644 "${LOCALDNS_CORE_FILE}" | ||
| echo "${LOCALDNS_GENERATED_COREFILE}" | base64 -d > "${LOCALDNS_CORE_FILE}" || exit $ERR_LOCALDNS_FAIL |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOCALDNS_GENERATED_COREFILE is produced by GenerateLocalDNSCoreFile via getBase64EncodedGzippedCustomScriptFromStr (base64 of gzip content). Here it's only base64-decoded, so the on-disk corefile will be gzip-compressed bytes and localdns.sh/systemd will treat it as an invalid Corefile. Decode + gunzip (or change the generator to return plain base64 text for this path) so /opt/azure/containers/localdns/localdns.corefile is written as plain text.
| echo "${LOCALDNS_GENERATED_COREFILE}" | base64 -d > "${LOCALDNS_CORE_FILE}" || exit $ERR_LOCALDNS_FAIL | |
| echo "${LOCALDNS_GENERATED_COREFILE}" | base64 -d | gunzip > "${LOCALDNS_CORE_FILE}" || exit $ERR_LOCALDNS_FAIL |
| nodeBootstrapping, err = ab.GetNodeBootstrapping(ctx, s.Runtime.NBC) | ||
| require.NoError(s.T, err) | ||
| cse = nodeBootstrapping.CSE | ||
| customData = nodeBootstrapping.CustomData | ||
| // #cloud-config\n | ||
| customData = "I2Nsb3VkLWNvbmZpZwoK" | ||
| } |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the NBC (BootstrapConfigMutator) path, this replaces the AgentBaker-provided nodeBootstrapping.CustomData with a hardcoded "#cloud-config" payload. Unless the VMSS model is guaranteed to no longer rely on CustomData to write required files/units, this will drop critical provisioning inputs and can make E2E scenarios fail in non-scriptless/older-VHD cases. Prefer using nodeBootstrapping.CustomData (or gate this change behind an explicit scriptless flag and ensure required artifacts are present on the image).
65d3eee to
e379183
Compare
What this PR does / why we need it:
Switch to using scriptless but with CSE for Phase 1
Which issue(s) this PR fixes:
Fixes #