Validate kubelet configuration and nspawn LocalDNS integration - #575
Draft
hbc (bcho) wants to merge 37 commits into
Draft
Validate kubelet configuration and nspawn LocalDNS integration#575hbc (bcho) wants to merge 37 commits into
hbc (bcho) wants to merge 37 commits into
Conversation
# Conflicts: # hack/agent/e2e-kind/e2e.py # pkg/agent/config/config.go # pkg/agent/goalstates/resolve.go # pkg/agent/phases/nodestart/assets/kubelet.service # pkg/agent/phases/nodestart/kubelet.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR integrates two related agent features for systemd-nspawn workers: (1) generating a kubelet KubeletConfiguration YAML from an agent baseline plus user overlay, and (2) adding an optional machine-local CoreDNS (“LocalDNS”) service with host-network NOTRACK support, plus validation paths across online/offline artifact flows.
Changes:
- Add LocalDNS goal-state, config validation, rootfs provisioning, node-start orchestration, reset cleanup, and E2E validation for the nspawn-local CoreDNS cache.
- Switch kubelet systemd to use a generated
--config=/var/lib/kubelet/config.yaml, and add support for a JSON-shaped overlay + image credential provider settings. - Extend offline/online artifact tooling and CRDs/APIs to include CoreDNS artifacts and LocalDNS configuration wiring.
Reviewed changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/agent/phases/rootfs/resolved.go | When LocalDNS is enabled, rewrites the machine resolver to point at the node listener. |
| pkg/agent/phases/rootfs/provision.go | Adds LocalDNS provisioning step to the rootfs pipeline. |
| pkg/agent/phases/rootfs/preflight_remote.go | Adds reachability preflight for the CoreDNS artifact source. |
| pkg/agent/phases/rootfs/preflight_goal_state.go | Conditionally includes LocalDNS artifact preflight when enabled. |
| pkg/agent/phases/rootfs/localdns.go | Implements CoreDNS installation, unit wiring, and required-plugin verification in the rootfs. |
| pkg/agent/phases/rootfs/localdns_test.go | Unit test for LocalDNS rootfs provisioning behavior. |
| pkg/agent/phases/rootfs/assets/localdns.slice | Adds the LocalDNS systemd slice template for resource limits. |
| pkg/agent/phases/rootfs/assets/localdns.service | Adds the LocalDNS systemd service unit. |
| pkg/agent/phases/rootfs/assets/localdns-supervisor.sh | Adds a supervisor script that manages readiness + watchdog behavior for CoreDNS. |
| pkg/agent/phases/reset/tasks_test.go | Updates reset task naming expectations to include LocalDNS cleanup. |
| pkg/agent/phases/reset/network.go | Adds LocalDNS host-network cleanup (NOTRACK rules, interface, unit/script removal). |
| pkg/agent/phases/nodestart/start.go | Inserts LocalDNS configure/network setup/wait tasks into node start ordering. |
| pkg/agent/phases/nodestart/localdns.go | Adds node-start LocalDNS reconciliation (Corefile/upstreams/env/slice, host network unit, readiness wait). |
| pkg/agent/phases/nodestart/kubelet.go | Writes kubelet configuration YAML and updates unit rendering inputs. |
| pkg/agent/phases/nodestart/kubelet_test.go | Adds kubelet configuration YAML test and adjusts flag expectations. |
| pkg/agent/phases/nodestart/assets/unbounded-localdns-network.sh | Adds host-side script to create dummy iface and reconcile NOTRACK rules. |
| pkg/agent/phases/nodestart/assets/unbounded-localdns-network.service | Adds host systemd oneshot unit to run the network reconciliation. |
| pkg/agent/phases/nodestart/assets/kubelet.service | Switches kubelet ExecStart to reference the generated --config file. |
| pkg/agent/phases/nodestart/assets/20-node-config.conf | Moves clusterDNS/clientCA config out of flags; adds image credential provider flags. |
| pkg/agent/phases/nodestart/assets/10-kubeconfig.conf | Removes --rotate-certificates flag from kubeconfig args (now set in config file). |
| pkg/agent/phases/host/preflight_host.go | Adds a LocalDNS conntrack capability preflight (iptables raw NOTRACK support). |
| pkg/agent/phases/host/os.go | Adds iptables to the required host packages lists. |
| pkg/agent/goalstates/rootfs.go | Adds LocalDNS to RootFS goal state. |
| pkg/agent/goalstates/resolve.go | Resolves LocalDNS; wires kubelet ClusterDNS/ResolvConf to LocalDNS when enabled; validates kubelet config. |
| pkg/agent/goalstates/resolve_test.go | Adds tests for kubelet overlay + image credential provider and rejection of unsupported config. |
| pkg/agent/goalstates/offline_artifacts.go | Requires CoreDNS version in offline manifest when LocalDNS is enabled; adds CoreDNS download override from bundle. |
| pkg/agent/goalstates/offline_artifacts_test.go | Adds coverage for offline CoreDNS requirements and override rendering. |
| pkg/agent/goalstates/nodestart.go | Adds LocalDNS to NodeStart goal state. |
| pkg/agent/goalstates/localdns.go | Implements LocalDNS goal-state resolution: upstream discovery, Corefile rendering, plugin normalization, metrics address resolution. |
| pkg/agent/goalstates/localdns_test.go | Adds tests for upstream parsing and Corefile rendering. |
| pkg/agent/goalstates/kubelet.go | Extends kubelet goal state with ResolvConf, config overlay, and image credential provider. |
| pkg/agent/goalstates/downloads.go | Adds CoreDNS to download overrides. |
| pkg/agent/goalstates/constants.go | Adds kubelet config path constant and LocalDNS resolv.conf path constant. |
| pkg/agent/config/localdns.go | Adds LocalDNS agent config schema, validation, and deepcopy. |
| pkg/agent/config/localdns_test.go | Adds unit tests for LocalDNS config validation and deepcopy. |
| pkg/agent/config/config.go | Adds LocalDNS to AgentConfig; adds kubelet Configuration + ImageCredentialProvider; validation + deepcopy support. |
| pkg/agent/config/config_test.go | Updates deepcopy test approach; adds tests for kubelet overlay validation and image credential provider validation. |
| pkg/agent/bootstrapartifacts/paths.go | Adds CoreDNS artifact path helper and required-path inclusion. |
| pkg/agent/bootstrapartifacts/manifest.go | Adds CoreDNS version to bootstrap manifest versions and normalization. |
| internal/provision/agent_config.go | Plumbs LocalDNS + CoreDNS download overrides from Machine specs into agent config/goal state. |
| internal/executil/exec.go | Adds command factories for iptables and curl. |
| internal/agentartifacts/artifacts.go | Adds CoreDNS default base URL and URL construction helper. |
| internal/agentartifacts/artifacts_test.go | Adds tests for CoreDNS URL generation behavior. |
| hack/cmd/agent-artifacts-builder/artifacts/artifacts.go | Adds CoreDNS to offline artifact planning; supports extracting a single file from a tar.gz download. |
| hack/cmd/agent-artifacts-builder/artifacts/artifacts_test.go | Updates plan tests to include CoreDNS artifact behavior and extraction metadata. |
| hack/agent/e2e-kind/node-configs/README.md | Documents kubelet configuration overlay and LocalDNS scenario switches. |
| hack/agent/e2e-kind/node-configs/localdns.json | Adds an E2E scenario enabling LocalDNS. |
| hack/agent/e2e-kind/node-configs/kubelet-configuration.json | Adds an E2E scenario validating kubelet overlay behavior. |
| hack/agent/e2e-kind/e2e.py | Adds kubelet configuration injection/validation and LocalDNS end-to-end validation + cleanup checks. |
| docs/content/reference/agent/configuration.md | Updates agent config reference for new Kubelet and LocalDNS fields. |
| designs/agent-nspawn-localdns.md | Adds a design doc describing the LocalDNS feature, contracts, and lifecycle behavior. |
| deploy/machina/crd/unbounded-cloud.io_machines.yaml | Extends CRD schema for CoreDNS download override and LocalDNS config. |
| deploy/machina/crd/unbounded-cloud.io_machineconfigurationversions.yaml | Extends MachineConfigurationVersion CRD schema with LocalDNS config. |
| deploy/machina/crd/unbounded-cloud.io_machineconfigurations.yaml | Extends MachineConfiguration CRD schema with LocalDNS config. |
| cmd/kubectl-unbounded/app/machine_manual_bootstrap.go | Adds --local-dns flag wiring to set Machine spec LocalDNS enabled. |
| cmd/agent/internal/daemon/nodeoperator.go | Treats kubelet configuration overlay, image credential provider, and LocalDNS as drift-relevant. |
| cmd/agent/internal/daemon/nodeoperator_test.go | Adds drift tests for kubelet configuration and image credential provider changes. |
| cmd/agent/internal/daemon/controller_node.go | Applies LocalDNS from MachineConfiguration template into agent config. |
| api/machina/v1alpha3/zz_generated.deepcopy.go | Updates generated deepcopy for new LocalDNS/CoreDNS fields. |
| api/machina/v1alpha3/machineconfiguration_types.go | Adds LocalDNS to MachineConfigurationAgent API type. |
| api/machina/v1alpha3/machine_types.go | Adds LocalDNS and CoreDNS download overrides to Machine agent spec APIs. |
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+6
| [Unit] | ||
| Description=Unbounded LocalDNS resource limits | ||
|
|
||
| [Slice] | ||
| CPUQuota={{.CPUQuota}} | ||
| MemoryMax={{.MemoryMax}}M |
Comment on lines
+64
to
+72
| for _, line := range strings.Split(string(hostResolvConf), "\n") { | ||
| if strings.HasPrefix(strings.TrimSpace(line), "nameserver ") { | ||
| continue | ||
| } | ||
|
|
||
| if line != "" { | ||
| lines = append(lines, line) | ||
| } | ||
| } |
Comment on lines
+221
to
+225
| for _, line := range strings.Split(string(original), "\n") { | ||
| if line != "" && !strings.HasPrefix(strings.TrimSpace(line), "nameserver ") { | ||
| lines = append(lines, line) | ||
| } | ||
| } |
Comment on lines
+65
to
+69
| for _, line := range strings.Split(string(c.goalState.LocalDNS.OriginalHostResolvConf), "\n") { | ||
| if line != "" && !strings.HasPrefix(strings.TrimSpace(line), "nameserver ") { | ||
| resolverLines = append(resolverLines, line) | ||
| } | ||
| } |
Comment on lines
+97
to
+100
| args := []string{"-w", "-t", "raw", "-m", "comment", "--comment", "unbounded-localdns: skip conntrack", "-D", values["-A"], "-p", values["-p"], "-d", values["-d"], "--dport", "53", "-j", "NOTRACK"} | ||
| if err := executil.RunCmd(ctx, t.log, executil.Iptables(), args...); err != nil { | ||
| return fmt.Errorf("remove LocalDNS NOTRACK rule: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
pkg/agent/phases/rootfs/assets/localdns.slice:5
- systemd CPUQuota values are percentages; without a trailing "%" this unit may fail to parse or behave unexpectedly.
pkg/agent/phases/nodestart/kubelet_test.go:141 - This test uses filepath.Join(machineDir, goalstates.KubeletConfigurationPath), but KubeletConfigurationPath is absolute. On Unix, filepath.Join will ignore machineDir, so the test will read from /var/lib/kubelet/config.yaml on the host instead of the temp machine dir.
pkg/agent/phases/rootfs/localdns.go:179 - CoreDNS archive extraction currently skips any tar entry whose cleaned name contains a slash. That will fail if the release .tgz stores the binary under a directory (common for many archives), even though utilio.DecompressTarGz already cleans/validates entry names against path traversal.
| return fmt.Errorf("marshal KubeletConfiguration: %w", err) | ||
| } | ||
|
|
||
| dest := filepath.Join(c.goalState.MachineDir, goalstates.KubeletConfigurationPath) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
pkg/agent/phases/rootfs/assets/localdns.slice:5
- CPUQuota in a systemd slice expects a percentage value (e.g. "200%"), but the template currently renders a bare number. This will make systemd reject or misinterpret the quota, so LocalDNS resource limiting can silently fail.
pkg/agent/phases/nodestart/kubelet.go:138 - filepath.Join(machineDir, goalstates.KubeletConfigurationPath) drops MachineDir because KubeletConfigurationPath is absolute (starts with "/"). This causes the agent to write the kubelet config into the host filesystem (e.g. /var/lib/kubelet/config.yaml) instead of the nspawn rootfs under MachineDir.
pkg/agent/phases/nodestart/kubelet.go:246 - The kubelet drop-in destination path uses filepath.Join(MachineDir, goalstates.KubeletServiceDropInDir, ...), but KubeletServiceDropInDir is absolute. This drops MachineDir and writes drop-ins into the host filesystem rather than the nspawn rootfs, so kubelet inside the machine won't pick up the generated args.
pkg/agent/phases/rootfs/localdns.go:103 - Using template.Must() here can panic the agent process (and bypass the returned error path) if the embedded template ever becomes invalid (merge conflict, corruption, etc.). Since this function already returns an error, parse the template normally and surface a wrapped error instead of panicking.
Comment on lines
201
to
205
| if err := assetsTemplate.ExecuteTemplate(buf, "kubelet.service", map[string]any{ | ||
| "KubeletBinPath": spec.KubeletBinPath, | ||
| "KubeletBinPath": spec.KubeletBinPath, | ||
| "KubeletConfigurationPath": goalstates.KubeletConfigurationPath, | ||
| }); err != nil { | ||
| return err |
# Conflicts: # pkg/agent/config/config.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 67 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
pkg/agent/phases/rootfs/assets/localdns.slice:6
- CPUQuota in systemd slice units expects a percentage value (e.g. "200%"). This template omits the '%' suffix, unlike the nodestart localdns.slice, so the rendered unit is likely invalid or interpreted unexpectedly.
pkg/agent/phases/nodestart/kubelet.go:140 - filepath.Join(MachineDir, goalstates.KubeletConfigurationPath) will ignore MachineDir because KubeletConfigurationPath is absolute ("/var/lib/..."), causing the config to be written to the host filesystem instead of the machine rootfs (and tests to read/write the wrong location).
pkg/agent/phases/rootfs/resolved.go:75 - The LocalDNS resolv.conf rewrite only removes lines that start with "nameserver " (single space) after trimming; it will miss valid forms like "nameserver\t1.1.1.1" and can leave upstream nameservers in place. It also doesn’t drop whitespace-only lines. This can result in multiple nameservers even when LocalDNS is enabled.
…localdns # Conflicts: # designs/agent-nspawn-localdns.md
…nto hbc/alpha-version
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 67 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
pkg/agent/phases/rootfs/resolved.go:68
- When LocalDNS is enabled, this filter only drops lines that start with the literal prefix "nameserver " (space). That misses valid resolv.conf formats like
nameserver\t1.2.3.4or multiple spaces, so the machine resolv.conf can end up with both upstream and LocalDNS nameservers.
pkg/agent/phases/rootfs/assets/localdns.slice:6 - CPUQuota in systemd slice units expects a percentage (e.g. "200%"). This template omits the trailing '%', while the nodestart variant includes it, so the generated slice may be rejected or misinterpreted by systemd.
pkg/agent/config/config.go:432 - The agent always forces kubelet
resolvConfwhen rendering config.yaml (from goal state / LocalDNS), so allowing users to setKubelet.Configuration.resolvConfwould be silently ignored. Consider rejecting this key as agent-owned to avoid surprising behavior.
for _, field := range []string{
"apiVersion",
"kind",
"authentication",
"clusterDNS",
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 67 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/machina/v1alpha3/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
pkg/agent/phases/nodestart/kubelet.go:140
- filepath.Join ignores the machine root when the second argument is absolute. With goalstates.KubeletConfigurationPath starting with "/", this writes to the host "/var/lib/kubelet/config.yaml" instead of into the machine rootfs under MachineDir.
pkg/agent/phases/rootfs/assets/localdns.slice:5 - This slice template renders CPUQuota without a percent sign, but the nodestart version includes "%". Keeping these consistent avoids generating a CPUQuota value that systemd may parse differently or reject.
pkg/agent/phases/rootfs/resolved.go:72 - The nameserver stripping logic only matches lines beginning with "nameserver " (space). Valid resolv.conf entries like "nameserver\t1.1.1.1" or multiple spaces will not be removed, so the machine may still use host nameservers instead of LocalDNS.
pkg/agent/phases/nodestart/kubelet_test.go:144 - This test reads the kubelet config using filepath.Join(machineDir, goalstates.KubeletConfigurationPath), but because KubeletConfigurationPath is absolute that join discards machineDir. It will read from the host path instead of the temp machineDir (and can fail depending on the host filesystem).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This draft PR is for validation of the combined kubelet configuration override and nspawn LocalDNS changes. It is not ready for final review or merge.
Validation
hbc/kubelet-flagsandfeature/agent-nspawn-localdnsNote
make fmtapplied formatting, but its lint phase could not complete in the local environment because the OpenSSL development headeropenssl/aes.his unavailable.