Skip to content

[teleport-update] Read proxy from teleport.yaml to improve UX #51633

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.

Sign up for GitHub

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

Merged
sclevine merged 3 commits into from
Feb 7, 2025
Merged

[teleport-update] Read proxy from teleport.yaml to improve UX #51633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e062489
derive proxy from config
sclevine Jan 30, 2025
e50ff99
fix parsing
sclevine Feb 6, 2025
1bfc3e3
cleanup
sclevine Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 100 additions & 14 deletions lib/autoupdate/agent/updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"time"

"github.com/gravitational/trace"
"gopkg.in/yaml.v3"

"github.com/gravitational/teleport/api/client/webclient"
"github.com/gravitational/teleport/api/constants"
Expand Down Expand Up @@ -102,7 +103,8 @@ func NewLocalUpdater(cfg LocalUpdaterConfig, ns *Namespace) (*Updater, error) {
Log: cfg.Log,
Pool: certPool,
InsecureSkipVerify: cfg.InsecureSkipVerify,
ConfigPath: ns.updaterConfigFile,
UpdateConfigPath: ns.updaterConfigFile,
TeleportConfigPath: ns.configFile,
Installer: &LocalInstaller{
InstallDir: ns.versionsDir,
LinkBinDir: ns.linkDir,
Expand Down Expand Up @@ -181,8 +183,10 @@ type Updater struct {
Pool *x509.CertPool
// InsecureSkipVerify skips TLS verification.
InsecureSkipVerify bool
// ConfigPath contains the path to the agent auto-updates configuration.
ConfigPath string
// UpdateConfigPath contains the path to the agent auto-updates configuration.
UpdateConfigPath string
// TeleportConfig contains the path to Teleport's configuration.
TeleportConfigPath string
// Installer manages installations of the Teleport agent.
Installer Installer
// Process manages a running instance of Teleport.
Expand Down Expand Up @@ -305,14 +309,22 @@ func toPtr[T any](t T) *T {
// This function is idempotent.
func (u *Updater) Install(ctx context.Context, override OverrideConfig) error {
// Read configuration from update.yaml and override any new values passed as flags.
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
if err := validateConfigSpec(&cfg.Spec, override); err != nil {
return trace.Wrap(err)
}

agentProxy := u.findAgentProxy(ctx)
if cfg.Spec.Proxy == "" {
cfg.Spec.Proxy = agentProxy
} else if agentProxy != "" &&
!sameProxies(cfg.Spec.Proxy, agentProxy) {
u.Log.WarnContext(ctx, "Proxy specified in update.yaml does not match teleport.yaml. Unexpected updates may occur.", "update_proxy", cfg.Spec.Proxy, "teleport_proxy", agentProxy)
}

active := cfg.Status.Active
skip := deref(cfg.Status.Skip)

Expand Down Expand Up @@ -353,18 +365,87 @@ func (u *Updater) Install(ctx context.Context, override OverrideConfig) error {
// Only write the configuration file if the initial update succeeds.
// Note: skip_version is never set on failed enable, only failed update.

if err := writeConfig(u.ConfigPath, cfg); err != nil {
if err := writeConfig(u.UpdateConfigPath, cfg); err != nil {
return trace.Wrap(err, "failed to write %s", updateConfigName)
}
u.Log.InfoContext(ctx, "Configuration updated.")
return trace.Wrap(u.notices(ctx))
}

// proxyAddrConfig contains potential proxy server addresses from teleport.yaml.
type proxyAddrConfig struct {
Teleport proxyAddrTeleport `yaml:"teleport"`
}

type proxyAddrTeleport struct {
AuthServers []string `yaml:"auth_servers"`
AuthServer string `yaml:"auth_server"`
ProxyServer string `yaml:"proxy_server"`
}

// findAgentProxy finds a proxy in teleport.yaml if not specified or set in update configuration.
// Note that any implicitly defaulted port in teleport.yaml is explicitly defaulted (to 3080) by this method.
func (u *Updater) findAgentProxy(ctx context.Context) string {
f, err := libutils.OpenFileAllowingUnsafeLinks(u.TeleportConfigPath)
if err != nil {
u.Log.DebugContext(ctx, "Unable to open Teleport config to read proxy", "config", u.TeleportConfigPath, errorKey, err)
return ""
}
defer f.Close()
var cfg proxyAddrConfig
if err := yaml.NewDecoder(f).Decode(&cfg); err != nil {
u.Log.DebugContext(ctx, "Unable to parse Teleport config to read proxy", "config", u.TeleportConfigPath, errorKey, err)
return ""
}

var addr string
var port int
switch t := cfg.Teleport; {
case t.ProxyServer != "":
addr = t.ProxyServer
port = libdefaults.HTTPListenPort
case t.AuthServer != "":
addr = t.AuthServer
port = libdefaults.AuthListenPort
Comment thread
sclevine marked this conversation as resolved.
case len(t.AuthServers) > 0:
addr = t.AuthServers[0]
port = libdefaults.AuthListenPort
default:
u.Log.DebugContext(ctx, "Unable to find proxy in Teleport config", "config", u.TeleportConfigPath, errorKey, err)
return ""
}
netaddr, err := libutils.ParseHostPortAddr(addr, port)
if err != nil {
u.Log.DebugContext(ctx, "Unable to parse proxy in Teleport config", "config", u.TeleportConfigPath, "proxy_addr", addr, "proxy_port", port, errorKey, err)
return ""
}
return netaddr.String()
}

// sameProxies returns true if both proxies addresses are the same.
// Note that the port is defaulted to 443, which is different from teleport.yaml's default.
func sameProxies(a, b string) bool {
const defaultPort = 443
if a == b {
return true
}
addrA, err := libutils.ParseAddr(a)
if err != nil {
return false
}
addrB, err := libutils.ParseAddr(b)
if err != nil {
return false
}
return addrA.Host() == addrB.Host() &&
addrA.Port(defaultPort) == addrB.Port(defaultPort)
}

// Remove removes everything created by the updater for the given namespace.
// Before attempting this, Remove attempts to gracefully recover the system-packaged version of Teleport (if present).
// This function is idempotent.
func (u *Updater) Remove(ctx context.Context) error {
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
Expand Down Expand Up @@ -491,7 +572,7 @@ func isActiveOrEnabled(ctx context.Context, s Process) (bool, error) {
func (u *Updater) Status(ctx context.Context) (Status, error) {
var out Status
// Read configuration from update.yaml.
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return out, trace.Wrap(err, "failed to read %s", updateConfigName)
}
Expand All @@ -513,7 +594,7 @@ func (u *Updater) Status(ctx context.Context) (Status, error) {
// Disable disables agent auto-updates.
// This function is idempotent.
func (u *Updater) Disable(ctx context.Context) error {
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
Expand All @@ -522,7 +603,7 @@ func (u *Updater) Disable(ctx context.Context) error {
return nil
}
cfg.Spec.Enabled = false
if err := writeConfig(u.ConfigPath, cfg); err != nil {
if err := writeConfig(u.UpdateConfigPath, cfg); err != nil {
return trace.Wrap(err, "failed to write %s", updateConfigName)
}
return nil
Expand All @@ -531,7 +612,7 @@ func (u *Updater) Disable(ctx context.Context) error {
// Unpin allows the current version to be changed by Update.
// This function is idempotent.
func (u *Updater) Unpin(ctx context.Context) error {
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
Expand All @@ -540,7 +621,7 @@ func (u *Updater) Unpin(ctx context.Context) error {
return nil
}
cfg.Spec.Pinned = false
if err := writeConfig(u.ConfigPath, cfg); err != nil {
if err := writeConfig(u.UpdateConfigPath, cfg); err != nil {
return trace.Wrap(err, "failed to write %s", updateConfigName)
}
return nil
Expand All @@ -553,13 +634,18 @@ func (u *Updater) Unpin(ctx context.Context) error {
// This function is idempotent.
func (u *Updater) Update(ctx context.Context, now bool) error {
// Read configuration from update.yaml and override any new values passed as flags.
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
if err := validateConfigSpec(&cfg.Spec, OverrideConfig{}); err != nil {
return trace.Wrap(err)
}
if p := u.findAgentProxy(ctx); p != "" &&
!sameProxies(cfg.Spec.Proxy, p) {
u.Log.WarnContext(ctx, "Proxy specified in update.yaml does not match teleport.yaml. Unexpected updates may occur.", "update_proxy", cfg.Spec.Proxy, "teleport_proxy", p)
}

active := cfg.Status.Active
skip := deref(cfg.Status.Skip)
if !cfg.Spec.Enabled {
Expand Down Expand Up @@ -624,7 +710,7 @@ func (u *Updater) Update(ctx context.Context, now bool) error {
}

updateErr := u.update(ctx, cfg, target, false, resp.AGPL)
writeErr := writeConfig(u.ConfigPath, cfg)
writeErr := writeConfig(u.UpdateConfigPath, cfg)
if writeErr != nil {
writeErr = trace.Wrap(writeErr, "failed to write %s", updateConfigName)
} else {
Expand Down Expand Up @@ -894,7 +980,7 @@ func (u *Updater) cleanup(ctx context.Context, keep []Revision) error {
// LinkPackage returns an error only if an unknown version of Teleport is present (e.g., manually copied files).
// This function is idempotent.
func (u *Updater) LinkPackage(ctx context.Context) error {
cfg, err := readConfig(u.ConfigPath)
cfg, err := readConfig(u.UpdateConfigPath)
if err != nil {
return trace.Wrap(err, "failed to read %s", updateConfigName)
}
Expand Down
148 changes: 148 additions & 0 deletions lib/autoupdate/agent/updater_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1552,6 +1552,154 @@ func TestUpdater_Install(t *testing.T) {
}
}

func TestUpdater_findAgentProxy(t *testing.T) {
t.Parallel()

tests := []struct {
name string
cfg *proxyAddrTeleport
want string
}{
{
name: "full",
cfg: &proxyAddrTeleport{
ProxyServer: "https://example.com:8080",
},
want: "example.com:8080",
},
{
name: "protocol and host",
cfg: &proxyAddrTeleport{
ProxyServer: "https://example.com",
},
want: "example.com:3080",
},
{
name: "host and port",
cfg: &proxyAddrTeleport{
ProxyServer: "example.com:443",
},
want: "example.com:443",
},
{
name: "host",
cfg: &proxyAddrTeleport{
ProxyServer: "example.com",
},
want: "example.com:3080",
},
{
name: "auth server (v3)",
cfg: &proxyAddrTeleport{
AuthServer: "example.com",
},
want: "example.com:3025",
},
{
name: "auth server (v1/2)",
cfg: &proxyAddrTeleport{
AuthServers: []string{
"one.example.com",
"two.example.com",
},
},
want: "one.example.com:3025",
},
{
name: "proxy priority",
cfg: &proxyAddrTeleport{
ProxyServer: "one.example.com",
AuthServer: "two.example.com",
AuthServers: []string{"three.example.com"},
},
want: "one.example.com:3080",
},
{
name: "auth priority",
cfg: &proxyAddrTeleport{
AuthServer: "two.example.com",
AuthServers: []string{"three.example.com"},
},
want: "two.example.com:3025",
},
{
name: "missing",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ns := &Namespace{
configFile: filepath.Join(t.TempDir(), "teleport.yaml"),
}
if tt.cfg != nil {
out, err := yaml.Marshal(proxyAddrConfig{Teleport: *tt.cfg})
require.NoError(t, err)
err = os.WriteFile(ns.configFile, out, os.ModePerm)
require.NoError(t, err)
}

updater, err := NewLocalUpdater(LocalUpdaterConfig{}, ns)
require.NoError(t, err)
ctx := context.Background()
s := updater.findAgentProxy(ctx)
require.Equal(t, tt.want, s)
})
}
}

func TestSameProxies(t *testing.T) {
t.Parallel()

tests := []struct {
name string
a, b string
match bool
}{
{
name: "protocol missing with port",
a: "https://example.com:8080",
b: "example.com:8080",
match: true,
},
{
name: "protocol missing without port",
a: "https://example.com",
b: "example.com",
match: true,
Comment on lines +1666 to +1668
Copy link
Copy Markdown
Contributor

@nklaassen nklaassen Jan 30, 2025

Choose a reason for hiding this comment

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

re: my other comment, i don't think addresses without ports should match if they will default to use different ports

Copy link
Copy Markdown
Member Author

@sclevine sclevine Feb 1, 2025

Choose a reason for hiding this comment

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

See above

},
{
name: "same with port",
a: "example.com:443",
b: "example.com:443",
match: true,
},
{
name: "does not set default teleport port",
a: "example.com",
b: "example.com:3080",
match: false,
},
{
name: "does set default standard port",
a: "example.com",
b: "example.com:443",
match: true,
},
{
name: "other formats if equal",
a: "@123",
b: "@123",
match: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := sameProxies(tt.a, tt.b)
require.Equal(t, tt.match, s)
})
}
}

func TestUpdater_Setup(t *testing.T) {
t.Parallel()

Expand Down