Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client tools auto update logic for multiple platforms #46587

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vapopov
Copy link
Contributor

@vapopov vapopov commented Sep 13, 2024

In this PR implemented logic for client tools autoupdate {tsh, tctl}, added integration tests for simulations of downloading, unpacking and replacing tools binaries from test CDN server (pre-combiled binary which contains only separated updater logic).

changelog: Added auto update logic for tsh, tctl client tools

@vapopov vapopov force-pushed the vapopov/client-tools-autoupdate-merge branch from 974e35d to 433f481 Compare September 13, 2024 18:01
@vapopov vapopov self-assigned this Sep 13, 2024

return archive, archive + ".sha256", nil
}

Copy link
Member

@sclevine sclevine Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put together a more productionized version of these methods for the agent version, which adds disk space checking, timeouts, context cancellation, etc.: https://github.com/gravitational/teleport/compare/sclevine/teleport-update-enable

We could try to reuse those, but the use cases are a bit different. For now, you might want to look at some of the differences. E.g., use of the client and contexts to handle graceful cancellation, and limiting the number of bytes read over the network.

Add request timeout and default client
Feature flag check
@vapopov vapopov force-pushed the vapopov/client-tools-autoupdate-merge branch from 6e7505c to 9e215fd Compare September 23, 2024 18:22
return requestedVersion, true
}

return toolsVersion, false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russjones If the user downloads newer version of tsh/tctl and let say execute it from download directory, should we alway re-execute with saved version from tools directory (where version might be older)

@@ -0,0 +1,26 @@
//go:build webassets_ent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to introduce new build tag for enterprise version instead of reusing this one for web assets?

@vapopov vapopov marked this pull request as ready for review September 23, 2024 18:58
@github-actions github-actions bot added size/xl tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 23, 2024
@public-teleport-github-review-bot

@vapopov - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.


switch runtime.GOOS {
case "darwin":
archive = baseUrl + "/tsh-" + toolsVersion + ".pkg"
Copy link
Contributor Author

@vapopov vapopov Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be replaced with "/teleport" + toolsVersion + ".pkg" only one issue we currently have is that teleport-{version}.pkg has tsh and tctl binary without support of the TouchID for tsh, so we need to install tsh-{version}.pkg additionally, this one is valid only for the <v16, for the >v17 we should have everything in one teleport-{version}.pkg package

Would be better to have separate packages for all platforms with the tsh and tctl only to minimize the download size for the client autoupdate.

cc @russjones

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2k lines is a bit intense to review, I'll definitely miss some stuff here.

Would you mind splitting the PR in smaller chunks? I think at least the webclient, common.update and tctl changes can be separated, but feel free to split using any other criteria.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tool's content should be limited to bootstrapping the CLI and commands. Complex logic should live in lib/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will try to split the logic, but it actually interconnected. Locking might be delivered separately, also integration tests kind of heavy. Webclient btw temporally added until this PR is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/xl tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants