[teleport-update] Add enable command #47565

sclevine · 2024-10-15T01:28:52Z

This PR adds the enable subcommand to the teleport-update binary specified by #40190.

The teleport-update binary will be used to enable, disable, and trigger automatic Teleport agent updates. The new auto-updates system manages a local installation of the cluster-specified version of Teleport stored in /var/lib/teleport/versions.

This is the second in a series of PRs extracted and tested from this proof-of-concept: #46357
Initial scaffolding PR: #46418

This PR is missing a number of features necessary for the teleport-update binary to ship, including:

Old version cleanup
Restarting the Teleport agent

RFD: #47126
Goal (internal): https://github.com/gravitational/cloud/issues/10289

root@7e41707a19a4:/teleport# ./tool/teleport-update/teleport-update enable -h
usage: teleport-updater enable [<flags>]

Enable agent auto-updates and perform initial update.

Flags:
  -d, --[no-]debug  Verbose logging to stdout.
      --data-dir    Teleport data directory. Access to this directory should be limited.
      --log-format  Controls the format of output logs. Can be `json` or `text`. Defaults to `text`.
  -p, --proxy       Address of the Teleport Proxy.
  -g, --group       Update group for this agent installation.
  -t, --template    Go template used to override Teleport download URL.
  -f, --version     Force the provided version instead of querying it from the Teleport cluster.

Example:

root@7e41707a19a4:/teleport# ./tool/teleport-update/teleport-update enable --data-dir=./temp/ --proxy=levine.teleport.sh
2024-10-15T01:19:20Z INFO [UPDATER]   Downloading Teleport tarball. url:https://cdn.teleport.dev/teleport-v16.3.0-linux-arm64-bin.tar.gz size:153895772 autoupdate/installer.go:240
2024-10-15T01:19:25Z INFO [UPDATER]   Extracting Teleport tarball. path:temp/versions/16.3.0 size:615014400 autoupdate/installer.go:277
2024-10-15T01:19:29Z INFO [UPDATER]   Target version successfully installed. version:16.3.0 autoupdate/agent.go:242
2024-10-15T01:19:29Z INFO [UPDATER]   Configuration updated. autoupdate/agent.go:251
root@7e41707a19a4:/teleport# ./tool/teleport-update/teleport-update enable --data-dir=./temp/ --proxy=levine.teleport.sh
2024-10-15T01:19:33Z INFO [UPDATER]   Target version already installed. version:16.3.0 autoupdate/agent.go:244
2024-10-15T01:19:33Z INFO [UPDATER]   Configuration updated. autoupdate/agent.go:251
root@7e41707a19a4:/teleport# ls temp/versions/
16.3.0  update.yaml
root@7e41707a19a4:/teleport# cat temp/versions/update.yaml
version: v1
kind: update_config
spec:
    proxy: levine.teleport.sh
    group: ""
    url_template: ""
    enabled: true
status:
    active_version: 16.3.0
root@7e41707a19a4:/teleport# ./tool/teleport-update/teleport-update disable --data-dir=./temp/
root@7e41707a19a4:/teleport# cat temp/versions/update.yaml
version: v1
kind: update_config
spec:
    proxy: levine.teleport.sh
    group: ""
    url_template: ""
    enabled: false
status:
    active_version: 16.3.0

lib/autoupdate/agent.go

lib/autoupdate/installer.go

lib/autoupdate/agent.go

lib/autoupdate/installer.go

vapopov

lgtm

vapopov · 2024-10-16T22:13:32Z

lib/autoupdate/agent/updater.go

