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/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/cmd/bootstrap.go b/cmd/agent/internal/cmd/bootstrap.go index e00baf1a9..deb7e1b41 100644 --- a/cmd/agent/internal/cmd/bootstrap.go +++ b/cmd/agent/internal/cmd/bootstrap.go @@ -64,16 +64,52 @@ 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 } - 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: resolvedPrefix, + }, nil } func (s *agentStages) EnsureHostClean(ctx context.Context) error { 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) +} 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/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/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/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") +} diff --git a/pkg/agent/goalstates/hostpaths.go b/pkg/agent/goalstates/hostpaths.go new file mode 100644 index 000000000..90d9fbdc7 --- /dev/null +++ b/pkg/agent/goalstates/hostpaths.go @@ -0,0 +1,173 @@ +// 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 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)) + 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) +}