From 364cb10b808fa152bea996a78adb74837af7979d Mon Sep 17 00:00:00 2001 From: Philip Lombardi <893096+plombardi89@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:50:01 -0400 Subject: [PATCH 1/4] agent: add the host installation prefix and resolve paths from it The agent writes its own host-side files to hard-coded paths under /usr/local: the daemon binaries and their blue-green links, the nspawn lifecycle helper, the daemon recovery script, and the LocalDNS network helper. On a host with a read-only /usr none of those writes can succeed, so the agent cannot be installed at all. Add AgentConfig.HostPrefix and a resolver that derives the host-side layout from it. Paths inside the nspawn machine are untouched: they are relative and joined with the machine directory, and conflating the two would break every host. The prefix is declared, never inferred. Where the agent may write is a property of the filesystem, not of the distribution, so keying on distro identity would misclassify a hardened host with a read-only /usr and would silently relocate files on any host whose os-release changed. A wrong guess is expensive to recover from, because the lifecycle helper path is baked as an absolute path into the nspawn drop-in and the config regeneration unit. The accepted syntax is narrow on purpose. The prefix is interpolated into generated systemd units and into a shell script, neither of which quotes it, so rather than adding two kinds of escaping that every consumer must keep correct, the value is constrained to be inert in both. Teardown and existing-deployment detection need to sweep both the configured prefix and the default, so that changing the prefix cannot orphan files or let a dirty host be silently reprovisioned; KnownHostPrefixes and MergeHostPrefixes exist for that and are used by the callers that follow. Nothing consumes the resolver yet. This is the model and its validation, so the changes that convert each caller can be read on their own. Hosts that do not set a prefix resolve to exactly the paths they had before, pinned by a regression test against the existing constants. --- pkg/agent/config/config.go | 75 +++++++++++ pkg/agent/config/config_test.go | 69 ++++++++++ pkg/agent/goalstates/hostpaths.go | 172 +++++++++++++++++++++++++ pkg/agent/goalstates/hostpaths_test.go | 116 +++++++++++++++++ 4 files changed, 432 insertions(+) create mode 100644 pkg/agent/goalstates/hostpaths.go create mode 100644 pkg/agent/goalstates/hostpaths_test.go diff --git a/pkg/agent/config/config.go b/pkg/agent/config/config.go index c8900082a..9f4602c8d 100644 --- a/pkg/agent/config/config.go +++ b/pkg/agent/config/config.go @@ -77,6 +77,18 @@ type AgentConfig struct { // Empty remains unobserved for legacy installations; it is not inferred from // the host distribution. The daemon reports explicit values in Machine status. ProvisioningFormat string `json:"ProvisioningFormat,omitempty"` + + // HostPrefix is the installation prefix for the agent's own host-side + // files: the daemon binaries under /bin and helper scripts + // under /libexec. It does not affect paths inside the nspawn + // machine, which are always relative to the machine directory. + // + // Empty means /usr/local, so hosts that do not set it are unaffected. Hosts + // with a read-only /usr must set it to a writable prefix; the agent refuses + // to bootstrap rather than guessing one, because where the agent may write + // is a property of the filesystem and not something that can be safely + // inferred from the distribution. + HostPrefix string `json:"HostPrefix,omitempty"` } const ( @@ -94,6 +106,65 @@ func ValidateProvisioningFormat(format string) error { } } +// hostPrefixAllowedRune reports whether r may appear in a host installation +// prefix. +// +// The prefix is interpolated into generated systemd units and into a shell +// script, neither of which quotes it. Rather than adding two kinds of escaping +// and having to keep them correct in every consumer, the accepted syntax is +// narrow enough that the value is inert in both contexts: no whitespace, no +// quoting or substitution characters, and no systemd "%" specifiers. +func hostPrefixAllowedRune(r rune) bool { + switch { + case r >= 'a' && r <= 'z': + return true + case r >= 'A' && r <= 'Z': + return true + case r >= '0' && r <= '9': + return true + case r == '/' || r == '.' || r == '_' || r == '-': + return true + default: + return false + } +} + +// ValidateHostPrefix checks that a configured host installation prefix is an +// absolute, normalized path that can hold a bin and libexec directory, and that +// it is safe to interpolate into the assets generated from it. An empty prefix +// is valid and selects the default. +func ValidateHostPrefix(prefix string) error { + trimmed := strings.TrimSpace(prefix) + if trimmed == "" { + return nil + } + + if !filepath.IsAbs(trimmed) { + return fmt.Errorf("HostPrefix must be an absolute path") + } + + if cleaned := filepath.Clean(trimmed); cleaned != trimmed { + return fmt.Errorf("HostPrefix must be a normalized path, for example %s", cleaned) + } + + if trimmed == "/" { + return fmt.Errorf("HostPrefix must not be the filesystem root") + } + + // Report the offending character rather than only the rule, because the + // caller cannot otherwise tell which byte of a long path was rejected. + for _, r := range trimmed { + if !hostPrefixAllowedRune(r) { + return fmt.Errorf( + "HostPrefix may only contain letters, digits, '/', '.', '_' and '-', but contains %q", + r, + ) + } + } + + return nil +} + // AgentOfflineArtifacts configures a complete offline source for binaries the // agent installs into the nspawn rootfs. type AgentOfflineArtifacts struct { @@ -250,6 +321,10 @@ func (a *AgentConfig) Validate() error { errs = append(errs, err) } + if err := ValidateHostPrefix(a.HostPrefix); err != nil { + errs = append(errs, err) + } + apiServer := strings.TrimSpace(a.Kubelet.ApiServer) if apiServer == "" { errs = append(errs, fmt.Errorf("Kubelet.ApiServer is required")) diff --git a/pkg/agent/config/config_test.go b/pkg/agent/config/config_test.go index e1709bbe1..efecca055 100644 --- a/pkg/agent/config/config_test.go +++ b/pkg/agent/config/config_test.go @@ -582,3 +582,72 @@ func TestAgentConfig_BackfillNodeName_UsesHostHostname(t *testing.T) { assert.Equal(t, want, cfg.NodeName) } + +// TestValidateHostPrefix pins what may be configured as an installation prefix. +// The value is interpolated into generated systemd units and into a shell +// script, neither of which quotes it, so the accepted syntax is deliberately +// narrow enough to be inert in both rather than requiring two kinds of +// escaping that every consumer would have to keep correct. +func TestValidateHostPrefix(t *testing.T) { + t.Parallel() + + for _, prefix := range []string{ + "", + "/usr/local", + "/opt/unbounded", + "/var/lib/unbounded-agent", + "/opt/Unbounded_1.0-rc.2", + } { + if err := ValidateHostPrefix(prefix); err != nil { + t.Errorf("ValidateHostPrefix(%q) = %v, want nil", prefix, err) + } + } + + for _, tc := range []struct{ prefix, reason string }{ + {"usr/local", "relative"}, + {"./opt", "relative"}, + {"/opt/", "trailing separator is not normalized"}, + {"/opt/../opt", "unnormalized"}, + {"/", "filesystem root"}, + {"/opt/un bounded", "whitespace"}, + {"/opt/$HOME", "shell substitution"}, + {"/opt/%i", "systemd specifier"}, + {"/opt/un;rm -rf /", "shell metacharacter"}, + {"/opt/\"quoted\"", "quoting"}, + {"/opt/un`cmd`", "command substitution"}, + } { + if err := ValidateHostPrefix(tc.prefix); err == nil { + t.Errorf("ValidateHostPrefix(%q) = nil, want an error (%s)", tc.prefix, tc.reason) + } + } +} + +// TestValidateRejectsBadHostPrefix checks the prefix is actually reached by +// whole-config validation, not merely validatable in isolation. +func TestValidateRejectsBadHostPrefix(t *testing.T) { + t.Parallel() + + cfg := validAgentConfigForHostPrefix() + if err := cfg.Validate(); err != nil { + t.Fatalf("baseline config should be valid: %v", err) + } + + cfg.HostPrefix = "/opt/$INJECTED" + if err := cfg.Validate(); err == nil { + t.Fatal("Validate() = nil, want an error for an unsafe HostPrefix") + } + + cfg.HostPrefix = "/opt/unbounded" + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate() = %v, want nil for a valid HostPrefix", err) + } +} + +func validAgentConfigForHostPrefix() *AgentConfig { + return &AgentConfig{ + MachineName: "machine", + NodeName: "node", + Cluster: AgentClusterConfig{ClusterDNS: "10.96.0.10"}, + Kubelet: AgentKubeletConfig{ApiServer: "https://api.example.test"}, + } +} diff --git a/pkg/agent/goalstates/hostpaths.go b/pkg/agent/goalstates/hostpaths.go new file mode 100644 index 000000000..ff7b42e7b --- /dev/null +++ b/pkg/agent/goalstates/hostpaths.go @@ -0,0 +1,172 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 + +package goalstates + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + + "github.com/Azure/unbounded/pkg/agent/config" +) + +// DefaultHostPrefix is the installation prefix used when the agent config does +// not set one. +const DefaultHostPrefix = "/usr/local" + +// Base names of the agent's own host-side files. They are joined with the +// resolved prefix rather than being absolute constants so that hosts with a +// read-only /usr can place them somewhere writable. +const ( + daemonBinaryName = "unbounded-agent" + daemonBinaryBlueName = "unbounded-agent-blue" + daemonBinaryGreenName = "unbounded-agent-green" + daemonBinaryCurrentName = "unbounded-agent-current" + daemonBinaryLastGoodName = "unbounded-agent-last-good" + nspawnLifecycleName = "unbounded-agent-nspawn-lifecycle" + daemonRecoveryScriptName = "unbounded-agent-daemon-recovery.sh" + localDNSNetworkHelperName = "unbounded-localdns-network" +) + +// HostPaths is the resolved host-side layout of the agent's own files under an +// installation prefix. +// +// These are paths on the host. Files inside the nspawn machine are always +// resolved relative to the machine directory and are unaffected by the prefix. +type HostPaths struct { + // Prefix is the resolved installation prefix. + Prefix string + // BinDir is /bin. + BinDir string + // LibexecDir is /libexec. + LibexecDir string + + // NSpawnLifecycleBinary is the rollback-stable helper invoked by the + // generated nspawn hook units. + NSpawnLifecycleBinary string + // DaemonRecoveryScript is executed by the daemon recovery unit. + DaemonRecoveryScript string + // LocalDNSNetworkHelper backs unbounded-localdns-network.service. + LocalDNSNetworkHelper string +} + +// HostPrefixOrDefault returns the configured prefix, or DefaultHostPrefix when +// it is empty. +func HostPrefixOrDefault(prefix string) string { + if trimmed := strings.TrimSpace(prefix); trimmed != "" { + return trimmed + } + + return DefaultHostPrefix +} + +// ResolveHostPaths returns the host-side agent layout for an installation +// prefix. An empty prefix selects DefaultHostPrefix. +func ResolveHostPaths(prefix string) HostPaths { + resolved := HostPrefixOrDefault(prefix) + binDir := filepath.Join(resolved, "bin") + libexecDir := filepath.Join(resolved, "libexec") + + return HostPaths{ + Prefix: resolved, + BinDir: binDir, + LibexecDir: libexecDir, + NSpawnLifecycleBinary: filepath.Join(binDir, nspawnLifecycleName), + DaemonRecoveryScript: filepath.Join(binDir, daemonRecoveryScriptName), + LocalDNSNetworkHelper: filepath.Join(libexecDir, localDNSNetworkHelperName), + } +} + +// KnownHostPrefixes returns the prefixes that teardown and existing-deployment +// detection must consider. +// +// A host provisioned before the prefix was configurable, or by an agent using a +// different prefix, still has files under the default. Cleanup and +// already-provisioned checks therefore look at both, so that changing the +// prefix cannot orphan files or let a dirty host be silently reprovisioned. +func KnownHostPrefixes(prefix string) []string { + resolved := HostPrefixOrDefault(prefix) + if resolved == DefaultHostPrefix { + return []string{DefaultHostPrefix} + } + + return []string{resolved, DefaultHostPrefix} +} + +// MergeHostPrefixes returns every distinct prefix teardown must sweep, given +// candidates gathered from different sources. +// +// Teardown cannot rely on any single source. The installation record has the +// prefix from before the first mutation but may be absent on hosts provisioned +// by an older agent; the applied config has it only once the node started. An +// empty candidate contributes nothing but never suppresses the default. +func MergeHostPrefixes(candidates ...string) []string { + var ( + out []string + seen = map[string]struct{}{} + ) + + add := func(prefix string) { + if _, ok := seen[prefix]; ok { + return + } + + seen[prefix] = struct{}{} + + out = append(out, prefix) + } + + for _, candidate := range candidates { + if strings.TrimSpace(candidate) == "" { + continue + } + + for _, prefix := range KnownHostPrefixes(candidate) { + add(prefix) + } + } + + add(DefaultHostPrefix) + + return out +} + +// HostPrefixFromAppliedConfig returns the installation prefix recorded in the +// applied config of whichever machine is provisioned on this host. +// +// Processes started by systemd, such as the agent daemon and the nspawn +// lifecycle hooks, cannot inherit the prefix from the environment that +// bootstrapped the host. The applied config is the authoritative record: it is +// written once at bootstrap and re-read here so that later upgrades and +// teardown resolve the same paths the bootstrap used. +// +// An absent or unreadable config yields the default prefix, which is what a +// host provisioned before the prefix was configurable actually has on disk. +// +// Note that the applied config only exists once the node has started. Callers +// that must work after a *failed* bootstrap should prefer the installation +// record, which is written before the first mutation; see +// installstate.Record.HostPrefix. +func HostPrefixFromAppliedConfig() string { + for _, name := range []string{NSpawnMachineKube1, NSpawnMachineKube2} { + data, err := os.ReadFile(AppliedConfigPath(name)) + if err != nil { + continue + } + + // Only the prefix is needed here, so decode into the shared config type + // rather than a consumer-specific wrapper. Unknown fields are ignored. + var cfg config.AgentConfig + if err := json.Unmarshal(data, &cfg); err != nil { + continue + } + + if prefix := HostPrefixOrDefault(cfg.HostPrefix); prefix != DefaultHostPrefix { + return prefix + } + } + + return DefaultHostPrefix +} diff --git a/pkg/agent/goalstates/hostpaths_test.go b/pkg/agent/goalstates/hostpaths_test.go new file mode 100644 index 000000000..63b02256a --- /dev/null +++ b/pkg/agent/goalstates/hostpaths_test.go @@ -0,0 +1,116 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 + +package goalstates + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Azure/unbounded/pkg/agent/config" +) + +func TestHostPrefixOrDefault(t *testing.T) { + t.Parallel() + + assert.Equal(t, DefaultHostPrefix, HostPrefixOrDefault("")) + assert.Equal(t, DefaultHostPrefix, HostPrefixOrDefault(" ")) + assert.Equal(t, "/opt/unbounded", HostPrefixOrDefault("/opt/unbounded")) + assert.Equal(t, "/opt/unbounded", HostPrefixOrDefault(" /opt/unbounded ")) +} + +// TestResolveHostPathsDefaultsAreUnchanged pins the pre-existing absolute paths. +// Hosts that do not configure a prefix must keep exactly the layout they had +// before the prefix became configurable. +func TestResolveHostPathsDefaultsAreUnchanged(t *testing.T) { + t.Parallel() + + paths := ResolveHostPaths("") + + assert.Equal(t, "/usr/local", paths.Prefix) + assert.Equal(t, "/usr/local/bin", paths.BinDir) + assert.Equal(t, "/usr/local/libexec", paths.LibexecDir) + assert.Equal(t, "/usr/local/bin/unbounded-agent-nspawn-lifecycle", paths.NSpawnLifecycleBinary) + assert.Equal(t, "/usr/local/bin/unbounded-agent-daemon-recovery.sh", paths.DaemonRecoveryScript) + assert.Equal(t, "/usr/local/libexec/unbounded-localdns-network", paths.LocalDNSNetworkHelper) +} + +func TestResolveHostPathsWithPrefix(t *testing.T) { + t.Parallel() + + paths := ResolveHostPaths("/opt/unbounded") + + assert.Equal(t, "/opt/unbounded", paths.Prefix) + assert.Equal(t, "/opt/unbounded/bin", paths.BinDir) + assert.Equal(t, "/opt/unbounded/libexec", paths.LibexecDir) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-nspawn-lifecycle", paths.NSpawnLifecycleBinary) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-daemon-recovery.sh", paths.DaemonRecoveryScript) + assert.Equal(t, "/opt/unbounded/libexec/unbounded-localdns-network", paths.LocalDNSNetworkHelper) +} + +// TestResolvedAgentUpgradePathsDefaultsAreUnchanged is the equivalent regression +// guard for the blue-green daemon binary layout. +// TestResolvedAgentUpgradePathsEnvOverridesPrefix keeps the existing escape +// hatch working: an explicit environment override wins over the prefix. +func TestKnownHostPrefixes(t *testing.T) { + t.Parallel() + + assert.Equal(t, []string{DefaultHostPrefix}, KnownHostPrefixes("")) + assert.Equal(t, []string{DefaultHostPrefix}, KnownHostPrefixes(DefaultHostPrefix)) + + // A non-default prefix must still sweep the default, so that a host + // provisioned under the old layout is not left with orphaned files. + assert.Equal(t, []string{"/opt/unbounded", DefaultHostPrefix}, KnownHostPrefixes("/opt/unbounded")) +} + +func TestHostPrefixFromAppliedConfig(t *testing.T) { + // AppliedConfigPath is absolute, so redirect it by pointing AgentConfigDir's + // consumers at a temporary root is not possible; instead assert the + // fallback, which is the branch reachable without writing to /etc. + assert.Equal(t, DefaultHostPrefix, HostPrefixFromAppliedConfig()) +} + +// TestHostPrefixRoundTripsThroughAppliedConfig proves the persisted config +// carries the prefix, which is what lets systemd-started processes resolve the +// same paths the bootstrap used. +func TestHostPrefixRoundTripsThroughAppliedConfig(t *testing.T) { + t.Parallel() + + cfg := config.AgentConfig{MachineName: "m", HostPrefix: "/opt/unbounded"} + + data, err := json.Marshal(cfg) + require.NoError(t, err) + + path := filepath.Join(t.TempDir(), "applied-config.json") + require.NoError(t, os.WriteFile(path, data, 0o600)) + + raw, err := os.ReadFile(path) + require.NoError(t, err) + + var decoded config.AgentConfig + require.NoError(t, json.Unmarshal(raw, &decoded)) + + assert.Equal(t, "/opt/unbounded", decoded.HostPrefix) + assert.Equal(t, "/opt/unbounded/bin", ResolveHostPaths(decoded.HostPrefix).BinDir) +} + +// TestDefaultHostPathsMatchTheExistingConstants is the regression guard for +// every host that does not set a prefix. Those hosts must resolve to exactly +// the paths they had before the prefix existed, because the lifecycle helper +// path is baked as an absolute path into generated systemd units that are +// already on disk. +func TestDefaultHostPathsMatchTheExistingConstants(t *testing.T) { + t.Parallel() + + paths := ResolveHostPaths("") + + require.Equal(t, DefaultHostPrefix, paths.Prefix) + require.Equal(t, filepath.Dir(DaemonBinaryPath), paths.BinDir) + require.Equal(t, NSpawnLifecycleBinaryPath, paths.NSpawnLifecycleBinary) + require.Equal(t, DaemonRecoveryScriptPath, paths.DaemonRecoveryScript) +} From cdb5de92bc699dcdd0eaaadbfbc651123314af85 Mon Sep 17 00:00:00 2001 From: Philip Lombardi <893096+plombardi89@users.noreply.github.com> Date: Fri, 18 Sep 2026 22:35:11 -0400 Subject: [PATCH 2/4] agent: resolve the daemon binaries under the installation prefix The blue-green agent binaries were absolute constants under /usr/local/bin. A host whose /usr is read-only cannot hold them there, which is the whole reason the prefix exists. ResolvedAgentUpgradePathsFor resolves them under a prefix instead. An empty prefix selects the default, and a test pins that the result is exactly the constants this package used before, because those paths are baked into generated units and into the blue-green symlinks of every host already installed. If the default drifted, an upgraded agent would look for its binaries where the host does not have them. The original entry point stays, deprecated, delegating to an empty prefix. It is published from pkg/ and callers outside this repository compose their own phases from it, so removing it would break them at compile time. Every caller inside the repository moves to the new one in this commit, because staticcheck's SA1019 is enabled and a split would not lint. All of them run under systemd or on the host with no config in hand, so they take the prefix from the applied config, which is what that lookup exists for. On a host that configures no prefix this resolves the default and nothing changes. The AgentUpgrade signal path is deliberately not prefixed: it is state about an upgrade rather than part of the installed layout, and it already lives under the agent config directory, which stays writable on such hosts. One caller passed the function as a value rather than calling it, so a search for call sites missed it and only the linter found it. It is now wrapped, so the prefix is read when the command runs rather than when it is constructed. --- cmd/agent/internal/cmd/agentupgrade.go | 15 ++++-- cmd/agent/internal/daemon/agentupgrade.go | 4 +- cmd/agent/internal/daemon/lifecycle.go | 6 +-- pkg/agent/goalstates/agentupgrade.go | 33 ++++++++++-- pkg/agent/goalstates/agentupgrade_test.go | 63 +++++++++++++++++++++-- 5 files changed, 103 insertions(+), 18 deletions(-) diff --git a/cmd/agent/internal/cmd/agentupgrade.go b/cmd/agent/internal/cmd/agentupgrade.go index a11eebc88..fad1f5bb2 100644 --- a/cmd/agent/internal/cmd/agentupgrade.go +++ b/cmd/agent/internal/cmd/agentupgrade.go @@ -42,10 +42,17 @@ type hostAgentUpgradeHandler struct { func newCmdHostAgentUpgrade(cmdCtx *CommandContext) *cobra.Command { handler := &hostAgentUpgradeHandler{ - cmdCtx: cmdCtx, - writer: os.Stdout, - executable: os.Executable, - resolvedPath: goalstates.ResolvedAgentUpgradePaths, + cmdCtx: cmdCtx, + writer: os.Stdout, + executable: os.Executable, + // Wrapped rather than referenced directly so the prefix is read when + // the command runs, not when it is constructed. This runs on the host + // rather than under systemd, but the applied config is still the + // authority: the prefix belongs to the installation, not to whatever + // environment happens to be invoking the upgrade. + resolvedPath: func() (goalstates.AgentUpgradePaths, error) { + return goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) + }, geteuid: os.Geteuid, installation: installstate.DefaultStore(), } diff --git a/cmd/agent/internal/daemon/agentupgrade.go b/cmd/agent/internal/daemon/agentupgrade.go index eb0bb2dd2..979bb1206 100644 --- a/cmd/agent/internal/daemon/agentupgrade.go +++ b/cmd/agent/internal/daemon/agentupgrade.go @@ -61,7 +61,7 @@ func parseAgentUpgradeRequest(parameters map[string]string) (agentUpgradeRequest } func upgradeDaemonBinary(ctx context.Context, log *slog.Logger, request agentUpgradeRequest) error { - paths, err := goalstates.ResolvedAgentUpgradePaths() + paths, err := goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) if err != nil { return fmt.Errorf("resolve current daemon binary symlink: %w", err) } @@ -85,7 +85,7 @@ func upgradeDaemonBinary(ctx context.Context, log *slog.Logger, request agentUpg } func newAgentUpgradeSignalOperator() (agentUpgradeSignalOperator, error) { - paths, err := goalstates.ResolvedAgentUpgradePaths() + paths, err := goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) if err != nil { return nil, fmt.Errorf("resolve AgentUpgrade signal path: %w", err) } diff --git a/cmd/agent/internal/daemon/lifecycle.go b/cmd/agent/internal/daemon/lifecycle.go index 8cd40302d..9ddc969cc 100644 --- a/cmd/agent/internal/daemon/lifecycle.go +++ b/cmd/agent/internal/daemon/lifecycle.go @@ -51,7 +51,7 @@ func EnableDaemon(log *slog.Logger) phases.Task { func (d *enableDaemon) Name() string { return "enable-daemon" } func (d *enableDaemon) Do(ctx context.Context) error { - paths, err := goalstates.ResolvedAgentUpgradePaths() + paths, err := goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) if err != nil { return fmt.Errorf("resolve current daemon binary symlink: %w", err) } @@ -158,7 +158,7 @@ func usableDaemonBinary(path string) bool { } func renderDaemonAsset(name string, content []byte) ([]byte, error) { - paths, err := goalstates.ResolvedAgentUpgradePaths() + paths, err := goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) if err != nil { return nil, err } @@ -339,7 +339,7 @@ func removeOwnedFile(path string) error { // active daemon already proves it resolved an applied config at startup, so the // applied-config check belongs to RepairDaemon rather than here. func VerifyDaemonInstalled(ctx context.Context, log *slog.Logger) error { - paths, err := goalstates.ResolvedAgentUpgradePaths() + paths, err := goalstates.ResolvedAgentUpgradePathsFor(goalstates.HostPrefixFromAppliedConfig()) if err != nil { return err } diff --git a/pkg/agent/goalstates/agentupgrade.go b/pkg/agent/goalstates/agentupgrade.go index 9fb64dc30..0e1df0755 100644 --- a/pkg/agent/goalstates/agentupgrade.go +++ b/pkg/agent/goalstates/agentupgrade.go @@ -24,13 +24,36 @@ type AgentUpgradePaths struct { // ResolvedAgentUpgradePaths returns the host-side agent binary paths after // applying environment overrides. +// +// Deprecated: use ResolvedAgentUpgradePathsFor, which resolves the binaries +// under a configured installation prefix. This entry point is equivalent to +// passing an empty prefix and is kept for callers outside this repository. func ResolvedAgentUpgradePaths() (AgentUpgradePaths, error) { + return ResolvedAgentUpgradePathsFor("") +} + +// ResolvedAgentUpgradePathsFor returns the host-side agent binary paths under an +// installation prefix, after applying environment overrides. +// +// An empty prefix selects DefaultHostPrefix, so a host that does not configure +// one resolves exactly the paths this package has always used. +// +// Environment overrides are absolute and win over the prefix. They name a +// specific file, which is more particular than a directory to look in, and the +// nspawn lifecycle hooks rely on that to pin a binary across an upgrade. +// +// The AgentUpgrade signal path is deliberately not prefixed. It lives under the +// agent config directory rather than the installation prefix, because it is +// state about an upgrade rather than part of the installed layout. +func ResolvedAgentUpgradePathsFor(prefix string) (AgentUpgradePaths, error) { + binDir := ResolveHostPaths(prefix).BinDir + paths := AgentUpgradePaths{ - BinaryPath: resolveDaemonBinaryPath(EnvDaemonBinary, DaemonBinaryPath), - BluePath: resolveDaemonBinaryPath(EnvDaemonBinaryBlue, DaemonBinaryBluePath), - GreenPath: resolveDaemonBinaryPath(EnvDaemonBinaryGreen, DaemonBinaryGreenPath), - CurrentPath: resolveDaemonBinaryPath(EnvDaemonBinaryCurrent, DaemonBinaryCurrentPath), - LastGoodPath: resolveDaemonBinaryPath(EnvDaemonBinaryLastGood, DaemonBinaryLastGoodPath), + BinaryPath: resolveDaemonBinaryPath(EnvDaemonBinary, filepath.Join(binDir, daemonBinaryName)), + BluePath: resolveDaemonBinaryPath(EnvDaemonBinaryBlue, filepath.Join(binDir, daemonBinaryBlueName)), + GreenPath: resolveDaemonBinaryPath(EnvDaemonBinaryGreen, filepath.Join(binDir, daemonBinaryGreenName)), + CurrentPath: resolveDaemonBinaryPath(EnvDaemonBinaryCurrent, filepath.Join(binDir, daemonBinaryCurrentName)), + LastGoodPath: resolveDaemonBinaryPath(EnvDaemonBinaryLastGood, filepath.Join(binDir, daemonBinaryLastGoodName)), SignalPath: resolveDaemonBinaryPath(EnvDaemonAgentUpgradeSignalPath, DaemonAgentUpgradeSignalPath), } diff --git a/pkg/agent/goalstates/agentupgrade_test.go b/pkg/agent/goalstates/agentupgrade_test.go index 0a50521ed..189a741e3 100644 --- a/pkg/agent/goalstates/agentupgrade_test.go +++ b/pkg/agent/goalstates/agentupgrade_test.go @@ -40,7 +40,7 @@ func TestResolvedAgentUpgradePaths(t *testing.T) { t.Setenv(EnvDaemonBinaryLastGood, lastGoodPath) t.Setenv(EnvDaemonAgentUpgradeSignalPath, signalPath) - paths, err := ResolvedAgentUpgradePaths() + paths, err := ResolvedAgentUpgradePathsFor("") require.NoError(t, err) assert.Equal(t, binaryPath, paths.BinaryPath) @@ -56,7 +56,7 @@ func TestResolvedAgentUpgradePaths_UsesDefaultsForBlankOverrides(t *testing.T) { t.Setenv(EnvDaemonBinary, "") t.Setenv(EnvDaemonBinaryBlue, " ") - paths, err := ResolvedAgentUpgradePaths() + paths, err := ResolvedAgentUpgradePathsFor("") require.NoError(t, err) assert.Equal(t, DaemonBinaryPath, paths.BinaryPath) @@ -87,7 +87,7 @@ func TestResolvedAgentUpgradePaths_ResolvesCurrentTarget(t *testing.T) { t.Setenv(EnvDaemonBinary, binaryPath) t.Setenv(EnvDaemonBinaryCurrent, currentPath) - paths, err := ResolvedAgentUpgradePaths() + paths, err := ResolvedAgentUpgradePathsFor("") require.NoError(t, err) assert.Equal(t, currentTargetPath, paths.CurrentTargetPath) @@ -97,8 +97,63 @@ func TestResolvedAgentUpgradePaths_CurrentTargetFallsBackToBinaryPath(t *testing t.Setenv(EnvDaemonBinary, "/agent") t.Setenv(EnvDaemonBinaryCurrent, filepath.Join(t.TempDir(), "missing-current")) - paths, err := ResolvedAgentUpgradePaths() + paths, err := ResolvedAgentUpgradePathsFor("") require.NoError(t, err) assert.Equal(t, "/agent", paths.CurrentTargetPath) } + +// TestResolvedAgentUpgradePathsForPrefix covers the reason the prefix-aware +// entry point exists: a host whose /usr is read-only cannot hold the agent's +// own binaries under /usr/local, so they move with the prefix. +// +// The signal path deliberately does not move. It is state about an upgrade +// rather than part of the installed layout, and it lives under the agent config +// directory, which is writable on such hosts. +func TestResolvedAgentUpgradePathsForPrefix(t *testing.T) { + paths, err := ResolvedAgentUpgradePathsFor("/opt/unbounded") + require.NoError(t, err) + + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent", paths.BinaryPath) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-blue", paths.BluePath) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-green", paths.GreenPath) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-current", paths.CurrentPath) + assert.Equal(t, "/opt/unbounded/bin/unbounded-agent-last-good", paths.LastGoodPath) + assert.Equal(t, DaemonAgentUpgradeSignalPath, paths.SignalPath) +} + +// TestResolvedAgentUpgradePathsForDefaultMatchesLegacyConstants pins that a host +// which configures no prefix resolves exactly what this package resolved before +// the prefix existed. +// +// These paths are baked into generated systemd units and into the blue-green +// symlinks on every host already in the field. If the default drifted, an +// upgraded agent would look for its binaries somewhere the installed host does +// not have them, and the daemon would fail to start with nothing having changed +// on disk. +func TestResolvedAgentUpgradePathsForDefaultMatchesLegacyConstants(t *testing.T) { + paths, err := ResolvedAgentUpgradePathsFor("") + require.NoError(t, err) + + assert.Equal(t, DaemonBinaryPath, paths.BinaryPath) + assert.Equal(t, DaemonBinaryBluePath, paths.BluePath) + assert.Equal(t, DaemonBinaryGreenPath, paths.GreenPath) + assert.Equal(t, DaemonBinaryCurrentPath, paths.CurrentPath) + assert.Equal(t, DaemonBinaryLastGoodPath, paths.LastGoodPath) + assert.Equal(t, DaemonAgentUpgradeSignalPath, paths.SignalPath) +} + +// TestDeprecatedResolvedAgentUpgradePathsStillWorks keeps the compatibility +// promise honest. The entry point is deprecated rather than removed because it +// is published from pkg/, and callers outside this repository compose their own +// phases from it. +func TestDeprecatedResolvedAgentUpgradePathsStillWorks(t *testing.T) { + //nolint:staticcheck // Exercising the deprecated entry point is the point. + legacy, err := ResolvedAgentUpgradePaths() + require.NoError(t, err) + + current, err := ResolvedAgentUpgradePathsFor("") + require.NoError(t, err) + + assert.Equal(t, current, legacy, "the deprecated entry point must stay equivalent to an empty prefix") +} From ee491a037e7487c7727d39fcd429924915c0f855 Mon Sep 17 00:00:00 2001 From: Philip Lombardi <893096+plombardi89@users.noreply.github.com> Date: Fri, 18 Sep 2026 22:39:16 -0400 Subject: [PATCH 3/4] agent: record the installation prefix before the first host mutation Teardown has to find the agent's own files. On a host that configures a prefix they are not under /usr/local, and after a bootstrap that failed before the node started there is nothing on the host that says where they are: the applied config carries the prefix but is not written until the node runs. The ownership record is written before any mutation, which makes it the only source that covers that window, so it carries the resolved prefix. Optional, and the schema version does not move. A record written by an agent that knows about the prefix stays readable by one that does not, because unknown fields are ignored, and a default installation writes no field at all so its record is byte-identical to one written before this existed. A test pins that, since the value of the compatibility is entirely in the absence. Resolved rather than configured, so the record names a real directory instead of an empty string meaning whatever the default happened to be. NewRecord takes it as a parameter rather than leaving it a field to set afterwards. Forgetting it would be silent and would only surface at teardown, on a host whose files are somewhere reset does not look. Also corrects a comment in the prefix lookup that pointed at this field before it existed. --- cmd/agent/internal/bootstrap/coordinator.go | 14 +++++- .../internal/bootstrap/coordinator_test.go | 6 +-- cmd/agent/internal/cmd/bootstrap.go | 9 +++- cmd/agent/internal/daemon/migration_test.go | 2 +- cmd/agent/internal/daemon/reset.go | 2 +- cmd/agent/internal/daemon/reset_test.go | 4 +- cmd/agent/internal/installstate/store.go | 28 +++++++++++- cmd/agent/internal/installstate/store_test.go | 44 ++++++++++++++++--- pkg/agent/goalstates/hostpaths.go | 5 ++- 9 files changed, 95 insertions(+), 19 deletions(-) diff --git a/cmd/agent/internal/bootstrap/coordinator.go b/cmd/agent/internal/bootstrap/coordinator.go index d5339bdbf..6a21f43d0 100644 --- a/cmd/agent/internal/bootstrap/coordinator.go +++ b/cmd/agent/internal/bootstrap/coordinator.go @@ -16,7 +16,17 @@ import ( "github.com/Azure/unbounded/cmd/agent/internal/installstate" ) -type Identity struct{ MachineName, ConfigFingerprint string } +// Identity is what makes one installation distinguishable from another. +// +// HostPrefix is the resolved installation prefix. It is carried here so the +// record written before the first host mutation knows where this installation +// puts its files, which is the only thing teardown can consult after a +// bootstrap that failed before the node started. +type Identity struct { + MachineName string + ConfigFingerprint string + HostPrefix string +} type Stages interface { EnsureHostClean(context.Context) error @@ -80,7 +90,7 @@ func (c *Coordinator) Run(ctx context.Context, id Identity) (Outcome, error) { return Outcome{}, err } - r, err = installstate.NewRecord(id.MachineName, id.ConfigFingerprint) + r, err = installstate.NewRecord(id.MachineName, id.ConfigFingerprint, id.HostPrefix) if err != nil { return Outcome{}, err } diff --git a/cmd/agent/internal/bootstrap/coordinator_test.go b/cmd/agent/internal/bootstrap/coordinator_test.go index 5076aeda7..c1a3fb080 100644 --- a/cmd/agent/internal/bootstrap/coordinator_test.go +++ b/cmd/agent/internal/bootstrap/coordinator_test.go @@ -104,7 +104,7 @@ func TestCompletedRecoveryDoesNotResolveRetiredBootstrapInputs(t *testing.T) { for _, repair := range []bool{false, true} { dir := t.TempDir() store := installstate.NewStore(filepath.Join(dir, "state"), filepath.Join(dir, "lock")) - r, err := installstate.NewRecord("machine", "fingerprint") + r, err := installstate.NewRecord("machine", "fingerprint", "") require.NoError(t, err) r.Phase = installstate.Complete @@ -140,7 +140,7 @@ func TestAdmissionFailurePreventsAllStageWork(t *testing.T) { t.Run(mode, func(t *testing.T) { dir := t.TempDir() store := installstate.NewStore(filepath.Join(dir, "state"), filepath.Join(dir, "lock")) - r, err := installstate.NewRecord("machine", "fingerprint") + r, err := installstate.NewRecord("machine", "fingerprint", "") require.NoError(t, err) if mode == "resetting" { @@ -170,7 +170,7 @@ func TestAdmissionFailurePreventsAllStageWork(t *testing.T) { func TestInterruptedRepairRemainsCompleteAndRetries(t *testing.T) { store := installstate.NewStore(t.TempDir(), filepath.Join(t.TempDir(), "lock")) - r, err := installstate.NewRecord("machine", "fingerprint") + r, err := installstate.NewRecord("machine", "fingerprint", "") require.NoError(t, err) require.NoError(t, store.MarkComplete(r)) stages := &fakeStages{store: store, fail: "repair", verifyErr: errInjected} diff --git a/cmd/agent/internal/cmd/bootstrap.go b/cmd/agent/internal/cmd/bootstrap.go index e00baf1a9..4e7c1ef4b 100644 --- a/cmd/agent/internal/cmd/bootstrap.go +++ b/cmd/agent/internal/cmd/bootstrap.go @@ -73,7 +73,14 @@ func bootstrapIdentity(cfg *provision.UnboundedAgentConfig) (bootstrap.Identity, return bootstrap.Identity{}, err } - return bootstrap.Identity{MachineName: cfg.MachineName, ConfigFingerprint: installstate.Fingerprint(data)}, nil + return bootstrap.Identity{ + MachineName: cfg.MachineName, + ConfigFingerprint: installstate.Fingerprint(data), + // Resolved rather than configured, so the record names a real directory + // instead of an empty string meaning "wherever the default was at the + // time", which is what teardown would have to guess from. + HostPrefix: goalstates.HostPrefixOrDefault(cfg.HostPrefix), + }, nil } func (s *agentStages) EnsureHostClean(ctx context.Context) error { diff --git a/cmd/agent/internal/daemon/migration_test.go b/cmd/agent/internal/daemon/migration_test.go index d095c5958..b39e00324 100644 --- a/cmd/agent/internal/daemon/migration_test.go +++ b/cmd/agent/internal/daemon/migration_test.go @@ -61,7 +61,7 @@ func TestStartupStandsDownWhileInstallationUnfinished(t *testing.T) { t.Parallel() store := installstate.NewStore(t.TempDir(), filepath.Join(t.TempDir(), "lock")) - record, err := installstate.NewRecord("machine-1", "fingerprint") + record, err := installstate.NewRecord("machine-1", "fingerprint", "") require.NoError(t, err) require.NoError(t, store.Save(record)) diff --git a/cmd/agent/internal/daemon/reset.go b/cmd/agent/internal/daemon/reset.go index 61a6cd435..d4808e37a 100644 --- a/cmd/agent/internal/daemon/reset.go +++ b/cmd/agent/internal/daemon/reset.go @@ -72,7 +72,7 @@ func recordForTeardown(log *slog.Logger, store *installstate.Store) (installstat log.Warn("installation record is unreadable; replacing it for teardown", "error", err) } - return installstate.NewRecord("legacy-reset", "legacy-reset") + return installstate.NewRecord("legacy-reset", "legacy-reset", "") } func resetUnderLock(ctx context.Context, log *slog.Logger, store *installstate.Store, inner phases.Task) error { diff --git a/cmd/agent/internal/daemon/reset_test.go b/cmd/agent/internal/daemon/reset_test.go index 2ae2b4b5e..633c914db 100644 --- a/cmd/agent/internal/daemon/reset_test.go +++ b/cmd/agent/internal/daemon/reset_test.go @@ -35,7 +35,7 @@ func TestResetRetainsOwnershipUntilTeardownAndSyncSucceed(t *testing.T) { t.Run(failure, func(t *testing.T) { dir := t.TempDir() store := installstate.NewStore(filepath.Join(dir, "state"), filepath.Join(dir, "lock")) - r, err := installstate.NewRecord("machine", "f") + r, err := installstate.NewRecord("machine", "f", "") require.NoError(t, err) r.Phase = installstate.Resetting @@ -128,7 +128,7 @@ func TestTeardownKeepsAReadableRecord(t *testing.T) { dir := t.TempDir() store := installstate.NewStore(filepath.Join(dir, "state"), filepath.Join(dir, "lock")) - saved, err := installstate.NewRecord("machine-1", "fingerprint-1") + saved, err := installstate.NewRecord("machine-1", "fingerprint-1", "") require.NoError(t, err) require.NoError(t, store.Save(saved)) diff --git a/cmd/agent/internal/installstate/store.go b/cmd/agent/internal/installstate/store.go index 7520f6fec..ace82b371 100644 --- a/cmd/agent/internal/installstate/store.go +++ b/cmd/agent/internal/installstate/store.go @@ -52,6 +52,21 @@ type Record struct { MachineName string `json:"machineName"` ConfigFingerprint string `json:"configFingerprint"` Phase Phase `json:"phase"` + + // HostPrefix is the resolved installation prefix, recorded so teardown can + // find the agent's own files without being told where they are. + // + // It is written before the first host mutation, which makes it the only + // source that survives a bootstrap that failed before the node started. The + // applied config carries the same prefix but does not exist until then, so + // reset on a half-built host has nothing else to go on. + // + // Optional, and absent means the default. The schema version does not move + // for it: a record written by an agent that knows about the prefix stays + // readable by one that does not, because unknown fields are ignored, and a + // record written before it existed is read here as the default, which is + // what such a host actually has on disk. + HostPrefix string `json:"hostPrefix,omitempty"` } func (r Record) Validate() error { @@ -157,7 +172,16 @@ func (s *Store) Remove() error { return err } -func NewRecord(machine, fingerprint string) (Record, error) { +// NewRecord returns a record for a fresh installation. +// +// hostPrefix is a parameter rather than a field callers set afterwards because +// forgetting it is silent and only surfaces at teardown, on a host whose files +// are somewhere reset would not look. An empty prefix means the default. +// +// The value is stored as given and not validated here. This package deals in +// stdlib and durability only, and pulling in config validation to re-check a +// string this agent wrote from an already validated config would buy little. +func NewRecord(machine, fingerprint, hostPrefix string) (Record, error) { id := make([]byte, 16) if _, err := rand.Read(id); err != nil { return Record{}, err @@ -165,7 +189,7 @@ func NewRecord(machine, fingerprint string) (Record, error) { return Record{ SchemaVersion: schemaVersion, InstallID: hex.EncodeToString(id), MachineName: machine, - ConfigFingerprint: fingerprint, Phase: Installing, + ConfigFingerprint: fingerprint, Phase: Installing, HostPrefix: hostPrefix, }, nil } diff --git a/cmd/agent/internal/installstate/store_test.go b/cmd/agent/internal/installstate/store_test.go index 2d482a836..6877cf98a 100644 --- a/cmd/agent/internal/installstate/store_test.go +++ b/cmd/agent/internal/installstate/store_test.go @@ -4,6 +4,7 @@ package installstate import ( + "encoding/json" "errors" "os" "path/filepath" @@ -25,7 +26,7 @@ func TestStoreLifecycle(t *testing.T) { require.NoError(t, s.Remove()) _, err := s.Load() require.ErrorIs(t, err, ErrNotFound) - r, err := NewRecord("machine", Fingerprint([]byte(`{"machineName":"machine"}`))) + r, err := NewRecord("machine", Fingerprint([]byte(`{"machineName":"machine"}`)), "") require.NoError(t, err) require.NoError(t, s.Save(r)) loaded, err := s.Load() @@ -47,7 +48,7 @@ func TestStoreLifecycle(t *testing.T) { func TestOwnershipAdmission(t *testing.T) { t.Parallel() - r, err := NewRecord("machine", "fingerprint") + r, err := NewRecord("machine", "fingerprint", "") require.NoError(t, err) for _, phase := range []Phase{Installing, Complete, Resetting} { @@ -104,7 +105,7 @@ func TestInstallationLockSurvivesStateRemoval(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { require.NoError(t, lock.Release()) }) - r, err := NewRecord("machine", "f") + r, err := NewRecord("machine", "f", "") require.NoError(t, err) require.NoError(t, s.Save(r)) require.NoError(t, s.Remove()) @@ -125,7 +126,7 @@ func TestRemoveRestoresOwnershipWhenUndurable(t *testing.T) { t.Parallel() s := testStore(t) - r, err := NewRecord("machine", "f") + r, err := NewRecord("machine", "f", "") require.NoError(t, err) r.Phase = Resetting @@ -176,7 +177,7 @@ func TestMutationAdmission(t *testing.T) { s := testStore(t) if phase != "" { - r, err := NewRecord("machine", "f") + r, err := NewRecord("machine", "f", "") require.NoError(t, err) r.Phase = phase @@ -227,3 +228,36 @@ func TestStoreIgnoresUnknownFields(t *testing.T) { require.NoError(t, err) require.Equal(t, Resume, disposition, "the record must still be usable, not merely parseable") } + +// TestRecordCarriesTheInstallationPrefix covers what the prefix is recorded +// for: teardown on a host where bootstrap failed before the node started. +// +// The applied config carries the same value but does not exist until the node +// runs, so on a half-built host this record is the only thing that knows where +// the agent put its files. Absent means the default, which is what a host +// installed before the prefix existed actually has on disk. +func TestRecordCarriesTheInstallationPrefix(t *testing.T) { + t.Parallel() + + s := testStore(t) + + prefixed, err := NewRecord("machine", "f", "/opt/unbounded") + require.NoError(t, err) + require.NoError(t, s.Save(prefixed)) + + loaded, err := s.Load() + require.NoError(t, err) + require.Equal(t, "/opt/unbounded", loaded.HostPrefix) + require.NoError(t, loaded.Validate()) + + // A default installation records nothing, so its record is byte-identical + // to one written before the field existed and stays readable by an agent + // that predates it. + def, err := NewRecord("machine", "f", "") + require.NoError(t, err) + + encoded, err := json.Marshal(def) + require.NoError(t, err) + require.NotContains(t, string(encoded), "hostPrefix", + "a default installation must not write the field, or older agents see a record they did not write") +} diff --git a/pkg/agent/goalstates/hostpaths.go b/pkg/agent/goalstates/hostpaths.go index ff7b42e7b..90d9fbdc7 100644 --- a/pkg/agent/goalstates/hostpaths.go +++ b/pkg/agent/goalstates/hostpaths.go @@ -147,8 +147,9 @@ func MergeHostPrefixes(candidates ...string) []string { // // Note that the applied config only exists once the node has started. Callers // that must work after a *failed* bootstrap should prefer the installation -// record, which is written before the first mutation; see -// installstate.Record.HostPrefix. +// record, which carries the same prefix and is written before the first host +// mutation. That package is internal to the agent binary, so it cannot be named +// from here. func HostPrefixFromAppliedConfig() string { for _, name := range []string{NSpawnMachineKube1, NSpawnMachineKube2} { data, err := os.ReadFile(AppliedConfigPath(name)) From 9d0ff4696865c4264db9e44e595211afcb1204cc Mon Sep 17 00:00:00 2001 From: Philip Lombardi <893096+plombardi89@users.noreply.github.com> Date: Fri, 18 Sep 2026 22:43:41 -0400 Subject: [PATCH 4/4] agent: make the installation prefix part of bootstrap identity The agent's own binaries live under the prefix, so starting with a different one is not a retry of the same installation. Continuing would leave the first installation's files where they are and build a second one beside them. Admission has to refuse and ask for a reset, which is what a changed fingerprint does. The delicate half is the other one. Every host already installed was fingerprinted without this input. If the default contributed a value, all of them would hash differently under an agent carrying this change, read as a different installation, and demand an explicit reset on upgrade over a field they never set. So the prefix enters the hash only when it resolves somewhere other than the default, and carries omitempty so that at the default it contributes nothing rather than an empty string. It is the resolved prefix that counts, not how it was written. Leaving it unset and naming /usr/local explicitly put the files in the same place, so they hash alike; telling an operator who wrote down what was already true that they must reset the host would be a poor trade for the precision. Verified by mutation, since all three ways to get this wrong are silent and affect every host in the field rather than the one under test: dropping omitempty, hashing the default instead of eliding it, and never hashing the prefix at all each fail a test. The fixtures carry a literal fingerprint, which is what makes the first two detectable at all. --- cmd/agent/internal/cmd/bootstrap.go | 33 +++++++++++- cmd/agent/internal/cmd/bootstrap_test.go | 65 ++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/cmd/agent/internal/cmd/bootstrap.go b/cmd/agent/internal/cmd/bootstrap.go index 4e7c1ef4b..deb7e1b41 100644 --- a/cmd/agent/internal/cmd/bootstrap.go +++ b/cmd/agent/internal/cmd/bootstrap.go @@ -64,11 +64,40 @@ func canonicalImageIdentity(image string) string { func bootstrapIdentity(cfg *provision.UnboundedAgentConfig) (bootstrap.Identity, error) { // Keep identity tied to the cluster and installed rootfs, while allowing // credentials and artifact locations to be refreshed for a retry. + // + // HostPrefix enters the hash only when it resolves somewhere other than the + // default, and carries omitempty so that at the default it contributes + // nothing at all. Every host already in the field was fingerprinted without + // this input; if the default hashed as a value, each of them would read as a + // different installation and demand an explicit reset on upgrade, for a + // field they never set. TestBootstrapV1CompatibilityFixtures catches that. + // + // It is the resolved prefix that matters, not how it was written. Leaving it + // unset and naming /usr/local explicitly put the files in the same place, so + // they are the same installation and must hash alike. + // + // A prefix that resolves elsewhere does belong in the identity. The agent's + // own files live under it, so starting with a different one is not a retry: + // it would leave the first installation behind and build a second one + // beside it. + resolvedPrefix := goalstates.HostPrefixOrDefault(cfg.HostPrefix) + + fingerprintedPrefix := resolvedPrefix + if fingerprintedPrefix == goalstates.DefaultHostPrefix { + fingerprintedPrefix = "" + } + data, err := json.Marshal(struct { KubernetesVersion string OCIImage string APIServer string - }{strings.TrimPrefix(cfg.Cluster.Version, "v"), canonicalImageIdentity(cfg.OCIImage), cfg.Kubelet.ApiServer}) + HostPrefix string `json:",omitempty"` + }{ + strings.TrimPrefix(cfg.Cluster.Version, "v"), + canonicalImageIdentity(cfg.OCIImage), + cfg.Kubelet.ApiServer, + fingerprintedPrefix, + }) if err != nil { return bootstrap.Identity{}, err } @@ -79,7 +108,7 @@ func bootstrapIdentity(cfg *provision.UnboundedAgentConfig) (bootstrap.Identity, // Resolved rather than configured, so the record names a real directory // instead of an empty string meaning "wherever the default was at the // time", which is what teardown would have to guess from. - HostPrefix: goalstates.HostPrefixOrDefault(cfg.HostPrefix), + HostPrefix: resolvedPrefix, }, nil } diff --git a/cmd/agent/internal/cmd/bootstrap_test.go b/cmd/agent/internal/cmd/bootstrap_test.go index 9dc480a4f..41cfb0738 100644 --- a/cmd/agent/internal/cmd/bootstrap_test.go +++ b/cmd/agent/internal/cmd/bootstrap_test.go @@ -256,3 +256,68 @@ func TestClassifyNodeStartFailure(t *testing.T) { }) } } + +// TestBootstrapFingerprintTracksTheInstallationPrefix covers both halves of how +// the prefix enters installation identity, because the two pull in opposite +// directions. +// +// Configuring a prefix has to change the fingerprint. The agent's own binaries +// live under it, so a start with a different prefix is not a retry of the same +// installation: continuing would leave the first installation's files behind +// and build a second one beside them. Admission must refuse and ask for a +// reset, which is what a changed fingerprint does. +// +// Configuring nothing has to change nothing. Every host already in the field +// was fingerprinted without this input, and if the default hashed differently +// each of them would read as a different installation and demand an explicit +// reset on upgrade, for a field they never set. +func TestBootstrapFingerprintTracksTheInstallationPrefix(t *testing.T) { + load := func(t *testing.T) *provision.UnboundedAgentConfig { + t.Helper() + + cfg, err := loadConfigFromFile(filepath.Join("testdata", "bootstrap-v1", "input.json")) + require.NoError(t, err) + + return cfg + } + + baseline, err := bootstrapIdentity(load(t)) + require.NoError(t, err) + + // Whitespace is not a configuration choice, so it must not be one here + // either; otherwise a stray space rewrites the identity of a default host. + for _, blank := range []string{"", " ", "\t"} { + cfg := load(t) + cfg.HostPrefix = blank + + unset, err := bootstrapIdentity(cfg) + require.NoError(t, err) + require.Equal(t, baseline.ConfigFingerprint, unset.ConfigFingerprint, + "an unset prefix must hash as it did before the field existed, got %q", blank) + } + + // Naming the default explicitly puts the files in the same place as leaving + // it unset, so the two are the same installation. Hashing them differently + // would tell an operator who wrote down what was already true that they + // must reset the host. + explicit := load(t) + explicit.HostPrefix = goalstates.DefaultHostPrefix + + explicitID, err := bootstrapIdentity(explicit) + require.NoError(t, err) + require.Equal(t, baseline.ConfigFingerprint, explicitID.ConfigFingerprint, + "identity follows where the files land, not how the prefix was spelled") + + moved := load(t) + moved.HostPrefix = "/opt/unbounded" + + movedID, err := bootstrapIdentity(moved) + require.NoError(t, err) + require.NotEqual(t, baseline.ConfigFingerprint, movedID.ConfigFingerprint, + "moving the installation prefix must not read as a retry of the same installation") + require.Equal(t, "/opt/unbounded", movedID.HostPrefix) + + // The record needs a real directory, not an empty string standing for + // whatever the default was when it was written. + require.Equal(t, goalstates.DefaultHostPrefix, baseline.HostPrefix) +}