From 5156850b830e2824185a6e6f54996f889f1a2daa Mon Sep 17 00:00:00 2001 From: Vadym Popov Date: Thu, 16 Jan 2025 22:01:36 -0800 Subject: [PATCH] Restore priority of env check over remote check In case of double re-execution case we should stop second one to prevent loop re-execution Drop localDir set during compilation --- integration/autoupdate/tools/main_test.go | 1 - .../autoupdate/tools/updater_tsh_test.go | 16 +++++++--- lib/autoupdate/tools/helper.go | 22 +++++-------- lib/autoupdate/tools/updater.go | 32 +++++++++++++++++-- 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/integration/autoupdate/tools/main_test.go b/integration/autoupdate/tools/main_test.go index 10517f7d1e0f0..71173892ca2f1 100644 --- a/integration/autoupdate/tools/main_test.go +++ b/integration/autoupdate/tools/main_test.go @@ -164,7 +164,6 @@ func buildBinary(output string, toolsDir string, version string, baseURL string, "go", "build", "-o", output, "-ldflags", strings.Join([]string{ fmt.Sprintf("-X 'github.com/gravitational/teleport/integration/autoupdate/tools/updater.version=%s'", version), - fmt.Sprintf("-X 'github.com/gravitational/teleport/lib/autoupdate/tools.toolsDir=%s'", toolsDir), fmt.Sprintf("-X 'github.com/gravitational/teleport/lib/autoupdate/tools.version=%s'", version), fmt.Sprintf("-X 'github.com/gravitational/teleport/lib/autoupdate/tools.baseURL=%s'", baseURL), }, " "), diff --git a/integration/autoupdate/tools/updater_tsh_test.go b/integration/autoupdate/tools/updater_tsh_test.go index 661b6bec18df7..01b9ca7679a42 100644 --- a/integration/autoupdate/tools/updater_tsh_test.go +++ b/integration/autoupdate/tools/updater_tsh_test.go @@ -45,7 +45,13 @@ import ( // and after auto update this not leads to recursive alias re-execution. func TestAliasLoginWithUpdater(t *testing.T) { ctx := context.Background() - t.Setenv(types.HomeEnvVar, toolsDir) + + homeDir := filepath.Join(t.TempDir(), "home") + require.NoError(t, os.MkdirAll(homeDir, 0700)) + installDir := filepath.Join(t.TempDir(), "local") + require.NoError(t, os.MkdirAll(installDir, 0700)) + + t.Setenv(types.HomeEnvVar, homeDir) alice, err := types.NewUser("alice") require.NoError(t, err) @@ -94,9 +100,9 @@ func TestAliasLoginWithUpdater(t *testing.T) { // Assign alias to the login command for test cluster. proxyAddr, err := rootServer.ProxyWebAddr() require.NoError(t, err) - configPath := filepath.Join(toolsDir, client.TSHConfigPath) + configPath := filepath.Join(homeDir, client.TSHConfigPath) require.NoError(t, os.MkdirAll(filepath.Dir(configPath), 0700)) - executable := filepath.Join(toolsDir, "tsh") + executable := filepath.Join(installDir, "tsh") out, err := yaml.Marshal(client.TSHConfig{ Aliases: map[string]string{ "loginalice": fmt.Sprintf( @@ -109,7 +115,7 @@ func TestAliasLoginWithUpdater(t *testing.T) { require.NoError(t, os.WriteFile(configPath, out, 0600)) // Fetch compiled test binary and install to tools dir [v1.2.3]. - err = tools.NewUpdater(toolsDir, testVersions[0], tools.WithBaseURL(baseURL)).Update(ctx, testVersions[0]) + err = tools.NewUpdater(installDir, testVersions[0], tools.WithBaseURL(baseURL)).Update(ctx, testVersions[0]) require.NoError(t, err) // Execute alias command which must be transformed to the login command. @@ -122,7 +128,7 @@ func TestAliasLoginWithUpdater(t *testing.T) { require.NoError(t, cmd.Run()) // Verify tctl status after login. - cmd = exec.CommandContext(ctx, filepath.Join(toolsDir, "tctl"), "status", "--insecure") + cmd = exec.CommandContext(ctx, filepath.Join(installDir, "tctl"), "status", "--insecure") cmd.Env = os.Environ() cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/lib/autoupdate/tools/helper.go b/lib/autoupdate/tools/helper.go index 3c078773ecaff..a3322f88d767b 100644 --- a/lib/autoupdate/tools/helper.go +++ b/lib/autoupdate/tools/helper.go @@ -36,8 +36,6 @@ var ( version = teleport.Version // baseURL is CDN URL for downloading official Teleport packages. baseURL = defaultBaseURL - // toolsDir is client tools directory for package download and installation. - toolsDir = "" ) // CheckAndUpdateLocal verifies if the TELEPORT_TOOLS_VERSION environment variable @@ -48,12 +46,10 @@ var ( // If $TELEPORT_HOME/bin contains downloaded client tools, it always re-executes // using the version from the home directory. func CheckAndUpdateLocal(ctx context.Context, reExecArgs []string) error { - var err error - if toolsDir == "" { - if toolsDir, err = Dir(); err != nil { - slog.WarnContext(ctx, "Client tools update is disabled", "error", err) - return nil - } + toolsDir, err := Dir() + if err != nil { + slog.WarnContext(ctx, "Client tools update is disabled", "error", err) + return nil } updater := NewUpdater(toolsDir, version, WithBaseURL(baseURL)) @@ -79,12 +75,10 @@ func CheckAndUpdateLocal(ctx context.Context, reExecArgs []string) error { // If $TELEPORT_HOME/bin contains downloaded client tools, it always re-executes // using the version from the home directory. func CheckAndUpdateRemote(ctx context.Context, proxy string, insecure bool, reExecArgs []string) error { - var err error - if toolsDir == "" { - if toolsDir, err = Dir(); err != nil { - slog.WarnContext(ctx, "Client tools update is disabled", "error", err) - return nil - } + toolsDir, err := Dir() + if err != nil { + slog.WarnContext(ctx, "Client tools update is disabled", "error", err) + return nil } updater := NewUpdater(toolsDir, version, WithBaseURL(baseURL)) // The user has typed a command like `tsh ssh ...` without being logged in, diff --git a/lib/autoupdate/tools/updater.go b/lib/autoupdate/tools/updater.go index 9ca924b15d536..4fc70ff064049 100644 --- a/lib/autoupdate/tools/updater.go +++ b/lib/autoupdate/tools/updater.go @@ -155,11 +155,31 @@ func (u *Updater) CheckLocal() (version string, reExec bool, err error) { return toolsVersion, true, nil } -// CheckRemote checks against the Proxy Service to determine if client tools need updating by requesting +// CheckRemote first checks the version set by the environment variable. If not set or disabled, +// it checks against the Proxy Service to determine if client tools need updating by requesting // the `webapi/find` handler, which stores information about the required client tools version to // operate with this cluster. It returns the semantic version that needs updating and whether // re-execution is necessary, by re-execution flag we understand that update and re-execute is required. func (u *Updater) CheckRemote(ctx context.Context, proxyAddr string, insecure bool) (version string, reExec bool, err error) { + // Check if the user has requested a specific version of client tools. + requestedVersion := os.Getenv(teleportToolsVersionEnv) + switch requestedVersion { + // The user has turned off any form of automatic updates. + case "off": + return "", false, nil + // Requested version already the same as client version. + case u.localVersion: + return u.localVersion, false, nil + // No requested version, we continue. + case "": + // Requested version that is not the local one. + default: + if _, err := semver.NewVersion(requestedVersion); err != nil { + return "", false, trace.Wrap(err, "checking that request version is semantic") + } + return requestedVersion, true, nil + } + certPool, err := x509.SystemCertPool() if err != nil { return "", false, trace.Wrap(err) @@ -317,7 +337,15 @@ func (u *Updater) Exec(args []string) (int, error) { if err := os.Unsetenv(teleportToolsVersionEnv); err != nil { return 0, trace.Wrap(err) } - env := append(os.Environ(), teleportToolsVersionEnv+"=off") + + env := os.Environ() + executablePath, err := os.Executable() + if err != nil { + return 0, trace.Wrap(err) + } + if path == executablePath { + env = append(env, teleportToolsVersionEnv+"=off") + } if runtime.GOOS == constants.WindowsOS { cmd := exec.Command(path, args...)