feedback pt 1

lib/autoupdate/agent/process.go

@@ -322,13 +322,10 @@ func (s SystemdService) IsPresent(ctx context.Context) (bool, error) {
 		return false, trace.Wrap(err)
 	}
 	code := s.systemctl(ctx, slog.LevelDebug, "list-unit-files", "--quiet", s.ServiceName)
-	switch {
-	case code < 0:
+	if code < 0 {
 		return false, trace.Errorf("unable to determine if systemd service %s is present", s.ServiceName)
-	case code == 0:
-		return true, nil
 	}
-	return false, nil
+	return code == 0, nil
 }
 
 // checkSystem returns an error if the system is not compatible with this process manager.

lib/autoupdate/agent/setup.go

@@ -239,9 +239,10 @@ func (ns *Namespace) Setup(ctx context.Context) error {
 		}
 		if enabled {
 			if err := oldTimer.Disable(ctx, true); err != nil {
-				return trace.Wrap(err, "failed to disable deprecated teleport-upgrade systemd timer")
+				ns.log.ErrorContext(ctx, "The deprecated teleport-ent-updater package is installed on this server, and it cannot be disabled due to an error. You must remove the teleport-ent-updater package after verifying that teleport-update is working.", errorKey, err)
+			} else {
+				ns.log.WarnContext(ctx, "The deprecated teleport-ent-updater package is installed on this server. This package has been disabled to prevent conflicts. Please remove the teleport-ent-updater package after verifying that teleport-update is working.")
 			}
-			ns.log.WarnContext(ctx, "The deprecated teleport-ent-updater package is installed on this server. This package has been disabled to prevent conflicts. Please remove the teleport-ent-updater package after verifying that teleport-update is working.")
 		}
 	}
 	return nil
@@ -284,9 +285,10 @@ func (ns *Namespace) Teardown(ctx context.Context) error {
 		}
 		if present {
 			if err := oldTimer.Enable(ctx, true); err != nil {
-				return trace.Wrap(err, "failed to disable deprecated teleport-upgrade systemd timer")
+				ns.log.ErrorContext(ctx, "The deprecated teleport-ent-updater package is installed on this server, and it cannot be re-enabled due to an error. Please fix the teleport-ent-updater package if you intend to use the deprecated updater.", errorKey, err)
+			} else {
+				ns.log.WarnContext(ctx, "The deprecated teleport-ent-updater package is installed on this server. This package has been re-enabled to ensure continued updates. To disable automatic updates entirely, please remove the teleport-ent-updater package.")
 			}
-			ns.log.WarnContext(ctx, "The deprecated teleport-ent-updater package is installed on this server. This package has been re-enabled to ensure continued updates. To disable automatic updates entirely, please remove the teleport-ent-updater package.")
 		}
 	}
 	return nil

lib/autoupdate/agent/telemetry.go

@@ -26,36 +26,55 @@ import (
 	"github.com/gravitational/trace"
 )
 
-// IsActive returns true if the local Teleport binary is managed by teleport-update.
+// IsManagedByUpdater returns true if the local Teleport binary is managed by teleport-update.
 // Note that true may be returned even if auto-updates is disabled or the version is pinned.
