feat: support localdns hosts plugin refresh override#8895
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring the AKS LocalDNS hosts plugin refresh interval end-to-end (datamodel → template/provisioning env → CSE behavior), replacing reliance on the fixed 15-minute systemd timer cadence when an explicit positive interval is provided.
Changes:
- Extend the AgentBaker datamodel and template func map to carry
hostsPluginRefreshIntervalInSecondsinto provisioning. - Extend aks-node-controller proto + parser to emit
LOCALDNS_HOSTS_PLUGIN_REFRESH_INTERVAL_IN_SECONDSfor scriptless provisioning. - Update Linux CSE (
cse_config.sh) to rewriteaks-localdns-hosts-setup.timerOnUnitActiveSecwhen a valid positive interval is provided, with new/updated tests.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/parts/linux/cloud-init/artifacts/cse_config_spec.sh | Adds ShellSpec coverage for timer interval override behavior. |
| pkg/agent/datamodel/types.go | Adds HostsPluginRefreshIntervalInSeconds to LocalDNSProfile JSON schema. |
| pkg/agent/baker.go | Adds template function to render refresh interval into CSE templates. |
| pkg/agent/baker_test.go | Unit tests for the new template function behavior (nil/invalid/valid). |
| parts/linux/cloud-init/artifacts/cse_config.sh | Applies the provided interval by editing the systemd timer before enabling it. |
| parts/linux/cloud-init/artifacts/cse_cmd.sh | Emits the new refresh interval env var for legacy CSE flow. |
| aks-node-controller/proto/aksnodeconfig/v1/localdns_config.proto | Adds optional refresh interval field to LocalDNS proto. |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go | Regenerates protobuf bindings for the new field. |
| aks-node-controller/parser/parser.go | Exposes the new interval as a CSE env var in scriptless mode. |
| aks-node-controller/parser/parser_test.go | Validates env var emission in integration-style parser tests. |
| aks-node-controller/parser/helper.go | Adds helper to stringify/validate refresh interval from proto config. |
| aks-node-controller/parser/helper_test.go | Unit tests for the new helper’s nil/invalid/valid cases. |
Files not reviewed (1)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/localdns_config.pb.go: Generated file
38e6e82 to
d03db7d
Compare
| ;; | ||
| *) | ||
| if [ "${hosts_plugin_refresh_interval}" -gt 0 ]; then | ||
| sed -i "s/^OnUnitActiveSec=.*/OnUnitActiveSec=${hosts_plugin_refresh_interval}s/" "${hosts_setup_timer}" |
There was a problem hiding this comment.
Seems very hacky. I would think that there would be some form of timer.template and then you render it either on agentbaker side or aks-node-controller side?
There was a problem hiding this comment.
Addressed in commit 8366dcd.
Instead of rewriting the shipped timer unit in place, the refresh override now uses a systemd drop-in:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1824 to 1848 in 8366dcd
The base timer file stays generic/default-only here:
yewmsft
left a comment
There was a problem hiding this comment.
nice, right direction — configurable over the hardcoded 15min, additive optional field, empty preserves default. one blocking thing (AccuracySec, else sub-minute intervals silently don't fire) + a few asks inline.
| ;; | ||
| *) | ||
| if [ "${hosts_plugin_refresh_interval}" -gt 0 ]; then | ||
| sed -i "s/^OnUnitActiveSec=.*/OnUnitActiveSec=${hosts_plugin_refresh_interval}s/" "${hosts_setup_timer}" |
There was a problem hiding this comment.
this only rewrites OnUnitActiveSec, but the shipped aks-localdns-hosts-setup.timer also has AccuracySec=1min. systemd won't fire more accurately than the accuracy window, so OnUnitActiveSec=5s gets coalesced to ~1min — sub-minute cadence won't actually happen. RP (16401188) is passing seconds, so this silently won't deliver. need to drop AccuracySec too:
sed -i "s/^AccuracySec=.*/AccuracySec=1s/" "${hosts_setup_timer}"did you verify on a real node that a small value actually fires at that cadence? i don't think it does as-is.
There was a problem hiding this comment.
Addressed in commit 8366dcd.
The override now uses a systemd drop-in and sets both OnUnitActiveSec and AccuracySec=1s here:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1824 to 1848 in 8366dcd
ShellSpec coverage for the generated override is here:
AgentBaker/spec/parts/linux/cloud-init/artifacts/cse_config_spec.sh
Lines 1507 to 1515 in 8366dcd
I did not run a live-node cadence check in this branch; the validation here was ShellSpec/shellcheck rather than a real node.
| echo "Warning: LOCALDNS_HOSTS_PLUGIN_REFRESH_INTERVAL_IN_SECONDS must be a positive integer, got '${hosts_plugin_refresh_interval}'. Using default timer interval." | ||
| ;; | ||
| *) | ||
| if [ "${hosts_plugin_refresh_interval}" -gt 0 ]; then |
There was a problem hiding this comment.
this is the only bound — RP validates >0, so 1 is legal and would hit live DNS every second on every localdns node. do we want a floor? at least call out a sane min.
There was a problem hiding this comment.
Addressed in commit 8366dcd.
Added a 5s floor here:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1824 to 1862 in 8366dcd
| // getLocalDnsHostsPluginRefreshIntervalInSeconds returns the refresh interval in seconds | ||
| // for the LocalDNS hosts plugin timer. Empty string means use the default timer cadence. | ||
| func getLocalDnsHostsPluginRefreshIntervalInSeconds(config *aksnodeconfigv1.Configuration) string { | ||
| if config == nil || config.GetLocalDnsProfile() == nil { |
There was a problem hiding this comment.
GetLocalDnsProfile() and GetHostsPluginRefreshIntervalInSeconds() are nil-safe on nil receivers. drop this guard and just call the getters:
refresh := config.GetLocalDnsProfile().GetHostsPluginRefreshIntervalInSeconds()
if refresh <= 0 {
return ""
}(baker.go is a plain datamodel struct, not proto — keep the nil checks there.)
There was a problem hiding this comment.
Addressed in commit 8366dcd.
Dropped the explicit nil guard and now rely on the proto getters nil-safe behavior here:
AgentBaker/aks-node-controller/parser/helper.go
Lines 892 to 899 in 8366dcd
| The output should include "systemctlEnableAndStartNoBlock aks-localdns-hosts-setup.timer 30" | ||
| End | ||
|
|
||
| It 'should update the timer refresh interval when provided' |
There was a problem hiding this comment.
only the happy path is covered. the *[!0-9]* warning branch and the empty/0 -> default-preserved path never run in test. add a bad-format case (e.g. "abc") asserting the timer stays 15min, and an unset case. otherwise would this short circuit your failure code path?
There was a problem hiding this comment.
Addressed in commit 8366dcd.
Added coverage for the provided interval, unset/default-restore, invalid-format/default-preserved, and below-min/default-preserved cases here:
AgentBaker/spec/parts/linux/cloud-init/artifacts/cse_config_spec.sh
Lines 1507 to 1545 in 8366dcd
| LOCALDNS_COREFILE_WITH_HOSTS=${LOCALDNS_COREFILE_WITH_HOSTS:-} | ||
| SHOULD_ENABLE_HOSTS_PLUGIN=${SHOULD_ENABLE_HOSTS_PLUGIN:-false} | ||
| LOCALDNS_CRITICAL_FQDNS=${LOCALDNS_CRITICAL_FQDNS:-} | ||
| LOCALDNS_HOSTS_PLUGIN_REFRESH_INTERVAL_IN_SECONDS=${LOCALDNS_HOSTS_PLUGIN_REFRESH_INTERVAL_IN_SECONDS:-} |
There was a problem hiding this comment.
who reads this? the interval only drives the timer rewrite at provision time — the periodic aks-localdns-hosts-setup service doesn't consume it. same for the write at L1799. if nobody reads them, drop both.
There was a problem hiding this comment.
Addressed in commit 8366dcd.
The interval is no longer written into either localdns environment file. The generated localdns env now stops at LOCALDNS_CRITICAL_FQDNS here:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1647 to 1653 in 8366dcd
And the minimal hosts-setup env write is only:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1808 to 1815 in 8366dcd
The interval is only consumed at provisioning time for the timer drop-in here:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1824 to 1848 in 8366dcd
| local hosts_plugin_refresh_interval="${LOCALDNS_HOSTS_PLUGIN_REFRESH_INTERVAL_IN_SECONDS:-}" | ||
| if [ -n "${hosts_plugin_refresh_interval}" ]; then | ||
| case "${hosts_plugin_refresh_interval}" in | ||
| *[!0-9]*) |
There was a problem hiding this comment.
nit: this warning and the -gt 0 else below print the same message — collapse. also the "Refresh every 15 minutes..." comment in aks-localdns-hosts-setup.timer goes stale once you sed the value; make it generic.
There was a problem hiding this comment.
Addressed in commit 8366dcd.
The warning is collapsed to the single default-preserving branch here:
AgentBaker/parts/linux/cloud-init/artifacts/cse_config.sh
Lines 1860 to 1862 in 8366dcd
And the shipped timer comments are generic/default-only here:
What this PR does / why we need it:
This change lets AgentBaker carry a LocalDNS hosts plugin refresh interval from node bootstrapping config down to provisioning, instead of relying on the hardcoded 15 minute timer cadence.
It adds the new LocalDNS field to the datamodel and aks-node-controller proto/parser paths, emits the value into both legacy CSE and scriptless env, and updates
cse_config.shto rewriteaks-localdns-hosts-setup.timerwhen a positive interval is provided. When the field is unset or invalid, provisioning keeps the existing default behavior.This complements the corresponding AKS-RP change in PR 16401188.
Validation run:
make validate-shelldocker run --rm -v /home/sakwa/AgentBaker:/workspace -w /workspace shellspec-docker --shell bash --format d spec/parts/linux/cloud-init/artifacts/cse_config_spec.shgo test ./pkg/agent/...cd aks-node-controller && go test ./parser./hack/tools/bin/golangci-lint run ./pkg/agent/...cd aks-node-controller && ../hack/tools/bin/golangci-lint run ./parser/...Which issue(s) this PR fixes:
Fixes #