From cf0a6b8079cf1d63fc708e26d1377574bb0f2651 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 24 Oct 2024 01:42:21 -0400 Subject: [PATCH 01/24] wip --- lib/autoupdate/agent/installer.go | 77 ++++++++++++++++++++----- lib/autoupdate/agent/process.go | 42 ++++++++++++++ lib/autoupdate/agent/updater.go | 95 +++++++++++++++++++++++-------- 3 files changed, 174 insertions(+), 40 deletions(-) create mode 100644 lib/autoupdate/agent/process.go diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 4da41e8e55509..5fc13b6c79266 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -82,9 +82,6 @@ type LocalInstaller struct { ReservedFreeInstallDisk uint64 } -// ErrLinked is returned when a linked version cannot be removed. -var ErrLinked = errors.New("linked version cannot be removed") - // Remove a Teleport version directory from InstallDir. // This function is idempotent. func (li *LocalInstaller) Remove(ctx context.Context, version string) error { @@ -375,50 +372,100 @@ func (li *LocalInstaller) List(ctx context.Context) (versions []string, err erro } // Link the specified version into the system LinkBinDir. -func (li *LocalInstaller) Link(ctx context.Context, version string) error { +func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) { versionDir, err := li.versionDir(version) if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) } // ensure target directories exist before trying to create links err = os.MkdirAll(li.LinkBinDir, 0755) if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) } err = os.MkdirAll(li.LinkServiceDir, 0755) if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) } // create binary links binDir := filepath.Join(versionDir, "bin") entries, err := os.ReadDir(binDir) if err != nil { - return trace.Errorf("failed to find Teleport binary directory: %w", err) + return nil, trace.Errorf("failed to find Teleport binary directory: %w", err) } + + // setup revert function + type symlink struct { + old, new string + } + var revertLinks []symlink + revert = func(ctx context.Context) bool { + ok := true + for _, l := range revertLinks { + err := renameio.Symlink(l.old, l.new) + if err != nil { + ok = false + li.Log.ErrorContext(ctx, "Failed to revert symlink", "old", l.old, "new", l.new, "err", err) + } + } + return ok + } + // revert on error + defer func() { + if err != nil { + revert(ctx) + } + }() + + // create binary symlinks + var linked int for _, entry := range entries { if entry.IsDir() { continue } - err := renameio.Symlink(filepath.Join(binDir, entry.Name()), filepath.Join(li.LinkBinDir, entry.Name())) + oldname := filepath.Join(binDir, entry.Name()) + newname := filepath.Join(li.LinkBinDir, entry.Name()) + orig, err := os.Readlink(newname) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, trace.Wrap(err) + } + err = renameio.Symlink(oldname, newname) if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) + } + if orig != "" { + revertLinks = append(revertLinks, symlink{ + old: orig, + new: newname, + }) } linked++ } if linked == 0 { - return trace.Errorf("no binaries available to link") + return nil, trace.Errorf("no binaries available to link") } // create systemd service link - service := filepath.Join(versionDir, servicePath) - err = renameio.Symlink(service, filepath.Join(li.LinkServiceDir, filepath.Base(servicePath))) + + oldname := filepath.Join(versionDir, servicePath) + newname := filepath.Join(li.LinkServiceDir, filepath.Base(servicePath)) + orig, err := os.Readlink(newname) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, trace.Wrap(err) + } + err = renameio.Symlink(oldname, newname) if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) } - return nil + if orig != "" { + revertLinks = append(revertLinks, symlink{ + old: orig, + new: newname, + }) + } + return revert, nil } // versionDir returns the storage directory for a Teleport version. diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go new file mode 100644 index 0000000000000..a2a7e2085e635 --- /dev/null +++ b/lib/autoupdate/agent/process.go @@ -0,0 +1,42 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package agent + +import ( + "context" + "log/slog" +) + +// SystemdService manages a Teleport systemd service. +type SystemdService struct { + ServiceName string + // Log contains a logger. + Log *slog.Logger +} + +func (s SystemdService) Reload(ctx context.Context) error { + s.Log.InfoContext(ctx, "Teleport gracefully reloaded.") + s.Log.WarnContext(ctx, "Teleport ungracefully restarted.") + s.Log.WarnContext(ctx, "Teleport not running.") + panic("implement me") +} + +func (s SystemdService) Sync(ctx context.Context) error { + panic("implement me") +} diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 7071f16e42d15..6250a05b3bb9f 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -166,26 +166,45 @@ type Updater struct { ConfigPath string // Installer manages installations of the Teleport agent. Installer Installer + // Process manages a running instance of Teleport. + Process Process } // Installer provides an API for installing Teleport agents. type Installer interface { // Install the Teleport agent at version from the download template. - // This function must be idempotent. + // Install must be idempotent. Install(ctx context.Context, version, template string, flags InstallFlags) error // Link the Teleport agent at version into the system location. - // This function must be idempotent. - Link(ctx context.Context, version string) error + // Link must be idempotent. + // The revert function must restore the previous linking, returning false on failure. + Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) // List the installed versions of Teleport. List(ctx context.Context) (versions []string, err error) // Remove the Teleport agent at version. - // This function must be idempotent. + // Remove must be idempotent. + // Returns ErrLinked if unable to remove due to being linked. Remove(ctx context.Context, version string) error } +// ErrLinked is returned when a linked version cannot be removed. +var ErrLinked = errors.New("linked version cannot be removed") + +// Process provides an API for interacting with a running Teleport process. +type Process interface { + // Reload must reload the Teleport process as gracefully as possible. + // If the process is not healthy after reloading, Reload must return an error. + // Reload must return nil if the process did not require reloading. + Reload(ctx context.Context) error + // Sync must validate and synchronize process configuration. + // Sync may need to be called before the process is reloaded to ensure it is successful. + Sync(ctx context.Context) error +} + // InstallFlags sets flags for the Teleport installation type InstallFlags int +// TODO(sclevine): add flags for need_restart and selinux config const ( // FlagEnterprise installs enterprise Teleport FlagEnterprise InstallFlags = 1 << iota @@ -215,7 +234,7 @@ type OverrideConfig struct { // This function is idempotent. func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { // Read configuration from update.yaml and override any new values passed as flags. - cfg, err := u.readConfig(u.ConfigPath) + cfg, err := readConfig(u.ConfigPath) if err != nil { return trace.Errorf("failed to read %s: %w", updateConfigName, err) } @@ -229,7 +248,7 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { cfg.Spec.URLTemplate = override.URLTemplate } cfg.Spec.Enabled = true - if err := validateUpdatesSpec(&cfg.Spec); err != nil { + if err := validateConfigSpec(&cfg.Spec); err != nil { return trace.Wrap(err) } @@ -287,14 +306,40 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if err != nil { return trace.Errorf("failed to install: %w", err) } - err = u.Installer.Link(ctx, desiredVersion) + revert, err := u.Installer.Link(ctx, desiredVersion) if err != nil { return trace.Errorf("failed to link: %w", err) } + if err := u.Process.Sync(ctx); err != nil { + // If sync fails, we may have left the host in a bad state, so we revert and re-Sync. + u.Log.ErrorContext(ctx, "Reverting symlinks due to invalid configuration.") + if ok := revert(ctx); !ok { + u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks. Installation likely broken.") + } + if err := u.Process.Sync(ctx); err != nil { + u.Log.ErrorContext(ctx, "Failed to sync configuration after failed restart.", "error", err) + } + return trace.Errorf("failed to validate configuration for new version %q of Teleport: %w", desiredVersion, err) + } if cfg.Status.ActiveVersion != desiredVersion { + u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) + err = u.Process.Reload(ctx) + if err != nil { + // If reloading Teleport at the new version fails, revert, resync, and reload. + u.Log.ErrorContext(ctx, "Reverting symlinks due to failed restart.") + if ok := revert(ctx); !ok { + u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks. Installation likely broken.") + } + if err := u.Process.Sync(ctx); err != nil { + u.Log.ErrorContext(ctx, "Invalid configuration found after failed restart. Attempting restart anyways.", "error", err) + } + if err := u.Process.Reload(ctx); err != nil { + u.Log.ErrorContext(ctx, "Failed to revert Teleport.", "error", err) + } + return trace.Errorf("failed to start new version %q of Teleport: %w", desiredVersion, err) + } cfg.Status.BackupVersion = cfg.Status.ActiveVersion cfg.Status.ActiveVersion = desiredVersion - u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) } else { u.Log.InfoContext(ctx, "Target version successfully validated.", "version", desiredVersion) } @@ -311,29 +356,17 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { } // Always write the configuration file if enable succeeds. - if err := u.writeConfig(u.ConfigPath, cfg); err != nil { + if err := writeConfig(u.ConfigPath, cfg); err != nil { return trace.Errorf("failed to write %s: %w", updateConfigName, err) } u.Log.InfoContext(ctx, "Configuration updated.") return nil } -func validateUpdatesSpec(spec *UpdateSpec) error { - if spec.URLTemplate != "" && - !strings.HasPrefix(strings.ToLower(spec.URLTemplate), "https://") { - return trace.Errorf("Teleport download URL must use TLS (https://)") - } - - if spec.Proxy == "" { - return trace.Errorf("Teleport proxy URL must be specified with --proxy or present in %s", updateConfigName) - } - return nil -} - // Disable disables agent auto-updates. // This function is idempotent. func (u *Updater) Disable(ctx context.Context) error { - cfg, err := u.readConfig(u.ConfigPath) + cfg, err := readConfig(u.ConfigPath) if err != nil { return trace.Errorf("failed to read %s: %w", updateConfigName, err) } @@ -342,14 +375,14 @@ func (u *Updater) Disable(ctx context.Context) error { return nil } cfg.Spec.Enabled = false - if err := u.writeConfig(u.ConfigPath, cfg); err != nil { + if err := writeConfig(u.ConfigPath, cfg); err != nil { return trace.Errorf("failed to write %s: %w", updateConfigName, err) } return nil } // readConfig reads UpdateConfig from a file. -func (*Updater) readConfig(path string) (*UpdateConfig, error) { +func readConfig(path string) (*UpdateConfig, error) { f, err := os.Open(path) if errors.Is(err, fs.ErrNotExist) { return &UpdateConfig{ @@ -375,7 +408,7 @@ func (*Updater) readConfig(path string) (*UpdateConfig, error) { } // writeConfig writes UpdateConfig to a file atomically, ensuring the file cannot be corrupted. -func (*Updater) writeConfig(filename string, cfg *UpdateConfig) error { +func writeConfig(filename string, cfg *UpdateConfig) error { opts := []renameio.Option{ renameio.WithPermissions(0755), renameio.WithExistingPermissions(), @@ -391,3 +424,15 @@ func (*Updater) writeConfig(filename string, cfg *UpdateConfig) error { } return trace.Wrap(t.CloseAtomicallyReplace()) } + +func validateConfigSpec(spec *UpdateSpec) error { + if spec.URLTemplate != "" && + !strings.HasPrefix(strings.ToLower(spec.URLTemplate), "https://") { + return trace.Errorf("Teleport download URL must use TLS (https://)") + } + + if spec.Proxy == "" { + return trace.Errorf("Teleport proxy URL must be specified with --proxy or present in %s", updateConfigName) + } + return nil +} From 21a6f599b19a6522c2f6ef1bd7f3cefc7ef6c28a Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 24 Oct 2024 21:26:49 -0400 Subject: [PATCH 02/24] cleanup --- lib/autoupdate/agent/installer.go | 2 +- lib/autoupdate/agent/process.go | 107 ++++++++++++++++++++++++++++-- lib/autoupdate/agent/updater.go | 35 +++++++--- 3 files changed, 129 insertions(+), 15 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 5fc13b6c79266..215316415fb71 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -99,7 +99,7 @@ func (li *LocalInstaller) Remove(ctx context.Context, version string) error { return trace.Errorf("failed to determine if linked: %w", err) } if linked { - return trace.Wrap(ErrLinked) + return trace.Errorf("refusing to remove: %w", ErrLinked) } // invalidate checksum first, to protect against partially-removed diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index a2a7e2085e635..d365dba40933f 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -19,24 +19,123 @@ package agent import ( + "bytes" "context" + "errors" "log/slog" + "os" + "os/exec" + + "github.com/gravitational/trace" ) // SystemdService manages a Teleport systemd service. type SystemdService struct { + // ServiceName specifies the systemd service name. ServiceName string // Log contains a logger. Log *slog.Logger } func (s SystemdService) Reload(ctx context.Context) error { + if err := s.checkSystem(ctx); err != nil { + return trace.Wrap(err) + } + code := s.systemctl(ctx, slog.LevelDebug, "is-active", "--quiet", s.ServiceName) + if code < 0 { + return trace.Errorf("unable to determine if systemd service is active") + } + if code > 0 { + s.Log.WarnContext(ctx, "Teleport service not running.") + return trace.Wrap(ErrNotNeeded) + } + code = s.systemctl(ctx, slog.LevelError, "reload", s.ServiceName) + if code < 0 { + return trace.Errorf("unable to attempt reload of Teleport systemd service") + } + if code > 0 { + code = s.systemctl(ctx, slog.LevelError, "try-restart", s.ServiceName) + if code != 0 { + return trace.Errorf("hard restart of systemd failed") + } + s.Log.WarnContext(ctx, "Teleport ungracefully restarted. Connections may have been dropped.") + return nil + } s.Log.InfoContext(ctx, "Teleport gracefully reloaded.") - s.Log.WarnContext(ctx, "Teleport ungracefully restarted.") - s.Log.WarnContext(ctx, "Teleport not running.") - panic("implement me") + return nil } func (s SystemdService) Sync(ctx context.Context) error { - panic("implement me") + if err := s.checkSystem(ctx); err != nil { + return trace.Wrap(err) + } + code := s.systemctl(ctx, slog.LevelError, "daemon-reload") + if code != 0 { + return trace.Errorf("unable to reload systemd configuration") + } + return nil +} + +func (s SystemdService) checkSystem(ctx context.Context) error { + _, err := os.Stat("/run/systemd/system") + if errors.Is(err, os.ErrNotExist) { + s.Log.ErrorContext(ctx, "This system does not support SystemD, which is required by the updater.") + return trace.Wrap(ErrNotSupported) + } + return trace.Wrap(err) +} + +func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args ...string) int { + cmd := exec.CommandContext(ctx, "systemctl", args...) + stderr := &lineLogger{ctx: ctx, log: s.Log, level: errLevel} + stdout := &lineLogger{ctx: ctx, log: s.Log, level: slog.LevelDebug} + cmd.Stderr = stderr + cmd.Stdout = stdout + err := cmd.Run() + stderr.Flush() + stdout.Flush() + code := cmd.ProcessState.ExitCode() + if code == 255 { + code = -1 + } + if err != nil { + s.Log.Log(ctx, errLevel, "Failed to run systemctl.", + "args", args, + "code", code, + "error", err) + } + return code +} + +type lineLogger struct { + ctx context.Context + log *slog.Logger + level slog.Level + + last bytes.Buffer +} + +func (w *lineLogger) Write(p []byte) (n int, err error) { + lines := bytes.Split(p, []byte("\n")) + if len(lines) > 0 { + n, err = w.last.Write(lines[0]) + lines = lines[1:] + } + if len(lines) == 0 || err != nil { + return n, trace.Wrap(err) + } + w.log.Log(w.ctx, w.level, w.last.String()) + w.last.Reset() + for _, line := range lines[:len(lines)-1] { + w.log.Log(w.ctx, w.level, string(line)) + n += len(line) + } + n2, err := w.last.Write(lines[0]) + n += n2 + return n, trace.Wrap(err) +} + +func (w *lineLogger) Flush() { + w.log.Log(w.ctx, w.level, w.last.String()) + w.last.Reset() } diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 6250a05b3bb9f..4de574b784645 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -135,6 +135,10 @@ func NewLocalUpdater(cfg LocalUpdaterConfig) (*Updater, error) { ReservedFreeTmpDisk: reservedFreeDisk, ReservedFreeInstallDisk: reservedFreeDisk, }, + Process: &SystemdService{ + ServiceName: "teleport.service", + Log: cfg.Log, + }, }, nil } @@ -175,29 +179,41 @@ type Installer interface { // Install the Teleport agent at version from the download template. // Install must be idempotent. Install(ctx context.Context, version, template string, flags InstallFlags) error - // Link the Teleport agent at version into the system location. - // Link must be idempotent. + // Link the Teleport agent at the specified version into the system location. // The revert function must restore the previous linking, returning false on failure. + // Link must be idempotent. Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) // List the installed versions of Teleport. List(ctx context.Context) (versions []string, err error) // Remove the Teleport agent at version. + // Must return ErrLinked if unable to remove due to being linked. // Remove must be idempotent. - // Returns ErrLinked if unable to remove due to being linked. Remove(ctx context.Context, version string) error } -// ErrLinked is returned when a linked version cannot be removed. -var ErrLinked = errors.New("linked version cannot be removed") +var ( + // ErrLinked is returned when a linked version cannot be operated on. + ErrLinked = errors.New("version is linked") + // ErrNotNeeded is returned when the operation is not needed. + ErrNotNeeded = errors.New("not needed") + // ErrNotSupported is returned when the operation is not supported on the platform. + ErrNotSupported = errors.New("not supported on this platform") +) // Process provides an API for interacting with a running Teleport process. type Process interface { // Reload must reload the Teleport process as gracefully as possible. // If the process is not healthy after reloading, Reload must return an error. - // Reload must return nil if the process did not require reloading. + // If the process did not require reloading, Reload must return ErrNotNeeded. + // E.g., if the process is not enabled, or it was already reloaded after the last Sync. + // If the type implementing Process does not support the system process manager, + // Reload must return ErrNotSupported. Reload(ctx context.Context) error // Sync must validate and synchronize process configuration. - // Sync may need to be called before the process is reloaded to ensure it is successful. + // After the linked Teleport installation is changed, failure to call Sync without + // error before Reload may result in undefined behavior. + // If the type implementing Process does not support the system process manager, + // Sync must return ErrNotSupported. Sync(ctx context.Context) error } @@ -323,8 +339,7 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { } if cfg.Status.ActiveVersion != desiredVersion { u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) - err = u.Process.Reload(ctx) - if err != nil { + if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { // If reloading Teleport at the new version fails, revert, resync, and reload. u.Log.ErrorContext(ctx, "Reverting symlinks due to failed restart.") if ok := revert(ctx); !ok { @@ -333,7 +348,7 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if err := u.Process.Sync(ctx); err != nil { u.Log.ErrorContext(ctx, "Invalid configuration found after failed restart. Attempting restart anyways.", "error", err) } - if err := u.Process.Reload(ctx); err != nil { + if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { u.Log.ErrorContext(ctx, "Failed to revert Teleport.", "error", err) } return trace.Errorf("failed to start new version %q of Teleport: %w", desiredVersion, err) From c1583e5e38e21fc3f10233b7a3cc5a572d6fc21c Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 24 Oct 2024 21:56:24 -0400 Subject: [PATCH 03/24] comments --- lib/autoupdate/agent/process.go | 10 ++++++++ lib/autoupdate/agent/updater.go | 43 +++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index d365dba40933f..165b82f350b8d 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -37,6 +37,9 @@ type SystemdService struct { Log *slog.Logger } +// Reload a systemd service. +// Attempts a graceful reload before a hard restart. +// See Process interface for more details. func (s SystemdService) Reload(ctx context.Context) error { if err := s.checkSystem(ctx); err != nil { return trace.Wrap(err) @@ -65,6 +68,8 @@ func (s SystemdService) Reload(ctx context.Context) error { return nil } +// Sync systemd service configuration by running systemctl daemon-reload. +// See Process interface for more details. func (s SystemdService) Sync(ctx context.Context) error { if err := s.checkSystem(ctx); err != nil { return trace.Wrap(err) @@ -76,6 +81,7 @@ func (s SystemdService) Sync(ctx context.Context) error { return nil } +// checkSystem returns an error if the system is not compatible with this process manager. func (s SystemdService) checkSystem(ctx context.Context) error { _, err := os.Stat("/run/systemd/system") if errors.Is(err, os.ErrNotExist) { @@ -85,6 +91,9 @@ func (s SystemdService) checkSystem(ctx context.Context) error { return trace.Wrap(err) } +// systemctl returns a systemctl subcommand, converting the output to logs. +// Output sent to stdout is logged at debug level. +// Output sent to stderr is logged at the level specified by errLevel. func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args ...string) int { cmd := exec.CommandContext(ctx, "systemctl", args...) stderr := &lineLogger{ctx: ctx, log: s.Log, level: errLevel} @@ -107,6 +116,7 @@ func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args return code } +// lineLogger logs each line written to it. type lineLogger struct { ctx context.Context log *slog.Logger diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 4de574b784645..00df4735b17e3 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -254,26 +254,16 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if err != nil { return trace.Errorf("failed to read %s: %w", updateConfigName, err) } - if override.Proxy != "" { - cfg.Spec.Proxy = override.Proxy - } - if override.Group != "" { - cfg.Spec.Group = override.Group - } - if override.URLTemplate != "" { - cfg.Spec.URLTemplate = override.URLTemplate - } - cfg.Spec.Enabled = true - if err := validateConfigSpec(&cfg.Spec); err != nil { + if err := validateConfigSpec(&cfg.Spec, override); err != nil { return trace.Wrap(err) } // Lookup target version from the proxy. + addr, err := libutils.ParseAddr(cfg.Spec.Proxy) if err != nil { return trace.Errorf("failed to parse proxy server address: %w", err) } - desiredVersion := override.ForceVersion var flags InstallFlags if desiredVersion == "" { @@ -313,7 +303,9 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { u.Log.WarnContext(ctx, "Failed to remove backup version of Teleport before new install.", "error", err) } } - // If the active version and target don't match, kick off upgrade. + + // Install the desired version (or validate existing installation) + template := cfg.Spec.URLTemplate if template == "" { template = cdnURITemplate @@ -326,8 +318,11 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if err != nil { return trace.Errorf("failed to link: %w", err) } + + // Sync process configuration after linking. + if err := u.Process.Sync(ctx); err != nil { - // If sync fails, we may have left the host in a bad state, so we revert and re-Sync. + // If sync fails, we may have left the host in a bad state, so we revert linking and re-Sync. u.Log.ErrorContext(ctx, "Reverting symlinks due to invalid configuration.") if ok := revert(ctx); !ok { u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks. Installation likely broken.") @@ -337,9 +332,13 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { } return trace.Errorf("failed to validate configuration for new version %q of Teleport: %w", desiredVersion, err) } + + // Restart Teleport if necessary. + if cfg.Status.ActiveVersion != desiredVersion { u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { + // If reloading Teleport at the new version fails, revert, resync, and reload. u.Log.ErrorContext(ctx, "Reverting symlinks due to failed restart.") if ok := revert(ctx); !ok { @@ -362,6 +361,8 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { u.Log.InfoContext(ctx, "Backup version set.", "version", v) } + // Check if manual cleanup might be needed. + versions, err := u.Installer.List(ctx) if err != nil { return trace.Errorf("failed to list installed versions: %w", err) @@ -371,6 +372,8 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { } // Always write the configuration file if enable succeeds. + + cfg.Spec.Enabled = true if err := writeConfig(u.ConfigPath, cfg); err != nil { return trace.Errorf("failed to write %s: %w", updateConfigName, err) } @@ -440,12 +443,20 @@ func writeConfig(filename string, cfg *UpdateConfig) error { return trace.Wrap(t.CloseAtomicallyReplace()) } -func validateConfigSpec(spec *UpdateSpec) error { +func validateConfigSpec(spec *UpdateSpec, override OverrideConfig) error { + if override.Proxy != "" { + spec.Proxy = override.Proxy + } + if override.Group != "" { + spec.Group = override.Group + } + if override.URLTemplate != "" { + spec.URLTemplate = override.URLTemplate + } if spec.URLTemplate != "" && !strings.HasPrefix(strings.ToLower(spec.URLTemplate), "https://") { return trace.Errorf("Teleport download URL must use TLS (https://)") } - if spec.Proxy == "" { return trace.Errorf("Teleport proxy URL must be specified with --proxy or present in %s", updateConfigName) } From f5984698cad9ac70dac5740e76b4f67f024469de Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 24 Oct 2024 22:06:23 -0400 Subject: [PATCH 04/24] test wip --- lib/autoupdate/agent/installer_test.go | 2 +- lib/autoupdate/agent/updater_test.go | 31 ++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/autoupdate/agent/installer_test.go b/lib/autoupdate/agent/installer_test.go index 2602704208855..50ab938ce36d2 100644 --- a/lib/autoupdate/agent/installer_test.go +++ b/lib/autoupdate/agent/installer_test.go @@ -397,7 +397,7 @@ func TestLocalInstaller_Remove(t *testing.T) { ctx := context.Background() if tt.linkedVersion != "" { - err = installer.Link(ctx, tt.linkedVersion) + _, err = installer.Link(ctx, tt.linkedVersion) require.NoError(t, err) } err = installer.Remove(ctx, tt.removeVersion) diff --git a/lib/autoupdate/agent/updater_test.go b/lib/autoupdate/agent/updater_test.go index e817851fed1f7..f0e6caafeb5df 100644 --- a/lib/autoupdate/agent/updater_test.go +++ b/lib/autoupdate/agent/updater_test.go @@ -328,9 +328,11 @@ func TestUpdater_Enable(t *testing.T) { installedFlags = flags return tt.installErr }, - FuncLink: func(_ context.Context, version string) error { + FuncLink: func(_ context.Context, version string) (revert func(context.Context) bool, err error) { linkedVersion = version - return nil + return func(_ context.Context) bool { + return true + }, nil }, FuncList: func(_ context.Context) (versions []string, err error) { return []string{"old"}, nil @@ -340,6 +342,14 @@ func TestUpdater_Enable(t *testing.T) { return nil }, } + updater.Process = &testProcess{ + FuncReload: func(_ context.Context) error { + return nil + }, + FuncSync: func(_ context.Context) error { + return nil + }, + } ctx := context.Background() err = updater.Enable(ctx, tt.userCfg) @@ -377,7 +387,7 @@ func blankTestAddr(s []byte) []byte { type testInstaller struct { FuncInstall func(ctx context.Context, version, template string, flags InstallFlags) error FuncRemove func(ctx context.Context, version string) error - FuncLink func(ctx context.Context, version string) error + FuncLink func(ctx context.Context, version string) (revert func(context.Context) bool, err error) FuncList func(ctx context.Context) (versions []string, err error) } @@ -389,10 +399,23 @@ func (ti *testInstaller) Remove(ctx context.Context, version string) error { return ti.FuncRemove(ctx, version) } -func (ti *testInstaller) Link(ctx context.Context, version string) error { +func (ti *testInstaller) Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) { return ti.FuncLink(ctx, version) } func (ti *testInstaller) List(ctx context.Context) (versions []string, err error) { return ti.FuncList(ctx) } + +type testProcess struct { + FuncReload func(ctx context.Context) error + FuncSync func(ctx context.Context) error +} + +func (tp *testProcess) Reload(ctx context.Context) error { + return tp.FuncReload(ctx) +} + +func (tp *testProcess) Sync(ctx context.Context) error { + return tp.FuncSync(ctx) +} From 035cfa808ed7430ee3c69398de576e61c986c612 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 24 Oct 2024 22:59:41 -0400 Subject: [PATCH 05/24] test link revert --- lib/autoupdate/agent/installer.go | 17 ++- lib/autoupdate/agent/installer_test.go | 163 ++++++++++++++++++++++--- 2 files changed, 158 insertions(+), 22 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 215316415fb71..6a0139b67ab19 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -31,6 +31,7 @@ import ( "os" "path/filepath" "runtime" + "syscall" "text/template" "time" @@ -400,7 +401,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func old, new string } var revertLinks []symlink - revert = func(ctx context.Context) bool { + revertFunc := func(ctx context.Context) bool { ok := true for _, l := range revertLinks { err := renameio.Symlink(l.old, l.new) @@ -414,7 +415,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func // revert on error defer func() { if err != nil { - revert(ctx) + revertFunc(ctx) } }() @@ -428,6 +429,11 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func oldname := filepath.Join(binDir, entry.Name()) newname := filepath.Join(li.LinkBinDir, entry.Name()) orig, err := os.Readlink(newname) + if errors.Is(err, os.ErrInvalid) || + errors.Is(err, syscall.EINVAL) { // workaround missing ErrInvalid wrapper + // important: do not attempt to replace a non-linked install of Teleport + return nil, trace.Errorf("refusing to replace file at %s", newname) + } if err != nil && !errors.Is(err, os.ErrNotExist) { return nil, trace.Wrap(err) } @@ -452,6 +458,11 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func oldname := filepath.Join(versionDir, servicePath) newname := filepath.Join(li.LinkServiceDir, filepath.Base(servicePath)) orig, err := os.Readlink(newname) + if errors.Is(err, os.ErrInvalid) || + errors.Is(err, syscall.EINVAL) { // workaround missing ErrInvalid wrapper + // important: do not attempt to replace a non-linked install of Teleport + return nil, trace.Errorf("refusing to replace file at %s", newname) + } if err != nil && !errors.Is(err, os.ErrNotExist) { return nil, trace.Wrap(err) } @@ -465,7 +476,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func new: newname, }) } - return revert, nil + return revertFunc, nil } // versionDir returns the storage directory for a Teleport version. diff --git a/lib/autoupdate/agent/installer_test.go b/lib/autoupdate/agent/installer_test.go index 50ab938ce36d2..ce3a1dd871b7a 100644 --- a/lib/autoupdate/agent/installer_test.go +++ b/lib/autoupdate/agent/installer_test.go @@ -196,16 +196,18 @@ func TestLocalInstaller_Link(t *testing.T) { const version = "new-version" tests := []struct { - name string - dirs []string - files []string + name string + installDirs []string + installFiles []string + existingLinks []string + existingFiles []string - links []string - errMatch string + resultLinks []string + errMatch string }{ { - name: "present", - dirs: []string{ + name: "present with new links", + installDirs: []string{ "bin", "bin/somedir", "etc", @@ -213,7 +215,7 @@ func TestLocalInstaller_Link(t *testing.T) { "etc/systemd/somedir", "somedir", }, - files: []string{ + installFiles: []string{ "bin/teleport", "bin/tsh", "bin/tbot", @@ -221,7 +223,7 @@ func TestLocalInstaller_Link(t *testing.T) { "README", }, - links: []string{ + resultLinks: []string{ "bin/teleport", "bin/tsh", "bin/tbot", @@ -229,15 +231,102 @@ func TestLocalInstaller_Link(t *testing.T) { }, }, { - name: "no links", - files: []string{"README"}, - dirs: []string{"bin"}, + name: "present with existing links", + installDirs: []string{ + "bin", + "bin/somedir", + "etc", + "etc/systemd", + "etc/systemd/somedir", + "somedir", + }, + installFiles: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + servicePath, + "README", + }, + existingLinks: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + "lib/systemd/system/teleport.service", + }, + + resultLinks: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + "lib/systemd/system/teleport.service", + }, + }, + { + name: "conflicting systemd files", + installDirs: []string{ + "bin", + "bin/somedir", + "etc", + "etc/systemd", + "etc/systemd/somedir", + "somedir", + }, + installFiles: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + servicePath, + "README", + }, + existingLinks: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + }, + existingFiles: []string{ + "lib/systemd/system/teleport.service", + }, + + errMatch: "refusing", + }, + { + name: "conflicting bin files", + installDirs: []string{ + "bin", + "bin/somedir", + "etc", + "etc/systemd", + "etc/systemd/somedir", + "somedir", + }, + installFiles: []string{ + "bin/teleport", + "bin/tsh", + "bin/tbot", + servicePath, + "README", + }, + existingLinks: []string{ + "bin/teleport", + "bin/tbot", + "lib/systemd/system/teleport.service", + }, + existingFiles: []string{ + "bin/tsh", + }, + + errMatch: "refusing", + }, + { + name: "no links", + installFiles: []string{"README"}, + installDirs: []string{"bin"}, errMatch: "no binaries", }, { - name: "no bin directory", - files: []string{"README"}, + name: "no bin directory", + installFiles: []string{"README"}, errMatch: "binary directory", }, @@ -251,16 +340,27 @@ func TestLocalInstaller_Link(t *testing.T) { err := os.MkdirAll(versionDir, 0o755) require.NoError(t, err) - for _, d := range tt.dirs { + for _, d := range tt.installDirs { err := os.Mkdir(filepath.Join(versionDir, d), os.ModePerm) require.NoError(t, err) } - for _, n := range tt.files { + for _, n := range tt.installFiles { err := os.WriteFile(filepath.Join(versionDir, n), []byte(filepath.Base(n)), os.ModePerm) require.NoError(t, err) } - linkDir := t.TempDir() + for _, n := range tt.existingLinks { + err := os.MkdirAll(filepath.Dir(filepath.Join(linkDir, n)), os.ModePerm) + require.NoError(t, err) + err = os.Symlink(filepath.Base(n)+".old", filepath.Join(linkDir, n)) + require.NoError(t, err) + } + for _, n := range tt.existingFiles { + err := os.MkdirAll(filepath.Dir(filepath.Join(linkDir, n)), os.ModePerm) + require.NoError(t, err) + err = os.WriteFile(filepath.Join(linkDir, n), []byte(filepath.Base(n)), os.ModePerm) + require.NoError(t, err) + } installer := &LocalInstaller{ InstallDir: versionsDir, @@ -269,19 +369,44 @@ func TestLocalInstaller_Link(t *testing.T) { Log: slog.Default(), } ctx := context.Background() - err = installer.Link(ctx, version) + revert, err := installer.Link(ctx, version) if tt.errMatch != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.errMatch) + + // verify automatic reverts + for _, link := range tt.existingLinks { + v, err := os.Readlink(filepath.Join(linkDir, link)) + require.NoError(t, err) + require.Equal(t, filepath.Base(link)+".old", v) + } + for _, n := range tt.existingFiles { + v, err := os.ReadFile(filepath.Join(linkDir, n)) + require.NoError(t, err) + require.Equal(t, filepath.Base(n), string(v)) + } return } require.NoError(t, err) - for _, link := range tt.links { + for _, link := range tt.resultLinks { v, err := os.ReadFile(filepath.Join(linkDir, link)) require.NoError(t, err) require.Equal(t, filepath.Base(link), string(v)) } + ok := revert(ctx) + require.True(t, ok) + + for _, link := range tt.existingLinks { + v, err := os.Readlink(filepath.Join(linkDir, link)) + require.NoError(t, err) + require.Equal(t, filepath.Base(link)+".old", v) + } + for _, n := range tt.existingFiles { + v, err := os.ReadFile(filepath.Join(linkDir, n)) + require.NoError(t, err) + require.Equal(t, filepath.Base(n), string(v)) + } }) } } From f2f9fc6dfff759173cda63be673ac83dc2fb8043 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:08:44 -0400 Subject: [PATCH 06/24] tests --- lib/autoupdate/agent/updater_test.go | 75 +++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/lib/autoupdate/agent/updater_test.go b/lib/autoupdate/agent/updater_test.go index f0e6caafeb5df..8cefd3a59e3e7 100644 --- a/lib/autoupdate/agent/updater_test.go +++ b/lib/autoupdate/agent/updater_test.go @@ -132,11 +132,16 @@ func TestUpdater_Enable(t *testing.T) { userCfg OverrideConfig installErr error flags InstallFlags + syncErr error + reloadErr error removedVersion string installedVersion string installedTemplate string requestGroup string + syncCalls int + reloadCalls int + revertCalls int errMatch string }{ { @@ -152,9 +157,12 @@ func TestUpdater_Enable(t *testing.T) { ActiveVersion: "old-version", }, }, + installedVersion: "16.3.0", installedTemplate: "https://example.com", requestGroup: "group", + syncCalls: 1, + reloadCalls: 1, }, { name: "config from user", @@ -174,8 +182,11 @@ func TestUpdater_Enable(t *testing.T) { URLTemplate: "https://example.com/new", ForceVersion: "new-version", }, + installedVersion: "new-version", installedTemplate: "https://example.com/new", + syncCalls: 1, + reloadCalls: 1, }, { name: "already enabled", @@ -189,8 +200,11 @@ func TestUpdater_Enable(t *testing.T) { ActiveVersion: "old-version", }, }, + installedVersion: "16.3.0", installedTemplate: cdnURITemplate, + syncCalls: 1, + reloadCalls: 1, }, { name: "insecure URL", @@ -201,6 +215,7 @@ func TestUpdater_Enable(t *testing.T) { URLTemplate: "http://example.com", }, }, + errMatch: "URL must use TLS", }, { @@ -213,7 +228,8 @@ func TestUpdater_Enable(t *testing.T) { }, }, installErr: errors.New("install error"), - errMatch: "install error", + + errMatch: "install error", }, { name: "version already installed", @@ -224,8 +240,11 @@ func TestUpdater_Enable(t *testing.T) { ActiveVersion: "16.3.0", }, }, + installedVersion: "16.3.0", installedTemplate: cdnURITemplate, + syncCalls: 1, + reloadCalls: 0, }, { name: "backup version removed on install", @@ -237,9 +256,12 @@ func TestUpdater_Enable(t *testing.T) { BackupVersion: "backup-version", }, }, + installedVersion: "16.3.0", installedTemplate: cdnURITemplate, removedVersion: "backup-version", + syncCalls: 1, + reloadCalls: 1, }, { name: "backup version kept for validation", @@ -251,26 +273,56 @@ func TestUpdater_Enable(t *testing.T) { BackupVersion: "backup-version", }, }, + installedVersion: "16.3.0", installedTemplate: cdnURITemplate, removedVersion: "", + syncCalls: 1, + reloadCalls: 0, }, { - name: "config does not exist", + name: "config does not exist", + installedVersion: "16.3.0", installedTemplate: cdnURITemplate, + syncCalls: 1, + reloadCalls: 1, }, { name: "FIPS and Enterprise flags", flags: FlagEnterprise | FlagFIPS, installedVersion: "16.3.0", installedTemplate: cdnURITemplate, + syncCalls: 1, + reloadCalls: 1, }, { name: "invalid metadata", cfg: &UpdateConfig{}, errMatch: "invalid", }, + { + name: "sync fails", + syncErr: errors.New("sync error"), + + installedVersion: "16.3.0", + installedTemplate: cdnURITemplate, + syncCalls: 2, + reloadCalls: 0, + revertCalls: 1, + errMatch: "sync error", + }, + { + name: "reload fails", + reloadErr: errors.New("reload error"), + + installedVersion: "16.3.0", + installedTemplate: cdnURITemplate, + syncCalls: 2, + reloadCalls: 2, + revertCalls: 1, + errMatch: "reload error", + }, } for _, tt := range tests { @@ -320,6 +372,7 @@ func TestUpdater_Enable(t *testing.T) { linkedVersion string removedVersion string installedFlags InstallFlags + revertCalls int ) updater.Installer = &testInstaller{ FuncInstall: func(_ context.Context, version, template string, flags InstallFlags) error { @@ -331,6 +384,7 @@ func TestUpdater_Enable(t *testing.T) { FuncLink: func(_ context.Context, version string) (revert func(context.Context) bool, err error) { linkedVersion = version return func(_ context.Context) bool { + revertCalls++ return true }, nil }, @@ -342,12 +396,18 @@ func TestUpdater_Enable(t *testing.T) { return nil }, } + var ( + syncCalls int + reloadCalls int + ) updater.Process = &testProcess{ - FuncReload: func(_ context.Context) error { - return nil - }, FuncSync: func(_ context.Context) error { - return nil + syncCalls++ + return tt.syncErr + }, + FuncReload: func(_ context.Context) error { + reloadCalls++ + return tt.reloadErr }, } @@ -365,6 +425,9 @@ func TestUpdater_Enable(t *testing.T) { require.Equal(t, tt.removedVersion, removedVersion) require.Equal(t, tt.flags, installedFlags) require.Equal(t, tt.requestGroup, requestedGroup) + require.Equal(t, tt.syncCalls, syncCalls) + require.Equal(t, tt.reloadCalls, reloadCalls) + require.Equal(t, tt.revertCalls, revertCalls) data, err := os.ReadFile(cfgPath) require.NoError(t, err) From 0441401751cc62c9f8adcb2df94d7358149ab296 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:18:54 -0400 Subject: [PATCH 07/24] cleanup --- lib/autoupdate/agent/installer.go | 45 ++++++++++++++++--------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 6a0139b67ab19..092653517b339 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -428,18 +428,9 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func } oldname := filepath.Join(binDir, entry.Name()) newname := filepath.Join(li.LinkBinDir, entry.Name()) - orig, err := os.Readlink(newname) - if errors.Is(err, os.ErrInvalid) || - errors.Is(err, syscall.EINVAL) { // workaround missing ErrInvalid wrapper - // important: do not attempt to replace a non-linked install of Teleport - return nil, trace.Errorf("refusing to replace file at %s", newname) - } - if err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, trace.Wrap(err) - } - err = renameio.Symlink(oldname, newname) + orig, err := tryLink(oldname, newname) if err != nil { - return nil, trace.Wrap(err) + return nil, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) } if orig != "" { revertLinks = append(revertLinks, symlink{ @@ -457,18 +448,9 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func oldname := filepath.Join(versionDir, servicePath) newname := filepath.Join(li.LinkServiceDir, filepath.Base(servicePath)) - orig, err := os.Readlink(newname) - if errors.Is(err, os.ErrInvalid) || - errors.Is(err, syscall.EINVAL) { // workaround missing ErrInvalid wrapper - // important: do not attempt to replace a non-linked install of Teleport - return nil, trace.Errorf("refusing to replace file at %s", newname) - } - if err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, trace.Wrap(err) - } - err = renameio.Symlink(oldname, newname) + orig, err := tryLink(oldname, newname) if err != nil { - return nil, trace.Wrap(err) + return nil, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) } if orig != "" { revertLinks = append(revertLinks, symlink{ @@ -479,6 +461,25 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func return revertFunc, nil } +// tryLink attempts to create a symlink, atomically replacing an existing link if already present. +// If a non-symlink file or directory exists in newname already, tryLink errors. +func tryLink(oldname, newname string) (orig string, err error) { + orig, err = os.Readlink(newname) + if errors.Is(err, os.ErrInvalid) || + errors.Is(err, syscall.EINVAL) { // workaround missing ErrInvalid wrapper + // important: do not attempt to replace a non-linked install of Teleport + return orig, trace.Errorf("refusing to replace file at %s", newname) + } + if err != nil && !errors.Is(err, os.ErrNotExist) { + return orig, trace.Wrap(err) + } + err = renameio.Symlink(oldname, newname) + if err != nil { + return orig, trace.Wrap(err) + } + return orig, nil +} + // versionDir returns the storage directory for a Teleport version. // versionDir will fail if the version cannot be used to construct the directory name. // For example, it ensures that ".." cannot be provided to return a system directory. From 0a73a986e095c1058b6c95489913d0ea18510725 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:39:16 -0400 Subject: [PATCH 08/24] cleanup more --- lib/autoupdate/agent/installer.go | 67 ++++++++++++++++--------------- lib/autoupdate/agent/updater.go | 3 +- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 092653517b339..5b8a58ce436cd 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -374,53 +374,54 @@ func (li *LocalInstaller) List(ctx context.Context) (versions []string, err erro // Link the specified version into the system LinkBinDir. func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) { - versionDir, err := li.versionDir(version) - if err != nil { - return nil, trace.Wrap(err) - } - - // ensure target directories exist before trying to create links - err = os.MkdirAll(li.LinkBinDir, 0755) - if err != nil { - return nil, trace.Wrap(err) - } - err = os.MkdirAll(li.LinkServiceDir, 0755) - if err != nil { - return nil, trace.Wrap(err) - } - - // create binary links - binDir := filepath.Join(versionDir, "bin") - entries, err := os.ReadDir(binDir) - if err != nil { - return nil, trace.Errorf("failed to find Teleport binary directory: %w", err) - } - // setup revert function type symlink struct { old, new string } var revertLinks []symlink - revertFunc := func(ctx context.Context) bool { - ok := true + revert = func(ctx context.Context) bool { + // This function is safe to call repeatedly. + // Returns true only when all symlinks are successfully reverted. + var keep []symlink for _, l := range revertLinks { err := renameio.Symlink(l.old, l.new) if err != nil { - ok = false + keep = append(keep, l) li.Log.ErrorContext(ctx, "Failed to revert symlink", "old", l.old, "new", l.new, "err", err) } } - return ok + revertLinks = keep + return len(revertLinks) == 0 } - // revert on error + // revert immediately on error, so caller can ignore revert arg defer func() { if err != nil { - revertFunc(ctx) + revert(ctx) } }() - // create binary symlinks + versionDir, err := li.versionDir(version) + if err != nil { + return revert, trace.Wrap(err) + } + // ensure target directories exist before trying to create links + err = os.MkdirAll(li.LinkBinDir, 0755) + if err != nil { + return revert, trace.Wrap(err) + } + err = os.MkdirAll(li.LinkServiceDir, 0755) + if err != nil { + return revert, trace.Wrap(err) + } + + // create binary links + + binDir := filepath.Join(versionDir, "bin") + entries, err := os.ReadDir(binDir) + if err != nil { + return revert, trace.Errorf("failed to find Teleport binary directory: %w", err) + } var linked int for _, entry := range entries { if entry.IsDir() { @@ -430,7 +431,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func newname := filepath.Join(li.LinkBinDir, entry.Name()) orig, err := tryLink(oldname, newname) if err != nil { - return nil, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) + return revert, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) } if orig != "" { revertLinks = append(revertLinks, symlink{ @@ -441,7 +442,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func linked++ } if linked == 0 { - return nil, trace.Errorf("no binaries available to link") + return revert, trace.Errorf("no binaries available to link") } // create systemd service link @@ -450,7 +451,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func newname := filepath.Join(li.LinkServiceDir, filepath.Base(servicePath)) orig, err := tryLink(oldname, newname) if err != nil { - return nil, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) + return revert, trace.Errorf("failed to create symlink for %s: %w", filepath.Base(oldname), err) } if orig != "" { revertLinks = append(revertLinks, symlink{ @@ -458,7 +459,7 @@ func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func new: newname, }) } - return revertFunc, nil + return revert, nil } // tryLink attempts to create a symlink, atomically replacing an existing link if already present. diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 00df4735b17e3..313e0d326d310 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -180,8 +180,9 @@ type Installer interface { // Install must be idempotent. Install(ctx context.Context, version, template string, flags InstallFlags) error // Link the Teleport agent at the specified version into the system location. - // The revert function must restore the previous linking, returning false on failure. + // The revert function must restore the previous linking, returning false on any failure. // Link must be idempotent. + // Link's revert function must be idempotent. Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) // List the installed versions of Teleport. List(ctx context.Context) (versions []string, err error) From b760906789f93c43d271c23375b02df1c2547d43 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:43:53 -0400 Subject: [PATCH 09/24] comments --- lib/autoupdate/agent/installer_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/autoupdate/agent/installer_test.go b/lib/autoupdate/agent/installer_test.go index ce3a1dd871b7a..d4f58f782dc62 100644 --- a/lib/autoupdate/agent/installer_test.go +++ b/lib/autoupdate/agent/installer_test.go @@ -340,6 +340,7 @@ func TestLocalInstaller_Link(t *testing.T) { err := os.MkdirAll(versionDir, 0o755) require.NoError(t, err) + // setup files in version directory for _, d := range tt.installDirs { err := os.Mkdir(filepath.Join(versionDir, d), os.ModePerm) require.NoError(t, err) @@ -348,6 +349,8 @@ func TestLocalInstaller_Link(t *testing.T) { err := os.WriteFile(filepath.Join(versionDir, n), []byte(filepath.Base(n)), os.ModePerm) require.NoError(t, err) } + + // setup files in system links directory linkDir := t.TempDir() for _, n := range tt.existingLinks { err := os.MkdirAll(filepath.Dir(filepath.Join(linkDir, n)), os.ModePerm) @@ -374,7 +377,7 @@ func TestLocalInstaller_Link(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), tt.errMatch) - // verify automatic reverts + // verify automatic revert for _, link := range tt.existingLinks { v, err := os.Readlink(filepath.Join(linkDir, link)) require.NoError(t, err) @@ -385,18 +388,24 @@ func TestLocalInstaller_Link(t *testing.T) { require.NoError(t, err) require.Equal(t, filepath.Base(n), string(v)) } + + // ensure revert still succeeds + ok := revert(ctx) + require.True(t, ok) return } require.NoError(t, err) + // verify links for _, link := range tt.resultLinks { v, err := os.ReadFile(filepath.Join(linkDir, link)) require.NoError(t, err) require.Equal(t, filepath.Base(link), string(v)) } + + // verify manual revert ok := revert(ctx) require.True(t, ok) - for _, link := range tt.existingLinks { v, err := os.Readlink(filepath.Join(linkDir, link)) require.NoError(t, err) From 6b15e0989311aa56be38afc7bc65b6b7864b0915 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:46:16 -0400 Subject: [PATCH 10/24] comments --- lib/autoupdate/agent/installer.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 5b8a58ce436cd..d786b0477f210 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -85,6 +85,7 @@ type LocalInstaller struct { // Remove a Teleport version directory from InstallDir. // This function is idempotent. +// See Installer interface for additional specs. func (li *LocalInstaller) Remove(ctx context.Context, version string) error { // os.RemoveAll is dangerous because it can remove an entire directory tree. // We must validate the version to ensure that we remove only a single path @@ -117,6 +118,7 @@ func (li *LocalInstaller) Remove(ctx context.Context, version string) error { // Install a Teleport version directory in InstallDir. // This function is idempotent. +// See Installer interface for additional specs. func (li *LocalInstaller) Install(ctx context.Context, version, template string, flags InstallFlags) error { versionDir, err := li.versionDir(version) if err != nil { @@ -372,7 +374,9 @@ func (li *LocalInstaller) List(ctx context.Context) (versions []string, err erro return versions, nil } -// Link the specified version into the system LinkBinDir. +// Link the specified version into the system LinkBinDir and LinkServiceDir. +// The revert function restores the previous linking. +// See Installer interface for additional specs. func (li *LocalInstaller) Link(ctx context.Context, version string) (revert func(context.Context) bool, err error) { // setup revert function type symlink struct { From f78b5c6149c43fbfda52c03d997100ace7b4c4af Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 00:49:04 -0400 Subject: [PATCH 11/24] errors --- lib/autoupdate/agent/process.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 165b82f350b8d..05d09961b501f 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -49,7 +49,7 @@ func (s SystemdService) Reload(ctx context.Context) error { return trace.Errorf("unable to determine if systemd service is active") } if code > 0 { - s.Log.WarnContext(ctx, "Teleport service not running.") + s.Log.WarnContext(ctx, "Teleport systemd service not running.") return trace.Wrap(ErrNotNeeded) } code = s.systemctl(ctx, slog.LevelError, "reload", s.ServiceName) @@ -59,9 +59,9 @@ func (s SystemdService) Reload(ctx context.Context) error { if code > 0 { code = s.systemctl(ctx, slog.LevelError, "try-restart", s.ServiceName) if code != 0 { - return trace.Errorf("hard restart of systemd failed") + return trace.Errorf("hard restart of Teleport systemd service failed") } - s.Log.WarnContext(ctx, "Teleport ungracefully restarted. Connections may have been dropped.") + s.Log.WarnContext(ctx, "Teleport ungracefully restarted. Connections potentially dropped.") return nil } s.Log.InfoContext(ctx, "Teleport gracefully reloaded.") @@ -85,7 +85,7 @@ func (s SystemdService) Sync(ctx context.Context) error { func (s SystemdService) checkSystem(ctx context.Context) error { _, err := os.Stat("/run/systemd/system") if errors.Is(err, os.ErrNotExist) { - s.Log.ErrorContext(ctx, "This system does not support SystemD, which is required by the updater.") + s.Log.ErrorContext(ctx, "This system does not support systemd, which is required by the updater.") return trace.Wrap(ErrNotSupported) } return trace.Wrap(err) From 8f5c1104b974e74e78b7dde1eb1610e3de53b8ec Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 01:09:09 -0400 Subject: [PATCH 12/24] comments --- lib/autoupdate/agent/process.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 05d09961b501f..4a71e6d55b3f9 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -44,6 +44,12 @@ func (s SystemdService) Reload(ctx context.Context) error { if err := s.checkSystem(ctx); err != nil { return trace.Wrap(err) } + // Command error codes < 0 indicate that we are unable to run the command. + // Errors from s.systemctl are logged along with stderr and stdout (debug only). + + // If the service is not running, return ErrNotNeeded. + // Note systemctl reload returns an error if the unit is not active, and + // try-reload-or-restart is too recent of an addition for centos7. code := s.systemctl(ctx, slog.LevelDebug, "is-active", "--quiet", s.ServiceName) if code < 0 { return trace.Errorf("unable to determine if systemd service is active") @@ -52,11 +58,13 @@ func (s SystemdService) Reload(ctx context.Context) error { s.Log.WarnContext(ctx, "Teleport systemd service not running.") return trace.Wrap(ErrNotNeeded) } + // Attempt graceful reload of running service. code = s.systemctl(ctx, slog.LevelError, "reload", s.ServiceName) if code < 0 { return trace.Errorf("unable to attempt reload of Teleport systemd service") } if code > 0 { + // Graceful reload fails, try hard restart. code = s.systemctl(ctx, slog.LevelError, "try-restart", s.ServiceName) if code != 0 { return trace.Errorf("hard restart of Teleport systemd service failed") From 0bd95a5d3e8df4771b7d10e55b313510c87613ff Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 25 Oct 2024 02:00:03 -0400 Subject: [PATCH 13/24] linting --- lib/autoupdate/agent/process.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 4a71e6d55b3f9..cf8e0c2150d8a 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -142,10 +142,9 @@ func (w *lineLogger) Write(p []byte) (n int, err error) { if len(lines) == 0 || err != nil { return n, trace.Wrap(err) } - w.log.Log(w.ctx, w.level, w.last.String()) - w.last.Reset() + w.Flush() for _, line := range lines[:len(lines)-1] { - w.log.Log(w.ctx, w.level, string(line)) + w.log.Log(w.ctx, w.level, string(line)) //nolint:sloglint n += len(line) } n2, err := w.last.Write(lines[0]) @@ -154,6 +153,6 @@ func (w *lineLogger) Write(p []byte) (n int, err error) { } func (w *lineLogger) Flush() { - w.log.Log(w.ctx, w.level, w.last.String()) + w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint w.last.Reset() } From 15ed2618d380eda1c69d37f33a5bd958515a0efd Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 28 Oct 2024 15:43:22 -0400 Subject: [PATCH 14/24] fix bugs --- lib/autoupdate/agent/installer.go | 36 +++++++++++++++++------ lib/autoupdate/agent/process.go | 3 ++ lib/utils/unpack.go | 47 +++++++++++++++++-------------- tool/teleport-update/main.go | 4 ++- 4 files changed, 59 insertions(+), 31 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index d786b0477f210..dc1be518ae608 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -51,13 +51,13 @@ var ( // See utils.Extract for more details on how this list is parsed. // Paths must use tarball-style / separators (not filepath). tgzExtractPaths = []utils.ExtractPath{ - {Src: "teleport/examples/systemd/teleport.service", Dst: "etc/systemd/teleport.service"}, - {Src: "teleport/examples", Skip: true}, - {Src: "teleport/install", Skip: true}, - {Src: "teleport/README.md", Dst: "share/README.md"}, - {Src: "teleport/CHANGELOG.md", Dst: "share/CHANGELOG.md"}, - {Src: "teleport/VERSION", Dst: "share/VERSION"}, - {Src: "teleport", Dst: "bin"}, + {Src: "teleport/examples/systemd/teleport.service", Dst: "etc/systemd/teleport.service", DirMode: 0755}, + {Src: "teleport/examples", Skip: true, DirMode: 0755}, + {Src: "teleport/install", Skip: true, DirMode: 0755}, + {Src: "teleport/README.md", Dst: "share/README.md", DirMode: 0755}, + {Src: "teleport/CHANGELOG.md", Dst: "share/CHANGELOG.md", DirMode: 0755}, + {Src: "teleport/VERSION", Dst: "share/VERSION", DirMode: 0755}, + {Src: "teleport", Dst: "bin", DirMode: 0755}, } // servicePath contains the path to the Teleport SystemD service within the version directory. @@ -119,7 +119,7 @@ func (li *LocalInstaller) Remove(ctx context.Context, version string) error { // Install a Teleport version directory in InstallDir. // This function is idempotent. // See Installer interface for additional specs. -func (li *LocalInstaller) Install(ctx context.Context, version, template string, flags InstallFlags) error { +func (li *LocalInstaller) Install(ctx context.Context, version, template string, flags InstallFlags) (err error) { versionDir, err := li.versionDir(version) if err != nil { return trace.Wrap(err) @@ -175,11 +175,18 @@ func (li *LocalInstaller) Install(ctx context.Context, version, template string, if err != nil { return trace.Errorf("failed to download teleport: %w", err) } - // Seek to the start of the tgz file after writing if _, err := f.Seek(0, io.SeekStart); err != nil { return trace.Errorf("failed seek to start of download: %w", err) } + + // If interrupted, close the file immediately to stop extracting. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + go func() { + <-ctx.Done() + _ = f.Close() + }() // Check integrity before decompression if !bytes.Equal(newSum, pathSum) { return trace.Errorf("mismatched checksum, download possibly corrupt") @@ -193,6 +200,17 @@ func (li *LocalInstaller) Install(ctx context.Context, version, template string, if _, err := f.Seek(0, io.SeekStart); err != nil { return trace.Errorf("failed seek to start: %w", err) } + + // If there's an error after we start extracting, delete the version dir. + defer func() { + if err != nil { + if err := os.RemoveAll(versionDir); err != nil { + li.Log.WarnContext(ctx, "Failed to cleanup broken version extraction.", "error", err, "dir", versionDir) + } + } + }() + + // Extract tgz into version directory. if err := li.extract(ctx, versionDir, f, n); err != nil { return trace.Errorf("failed to extract teleport: %w", err) } diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index cf8e0c2150d8a..0d3bbd7a1ad9c 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -153,6 +153,9 @@ func (w *lineLogger) Write(p []byte) (n int, err error) { } func (w *lineLogger) Flush() { + if w.last.Len() == 0 { + return + } w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint w.last.Reset() } diff --git a/lib/utils/unpack.go b/lib/utils/unpack.go index 78b111daf8992..4515e19092909 100644 --- a/lib/utils/unpack.go +++ b/lib/utils/unpack.go @@ -50,7 +50,8 @@ func Extract(r io.Reader, dir string, paths ...ExtractPath) error { } else if err != nil { return trace.Wrap(err) } - if ok := filterHeader(header, paths); !ok { + dirMode, ok := filterHeader(header, paths) + if !ok { continue } err = sanitizeTarPath(header, dir) @@ -58,7 +59,7 @@ func Extract(r io.Reader, dir string, paths ...ExtractPath) error { return trace.Wrap(err) } - if err := extractFile(tarball, header, dir); err != nil { + if err := extractFile(tarball, header, dir, dirMode); err != nil { return trace.Wrap(err) } } @@ -74,11 +75,15 @@ type ExtractPath struct { Src, Dst string // Skip extracting the Src path and ignore Dst. Skip bool + // DirMode is the file mode for implicit parent directories in Dst. + DirMode os.FileMode } // filterHeader modifies the tar header by filtering it through the ExtractPaths. // filterHeader returns false if the tar header should be skipped. -func filterHeader(hdr *tar.Header, paths []ExtractPath) (include bool) { +// If no paths are provided, filterHeader assumes the header should be included, and sets +// the mode for implicit parent directories to teleport.DirMaskSharedGroup. +func filterHeader(hdr *tar.Header, paths []ExtractPath) (dirMode os.FileMode, include bool) { name := path.Clean(hdr.Name) for _, p := range paths { src := path.Clean(p.Src) @@ -98,14 +103,14 @@ func filterHeader(hdr *tar.Header, paths []ExtractPath) (include bool) { dst += "/" // tar directory headers end in / } hdr.Name = dst - return !p.Skip + return p.DirMode, !p.Skip default: // If name is a file, then // if src is an exact match to the file name, assume src is a file and write directly to dst, // otherwise, assume src is a directory prefix, and replace that prefix with dst. if src == name { hdr.Name = path.Clean(p.Dst) - return !p.Skip + return p.DirMode, !p.Skip } if src != "/" { src += "/" // ensure HasPrefix does not match partial names @@ -114,26 +119,26 @@ func filterHeader(hdr *tar.Header, paths []ExtractPath) (include bool) { continue } hdr.Name = path.Join(p.Dst, strings.TrimPrefix(name, src)) - return !p.Skip + return p.DirMode, !p.Skip } } - return len(paths) == 0 + return teleport.DirMaskSharedGroup, len(paths) == 0 } // extractFile extracts a single file or directory from tarball into dir. // Uses header to determine the type of item to create // Based on https://github.com/mholt/archiver -func extractFile(tarball *tar.Reader, header *tar.Header, dir string) error { +func extractFile(tarball *tar.Reader, header *tar.Header, dir string, dirMode os.FileMode) error { switch header.Typeflag { case tar.TypeDir: - return withDir(filepath.Join(dir, header.Name), nil) + return withDir(filepath.Join(dir, header.Name), dirMode, nil) case tar.TypeBlock, tar.TypeChar, tar.TypeReg, tar.TypeFifo: - return writeFile(filepath.Join(dir, header.Name), tarball, header.FileInfo().Mode()) + return writeFile(filepath.Join(dir, header.Name), tarball, header.FileInfo().Mode(), dirMode) case tar.TypeLink: - return writeHardLink(filepath.Join(dir, header.Name), filepath.Join(dir, header.Linkname)) + return writeHardLink(filepath.Join(dir, header.Name), filepath.Join(dir, header.Linkname), dirMode) case tar.TypeSymlink: - return writeSymbolicLink(filepath.Join(dir, header.Name), header.Linkname) + return writeSymbolicLink(filepath.Join(dir, header.Name), header.Linkname, dirMode) default: log.Warnf("Unsupported type flag %v for %v.", header.Typeflag, header.Name) } @@ -141,7 +146,7 @@ func extractFile(tarball *tar.Reader, header *tar.Header, dir string) error { } // sanitizeTarPath checks that the tar header paths resolve to a subdirectory -// path, and don't contain file paths or links that could escape the tar file +// path, and don't contain file paths or links that could escape the tar fileteleport.DirMaskSharedGroup // like ../../etc/password. func sanitizeTarPath(header *tar.Header, dir string) error { // Sanitize all tar paths resolve to within the destination directory. @@ -168,8 +173,8 @@ func sanitizeTarPath(header *tar.Header, dir string) error { return nil } -func writeFile(path string, r io.Reader, mode os.FileMode) error { - err := withDir(path, func() error { +func writeFile(path string, r io.Reader, mode, dirMode os.FileMode) error { + err := withDir(path, dirMode, func() error { // Create file only if it does not exist to prevent overwriting existing // files (like session recordings). out, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, mode) @@ -182,24 +187,24 @@ func writeFile(path string, r io.Reader, mode os.FileMode) error { return trace.Wrap(err) } -func writeSymbolicLink(path string, target string) error { - err := withDir(path, func() error { +func writeSymbolicLink(path, target string, dirMode os.FileMode) error { + err := withDir(path, dirMode, func() error { err := os.Symlink(target, path) return trace.ConvertSystemError(err) }) return trace.Wrap(err) } -func writeHardLink(path string, target string) error { - err := withDir(path, func() error { +func writeHardLink(path, target string, dirMode os.FileMode) error { + err := withDir(path, dirMode, func() error { err := os.Link(target, path) return trace.ConvertSystemError(err) }) return trace.Wrap(err) } -func withDir(path string, fn func() error) error { - err := os.MkdirAll(filepath.Dir(path), teleport.DirMaskSharedGroup) +func withDir(path string, mode os.FileMode, fn func() error) error { + err := os.MkdirAll(filepath.Dir(path), mode) if err != nil { return trace.ConvertSystemError(err) } diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index 300da6736471a..2adce83a1877c 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -61,6 +61,8 @@ const ( versionsDirName = "versions" // lockFileName specifies the name of the file inside versionsDirName containing the flock lock preventing concurrent updater execution. lockFileName = ".lock" + // defaultLinkDir is the default location where Teleport binaries and services are linked. + defaultLinkDir = "/usr/local" ) var plog = logutils.NewPackageLogger(teleport.ComponentKey, teleport.ComponentUpdater) @@ -98,7 +100,7 @@ func Run(args []string) error { app.Flag("log-format", "Controls the format of output logs. Can be `json` or `text`. Defaults to `text`."). Default(libutils.LogFormatText).EnumVar(&ccfg.LogFormat, libutils.LogFormatJSON, libutils.LogFormatText) app.Flag("link-dir", "Directory to create system symlinks to binaries and services."). - Default(filepath.Join("usr", "local")).Hidden().StringVar(&ccfg.LinkDir) + Default(defaultLinkDir).Hidden().StringVar(&ccfg.LinkDir) app.HelpFlag.Short('h') From c27b6f3c3b9438b041c94aec9b5a3a155d64e972 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 28 Oct 2024 17:32:40 -0400 Subject: [PATCH 15/24] fix typo --- lib/utils/unpack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils/unpack.go b/lib/utils/unpack.go index 4515e19092909..14b213f08a173 100644 --- a/lib/utils/unpack.go +++ b/lib/utils/unpack.go @@ -146,7 +146,7 @@ func extractFile(tarball *tar.Reader, header *tar.Header, dir string, dirMode os } // sanitizeTarPath checks that the tar header paths resolve to a subdirectory -// path, and don't contain file paths or links that could escape the tar fileteleport.DirMaskSharedGroup +// path, and don't contain file paths or links that could escape the tar file // like ../../etc/password. func sanitizeTarPath(header *tar.Header, dir string) error { // Sanitize all tar paths resolve to within the destination directory. From 3ae929b1b53491824856f04aef34dbc17c5d1f53 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 28 Oct 2024 21:07:01 -0400 Subject: [PATCH 16/24] cleanup --- lib/autoupdate/agent/process.go | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 0d3bbd7a1ad9c..63e275728851f 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -51,28 +51,31 @@ func (s SystemdService) Reload(ctx context.Context) error { // Note systemctl reload returns an error if the unit is not active, and // try-reload-or-restart is too recent of an addition for centos7. code := s.systemctl(ctx, slog.LevelDebug, "is-active", "--quiet", s.ServiceName) - if code < 0 { + switch { + case code < 0: return trace.Errorf("unable to determine if systemd service is active") - } - if code > 0 { + case code > 0: s.Log.WarnContext(ctx, "Teleport systemd service not running.") return trace.Wrap(ErrNotNeeded) } // Attempt graceful reload of running service. code = s.systemctl(ctx, slog.LevelError, "reload", s.ServiceName) - if code < 0 { + switch { + case code < 0: return trace.Errorf("unable to attempt reload of Teleport systemd service") - } - if code > 0 { + case code > 0: // Graceful reload fails, try hard restart. code = s.systemctl(ctx, slog.LevelError, "try-restart", s.ServiceName) if code != 0 { return trace.Errorf("hard restart of Teleport systemd service failed") } s.Log.WarnContext(ctx, "Teleport ungracefully restarted. Connections potentially dropped.") - return nil + default: + s.Log.InfoContext(ctx, "Teleport gracefully reloaded.") } - s.Log.InfoContext(ctx, "Teleport gracefully reloaded.") + + // TODO(sclevine): Ensure restart was successful and verify healthcheck. + return nil } @@ -135,23 +138,34 @@ type lineLogger struct { func (w *lineLogger) Write(p []byte) (n int, err error) { lines := bytes.Split(p, []byte("\n")) + // Finish writing line if len(lines) > 0 { n, err = w.last.Write(lines[0]) lines = lines[1:] } + // Quit if no newline if len(lines) == 0 || err != nil { return n, trace.Wrap(err) } - w.Flush() + + // Newline found, log line + w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint + n += w.last.Len() + 1 + w.last.Reset() + + // Log lines that are already newline-terminated for _, line := range lines[:len(lines)-1] { w.log.Log(w.ctx, w.level, string(line)) //nolint:sloglint - n += len(line) + n += len(line) + 1 } - n2, err := w.last.Write(lines[0]) + + // Store remaining line non-newline-terminated line. + n2, err := w.last.Write(lines[len(lines)-1]) n += n2 return n, trace.Wrap(err) } +// Flush logs any trailing bytes that were never terminated with a newline. func (w *lineLogger) Flush() { if w.last.Len() == 0 { return From 40019c06fed9a2d13c382c36958ff7141b6e4a20 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Tue, 29 Oct 2024 11:41:18 -0400 Subject: [PATCH 17/24] cleanup --- lib/autoupdate/agent/installer.go | 3 +++ lib/autoupdate/agent/updater.go | 29 +++++++++++++++++++---------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index dc1be518ae608..5865137830b98 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -496,6 +496,9 @@ func tryLink(oldname, newname string) (orig string, err error) { if err != nil && !errors.Is(err, os.ErrNotExist) { return orig, trace.Wrap(err) } + if orig == oldname { + return orig, nil + } err = renameio.Symlink(oldname, newname) if err != nil { return orig, trace.Wrap(err) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 313e0d326d310..e3484292daeb5 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -320,17 +320,24 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { return trace.Errorf("failed to link: %w", err) } + // If we fail to revert after this point, the next update/enable will + // fix the link to restore the active version. + // Sync process configuration after linking. if err := u.Process.Sync(ctx); err != nil { + if errors.Is(err, context.Canceled) { + return trace.Errorf("sync cancelled") + } // If sync fails, we may have left the host in a bad state, so we revert linking and re-Sync. u.Log.ErrorContext(ctx, "Reverting symlinks due to invalid configuration.") if ok := revert(ctx); !ok { u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks. Installation likely broken.") - } - if err := u.Process.Sync(ctx); err != nil { + } else if err := u.Process.Sync(ctx); err != nil { u.Log.ErrorContext(ctx, "Failed to sync configuration after failed restart.", "error", err) } + u.Log.WarnContext(ctx, "Teleport updater encountered a configuration error and successfully reverted the installation.") + return trace.Errorf("failed to validate configuration for new version %q of Teleport: %w", desiredVersion, err) } @@ -339,18 +346,20 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if cfg.Status.ActiveVersion != desiredVersion { u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { - + if errors.Is(err, context.Canceled) { + return trace.Errorf("reload cancelled") + } // If reloading Teleport at the new version fails, revert, resync, and reload. u.Log.ErrorContext(ctx, "Reverting symlinks due to failed restart.") if ok := revert(ctx); !ok { - u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks. Installation likely broken.") - } - if err := u.Process.Sync(ctx); err != nil { - u.Log.ErrorContext(ctx, "Invalid configuration found after failed restart. Attempting restart anyways.", "error", err) - } - if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { - u.Log.ErrorContext(ctx, "Failed to revert Teleport.", "error", err) + u.Log.ErrorContext(ctx, "Failed to revert Teleport symlinks to older version. Installation likely broken.") + } else if err := u.Process.Sync(ctx); err != nil { + u.Log.ErrorContext(ctx, "Invalid configuration found after reverting Teleport to older version. Installation likely broken.", "error", err) + } else if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { + u.Log.ErrorContext(ctx, "Failed to revert Teleport to older version. Installation likely broken.", "error", err) } + u.Log.WarnContext(ctx, "Teleport updater encountered a configuration error and successfully reverted the installation.") + return trace.Errorf("failed to start new version %q of Teleport: %w", desiredVersion, err) } cfg.Status.BackupVersion = cfg.Status.ActiveVersion From 7b8d26b3868f5ac087f25ab0e236f81a04f9a7db Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Tue, 29 Oct 2024 11:57:06 -0400 Subject: [PATCH 18/24] fix revert --- lib/autoupdate/agent/installer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 5865137830b98..ce3b7a63fc07d 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -497,7 +497,7 @@ func tryLink(oldname, newname string) (orig string, err error) { return orig, trace.Wrap(err) } if orig == oldname { - return orig, nil + return "", nil } err = renameio.Symlink(oldname, newname) if err != nil { From 4429a580730d0a0f165154b315c367a75ecc8cf6 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Tue, 29 Oct 2024 12:30:57 -0400 Subject: [PATCH 19/24] lint --- lib/autoupdate/agent/process.go | 6 +++--- lib/autoupdate/agent/updater.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 63e275728851f..96d819eb86542 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -149,13 +149,13 @@ func (w *lineLogger) Write(p []byte) (n int, err error) { } // Newline found, log line - w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint + w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint // msg cannot be constant n += w.last.Len() + 1 w.last.Reset() // Log lines that are already newline-terminated for _, line := range lines[:len(lines)-1] { - w.log.Log(w.ctx, w.level, string(line)) //nolint:sloglint + w.log.Log(w.ctx, w.level, string(line)) //nolint:sloglint // msg cannot be constant n += len(line) + 1 } @@ -170,6 +170,6 @@ func (w *lineLogger) Flush() { if w.last.Len() == 0 { return } - w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint + w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint // msg cannot be constant w.last.Reset() } diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index e3484292daeb5..b82c3c6d419cb 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -327,7 +327,7 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { if err := u.Process.Sync(ctx); err != nil { if errors.Is(err, context.Canceled) { - return trace.Errorf("sync cancelled") + return trace.Errorf("sync canceled") } // If sync fails, we may have left the host in a bad state, so we revert linking and re-Sync. u.Log.ErrorContext(ctx, "Reverting symlinks due to invalid configuration.") @@ -347,7 +347,7 @@ func (u *Updater) Enable(ctx context.Context, override OverrideConfig) error { u.Log.InfoContext(ctx, "Target version successfully installed.", "version", desiredVersion) if err := u.Process.Reload(ctx); err != nil && !errors.Is(err, ErrNotNeeded) { if errors.Is(err, context.Canceled) { - return trace.Errorf("reload cancelled") + return trace.Errorf("reload canceled") } // If reloading Teleport at the new version fails, revert, resync, and reload. u.Log.ErrorContext(ctx, "Reverting symlinks due to failed restart.") From 0e5f5100b29c21081421757811fdef8dafb2cc67 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 31 Oct 2024 19:08:36 -0400 Subject: [PATCH 20/24] feedback --- lib/autoupdate/agent/process.go | 4 +- lib/autoupdate/agent/process_test.go | 69 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 lib/autoupdate/agent/process_test.go diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 96d819eb86542..37b6a10fb5aae 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -115,6 +115,8 @@ func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args stderr.Flush() stdout.Flush() code := cmd.ProcessState.ExitCode() + + // treat out-of-range code as an error with OS executing command, not as intentional 255 if code == 255 { code = -1 } @@ -150,7 +152,7 @@ func (w *lineLogger) Write(p []byte) (n int, err error) { // Newline found, log line w.log.Log(w.ctx, w.level, w.last.String()) //nolint:sloglint // msg cannot be constant - n += w.last.Len() + 1 + n += 1 w.last.Reset() // Log lines that are already newline-terminated diff --git a/lib/autoupdate/agent/process_test.go b/lib/autoupdate/agent/process_test.go new file mode 100644 index 0000000000000..8b86dd6e73cba --- /dev/null +++ b/lib/autoupdate/agent/process_test.go @@ -0,0 +1,69 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package agent + +import ( + "bytes" + "context" + "log/slog" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLineLogger(t *testing.T) { + t.Parallel() + + out := &bytes.Buffer{} + ll := lineLogger{ + ctx: context.Background(), + log: slog.New(slog.NewTextHandler(out, + &slog.HandlerOptions{ReplaceAttr: msgOnly}, + )), + } + + for _, e := range []struct { + v string + n int + }{ + {v: "a", n: 1}, + {v: "b\n", n: 2}, + {v: "c\nd", n: 3}, + {v: "e\nf\ng", n: 5}, + {v: "h", n: 1}, + {v: "", n: 0}, + {v: "i\n", n: 2}, + {v: "j", n: 1}, + } { + n, err := ll.Write([]byte(e.v)) + require.NoError(t, err) + require.Equal(t, e.n, n) + } + require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=ghi\n", out.String()) + ll.Flush() + require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=ghi\nmsg=j\n", out.String()) +} + +func msgOnly(_ []string, a slog.Attr) slog.Attr { + switch a.Key { + case "time", "level": + return slog.Attr{} + } + return slog.Attr{Key: a.Key, Value: a.Value} +} From d952fd57f044c0bb38d3358c3e2597af4d0c6bbf Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 31 Oct 2024 19:11:37 -0400 Subject: [PATCH 21/24] fix --- lib/autoupdate/agent/process.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 37b6a10fb5aae..69eb482c4c03f 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -116,7 +116,7 @@ func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args stdout.Flush() code := cmd.ProcessState.ExitCode() - // treat out-of-range code as an error with OS executing command, not as intentional 255 + // Treat out-of-range code as an error with OS executing the command, not as an intentionally returned 255. if code == 255 { code = -1 } From 2c2afa6928e6b8f124d604b122c54c12e60da5cf Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 1 Nov 2024 12:05:59 -0400 Subject: [PATCH 22/24] fix test --- lib/autoupdate/agent/process_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/autoupdate/agent/process_test.go b/lib/autoupdate/agent/process_test.go index 8b86dd6e73cba..5ffa70dd0091e 100644 --- a/lib/autoupdate/agent/process_test.go +++ b/lib/autoupdate/agent/process_test.go @@ -42,12 +42,14 @@ func TestLineLogger(t *testing.T) { v string n int }{ + {v: "", n: 0}, {v: "a", n: 1}, {v: "b\n", n: 2}, {v: "c\nd", n: 3}, {v: "e\nf\ng", n: 5}, {v: "h", n: 1}, {v: "", n: 0}, + {v: "\n", n: 1}, {v: "i\n", n: 2}, {v: "j", n: 1}, } { @@ -55,9 +57,9 @@ func TestLineLogger(t *testing.T) { require.NoError(t, err) require.Equal(t, e.n, n) } - require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=ghi\n", out.String()) + require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=gh\nmsg=i\n", out.String()) ll.Flush() - require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=ghi\nmsg=j\n", out.String()) + require.Equal(t, "msg=ab\nmsg=c\nmsg=de\nmsg=f\nmsg=gh\nmsg=i\nmsg=j\n", out.String()) } func msgOnly(_ []string, a slog.Attr) slog.Attr { From 188450cfc53fdd70fe2e1a16d74aef9ece322d58 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Fri, 1 Nov 2024 12:11:24 -0400 Subject: [PATCH 23/24] clarify comment --- lib/autoupdate/agent/process.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/process.go b/lib/autoupdate/agent/process.go index 69eb482c4c03f..eba70aa56a690 100644 --- a/lib/autoupdate/agent/process.go +++ b/lib/autoupdate/agent/process.go @@ -116,7 +116,9 @@ func (s SystemdService) systemctl(ctx context.Context, errLevel slog.Level, args stdout.Flush() code := cmd.ProcessState.ExitCode() - // Treat out-of-range code as an error with OS executing the command, not as an intentionally returned 255. + // Treat out-of-range exit code (255) as an error executing the command. + // This allows callers to treat codes that are more likely OS-related as execution errors + // instead of intentionally returned error codes. if code == 255 { code = -1 } From 8be663277ec6e55efd4ead739742b72419dcc285 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 4 Nov 2024 13:56:28 -0500 Subject: [PATCH 24/24] use afterfunc --- lib/autoupdate/agent/installer.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index ce3b7a63fc07d..96e72c0a5cfa3 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -183,10 +183,9 @@ func (li *LocalInstaller) Install(ctx context.Context, version, template string, // If interrupted, close the file immediately to stop extracting. ctx, cancel := context.WithCancel(ctx) defer cancel() - go func() { - <-ctx.Done() - _ = f.Close() - }() + context.AfterFunc(ctx, func() { + _ = f.Close() // safe to close file multiple times + }) // Check integrity before decompression if !bytes.Equal(newSum, pathSum) { return trace.Errorf("mismatched checksum, download possibly corrupt")