-func IsActive() (bool, error) {
+// The binary is considered managed if it lives under /opt/teleport, but not within the package
+// path at /opt/teleport/system.
+func IsManagedByUpdater() (bool, error) {
 	teleportPath, err := os.Readlink("/proc/self/exe")
 	if err != nil {
 		return false, trace.Wrap(err, "cannot find Teleport binary")
 	}
-	updaterBasePath := filepath.Clean(teleportOptDir) + "/"
-	absPath, err := filepath.Abs(teleportPath)
+	// Check if current binary is under the updater-managed path.
+	managed, err := hasParentDir(teleportPath, teleportOptDir)
 	if err != nil {
-		return false, trace.Wrap(err, "cannot get absolute path for Teleport binary")
+		return false, trace.Wrap(err)
 	}
-	if !strings.HasPrefix(absPath, updaterBasePath) {
+	if !managed {
 		return false, nil
 	}
-	systemDir := filepath.Join(teleportOptDir, systemNamespace)
-	return !strings.HasPrefix(absPath, systemDir), nil
+	// Return false if the binary is under the updater-managed path, but in the system prefix reserved for the package.
+	system, err := hasParentDir(teleportPath, filepath.Join(teleportOptDir, systemNamespace))
+	return !system, err
 }
 
-// IsDefault returns true if the local Teleport binary is both managed by teleport-update
+// IsManagedAndDefault returns true if the local Teleport binary is both managed by teleport-update
 // and the default installation (with teleport.service as the unit file name).
-func IsDefault() (bool, error) {
+// The binary is considered managed and default if it lives within /opt/teleport/default.
+func IsManagedAndDefault() (bool, error) {
 	teleportPath, err := os.Readlink("/proc/self/exe")
 	if err != nil {
 		return false, trace.Wrap(err, "cannot find Teleport binary")
 	}
-	defaultPath := filepath.Join(teleportOptDir, defaultNamespace) + "/"
-	absPath, err := filepath.Abs(teleportPath)
+	return hasParentDir(teleportPath, filepath.Join(teleportOptDir, defaultNamespace))
+}
+
+// hasParentDir returns true if dir is any parent directory of parent.
+// hasParentDir does not resolve symlinks, and requires that files be represented the same way in dir and parent.
+func hasParentDir(dir, parent string) (bool, error) {
+	// Note that os.Stat + os.SameFile would be more reliable,
+	// but does not work well for arbitrarily nested subdirectories.
+	absDir, err := filepath.Abs(dir)
 	if err != nil {
-		return false, trace.Wrap(err, "cannot get absolute path for Teleport binary")
+		return false, trace.Wrap(err, "cannot get absolute path for directory %s", dir)
+	}
+	absParent, err := filepath.Abs(parent)
+	if err != nil {
+		return false, trace.Wrap(err, "cannot get absolute path for parent directory %s", dir)
+	}
+	sep := string(filepath.Separator)
+	if !strings.HasSuffix(absParent, sep) {
+		absParent += sep
 	}
-	return strings.HasPrefix(absPath, defaultPath), nil
+	return strings.HasPrefix(absDir, absParent), nil
 }

lib/autoupdate/agent/telemetry_test.go

@@ -0,0 +1,91 @@
+package agent
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestHasParentDir(t *testing.T) {
+	tests := []struct {
+		name       string
+		path       string
+		parent     string
+		wantResult bool
+	}{
+		{
+			name:       "Has valid parent directory",
+			path:       "/opt/teleport/dir/test",
+			parent:     "/opt/teleport",
+			wantResult: true,
+		},
+		{
+			name:       "Has valid parent directory with slash",
+			path:       "/opt/teleport/dir/test",
+			parent:     "/opt/teleport/",
+			wantResult: true,
+		},
+		{
+			name:       "Parent directory is root",
+			path:       "/opt/teleport/dir",
+			parent:     "/",
+			wantResult: true,
+		},
+		{
+			name:       "Parent is the same as the path",
+			path:       "/opt/teleport/dir",
+			parent:     "/opt/teleport/dir",
+			wantResult: false,
+		},
+		{
+			name:       "Parent the same as the path but without slash",
+			path:       "/opt/teleport/dir/",
+			parent:     "/opt/teleport/dir",
+			wantResult: false,
+		},
+		{
+			name:       "Parent the same as the path but with slash",
+			path:       "/opt/teleport/dir",
+			parent:     "/opt/teleport/dir/",
+			wantResult: false,
+		},
+		{
+			name:       "Parent is substring of the path",
+			path:       "/opt/teleport/dir-place",
+			parent:     "/opt/teleport/dir",
+			wantResult: false,
+		},
+		{
+			name:       "Parent is in path",
+			path:       "/opt/teleport",
+			parent:     "/opt/teleport/dir",
+			wantResult: false,
+		},
+		{
+			name:       "Empty parent",
+			path:       "/opt/teleport/dir",
+			parent:     "",
+			wantResult: false,
+		},
+		{
+			name:       "Empty path",
+			path:       "",
+			parent:     "/opt/teleport",
+			wantResult: false,
+		},
+		{
+			name:       "Both empty",
+			path:       "",
+			parent:     "",
+			wantResult: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := hasParentDir(tt.path, tt.parent)
+			require.NoError(t, err)
+			require.Equal(t, tt.wantResult, result)
+		})
+	}
+}

lib/service/service.go

@@ -1238,7 +1238,7 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
 	}
 
 	// If the installation is managed by teleport-update, it supersedes the teleport-upgrader script.
-	ok, err := autoupdate.IsActive()
+	ok, err := autoupdate.IsManagedByUpdater()
 	if err != nil {
 		process.logger.WarnContext(process.ExitContext(), "Failed to determine if auto-updates are enabled.", "error", err)
 	} else if ok {
@@ -1288,7 +1288,7 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
 
 		switch upgraderKind {
 		case types.UpgraderKindTeleportUpdate:
-			isDefault, err := autoupdate.IsDefault()
+			isDefault, err := autoupdate.IsManagedAndDefault()
 			if err != nil {
 				return nil, trace.Wrap(err)
 			}