cleanup tests, adjust service link path

lib/autoupdate/agent/installer.go

@@ -56,7 +56,7 @@ const (
 )
 
 var (
-	// serviceDir contains the path to the Teleport SystemD service dir within the version directory.
+	// serviceDir contains the relative path to the Teleport SystemD service dir.
 	serviceDir = filepath.Join("lib", "systemd", "system")
 	// serviceName contains the name of the Teleport SystemD service file.
 	serviceName = "teleport.service"
@@ -371,7 +371,7 @@ func tgzExtractPaths(ent bool) []utils.ExtractPath {
 		prefix += "-ent"
 	}
 	return []utils.ExtractPath{
-		{Src: path.Join(prefix, "examples/systemd/teleport.service"), Dst: "lib/systemd/system/teleport.service", DirMode: systemDirMode},
+		{Src: path.Join(prefix, "examples/systemd/teleport.service"), Dst: filepath.Join(serviceDir, serviceName), DirMode: systemDirMode},
 		{Src: path.Join(prefix, "examples"), Skip: true, DirMode: systemDirMode},
 		{Src: path.Join(prefix, "install"), Skip: true, DirMode: systemDirMode},
 		{Src: path.Join(prefix, "README.md"), Dst: "share/README.md", DirMode: systemDirMode},
@@ -659,7 +659,7 @@ func (li *LocalInstaller) TryLinkSystem(ctx context.Context) error {
 
 // tryLinks create binary and service links for files in binDir and svcDir if links are not already present.
 // Existing links that point to files outside binDir or svcDir, as well as existing non-link files, will error.
-// tryLinks will not attempt to create any links if linking may result in an error.
+// tryLinks will not attempt to create any links if linking could result in an error.
 // However, concurrent changes to links may result in an error with partially-complete linking.
 func (li *LocalInstaller) tryLinks(ctx context.Context, binDir, svcDir string) error {
 	// ensure target directories exist before trying to create links
@@ -700,21 +700,21 @@ func (li *LocalInstaller) tryLinks(ctx context.Context, binDir, svcDir string) e
 		return trace.Errorf("no binaries available to link")
 	}
 
-	// validate that we can create the systemd service file before attempting linking
+	// link binaries that are missing links
+	for _, link := range links {
+		if err := os.Symlink(link.oldname, link.newname); err != nil {
+			return trace.Errorf("failed to create symlink for %s: %w", filepath.Base(link.oldname), err)
+		}
+	}
 
+	// if any binaries are linked from binDir, always link the service from svcDir
 	src := filepath.Join(svcDir, serviceName)
 	dst := filepath.Join(li.LinkServiceDir, serviceName)
-	err = tryCopy(dst, src, maxServiceFileSize)
+	_, err = forceCopy(dst, src, maxServiceFileSize)
 	if err != nil && !errors.Is(err, os.ErrExist) {
 		return trace.Errorf("error writing %s: %w", serviceName, err)
 	}
 
-	for _, link := range links {
-		if err := os.Symlink(link.oldname, link.newname); err != nil {
-			return trace.Errorf("failed to create symlink for %s: %w", filepath.Base(link.oldname), err)
-		}
-	}
-
 	return nil
 }
 
@@ -747,40 +747,6 @@ func needsLink(oldname, newname string) (ok bool, err error) {
 	return false, nil
 }
 
-// tryCopy attempts to atomically copy a file from src to dst.
-// tryCopy errors if a file or directory already exists at dst.
-// tryCopy returns ErrLinked if a regular file smaller than n with different data already exists.
-// tryCopy returns os.ErrExist if a regular file smaller than n with the same contents exists.
-// The src and any dst file must be smaller than n, or tryCopy errors.
-func tryCopy(dst, src string, n int64) error {
-	srcData, err := readFileN(src, n)
-	if err != nil {
-		return trace.Wrap(err)
-	}
-	fi, err := os.Lstat(dst)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return trace.Wrap(err)
-	}
-	if err == nil { // dst exists
-		if !fi.Mode().IsRegular() {
-			return trace.Errorf("refusing to replace irregular file at %s", dst)
-		}
-		dstData, err := readFileN(dst, n)
-		if err != nil {
-			return trace.Wrap(err)
-		}
-		if bytes.Equal(srcData, dstData) {
-			return os.ErrExist
-		}
-		return trace.Errorf("refusing to replace file at %s: %w", dst, ErrLinked)
-	}
-	err = renameio.WriteFile(dst, srcData, configFileMode)
-	if err != nil {
-		return trace.Wrap(err)
-	}
-	return 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.

lib/autoupdate/agent/testdata/TestUpdater_Enable/insecure_URL.golden

@@ -0,0 +1,10 @@
+version: v1
+kind: update_config
+spec:
+    proxy: ""
+    group: ""
+    url_template: http://example.com
+    enabled: false
+status:
+    active_version: ""
+    backup_version: ""

lib/autoupdate/agent/testdata/TestUpdater_LinkPackage/updates_disabled.golden → lib/autoupdate/agent/testdata/TestUpdater_Enable/invalid_metadata.golden

@@ -1,5 +1,5 @@
-version: v1
-kind: update_config
+version: ""
+kind: ""
 spec:
     proxy: ""
     group: ""

lib/autoupdate/agent/testdata/TestUpdater_Update/insecure_URL.golden

@@ -0,0 +1,10 @@
+version: v1
+kind: update_config
+spec:
+    proxy: localhost
+    group: ""
+    url_template: http://example.com
+    enabled: true
+status:
+    active_version: ""
+    backup_version: ""

lib/autoupdate/agent/testdata/TestUpdater_LinkPackage/updates_enabled.golden → lib/autoupdate/agent/testdata/TestUpdater_Update/install_error.golden

@@ -1,7 +1,7 @@
 version: v1
 kind: update_config
 spec:
-    proxy: ""
+    proxy: localhost
     group: ""
     url_template: ""
     enabled: true

lib/autoupdate/agent/testdata/TestUpdater_Update/invalid_metadata.golden

@@ -0,0 +1,10 @@
+version: ""
+kind: ""
+spec:
+    proxy: localhost
+    group: ""
+    url_template: ""
+    enabled: false
+status:
+    active_version: ""
+    backup_version: ""

lib/autoupdate/agent/testdata/TestUpdater_Update/reload_fails.golden

@@ -0,0 +1,10 @@
+version: v1
+kind: update_config
+spec:
+    proxy: localhost
+    group: ""
+    url_template: https://example.com
+    enabled: true
+status:
+    active_version: old-version
+    backup_version: backup-version

lib/autoupdate/agent/testdata/TestUpdater_Update/sync_fails.golden

@@ -0,0 +1,10 @@
+version: v1
+kind: update_config
+spec:
+    proxy: localhost
+    group: ""
+    url_template: https://example.com
+    enabled: true
+status:
+    active_version: old-version
+    backup_version: backup-version

lib/autoupdate/agent/updater.go

@@ -41,6 +41,13 @@ import (
 	libutils "github.com/gravitational/teleport/lib/utils"
 )
 
+const (
+	// DefaultLinkDir is the default location where Teleport is linked.
+	DefaultLinkDir = "/usr/local"
+	// DefaultSystemDir is the location where packaged Teleport binaries and services are installed.
+	DefaultSystemDir = "/usr/local/teleport-system"
+)
+
 const (
 	// cdnURITemplate is the default template for the Teleport tgz download.
 	cdnURITemplate = "https://cdn.teleport.dev/teleport{{if .Enterprise}}-ent{{end}}-v{{.Version}}-{{.OS}}-{{.Arch}}{{if .FIPS}}-fips{{end}}-bin.tar.gz"
@@ -50,6 +57,14 @@ const (
 	reservedFreeDisk = 10_000_000 // 10 MB
 )
 
+// Log keys
+const (
+	targetVersionKey = "target_version"
+	activeVersionKey = "active_version"
+	backupVersionKey = "backup_version"
+	errorKey         = "error"
+)
+
 const (
 	// updateConfigName specifies the name of the file inside versionsDirName containing configuration for the teleport update.
 	updateConfigName = "update.yaml"
@@ -59,14 +74,6 @@ const (
 	updateConfigKind    = "update_config"
 )
 
-// Log keys
-const (
-	targetVersionKey = "target_version"
-	activeVersionKey = "active_version"
-	backupVersionKey = "backup_version"
-	errorKey         = "error"
-)
-
 // UpdateConfig describes the update.yaml file schema.
 type UpdateConfig struct {
 	// Version of the configuration file
@@ -124,10 +131,10 @@ func NewLocalUpdater(cfg LocalUpdaterConfig) (*Updater, error) {
 		cfg.Log = slog.Default()
 	}
 	if cfg.LinkDir == "" {
-		cfg.LinkDir = "/usr/local"
+		cfg.LinkDir = DefaultLinkDir
 	}
 	if cfg.SystemDir == "" {
-		cfg.SystemDir = "/usr/local/teleport-system"
+		cfg.SystemDir = DefaultSystemDir
 	}
 	if cfg.VersionsDir == "" {
 		cfg.VersionsDir = filepath.Join(libdefaults.DataDir, "versions")
@@ -142,9 +149,9 @@ func NewLocalUpdater(cfg LocalUpdaterConfig) (*Updater, error) {
 			LinkBinDir: filepath.Join(cfg.LinkDir, "bin"),
 			// For backwards-compatibility with symlinks created by package-based installs, we always
 			// link into /lib/systemd/system, even though, e.g., /usr/local/lib/systemd/system would work.
-			LinkServiceDir:          "/lib/systemd/system",
+			LinkServiceDir:          filepath.Join("/", serviceDir),
 			SystemBinDir:            filepath.Join(cfg.SystemDir, "bin"),
-			SystemServiceDir:        filepath.Join(cfg.SystemDir, "lib", "systemd", "system"),
+			SystemServiceDir:        filepath.Join(cfg.SystemDir, serviceDir),
 			HTTP:                    client,
 			Log:                     cfg.Log,
 			ReservedFreeTmpDisk:     reservedFreeDisk,
@@ -637,6 +644,10 @@ 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)
+	}
 	u.Log.InfoContext(ctx, "Successfully linked system package installation.")
 	return nil
 }