Conversation
bernardjkim
requested review from
fheinecke,
fspmarshall,
hugoShaka,
jentfoo,
jimbishopp and
sclevine
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
Overall makes sense and I like the reduction in scope, just left a bunch of suggestions/clarifications.
jentfoo
left a comment
jentfoo
left a comment
There was a problem hiding this comment.
Some initial questions, thank you!
- More details on binary execution - More security details
|
|
||
| Ideally, we'd like this feature to be integrated seamlessly. Users of the Teleport client tools should not need to search for documentation and spend time figuring out how to enable auto updates for their cleint tools. | ||
|
|
||
| After client tools auto updates is deployed and the first time the client tools are executed, the user will be asked two questions: 1) if they would like to update to the latest version (default yes) and 2) if they would like to enroll in auto updates for client tools (default yes). |
There was a problem hiding this comment.
Would it make sense to turn this on by default when Cloud clusters are targeted, and require an opt-out command to disable it?
There was a problem hiding this comment.
I don't have a strong opinion on either way. I think most users would opt-in to auto updates so I think it'd be fine to turn this on by default.
It could also be nice to prompt the user when auto updates are first available to let the user know that auto updates is configurable.
There was a problem hiding this comment.
I don't think we should ask whether they want to enroll or not. Let's just enable it by default.
|
Sorry for the back and forth. After some more discussion we're leaning more towards just using the system package manager to update the client tools. This might end up being tedious to add support for all our installation methods, but the solution is a lot simpler and straight forward. |
|
|
||
| Another downside to this approach would be that upgrades will require sudo/admin privileges. | ||
|
|
||
| ### Caching (alternative) |
There was a problem hiding this comment.
Nit: I would move this section way down below, maybe under Appendix/Alternatives or something so the reader does not lose context of how things will actually work with the selected approach.
|
|
||
| ### Config | ||
|
|
||
| Automatic updates for client tools can be configured through tsh configuration. If `DISABLE_AUTO_UPDATE=true`, then auto updates for client tools will be disabled. If `DISABLE_UPDATE_PROMPT=true`, then the user will not be prompted to confirm the update. Both values will default to false. |
There was a problem hiding this comment.
Are DISABLE_AUTO_UPDATE and DISABLE_UPDATE_PROMPT environment variables? It's not super clear what "tsh configuration" means in this case. If these are env. vars, let's say so explicitly.
I think it would also make sense to add these options to tsh.yaml config and give an example of what tsh config will look like.
|
|
||
| Ideally, we'd like this feature to be integrated seamlessly. Users of the Teleport client tools should not need to search for documentation and spend time figuring out how to enable auto updates for their cleint tools. | ||
|
|
||
| After client tools auto updates is deployed and the first time the client tools are executed, the user will be asked two questions: 1) if they would like to update to the latest version (default yes) and 2) if they would like to enroll in auto updates for client tools (default yes). |
There was a problem hiding this comment.
I don't think we should ask whether they want to enroll or not. Let's just enable it by default.
|
|
||
| We should provide some observability into the download status. Whenever a new cloud-stable version is available for download, the progress of the update will be output to stdout. e.g. `New cloud-stable version of Teleport detected`, `Downloading latest version of tsh/tctl... `, `Updated tsh/tctl to version v13.2.3!`. | ||
|
|
||
| To avoid breaking existing scripts, a `--[no-]auto-update` flag will be included. Without the flag given, when a tty is detected and an update is available, the user will be prompted to update. If the flag is not provided and a tty is not detected, updates will be skipped to avoid breaking scripts. |
There was a problem hiding this comment.
Why do we need the flag at all? Why not just skip updates when there's no tty?
|
|
||
| ### Package update | ||
|
|
||
| The client tools will be updated using the package manager available on the system. Teleport supports multiple package repositories and installation methods so we'll need to handle the update differently on each system. This will probably require a decent amount of testing and maintenance work, but we think this is the simplest and most straight forward solution. |
There was a problem hiding this comment.
The client tools will be updated using the package manager available on the system.
This needs a little bit more clarification. Say, I'm on Ubuntu but installed teleport binary directly. Will auto-update still try to attempt to use apt in this case? There may not be apt repository configured.
I think we need to flesh out how detection/installation logic will work a bit more, for each supported package type (direct download, Debian/Ubuntu deb, RHEL/CentOS rpm, macOS pkg, etc.) and include this in the RFD.
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
|
Let's move the discussion to #39805. |
10 participants
Contributes to https://github.com/gravitational/cloud/issues/4880
Supersedes #28337
For the initial implementation we've decided to limit scope to Cloud users only. We also will not be supporting multiple versions as all Cloud tenants are expected to be running the same Teleport version.