From 980f655b282488a9eb12713f5f52d007ad104456 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Tue, 19 Nov 2024 22:59:29 -0500 Subject: [PATCH 1/6] unlink --- lib/autoupdate/agent/installer.go | 113 +++++++++++++++++++++++---- lib/autoupdate/agent/updater.go | 19 ++++- lib/autoupdate/agent/updater_test.go | 5 ++ tool/teleport-update/main.go | 36 +++++++++ 4 files changed, 156 insertions(+), 17 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index f03401063a3b8..5bb0d9f2ebd8c 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -443,6 +443,46 @@ func (li *LocalInstaller) LinkSystem(ctx context.Context) (revert func(context.C return revert, trace.Wrap(err) } +// TryLink links the specified version, but only in the case that +// no installation of Teleport is already linked or partially linked. +// See Installer interface for additional specs. +func (li *LocalInstaller) TryLink(ctx context.Context, version string) error { + versionDir, err := li.versionDir(version) + if err != nil { + return trace.Wrap(err) + } + return trace.Wrap(li.tryLinks(ctx, + filepath.Join(versionDir, "bin"), + filepath.Join(versionDir, serviceDir), + )) +} + +// TryLinkSystem links the system installation, but only in the case that +// no installation of Teleport is already linked or partially linked. +// See Installer interface for additional specs. +func (li *LocalInstaller) TryLinkSystem(ctx context.Context) error { + return trace.Wrap(li.tryLinks(ctx, li.SystemBinDir, li.SystemServiceDir)) +} + +// Unlink unlinks a version from LinkBinDir and LinkServiceDir. +// See Installer interface for additional specs. +func (li *LocalInstaller) Unlink(ctx context.Context, version string) error { + versionDir, err := li.versionDir(version) + if err != nil { + return trace.Wrap(err) + } + return trace.Wrap(li.removeLinks(ctx, + filepath.Join(versionDir, "bin"), + filepath.Join(versionDir, serviceDir), + )) +} + +// UnlinkSystem unlinks the system (package) version from LinkBinDir and LinkServiceDir. +// See Installer interface for additional specs. +func (li *LocalInstaller) UnlinkSystem(ctx context.Context) error { + return trace.Wrap(li.removeLinks(ctx, li.SystemBinDir, li.SystemServiceDir)) +} + // symlink from oldname to newname type symlink struct { oldname, newname string @@ -640,25 +680,66 @@ func readFileN(name string, n int64) ([]byte, error) { return data, trace.Wrap(err) } -// TryLink links the specified version, but only in the case that -// no installation of Teleport is already linked or partially linked. -// See Installer interface for additional specs. -func (li *LocalInstaller) TryLink(ctx context.Context, version string) error { - versionDir, err := li.versionDir(version) +func (li *LocalInstaller) removeLinks(ctx context.Context, binDir, svcDir string) error { + removeService := false + entries, err := os.ReadDir(binDir) + if err != nil { + return trace.Errorf("failed to find Teleport binary directory: %w", err) + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + oldname := filepath.Join(binDir, entry.Name()) + newname := filepath.Join(li.LinkBinDir, entry.Name()) + v, err := os.Readlink(newname) + if errors.Is(err, os.ErrNotExist) || + errors.Is(err, os.ErrInvalid) || + errors.Is(err, syscall.EINVAL) { + li.Log.DebugContext(ctx, "Link not present.", "oldname", oldname, "newname", newname) + continue + } + if err != nil { + return trace.Errorf("error reading link for %s: %w", filepath.Base(newname), err) + } + if v != oldname { + li.Log.DebugContext(ctx, "Skipping link to different binary.", "oldname", oldname, "newname", newname) + continue + } + if err := os.Remove(newname); err != nil { + return trace.Errorf("error removing link for %s: %w", filepath.Base(newname), err) + } + if filepath.Dir(newname) == "teleport" { + removeService = true + } + } + // only remove service if teleport was removed + if !removeService { + li.Log.DebugContext(ctx, "Teleport binary not unlinked. Skipping removal of teleport.service.") + return nil + } + src := filepath.Join(svcDir, serviceName) + srcBytes, err := readFileN(src, maxServiceFileSize) if err != nil { return trace.Wrap(err) } - return trace.Wrap(li.tryLinks(ctx, - filepath.Join(versionDir, "bin"), - filepath.Join(versionDir, serviceDir), - )) -} - -// TryLinkSystem links the system installation, but only in the case that -// no installation of Teleport is already linked or partially linked. -// See Installer interface for additional specs. -func (li *LocalInstaller) TryLinkSystem(ctx context.Context) error { - return trace.Wrap(li.tryLinks(ctx, li.SystemBinDir, li.SystemServiceDir)) + dst := filepath.Join(li.LinkServiceDir, serviceName) + dstBytes, err := readFileN(dst, maxServiceFileSize) + if errors.Is(err, os.ErrNotExist) { + li.Log.DebugContext(ctx, "Service not present.", "path", dst) + return nil + } + if err != nil { + return trace.Wrap(err) + } + if !bytes.Equal(srcBytes, dstBytes) { + li.Log.WarnContext(ctx, "Removed teleport binary link, but skipping removal of custom teleport.service.") + return nil + } + if err := os.Remove(dst); err != nil { + return trace.Errorf("error removing copy of %s: %w", filepath.Base(dst), err) + } + return nil } // tryLinks create binary and service links for files in binDir and svcDir if links are not already present. diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 8fa5d34c246c2..3f9e1a99dacef 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -255,10 +255,13 @@ type Installer interface { // Unlike Link, TryLink will fail if existing links to other locations are present. // TryLink must be idempotent. TryLink(ctx context.Context, version string) error - // TryLinkSystem links the system installation of Teleport into the linking locations. + // TryLinkSystem links the system (package) installation of Teleport into the linking locations. // Unlike LinkSystem, TryLinkSystem will fail if existing links to other locations are present. // TryLinkSystem must be idempotent. TryLinkSystem(ctx context.Context) error + // UnlinkSystem unlinks the system (package) installation of Teleport from the linking locations. + // TryLinkSystem must be idempotent. + UnlinkSystem(ctx context.Context) error // List the installed versions of Teleport. List(ctx context.Context) (versions []string, err error) // Remove the Teleport agent at version. @@ -717,3 +720,17 @@ func (u *Updater) LinkPackage(ctx context.Context) error { u.Log.InfoContext(ctx, "Successfully linked system package installation.") return nil } + +// UnlinkPackage removes links from the system (package) installation of Teleport, if they are present. +// This function is idempotent. +func (u *Updater) UnlinkPackage(ctx context.Context) error { + if err := u.Installer.UnlinkSystem(ctx); err != nil { + return trace.Errorf("failed to unlink system package installation: %w", err) + } + // TODO(sclevine): only if systemd files change + if err := u.Process.Sync(ctx); err != nil { + return trace.Errorf("failed to validate configuration for packaged installation of Teleport: %w", err) + } + u.Log.InfoContext(ctx, "Successfully linked system package installation.") + return nil +} diff --git a/lib/autoupdate/agent/updater_test.go b/lib/autoupdate/agent/updater_test.go index 01e6e4980f5de..e553241981134 100644 --- a/lib/autoupdate/agent/updater_test.go +++ b/lib/autoupdate/agent/updater_test.go @@ -996,6 +996,7 @@ type testInstaller struct { FuncLinkSystem func(ctx context.Context) (revert func(context.Context) bool, err error) FuncTryLink func(ctx context.Context, version string) error FuncTryLinkSystem func(ctx context.Context) error + FuncUnlinkSystem func(ctx context.Context) error FuncList func(ctx context.Context) (versions []string, err error) } @@ -1023,6 +1024,10 @@ func (ti *testInstaller) TryLinkSystem(ctx context.Context) error { return ti.FuncTryLinkSystem(ctx) } +func (ti *testInstaller) UnlinkSystem(ctx context.Context) error { + return ti.FuncUnlinkSystem(ctx) +} + func (ti *testInstaller) List(ctx context.Context) (versions []string, err error) { return ti.FuncList(ctx) } diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index 1db37feae4954..ae53f33dc1598 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -126,6 +126,7 @@ func Run(args []string) error { Short('s').Hidden().BoolVar(&ccfg.SelfSetup) linkCmd := app.Command("link-package", "Link the system installation of Teleport from the Teleport package, if auto-updates is disabled.") + unlinkCmd := app.Command("unlink-package", "Unlink the system installation of Teleport from the Teleport package.") setupCmd := app.Command("setup", "Write configuration files that run the update subcommand on a timer."). Hidden() @@ -151,6 +152,8 @@ func Run(args []string) error { err = cmdUpdate(ctx, &ccfg) case linkCmd.FullCommand(): err = cmdLink(ctx, &ccfg) + case unlinkCmd.FullCommand(): + err = cmdUnlink(ctx, &ccfg) case setupCmd.FullCommand(): err = cmdSetup(ctx, &ccfg) case versionCmd.FullCommand(): @@ -299,6 +302,39 @@ func cmdLink(ctx context.Context, ccfg *cliConfig) error { return nil } +// cmdUnlink remove system package links. +func cmdUnlink(ctx context.Context, ccfg *cliConfig) error { + updater, err := autoupdate.NewLocalUpdater(autoupdate.LocalUpdaterConfig{ + DataDir: ccfg.DataDir, + LinkDir: ccfg.LinkDir, + SystemDir: autoupdate.DefaultSystemDir, + SelfSetup: ccfg.SelfSetup, + Log: plog, + }) + if err != nil { + return trace.Errorf("failed to setup updater: %w", err) + } + + // Error if the updater is running. We could remove its links by accident. + unlock, err := libutils.FSTryReadLock(filepath.Join(ccfg.DataDir, lockFileName)) + if errors.Is(err, libutils.ErrUnsuccessfulLockTry) { + return trace.Errorf("updater is currently running") + } + if err != nil { + return trace.Errorf("failed to grab concurrent execution lock: %w", err) + } + defer func() { + if err := unlock(); err != nil { + plog.DebugContext(ctx, "Failed to close lock file", "error", err) + } + }() + + if err := updater.UnlinkPackage(ctx); err != nil { + return trace.Wrap(err) + } + return nil +} + // cmdSetup writes configuration files that are needed to run teleport-update update. func cmdSetup(ctx context.Context, ccfg *cliConfig) error { err := autoupdate.Setup(ctx, plog, ccfg.LinkDir, ccfg.DataDir) From 84a0bed96d4ddedb2afb52b83672a2d1ea52e427 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Wed, 20 Nov 2024 00:50:01 -0500 Subject: [PATCH 2/6] test --- lib/autoupdate/agent/installer.go | 5 +- lib/autoupdate/agent/installer_test.go | 188 ++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 6 deletions(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index 5bb0d9f2ebd8c..bd7a3aa26ec6e 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -707,9 +707,10 @@ func (li *LocalInstaller) removeLinks(ctx context.Context, binDir, svcDir string continue } if err := os.Remove(newname); err != nil { - return trace.Errorf("error removing link for %s: %w", filepath.Base(newname), err) + li.Log.ErrorContext(ctx, "Unable to remove link.", "oldname", oldname, "newname", newname, errorKey, err) + continue } - if filepath.Dir(newname) == "teleport" { + if filepath.Base(newname) == "teleport" { removeService = true } } diff --git a/lib/autoupdate/agent/installer_test.go b/lib/autoupdate/agent/installer_test.go index ee9701ac2202c..9b9c9b268490e 100644 --- a/lib/autoupdate/agent/installer_test.go +++ b/lib/autoupdate/agent/installer_test.go @@ -32,6 +32,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strconv" "strings" "testing" @@ -393,7 +394,7 @@ func TestLocalInstaller_Link(t *testing.T) { installer := &LocalInstaller{ InstallDir: versionsDir, LinkBinDir: filepath.Join(linkDir, "bin"), - LinkServiceDir: filepath.Join(linkDir, "lib/systemd/system"), + LinkServiceDir: filepath.Join(linkDir, serviceDir), Log: slog.Default(), } ctx := context.Background() @@ -635,7 +636,7 @@ func TestLocalInstaller_TryLink(t *testing.T) { installer := &LocalInstaller{ InstallDir: versionsDir, LinkBinDir: filepath.Join(linkDir, "bin"), - LinkServiceDir: filepath.Join(linkDir, "lib/systemd/system"), + LinkServiceDir: filepath.Join(linkDir, serviceDir), Log: slog.Default(), } ctx := context.Background() @@ -773,8 +774,8 @@ func TestLocalInstaller_Remove(t *testing.T) { installer := &LocalInstaller{ InstallDir: versionsDir, - LinkBinDir: linkDir, - LinkServiceDir: linkDir, + LinkBinDir: filepath.Join(linkDir, "bin"), + LinkServiceDir: filepath.Join(linkDir, serviceDir), Log: slog.Default(), } ctx := context.Background() @@ -796,6 +797,185 @@ func TestLocalInstaller_Remove(t *testing.T) { } } +func TestLocalInstaller_Unlink(t *testing.T) { + t.Parallel() + const version = "existing-version" + servicePath := filepath.Join(serviceDir, serviceName) + + tests := []struct { + name string + bins []string + svcOrig []byte + + links []symlink + svcCopy []byte + + remaining []string + errMatch string + }{ + { + name: "normal", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + svcCopy: []byte("orig"), + }, + { + name: "different services", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + svcCopy: []byte("custom"), + remaining: []string{servicePath}, + }, + { + name: "missing target service", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + }, + { + name: "missing source service", + bins: []string{"teleport", "tsh"}, + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + svcCopy: []byte("custom"), + remaining: []string{servicePath}, + errMatch: "no such", + }, + { + name: "missing teleport link", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + svcCopy: []byte("orig"), + remaining: []string{servicePath}, + }, + { + name: "missing other link", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + }, + svcCopy: []byte("orig"), + }, + { + name: "wrong teleport link", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "other", newname: "bin/teleport"}, + {oldname: "bin/tsh", newname: "bin/tsh"}, + }, + svcCopy: []byte("orig"), + remaining: []string{servicePath, "bin/teleport"}, + }, + { + name: "wrong other link", + bins: []string{"teleport", "tsh"}, + svcOrig: []byte("orig"), + links: []symlink{ + {oldname: "bin/teleport", newname: "bin/teleport"}, + {oldname: "wrong", newname: "bin/tsh"}, + }, + svcCopy: []byte("orig"), + remaining: []string{"bin/tsh"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + versionsDir := t.TempDir() + versionDir := filepath.Join(versionsDir, version) + err := os.MkdirAll(versionDir, 0o755) + require.NoError(t, err) + linkDir := t.TempDir() + + var files []smallFile + for _, n := range tt.bins { + files = append(files, smallFile{ + name: filepath.Join(versionDir, "bin", n), + data: []byte("binary"), + mode: os.ModePerm, + }) + } + if tt.svcOrig != nil { + files = append(files, smallFile{ + name: filepath.Join(versionDir, servicePath), + data: tt.svcOrig, + mode: os.ModePerm, + }) + } + if tt.svcCopy != nil { + files = append(files, smallFile{ + name: filepath.Join(linkDir, servicePath), + data: tt.svcCopy, + mode: os.ModePerm, + }) + } + + for _, n := range files { + err = os.MkdirAll(filepath.Dir(n.name), os.ModePerm) + require.NoError(t, err) + err = os.WriteFile(n.name, n.data, n.mode) + require.NoError(t, err) + } + for _, n := range tt.links { + newname := filepath.Join(linkDir, n.newname) + oldname := filepath.Join(versionDir, n.oldname) + err = os.MkdirAll(filepath.Dir(newname), os.ModePerm) + require.NoError(t, err) + err = os.Symlink(oldname, newname) + require.NoError(t, err) + } + + installer := &LocalInstaller{ + InstallDir: versionsDir, + LinkBinDir: filepath.Join(linkDir, "bin"), + LinkServiceDir: filepath.Join(linkDir, serviceDir), + Log: slog.Default(), + } + ctx := context.Background() + err = installer.Unlink(ctx, version) + if tt.errMatch != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMatch) + } else { + require.NoError(t, err) + } + for _, n := range tt.remaining { + _, err = os.Lstat(filepath.Join(linkDir, n)) + require.NoError(t, err) + } + for _, n := range tt.links { + if slices.Contains(tt.remaining, n.newname) { + continue + } + _, err = os.Lstat(filepath.Join(linkDir, n.newname)) + require.ErrorIs(t, err, os.ErrNotExist) + } + if !slices.Contains(tt.remaining, servicePath) { + _, err = os.Lstat(filepath.Join(linkDir, servicePath)) + require.ErrorIs(t, err, os.ErrNotExist) + } + }) + } +} + func TestLocalInstaller_List(t *testing.T) { installDir := t.TempDir() versions := []string{"v1", "v2"} From f42977a4cf32344448b7677e7235b522ab6c9d30 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Wed, 20 Nov 2024 01:32:02 -0500 Subject: [PATCH 3/6] lock type --- tool/teleport-update/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index ae53f33dc1598..2e53e10c7b905 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -316,7 +316,7 @@ func cmdUnlink(ctx context.Context, ccfg *cliConfig) error { } // Error if the updater is running. We could remove its links by accident. - unlock, err := libutils.FSTryReadLock(filepath.Join(ccfg.DataDir, lockFileName)) + unlock, err := libutils.FSTryWriteLock(filepath.Join(ccfg.DataDir, lockFileName)) if errors.Is(err, libutils.ErrUnsuccessfulLockTry) { return trace.Errorf("updater is currently running") } From 2c084cdea45c0be4face00d16ca3679b6da25e1c Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Wed, 20 Nov 2024 01:38:03 -0500 Subject: [PATCH 4/6] comments --- lib/autoupdate/agent/updater.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 3f9e1a99dacef..58ec5b0d98a25 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -713,9 +713,8 @@ func (u *Updater) LinkPackage(ctx context.Context) error { } else if err != nil { return trace.Errorf("failed to link system package installation: %w", err) } - // TODO(sclevine): only if systemd files change if err := u.Process.Sync(ctx); err != nil { - return trace.Errorf("failed to validate configuration for packaged installation of Teleport: %w", err) + return trace.Errorf("failed to sync systemd configuration: %w", err) } u.Log.InfoContext(ctx, "Successfully linked system package installation.") return nil @@ -727,10 +726,9 @@ func (u *Updater) UnlinkPackage(ctx context.Context) error { if err := u.Installer.UnlinkSystem(ctx); err != nil { return trace.Errorf("failed to unlink system package installation: %w", err) } - // TODO(sclevine): only if systemd files change if err := u.Process.Sync(ctx); err != nil { - return trace.Errorf("failed to validate configuration for packaged installation of Teleport: %w", err) + return trace.Errorf("failed to sync systemd configuration: %w", err) } - u.Log.InfoContext(ctx, "Successfully linked system package installation.") + u.Log.InfoContext(ctx, "Successfully unlinked system package installation.") return nil } From 3de9c67efb51a1c2267e57e97aa46ed53d863821 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 21 Nov 2024 00:50:38 -0500 Subject: [PATCH 5/6] cleanup --- lib/autoupdate/agent/installer.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go index bd7a3aa26ec6e..790c8dcd28560 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -39,6 +39,7 @@ import ( "github.com/google/renameio/v2" "github.com/gravitational/trace" + "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/utils" ) @@ -710,7 +711,7 @@ func (li *LocalInstaller) removeLinks(ctx context.Context, binDir, svcDir string li.Log.ErrorContext(ctx, "Unable to remove link.", "oldname", oldname, "newname", newname, errorKey, err) continue } - if filepath.Base(newname) == "teleport" { + if filepath.Base(newname) == teleport.ComponentTeleport { removeService = true } } From c8240e4ac09ebecf08a7ee405d5a80e16f5e5cd6 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Thu, 21 Nov 2024 11:35:02 -0500 Subject: [PATCH 6/6] Update lib/autoupdate/agent/installer.go Co-authored-by: Hugo Shaka --- 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 790c8dcd28560..a723b02aa3379 100644 --- a/lib/autoupdate/agent/installer.go +++ b/lib/autoupdate/agent/installer.go @@ -735,7 +735,7 @@ func (li *LocalInstaller) removeLinks(ctx context.Context, binDir, svcDir string return trace.Wrap(err) } if !bytes.Equal(srcBytes, dstBytes) { - li.Log.WarnContext(ctx, "Removed teleport binary link, but skipping removal of custom teleport.service.") + li.Log.WarnContext(ctx, "Removed teleport binary link, but skipping removal of custom teleport.service: the service file does not match the reference file for this version. The file might have been manually edited.") return nil } if err := os.Remove(dst); err != nil {