+)
+
+var (
+	featureFlag int


We would probably need to move this one to lib/autoupdate level, since both agent and tools packages use this variable.
Also updateVersionEnvVar = "TELEPORT_UPDATE_VERSION" kind of common
Can do it in mine PR after merging this one

That sounds good for now. Eventually I think the agent updater should query the cluster for this.

bl-nero

Great change, I like how easy it is to understand everything because of appropriate use of comments and overall clear structure.

bl-nero · 2024-10-18T07:14:41Z

lib/autoupdate/agent/updater.go

+	// 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"
+	// reservedFreeDisk is the minimum required free space left on disk during downloads.
+	reservedFreeDisk = 10_000_000 // 10 MB


How did we choose this value?

Arbitrarily 🙂

It might be better to use a percentage of the total space along with a maximum. I'll expand on the comment and add a TODO.

lib/autoupdate/agent/updater.go

bl-nero · 2024-10-18T07:25:02Z

lib/autoupdate/agent/updater.go

+// UpdateStatus describes the status field in update.yaml.
+type UpdateStatus struct {
+	// ActiveVersion is the currently active Teleport version.
+	ActiveVersion string `yaml:"active_version"`


How do we prevent this from drifting away from the actual state? I guess we shouldn't rely on a running teleport binary to report its own version, since if an agent crashes, there's nothing to query, but I'd recommend making an effort to update this every time an agent starts (unless it's something you've already thought about.)

In this PR, active_version is the only source of truth. This will become a problem in the next PR, which links the installation into /usr/local/bin, creating two definitions of active.

This is tricky to solve, since there's no way to link multiple binaries into /usr/local/bin atomically in a way that's resilient to, e.g., power loss. The strategy in the next PR will be to write active_version only after the install and linking is successful. That way, drift should only happen where the installed version is newer (or partially newer) than the active_version, which will get corrected by the updater on the next invocation (by checking the symlink paths).

This file is only written by teleport-update. teleport, tbot, and other agent-like processes that run concurrently with teleport-update are only intended to read from it.

bl-nero · 2024-10-18T07:37:56Z

lib/autoupdate/agent/updater.go

+// Installer provides an API for installing Teleport agents.
+type Installer interface {
+	// Install the Teleport agent at version from the download template.
+	Install(ctx context.Context, version, template string) error


Why do we need to pass the template from outside? If we create an installer knowing the existing config, perhaps the installer itself could already be aware of the template string it should use.

The download URL template is also persisted into update.yaml (in the case that it is overridden by the user). It could be confusing to pass the template to Enable for storage, but not use Enable's copy of the value during the installation that Enable initiates.

bl-nero · 2024-10-18T07:41:35Z

lib/autoupdate/agent/updater.go

+	Remove(ctx context.Context, version string) error
+}
+
+// UserConfig contains user overrides for installing Teleport agents.


Can we use "command line flags" instead of "user overrides" in this comment? It may be even better to rename the struct name to something like that.

Unless, of course, this structure can/will be filled from something other than CLI flags.

Good catch, meant to rename this after refactoring. Trying to keep all of the CLI-related terminology in tool/teleport-update, but this should be named something like OverrideConfig

lib/autoupdate/agent/updater_test.go

bl-nero · 2024-10-18T08:37:43Z

lib/autoupdate/agent/feature_ent.go

+package agent
+
+func init() {
+	featureFlag |= flagEnt


It's the first time I see something like this. How exactly does it work? Is there some sort of conditional compilation built into our makefiles?

I copied this from #47466. Both the client and agent packages will use the same flag set from lib/autoupdate in an upcoming PR. The build flags should be set when compiled via teleport.e. See the top of the file:

//go:build webassets_ent // +build webassets_ent

@vapopov I'm noticing now that this flag is only used in webassets_embed_ent.go, and the webassets prefix is confusing in this non-web-asset context. Would it make sense to look into making this flag more generic?

yes, I previously mentioned about this one #46587 (comment) since we don't have proper build flag for ent

Can we use the modules package for this instead of making new flags? This package can already tell you if the binary is OSS, Enterprise, FIPS, etc.

~~Maybe we open a separate PR that renames the build flag so that it can be used in a wider context?~~ (race condition on the Github comments)

I don't think modules.GetModules().IsEnterpriseBuild() will work as-is for teleport-update, since it doesn't have a separate tool/teleport-update target in teleport.e that runs modules.SetModules().

That said, we actually want the edition of the agent download to come from the cluster via /webapi/find before this ships, so I'll just default it to false and add a TODO for now.

@vapopov you should be able to modules for tsh and tctl, but tbot may not work for the reason mentioned.

lib/autoupdate/agent/installer.go

bl-nero · 2024-10-18T08:47:00Z

lib/autoupdate/agent/installer.go

+		return trace.Errorf("mismatched checksum, download possibly corrupt")
+	}
+	// Get uncompressed size of the tgz
+	n, err := uncompressedSize(f)


Nit: since we don't use n anywhere else, perhaps checking for uncompressed size should belong to the extract function. Up to you.

I went back and forth on this, but ended up leaving it out to simplify the contract between Install and helpers like extract. If we calculate the uncompressed size inside extract, we need an io.ReadSeeker, and I wanted to keep all of the seeking in the same function.

lib/autoupdate/agent/installer.go

bl-nero · 2024-10-18T09:03:57Z

@sclevine This comment is probably the only serious one here, the rest of it is minor stuff.

sclevine · 2024-10-21T13:29:31Z

lib/autoupdate/agent/installer.go

+			return nil
+		}
+		li.Log.WarnContext(ctx, "Removing version that does not match checksum.", "version", version)
+		if err := li.Remove(ctx, version); err != nil {


Note that this will remove the active installation if the checksum does not match. In the next PR, Remove will error if we try to remove a linked (and therefore active) installation.

sclevine added 10 commits October 10, 2024 20:42

Add enable scaffold

0eef6c8

add installer

3bdc3e7

refactor

21a0507

add enable tests

7c432a6

clean up download logic

bab8e18

Finish installer tests

fedc914

cleanup

37bc3cd

fix flags

46fb8e0

fix errors

bec25c3

logging

f0c343a

sclevine added the no-changelog Indicates that a PR does not require a changelog entry label Oct 15, 2024

sclevine requested review from vapopov and hugoShaka October 15, 2024 01:28

github-actions bot added the size/lg label Oct 15, 2024

github-actions bot requested review from bl-nero and probakowski October 15, 2024 01:29

sclevine mentioned this pull request Oct 15, 2024

Proof-of-concept draft for teleport-update binary implementation #46357

Closed

sclevine added 3 commits October 14, 2024 21:34

cleanup

2eab556

fix test

0aa4172

Fix download size logic

6e905d5

hugoShaka reviewed Oct 15, 2024

View reviewed changes

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

vapopov reviewed Oct 16, 2024

View reviewed changes

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

lib/autoupdate/installer.go Outdated Show resolved Hide resolved

vapopov reviewed Oct 16, 2024

View reviewed changes

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

lib/autoupdate/agent.go Outdated Show resolved Hide resolved

lib/autoupdate/installer.go Outdated Show resolved Hide resolved

sclevine added 6 commits October 16, 2024 16:49

remove agent prefixes

c1ecf82

namespace package

3d27750

rename file

1378714

feedback

74f20f2

fips and ent support

3e1586e

hide force version

84a7a2a

sclevine requested a review from hugoShaka October 16, 2024 21:19

sclevine requested a review from vapopov October 16, 2024 21:19

vapopov approved these changes Oct 16, 2024

View reviewed changes

bl-nero reviewed Oct 18, 2024

View reviewed changes

sclevine added 3 commits October 18, 2024 13:43

feedback

4f1033d

feedback 2

1ce6bee

fix test

ca5169e

sclevine requested a review from bl-nero October 18, 2024 18:20

move enterprise/fips to webapi

46c92f4

sclevine mentioned this pull request Oct 18, 2024

Client tools auto update #47466

Merged

sclevine commented Oct 21, 2024

View reviewed changes

bl-nero approved these changes Oct 21, 2024

View reviewed changes

hugoShaka approved these changes Oct 21, 2024

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from probakowski October 21, 2024 18:15

Fix interface

f97950e

sclevine enabled auto-merge October 21, 2024 18:30

sclevine added this pull request to the merge queue Oct 21, 2024

Merged via the queue into master with commit d0ca3bf Oct 21, 2024
41 checks passed

sclevine deleted the sclevine/teleport-update-enable2 branch October 21, 2024 19:05

This was referenced Oct 24, 2024

[teleport-update] Add linking into /usr/local #47879

Merged

[teleport-update] Add support for reloading the agent & reverting symlinks on failed reload #47929

Merged

sclevine mentioned this pull request Oct 31, 2024

[teleport-update] Add update subcommand #48244

Merged

sclevine mentioned this pull request Nov 8, 2024

[teleport-update] Add link subcommand #48712

Merged

This was referenced Nov 19, 2024

[teleport-update] Add support for systemd process management #49102

Closed

[teleport-update] Add systemd setup #49174

Merged

[teleport-update] PID-based failure detection and rollback #49175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[teleport-update] Add enable command #47565

[teleport-update] Add enable command #47565

sclevine commented Oct 15, 2024 •

edited

Loading

vapopov left a comment

vapopov Oct 16, 2024

sclevine Oct 16, 2024

bl-nero left a comment

bl-nero Oct 18, 2024

sclevine Oct 18, 2024

bl-nero Oct 18, 2024

sclevine Oct 18, 2024

bl-nero Oct 18, 2024

sclevine Oct 18, 2024 •

edited

Loading

bl-nero Oct 18, 2024

sclevine Oct 18, 2024 •

edited

Loading

bl-nero Oct 18, 2024

sclevine Oct 18, 2024

vapopov Oct 18, 2024

zmb3 Oct 18, 2024

sclevine Oct 18, 2024 •

edited

Loading

sclevine Oct 18, 2024 •

edited

Loading

bl-nero Oct 18, 2024

sclevine Oct 18, 2024

bl-nero commented Oct 18, 2024

sclevine Oct 21, 2024

[teleport-update] Add enable command #47565

[teleport-update] Add enable command #47565

Conversation

sclevine commented Oct 15, 2024 • edited Loading

vapopov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-nero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclevine Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclevine Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclevine Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

sclevine Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-nero commented Oct 18, 2024

Choose a reason for hiding this comment

sclevine commented Oct 15, 2024 •

edited

Loading

sclevine Oct 18, 2024 •

edited

Loading

sclevine Oct 18, 2024 •

edited

Loading

sclevine Oct 18, 2024 •

edited

Loading

sclevine Oct 18, 2024 •

edited

Loading